Fix missing transfers

[t2gran]

Summary

This PR fixes critical bugs in transfer generation that could cause missing transfers and incorrect routing results. It also refactors the transfer generation code for better maintainability and improves support for stops used in real-time updates.

The PR addresses two critical bugs:

1. StopPattern.canAlight() was always returning false for valid alighting stops at the last stop in a stop pattern
2. Transfer generation was filtering out transfersNotAllowed stops after pattern filtering, causing valid transfer alternatives to be dropped

Additionally, the PR refactors the pattern-based filtering logic into smaller, more focused classes and renames the IncludeEmptyRailStopsInTransfers feature to IncludeStopsUsedRealtimeInTransfers with improved semantics.

Issue

Fixes #7013

Critical Bug Fixes

StopPattern.canAlight() Logic Error

The StopPattern.canAlight(StopLocation stop) method had a critical copy-paste bug where it was skipping the last stop instead of the first stop:

// OLD (BUGGY):
boolean canAlight(StopLocation stop) {
  // We skip the last stop, not allowed for boarding  ← WRONG COMMENT (copy-pasted from canBoard)
  for (int i = 0; i < stops.length - 1; ++i) {        ← WRONG LOOP (skips last stop, not first)
    if (stop == stops[i] && canAlight(i)) {
      return true;
    }
  }
  return false;
}

Result: The method would always return false for the last stop in a pattern, even though that's typically where passengers SHOULD be able to alight.

The fix corrects the loop to skip the first stop instead:

// NEW (FIXED):
boolean canAlight(StopLocation stop) {
  // We skip the first stop, not allowed for alighting  ← CORRECT COMMENT
  for (int i = 1; i < stops.length; ++i) {              ← CORRECT LOOP (skips first stop)
    if (stop == stops[i] && canAlight(i)) {
      return true;
    }
  }
  return false;
}

Impact: This bug caused the last stop in patterns to be incorrectly excluded from transfer generation, potentially resulting in missing transfers, longer journeys, or no route found.

Missing Transfers Due to Filtering Order

In DirectTransferGenerator, stops with transfersNotAllowed() were being filtered out AFTER pattern filtering. This caused valid transfer alternatives to be dropped when the closest stop on a pattern had transfersNotAllowed set, even though there might be other nearby stops on the same pattern that should be included.

The fix moves the transfersNotAllowed() check to occur BEFORE pattern/flex filtering in PatternConsideringNearbyStopFinder:

// Remove transfersNotAllowed stops BEFORE we filter in Pattern and Flex Trips
nearbyStops = removeTransferNotAllowedStops(nearbyStops);

// Run TripPattern and FlexTrip filters
var result = filter.filterToStops(nearbyStops, reverseDirection);

Impact: This ensures that pattern filtering can select alternative stops when the closest stop is not available for transfers.

Architecture Improvements

Transfer Module Organization

Moved DirectTransferGenerator from graph_builder.module to graph_builder.module.transfer to better organize transfer-related code.

Filter Architecture Refactoring

Split the monolithic PatternConsideringNearbyStopFinder into a cleaner architecture with separated concerns:

New classes:

• NearbyStopFilter (interface) - Defines the contract for filtering nearby stops
• PatternNearbyStopFilter - Filters stops based on trip patterns (boarding/alighting rules)
• FlexTripNearbyStopFilter - Filters stops based on flex trip availability
• CompositeNearbyStopFilter - Composes multiple filters together
• MinMap - Utility class for tracking minimum values (moved from nearbystops package)

Refactored class:

• PatternConsideringNearbyStopFinder - Now orchestrates the filtering process using composed filters

This separation makes the code easier to understand, test, and extend with new filtering strategies.

Real-time Stop Support Enhancement

Feature Rename and Improved Semantics

Renamed IncludeEmptyRailStopsInTransfers to IncludeStopsUsedRealtimeInTransfers with improved functionality:

Old behavior:

• Only included rail stops without scheduled patterns
• Stops were checked directly during transfer generation
• Flag name focused on "empty" stops rather than the use case

New behavior:

• Introduces sometimesUsedRealtime field on RegularStop that is set during import
• Stops marked as sometimesUsedRealtime are included in transfer generation even without scheduled patterns
• Currently supports:
  • Rail stops (which often have late platform assignments)
  • Rail replacement bus stops (detected via NeTEx submode RAIL_REPLACEMENT_BUS)
