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

[Ready] Emit Merge for Motorways only #2657

Merged
merged 2 commits into from
Jul 30, 2016
Merged

Conversation

MoKob
Copy link

@MoKob MoKob commented Jul 14, 2016

The merge paradigm in its current implementation seems confusing.

In cases where we take a very slight turn onto a through street, the instruction is technically a merge onto a street.
However, the merge paradigm of merging to the left on a slight right turn seems to confuse more than it helps.

This PR changes the paradigm of merge to only consider motorway-like ramps for merging and emitting an instruction to go straight onto these merging situations.

It is based on #2586

@MoKob MoKob added the Guidance label Jul 14, 2016
@MoKob MoKob force-pushed the guidance/only-merge-motorways branch 4 times, most recently from 43fa8dd to 54f6c1a Compare July 15, 2016 14:01
@MoKob MoKob changed the title [wip] Guidance/only merge motorways [review] Guidance/only merge motorways Jul 15, 2016
}
else
{
const double constexpr MAX_COLLAPSE_DSTANCE = 30;
Copy link
Member

Choose a reason for hiding this comment

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

I missing in DSTANCE

@TheMarex TheMarex force-pushed the guidance/only-merge-motorways branch from 54f6c1a to c511d5e Compare July 26, 2016 10:16
@@ -62,6 +63,16 @@ inline void print( const extractor::guidance::Intersection & intersection )
std::cout << std::flush;
}

inline void print( const NodeBasedDynamicGraph &node_based_graph, const extractor::guidance::Intersection & intersection )
Copy link
Member

Choose a reason for hiding this comment

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

clang-format

@daniel-j-h daniel-j-h force-pushed the guidance/only-merge-motorways branch from c511d5e to 66e5af3 Compare July 27, 2016 13:28
@daniel-j-h
Copy link
Member

Resolved the two nitpicks I had with this.
@TheMarex can you have a last look, I think this is good to go in now.

@daniel-j-h daniel-j-h changed the title [review] Guidance/only merge motorways [Ready] Emit Merge for Motorways only Jul 28, 2016
// since `distance` does not refer to an actual distance but rather to the
// duration/weight of the traversal. We can only approximate the distance here
// or actually follow the full road. When 2399 lands, we can exchange here for a
// precalculated distance value.
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how 2399 helps here. On the contrary the distance here seems more correct?

@TheMarex TheMarex force-pushed the guidance/only-merge-motorways branch from a3c2880 to c471849 Compare July 30, 2016 21:55
@TheMarex TheMarex merged commit c471849 into master Jul 30, 2016
@MoKob MoKob deleted the guidance/only-merge-motorways branch August 8, 2016 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants