Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: generalize multimodal link chooser #3536

Merged
merged 4 commits into from
Nov 15, 2024

Conversation

sebhoerl
Copy link
Contributor

@sebhoerl sebhoerl commented Nov 2, 2024

Currently, the access/egress link of a car trip is only chosen based on the origin/destination facilities of the trip. We have use cases, where we know the location of a car (at a P&R station), so the "access link" should not be the activity location, but rather the location of the vehicle / station. The generalization of the interface as proposed in this PR allows implementing this kind of custom behavior (for instance by passing additional information through the Attributes of the RoutingRequest).

@Janekdererste
Copy link
Member

Do we need the other decideOnLink method then?

@sebhoerl
Copy link
Contributor Author

sebhoerl commented Nov 4, 2024

@Janekdererste This is just in the default implementation, because this one doesn't care about access/egress, but only about the facility location. So decideAccessLink and decideEgressLink delegate to decideOnLink, otherwise we duplicate the same code. But yes, we can make this one private in the default implementation.

@Janekdererste
Copy link
Member

Sorry, @sebhoerl, I didn't read carefully enough. I thought the method is still in the interface. You implemented it just like I would have expected :-)

@d-roeder
Copy link
Contributor

I really like the idea of ​​making this configurable.

However, when looking at the code I noticed one thing that might be problematic or lead to unintended behaviour. In

and following the behavior of this PR is overridden when the facility is assigned to a link. This could possibly be mitigated if accessEgressModeToLink is used. Not sure. However, I think there should be no hidden magic when whe have the explicit LinkChooser.

Another point (whith a scope somehow outside of this PR) is that in

Coord startCoord = NetworkUtils.findNearestPointOnLink(toFacility.getCoord(),egressActLink);
nearestPointOnLink is used. I think this leads to wormholes unless relativePositionOnLink is used, as the agent walks to any point on the link, but de facto starts at the end.

@sebhoerl
Copy link
Contributor Author

Hi @d-roeder, I just took a quick look at the code. The toFacility is actually the destination of the car trip, and egressActLink is the link chosen by the link chooser. So I think everything is fine, or am I mistaken?

@d-roeder
Copy link
Contributor

oh, you're right 👀

It could just be a bit confusing if the toFacility has no link set and NetworkRoutingInclAccessEgressModule then assumes that startLink==endLink. This makes sense in the default logic, but if arbitrary links can be set, then this is no longer necessarily correct.

@sebhoerl sebhoerl enabled auto-merge (squash) November 15, 2024 14:33
@sebhoerl sebhoerl merged commit 826595e into master Nov 15, 2024
49 checks passed
@sebhoerl sebhoerl deleted the feat/generalize-multimodal-link-chooser branch November 15, 2024 14:57
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.

3 participants