Skip to content

Ignore custom cost fields in CAR routing - #972

Merged
abyrd merged 4 commits into
devfrom
car-elevation-fix
Sep 3, 2025
Merged

abyrd merged 4 commits into
devfrom
car-elevation-fix

Conversation

@ansoncfit

Copy link
Copy Markdown
Member

This ignores custom costs (e.g. from elevation data) for the CAR street mode. Custom speeds for cars are handled separately.

Fixes #970

This ignores custom costs (e.g. from elevation data) for the CAR street mode. Custom speeds for cars are handled separately.
@ansoncfit
ansoncfit requested a review from abyrd July 25, 2025 17:49
@abyrd

abyrd commented Sep 3, 2025

Copy link
Copy Markdown
Member

Note, this PR is cherry-picking commit d5722b6 from the broader PR #812.

@abyrd abyrd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change assumes all CostField instances apply to all modes except cars. CostFields were intended to be generic, and could in principle apply costs to walking, bicycling, or driving. Our assumption was that users would know which modes they intended their costs to apply to, and would only employ such graphs for the intended modes.

In practice, the existing CostField implementations (elevation and sun exposure) do not apply to driving, so this change does provide an immediate improvement to car results.

However, it gives a false impression that mode is taken into account by CostFields, and bakes the undocumented assumption about which modes CostFields apply to into traversal logic. We should clearly document this assumption and the fact that it is an unrealistic simplification, as this has the potential to cause confusion in future development.

Although sun and hill costs arguably apply to both walking and biking, these costs would likely be different for biking. CostFields do not receive any information about what mode of transport is in use, so they cannot modify their behavior per mode. Implicitly, a CostField only applies to a single mode of transport (rather than "all modes but car").

Ideally CostFields would know which modes they applied to, and the list of CostFields would be filtered before the search happens. The logic currently in the StreetRouter constructor could be factored into a mode-aware factory method that would yield the correct TraversalTimeCalculator.

We don't need to implement this now, but should document it in the Javadoc of the relevant classes. I will propose some simple Javadoc changes.

@abyrd

abyrd commented Sep 3, 2025

Copy link
Copy Markdown
Member

StreetRouter is also explicitly single-use and can only have one StreetMode, so that mode could be specified up front and the TraversalTimeCalculator chosen accordingly (avoiding the MultistageTraversalTimeCalculator construct entirely when no CostFields apply).

@abyrd abyrd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved (added two commits with Javadoc and line comments)

@abyrd
abyrd enabled auto-merge September 3, 2025 15:54
@abyrd
abyrd merged commit c5e8479 into dev Sep 3, 2025
@abyrd
abyrd deleted the car-elevation-fix branch September 3, 2025 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GTFS and Elevation data affects carspeeds

2 participants