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

[ready] move guidance decisions into profiles #2586

Merged
merged 1 commit into from
Jul 25, 2016
Merged

[ready] move guidance decisions into profiles #2586

merged 1 commit into from
Jul 25, 2016

Conversation

MoKob
Copy link

@MoKob MoKob commented Jun 24, 2016

With the list of advanced guidance features getting longer, the amount of decisions that has been hard-coded in advance grows.
While it is not possible right now to put all decisions into configuration files, at least some information can be listed.

This PR aims at moving as much as possible into the profiles to make guidance more configurable and adjustable for walking/biking/driving.

It is related to problems seen in:

@MoKob MoKob self-assigned this Jun 24, 2016
@MoKob MoKob force-pushed the features/guidance/profiles branch from c891a5e to a98f829 Compare June 27, 2016 13:39
TheMarex
TheMarex reviewed Jun 27, 2016
View reviewed changes
include/extractor/guidance/classification_data.hpp
LOW_PRIORITY_ROAD // a road simply included for connectivity. Should be avoided at all cost
};
// a class that behaves like a motorway (separated directions)
bool motorway_class : 1;
Copy link
Member

Choose a reason for hiding this comment

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

Bitfields and bool's are apparently not cross-platform compatible (read: non-gcc this will break). The easiest solution that I have seen is to use std::uint32_t for all members that get packed into a 32bit block.

@TheMarex
Copy link
Member

This is a very good idea. 👍

@MoKob MoKob force-pushed the features/guidance/profiles branch from d396c62 to 30df5fe Compare June 29, 2016 09:05
@MoKob MoKob mentioned this pull request Jun 29, 2016
Closed
5 tasks
@MoKob MoKob force-pushed the features/guidance/profiles branch from 30df5fe to da11fa0 Compare June 29, 2016 09:22
@MoKob MoKob changed the title [WIP] move guidance decisions into profiles [review] move guidance decisions into profiles Jun 29, 2016
@MoKob MoKob changed the title [review] move guidance decisions into profiles [wip] move guidance decisions into profiles Jun 29, 2016
@MoKob MoKob changed the title [wip] move guidance decisions into profiles [review] move guidance decisions into profiles Jun 29, 2016
@MoKob
Copy link
Author

MoKob commented Jun 29, 2016

One problem I see right now is the road priorities. I've provided enums to give an indication of how they are used right now and what kind of distinctions are made.
The addition of roads for bicycles already improves some behaviour which is currently far from optimal (e.g. #2527), while maintaining same behaviour on cars.

In upcoming PRs (#2113), we will have to address further issues and potentially revisit the exposed values.

TheMarex
TheMarex reviewed Jul 1, 2016
View reviewed changes
docs/profiles.md

## Guidance

The guidance parameters in profiles are currently a work in progress. They can and will change without any major version change.
Copy link
Member

Choose a reason for hiding this comment

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

This is not how semantic versioning works. If we break the API we need to do a version bump. Please remove this paragraph.

@TheMarex
Copy link
Member

TheMarex commented Jul 1, 2016

Looks good to me. But needs a changelog entry because the profiles now depend on a new file guidance.lua which needs to be added to our profile update scripts (like in node-osrm).

@MoKob MoKob force-pushed the features/guidance/profiles branch from 9850100 to 7372392 Compare July 1, 2016 09:28
@MoKob MoKob changed the title [review] move guidance decisions into profiles [ready] move guidance decisions into profiles Jul 1, 2016
@TheMarex TheMarex added this to the 6.0 milestone Jul 1, 2016
This was referenced Jul 4, 2016
Merged
Closed
daniel-j-h
daniel-j-h reviewed Jul 6, 2016
View reviewed changes
include/extractor/guidance/road_classification.hpp
// the road priority is used as an indicator for forks. If the roads are of similar priority
// (difference <=1), we can see the road as a fork. Else one of the road classes is seen as
// obvious choice
RoadPriorityClass::Enum road_priority_class : 5;
Copy link
Member

Choose a reason for hiding this comment

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

If we bitpack this, it's a good idea to static assert on the size

Copy link
Author

Choose a reason for hiding this comment

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

done

@MoKob MoKob force-pushed the features/guidance/profiles branch 2 times, most recently from 6fc502b to 48dc131 Compare July 6, 2016 15:36
@MoKob MoKob mentioned this pull request Jul 8, 2016
Merged
@MoKob MoKob force-pushed the features/guidance/profiles branch from 48dc131 to b4dc10e Compare July 14, 2016 14:01
@MoKob MoKob changed the title [ready] move guidance decisions into profiles [ready-ish] move guidance decisions into profiles Jul 14, 2016
@MoKob
Copy link
Author

MoKob commented Jul 14, 2016

With the current rebase, some of the anticipate lane-tests are failing (even though they should be mostly unaffected).

Holding off on fixing this until #2642 and similar by @daniel-j-h have landed.

@MoKob MoKob mentioned this pull request Jul 14, 2016
Merged
@MoKob MoKob force-pushed the features/guidance/profiles branch 3 times, most recently from 2998dfa to 43b480f Compare July 15, 2016 13:10
@MoKob MoKob changed the title [ready-ish] move guidance decisions into profiles [ready] move guidance decisions into profiles Jul 15, 2016
@MoKob MoKob force-pushed the features/guidance/profiles branch 4 times, most recently from 2fedd39 to 837f4cd Compare July 22, 2016 12:23
@TheMarex TheMarex force-pushed the features/guidance/profiles branch from 837f4cd to 0de22a2 Compare July 22, 2016 16:05
@TheMarex
Copy link
Member

Rebased on master, which results in broken cucumber tests.

@TheMarex TheMarex changed the title [ready] move guidance decisions into profiles [not ready] move guidance decisions into profiles Jul 22, 2016
@TheMarex
Copy link
Member

Never mind. This seems to have been some broken local caching.

@TheMarex TheMarex changed the title [not ready] move guidance decisions into profiles [ready] move guidance decisions into profiles Jul 25, 2016
@TheMarex TheMarex modified the milestones: 5.4.0, 6.0 Jul 25, 2016
@TheMarex TheMarex force-pushed the features/guidance/profiles branch from 0de22a2 to f501507 Compare July 25, 2016 11:07
@TheMarex TheMarex force-pushed the features/guidance/profiles branch from f501507 to 1fc63e1 Compare July 25, 2016 11:08
@TheMarex TheMarex merged commit 1fc63e1 into master Jul 25, 2016
@TheMarex TheMarex deleted the features/guidance/profiles branch July 25, 2016 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@MoKob MoKob

Labels
Guidance Profile Refactor
Projects
None yet
Milestone
5.4.0
Development

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