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

Count actual roundabout exits, not just passed steps #4551

Closed
wants to merge 1 commit into from

Conversation

danpat
Copy link
Member

@danpat danpat commented Sep 28, 2017

Issue

When a node on a roundabout has multiple exits directly connected, we only count it as one exit from the roundabout, which throws off the exit number counting for following exits.

Usually this is a result of weird modeling in OSM - multiple exits from the same point on the roundabout is kind of odd, but we found one at http://www.openstreetmap.org/node/49861176, which was where http://www.openstreetmap.org/way/528145111 was connected until @willwhite adjusted it.

This PR looks at the actual exit counts at each step around the roundabout and tallies them up, rather than just counting the number of steps between the entry and the exit.

This could also be a precursor to fixing #2533 - one idea I had was possibly not counting exits that also have a mode change.

@MoKob can you eyeball this and see if my logic is sound? Also open to suggestions if there is a more elegant approach to iterating around the entry[] array :-)

TODO: proper left/right driving side lookup from the profile.

Tasklist

  • add regression / cucumber cases (see docs/testing.md)
  • review
  • adjust for comments

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@danpat danpat requested a review from MoKob September 28, 2017 02:33
// Count all the possible exits from from the step, minus one, which is the
// "continue on the roundabout" exit
const auto step_intersection = step.intersections.front();
const auto num_exits_from_step = std::count_if(std::begin(step_intersection.entry),
Copy link

Choose a reason for hiding this comment

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

This logic doesn't work for throughabouts. If a road (bidirectional) passes through a roundabout, the exit will be counted twice.

Copy link

Choose a reason for hiding this comment

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

Also, this would need to ignore exits that cannot be entered (exit against roundabout direction)

| g | i | ga,ci,ci,ci | depart,abcdefa-exit-1,exit rotary right,arrive |
| g | j | ga,cj,cj,cj | depart,abcdefa-exit-2,exit rotary right,arrive |
| g | k | ga,ck,ck,ck | depart,abcdefa-exit-3,exit rotary right,arrive |
| g | h | ga,dh,dh,dh | depart,abcdefa-exit-5,exit rotary right,arrive |
Copy link

Choose a reason for hiding this comment

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

How is l decided as being the fourth exit. It looks similar to i in terms of distance from g?

@miccolis
Copy link
Contributor

@danpat I though we discussed this and decided the current behavior is reasonable? Shouldn't we look for a case of a roundabout that is modeled like this and looks to be done correctly? ...the Logan circle was pretty much bad modeling.

@TheMarex
Copy link
Member

Capturing from chat with @danpat: He still thinks this is the right approach but need to revisit when he has time.

@danpat
Copy link
Member Author

danpat commented Oct 13, 2017

I chatted with @MoKob about this. Next step is to scan the planet for instances and see if this is even a legit way to model things - there's a nonzero chance that it's never correct to exit a roundabout more than once from a single node.

If it turns out that all instances of this are from poor modeling, this or should change into something that logs warnings during extraction.

@DennisOSRM
Copy link
Collaborator

closing stale PR. Reopen if still relevant.

@DennisOSRM DennisOSRM closed this May 10, 2024
@DennisOSRM DennisOSRM deleted the multi_exit_roundabout_nodes branch May 10, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants