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

Add exit rotary/roundabout #150

Merged
merged 1 commit into from
Sep 11, 2017
Merged

Add exit rotary/roundabout #150

merged 1 commit into from
Sep 11, 2017

Conversation

bsudekum
Copy link

Adds handling for new types exit rotary and exit roundabout.

This can be handled as a simple turn, no special instruction needed.

/cc @1ec5 @mcwhittemore

Copy link
Member

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

This looks good, assuming it’s safe to treat the exit instruction as a simple turn. What about throughabouts – does OSRM emit an exit instruction for those? If so, would “Go straight” be an appropriate exit instruction?

Would you mind adding to the changelog?

@bsudekum
Copy link
Author

Unsure about throughabouts. Thoughts @danpat @daniel-j-h?

@bsudekum
Copy link
Author

@1ec5 will update the changelog when we do a release. More items might go into the next release, unsure if it will be a 0.6.2 or 0.7.0 release.

@1ec5
Copy link
Member

1ec5 commented Sep 11, 2017

will update the changelog when we do a release.

Normally we maintain a running changelog with “master” or “unreleased” as the section heading.

What about throughabouts – does OSRM emit an exit instruction for those? If so, would “Go straight” be an appropriate exit instruction?

Apparently throughabouts get no instructions at all: Project-OSRM/osrm-backend#4333.

@bsudekum bsudekum merged commit e88b599 into master Sep 11, 2017
@bsudekum bsudekum deleted the exit_roundabout branch September 11, 2017 18:03
@mcwhittemore
Copy link
Contributor

@bsudekum - this just adds tests right because default should work here?

@bsudekum
Copy link
Author

@mcwhittemore yep. In the future if we'd like to add special handling for rotary/roundabout exits, we can.

@1ec5
Copy link
Member

1ec5 commented Sep 11, 2017

@bsudekum
Copy link
Author

@1ec5 I think it may have to do with: #148

Logging memo locally while running tests, I'm seeing:

ok 7320 other/motorway_ref_has_number_uturn.json/uk
Quẹo ngược lại {way_name}
Quẹo ngược lại Ref1
Quẹo ngược lại Ref1
Quẹo ngược lại Ref1
Quẹo ngược lại Ref1
Quẹo ngược lại Ref1
Quẹo ngược lại Ref1
Quẹo ngược lại Ref1
Quẹo ngược lại Ref1
ok 7321 other/motorway_ref_has_number_uturn.json/vi
调头上{way_name}
调头上Ref1
调头上Ref1
调头上Ref1
调头上Ref1
调头上Ref1
调头上Ref1
调头上Ref1
调头上Ref1
ok 7322 other/motorway_ref_has_number_uturn.json/zh-Hans
{ name: 'Cool highway', ref: 'Ref1' }
/Users/bobby/Documents/mapbox/osrm-text-instructions/index.js:205
                return memo.replace('{' + token + '}', tokens[token]);
                            ^

TypeError: memo.replace is not a function
    at /Users/bobby/Documents/mapbox/osrm-text-instructions/index.js:205:29
    at Array.reduce (native)
    at Object.tokenize (/Users/bobby/Documents/mapbox/osrm-text-instructions/index.js:203:47)
    at Object.compile (/Users/bobby/Documents/mapbox/osrm-text-instructions/index.js:153:32)
    at Object.keys.forEach (/Users/bobby/Documents/mapbox/osrm-text-instructions/test/index_test.js:194:49)
    at Array.forEach (native)
    at /Users/bobby/Documents/mapbox/osrm-text-instructions/test/index_test.js:192:51
    at Array.forEach (native)
    at /Users/bobby/Documents/mapbox/osrm-text-instructions/test/index_test.js:179:55
    at Array.forEach (native)```

Looking into this.

@1ec5
Copy link
Member

1ec5 commented Sep 11, 2017

Thanks, it looks like #148 collided with #149. Fixed in #152.

@1ec5
Copy link
Member

1ec5 commented Sep 14, 2017

will update the changelog when we do a release.

This didn’t happen. It can be helpful to add to the changelog as part of the PR, when the changes are top of mind.

/cc @bsudekum @mcwhittemore

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