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.

Sign up for GitHub

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

Jump to bottom

static turn instructions #2080

Merged
merged 6 commits into from
Mar 18, 2016
Merged

static turn instructions #2080

merged 6 commits into from
Mar 18, 2016

Conversation

MoKob
Copy link

@MoKob MoKob commented Mar 10, 2016

For this branch to be merged, the following needs to happen:

  • do not expose the internal conflict resolution
  • do not expose new name on near identical names
  • add road classes (e.g. primary vs residential) to fork detection
  • clean-up response to only return valid turn candidates

- [ ] add cucumber tests for static turn instructions
- [x] motorways
- [ ] fork
- [ ] end of road
- [ ] new name
- [ ] suppressed
- [ ] continue
- [ ] merge
- [ ] ramp

  • test roundabouts
    • enter
    • immediately exit
    • enter and exit
    • only exit
  • review, I am looking at you @daniel-j-h

TheMarex
TheMarex reviewed Mar 10, 2016
View reviewed changes
include/extractor/guidance/turn_instruction.hpp
@@ -43,8 +43,14 @@ enum TurnType // at the moment we can support 32 turn types, without increasing
NewName, // no turn, but name changes
Copy link
Member

Choose a reason for hiding this comment

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

Can we merge NewName and Notification? Notification just tells the user that a street attribute (like name or travel mode) changed. This would be forward compatible if we add more attributes for which we might want to emit a Notification. Also this is not exclusive, no? The street name and the mode can change at the same time.

/cc @karenzshea

Copy link
Author

Choose a reason for hiding this comment

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

Right Now Notification is Unused due to data not being exposed. I wouldn't worry about that one right now. The moment we get more data into the system, we have to open a discussion of what to expose.

@TheMarex TheMarex force-pushed the features/guidance-features-5-0 branch from 32f6ff2 to ca19cbb Compare March 13, 2016 03:37
@TheMarex
Copy link
Member

⚠️ rebased on the new rewrite/new-api.

@karenzshea karenzshea added this to the OSRM 5.0 milestone Mar 15, 2016
@TheMarex TheMarex force-pushed the features/guidance-features-5-0 branch from c49a099 to a719be9 Compare March 15, 2016 20:28
@TheMarex
Copy link
Member

⚠️ rebased on rewrite/new-api.

@MoKob MoKob force-pushed the features/guidance-features-5-0 branch 2 times, most recently from 94371a4 to d2ac23c Compare March 17, 2016 13:10
@MoKob MoKob force-pushed the features/guidance-features-5-0 branch from 1d29716 to 167bc44 Compare March 18, 2016 09:08
daniel-j-h
daniel-j-h reviewed Mar 18, 2016
View reviewed changes
include/extractor/guidance/toolkit.hpp
#include <map>
#include <cmath>
#include <cstdint>
#include <string>
#include <iostream>
Copy link
Member

Choose a reason for hiding this comment

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

Can be removed?

@MoKob MoKob mentioned this pull request Mar 18, 2016
Closed
10 tasks
@MoKob
Copy link
Author

MoKob commented Mar 18, 2016

Moved the remaining tests here: #2114

@MoKob MoKob changed the title [wip] static turn instructions static turn instructions Mar 18, 2016
@MoKob MoKob force-pushed the features/guidance-features-5-0 branch from b84a3c0 to 5d39e7d Compare March 18, 2016 13:41
@MoKob MoKob force-pushed the features/guidance-features-5-0 branch from 5d39e7d to 7cce748 Compare March 18, 2016 13:46
@MoKob MoKob mentioned this pull request Mar 18, 2016
Merged
10 tasks
@TheMarex
Copy link
Member

⚠️ rebasing on rewrite/new-api.

@TheMarex TheMarex force-pushed the features/guidance-features-5-0 branch from b9cea74 to 45289ce Compare March 18, 2016 16:04
TheMarex
TheMarex reviewed Mar 18, 2016
View reviewed changes
include/util/name_table.hpp

public:
NameTable(const std::string &filename);
std::string get_name_for_id(const unsigned name_id) const;
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be CamelCase.

@TheMarex TheMarex force-pushed the features/guidance-features-5-0 branch from 45289ce to 88db0ba Compare March 18, 2016 18:13
@TheMarex TheMarex force-pushed the features/guidance-features-5-0 branch from 5861db0 to 497e415 Compare March 18, 2016 18:29
@TheMarex
Copy link
Member

Okay this is in good shape now. Good work @MoKob! Merging back to the rewrite branch.

@TheMarex TheMarex merged commit 497e415 into rewrite/new-api Mar 18, 2016
@TheMarex TheMarex deleted the features/guidance-features-5-0 branch March 18, 2016 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
OSRM 5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@MoKob @TheMarex @daniel-j-h @karenzshea