• More accurately describes the use case: supporting stops that will be used via real-time updates
• Better separation of concerns: tagging happens at import time, transfer generation uses the tag

Key Architectural Improvement

The new approach separates the identification of real-time stops (during import) from their usage in transfer generation:

1. Import time (NeTEx): Stop is tagged with sometimesUsedRealtime = true based on mode/submode
2. Transfer generation: PatternNearbyStopFilter checks the sometimesUsedRealtime flag to include these stops

This architecture makes it easy to extend support to GTFS feeds in the future - the transfer generation code doesn't need to change, only the import logic needs to implement the tagging for GTFS stops.

Implementation Details

Added sometimesUsedRealtime field to RegularStop:

• Set during import for stops likely to be used by real-time updates
• Currently only implemented in NeTEx import (QuayMapper)
• Used in PatternNearbyStopFilter.includeFromStop() and filterToStops() to include these stops even without scheduled patterns
• Only applied when IncludeStopsUsedRealtimeInTransfers feature flag is enabled

NeTEx Mapping (QuayMapper):

boolean sometimesUsedRealtime = false;

if (OTPFeature.IncludeStopsUsedRealtimeInTransfers.isOn()) {
  if (transitMode.mainMode() == RAIL) {
    sometimesUsedRealtime = true;
  }
  // Rail replacement buses are often used for service disruptions
  else if (subMode != null && subMode.equals(RAIL_REPLACEMENT_BUS_VALUE)) {
    sometimesUsedRealtime = true;
  }
}

Transfer Generation Usage (PatternNearbyStopFilter):

@Override
public boolean includeFromStop(FeedScopedId id, boolean reverseDirection) {
  var stop = transitService.getRegularStop(id);
  boolean hasPatterns = !findPatternsForStop(stop, !reverseDirection).isEmpty();
  return hasPatterns || includeStopUsedRealtime(stop);
}

private boolean includeStopUsedRealtime(RegularStop stop) {
  return OTPFeature.IncludeStopsUsedRealtimeInTransfers.isOn() &&
         stop.isSometimesUsedRealtime();
}

Future extensibility: The same pattern can be implemented for GTFS feeds by adding similar logic to the GTFS import process. The transfer generation code already supports it - no changes needed there.

Testing

Unit Tests

• StopPatternTest: Added tests for canAlight() method to ensure correct behavior
• DirectTransferGeneratorTest: Completely restructured with improved test names and assertions
  • Split CAR-specific tests into DirectTransferGeneratorCarTest
  • Added visual diagram (DirectTransferGeneratorTest.drawio.png) to illustrate test scenarios
  • Improved test clarity and coverage
• GraphRoutingTest: Added support for injecting stops to enable better test scenarios

Manual Testing

The changes have been validated with the existing integration tests and do not introduce performance regressions.

Documentation

• Updated doc/user/Configuration.md with the renamed feature flag IncludeStopsUsedRealtimeInTransfers
• Improved JavaDoc throughout the refactored classes
• Added detailed comments explaining the filtering logic and order of operations

Performance Impact

Significant improvement in graph building performance and graph size:

When building the Norway graph, the number of transfers decreased from 651,965 to 546,892 (a reduction of ~16% or 105,073 transfers).

Why the reduction?

The improved filtering logic removes unnecessary transfers more effectively:

1. Correct canAlight() logic: The bug fix ensures we only consider stops where passengers can actually alight, eliminating invalid transfers
2. Better filtering order: Moving transfersNotAllowed check before pattern filtering prevents unnecessary computation
3. Pattern-based filtering: Only includes the closest stop on each pattern, not all nearby stops

Benefits:

• Faster graph building: Fewer transfers to generate and process
• Smaller graph size: ~16% fewer transfer objects in memory and serialized graphs
• No routing performance impact: The removed transfers were either invalid (bug Make transportation extremely easy #1) or suboptimal (pattern filtering working correctly)

Breaking Changes

⚠️ Configuration Change Required: Feature Flag Renamed

The OTP feature flag has been renamed:

• Old name: IncludeEmptyRailStopsInTransfers
• New name: IncludeStopsUsedRealtimeInTransfers

Migration steps:

1. Check your otp-config.json for the old feature flag:

   {
     "otpFeatures": {
       "IncludeEmptyRailStopsInTransfers": true
     }
   }

2. Update to the new name:

   {
     "otpFeatures": {
       "IncludeStopsUsedRealtimeInTransfers": true
     }
   }

Behavioral changes:

• The feature now also includes rail replacement bus stops (NeTEx submode RAIL_REPLACEMENT_BUS) in addition to rail stops
• Stop identification happens at import time (stops are tagged with sometimesUsedRealtime), not during transfer generation
• The change is more accurate in identifying stops that will be used by real-time updates

Checklist

• ✅ Tests added/updated
• ✅ Code follows OTP style guidelines (Prettier, JavaDoc)
• ✅ Java Documentation updated for all non-trivial public classes and methods
• ✅ Configuration documentation updated
• ✅ Performance verified
  • Norway graph: 16% reduction in transfers (651,965 → 546,892)
  • Performance test branch created: fix-missing-tx-performance-test
  • Results will be available on OTP Performance Dashboard

Changelog

• ✅ Should be included in changelog
  • Critical bug fixes that affect routing correctness
  • Feature flag rename (breaking change)
  • Improved support for real-time stop usage

Bumping the serialization version id

• ✅ Serialization version bump required
  • Added new field sometimesUsedRealtime to RegularStop class
  • This is a change to the serialized graph structure

[miklcct]

We are using this feature for GTFS feeds so this is a concern to us. The original feature, which is part of the 2.8.1 release, works in both GTFS and NeTEx.

[codecov]

Codecov Report

❌ Patch coverage is 87.20000% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.31%. Comparing base (7de21ea) to head (c6f8673).
⚠️ Report is 92 commits behind head on dev-2.x.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
.../org/opentripplanner/netex/mapping/QuayMapper.java	54.54%	4 Missing and 1 partial ⚠️
...ule/transfer/filter/CompositeNearbyStopFilter.java	80.95%	2 Missing and 2 partials ⚠️
...dule/transfer/filter/FlexTripNearbyStopFilter.java	80.00%	2 Missing and 1 partial ⚠️
...fer/filter/PatternConsideringNearbyStopFinder.java	87.50%	1 Missing and 1 partial ⚠️
...a/org/opentripplanner/gtfs/mapping/StopMapper.java	90.00%	0 Missing and 1 partial ⚠️
...pentripplanner/transit/model/site/RegularStop.java	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #7042      +/-   ##
=============================================
+ Coverage      72.29%   72.31%   +0.02%     
- Complexity     20182    20217      +35     
=============================================
  Files           2189     2192       +3     
  Lines          81217    81288      +71     
  Branches        8140     8147       +7     
=============================================
+ Hits           58712    58784      +72     
  Misses         19630    19630              
+ Partials        2875     2874       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[leonardehrenfried]

There are few things I would like a clarification on.

[leonardehrenfried]

Please resolve the conflicts

[vpaturet]

The old feature is replaced by a new one that currently supports only NeTEx. Does it mean loss of functionality for GTFS-based deployments?

[t2gran]

The old feature is replaced by a new one that currently supports only NeTEx. Does it mean loss of functionality for GTFS-based deployments?
(@vpaturet)

We are using this feature for GTFS feeds so this is a concern to us. The original feature, which is part of the 2.8.1 release, works in both GTFS and NeTEx.
(@miklcct)

I missed that there is an extension in GTFS that set the vehicleType - I will fix the GTFS import as well. That means this is going to work the same way with NeTEx and GTFS - including empty RailReplacementBus services.

[leonardehrenfried]

Suggested change

	// S0 <-> S23 is too fare, not found in neardby search
	// S0 <-> S23 is too far, not found in nearby search

[leonardehrenfried]

I don't want to start a linguistic argument, but sometimesUsesRealtime is already correct English.

[leonardehrenfried]

So you could drop the is prefix.

[leonardehrenfried]

This is already good to go, IMO, but I noticed a few tiny things that you may want to consider. I will approve once Vincent does.

[leonardehrenfried]

If you wish, you can apply my changes but that's not required.