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

Move PostProcessing onto Route Steps #2116

Merged
merged 4 commits into from
Mar 23, 2016

Conversation

MoKob
Copy link

@MoKob MoKob commented Mar 18, 2016

This pull request tracks the progress of moving the Guidance Post-Processing options onto the Route Step.

It consists of the following task:
-[]Adjust turn collapsing in assemble_leg.hpp: This process assumes name-ids to be the only decision that decides whether a turn is kept or not. This needs to be adjusted to compress only no-turns. Some instructions are actually required to stay on a road.
Since this is only a heuristic for the route name and would require a lot of changes, keeping the name as only source of naming the route seems like the better option.

  • Move Post Processing away from PathData and past assemble_steps.cpp to work on route-steps. This would require the insertion of temporary route steps for silent instructions that can then be collapsed, or a full integration of the post-processing with assemble_steps. I'd prefer the former (@TheMarex, whats your take on this?)
  • Post-Process Temporary Route Steps: Due to the adjustment above, some silent instructions might remain in the route-steps. Post-processing has to combine these accordingly.
    • we have to count exits for roundabouts
    • In addition, we can add information on intersections in between the instructions. Every silent turn can be added to a list of intersections with respective distances along the current step geometry (thank you for the proposal @TheMarex)
  • clean up silent instructions -- remove silent instructions from output to prevent them from being shown in the json result.
    • remove silent instructions from output
    • potentially sync leg-geometry with new instructions?
  • static turn instructions #2080 needs to be merged prior to this
  • find out why path length and segments do not match
  • get @daniel-j-h 's blessing

@MoKob MoKob force-pushed the fix/guidance/postprocessing branch from 7cce748 to ebf9555 Compare March 21, 2016 17:07
@MoKob
Copy link
Author

MoKob commented Mar 22, 2016

@TheMarex I have a question regarding the leg_geometry.

After https://github.com/Project-OSRM/osrm-backend/blob/fix/guidance/postprocessing/src/engine/guidance/post_processing.cpp#L232-L241, which removes temporary instructions in post-processing to count roundabouts/intersections, the leg-geometry segments are out of sync with the actual turn-instructions.

Should we undertake the effort to set the correct indices again? Essentially translating the turn-instruction geometry indices back into the segments of the geometry? The coordinates itself should be unaffected.

@MoKob MoKob force-pushed the fix/guidance/postprocessing branch from 5c28605 to db6f9b9 Compare March 22, 2016 12:47
@MoKob
Copy link
Author

MoKob commented Mar 22, 2016

An additional item to discuss: to output the intersections in between, should we output the duration/distance along the currently looked at segment, or the distances/durations between the different intersections themselves?

So for (S) --a -- (I) -- b -- (I2) -- c -- (T), should we rather output:

Option 1: Depart S, (a+b+c), Intersections: [(I,a), (I2,a+b)]

or

Option 2: Depart S, (a+b+c), Intersections: [(I,a), (I2,b)]

In both cases, we would end up adding some additional information down the line on the intersection shape.

Edit: currently option 2 is implemented, both come with coordinate of the intersection

@MoKob MoKob force-pushed the fix/guidance/postprocessing branch from 40efa38 to 4f15498 Compare March 22, 2016 14:00
@daniel-j-h daniel-j-h force-pushed the fix/guidance/postprocessing branch 3 times, most recently from 057eb59 to ca4c895 Compare March 22, 2016 14:21
const LegGeometry &leg_geometry,
const std::size_t segment_index,
const unsigned exit);
const std::size_t segment_index);
Copy link
Member

Choose a reason for hiding this comment

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

From what I can see I think you only need the instruction to pass it on (and you do not move it), so a const-ref (or const value since it should be cheap to copy it) should be fine, too.

@MoKob MoKob force-pushed the fix/guidance/postprocessing branch from be4ad82 to 5487542 Compare March 22, 2016 16:46
@MoKob
Copy link
Author

MoKob commented Mar 22, 2016

Might be related: #1575

@MoKob MoKob force-pushed the fix/guidance/postprocessing branch from 5487542 to 05c8834 Compare March 23, 2016 08:58
@MoKob
Copy link
Author

MoKob commented Mar 23, 2016

Path length in front-end are rounded to large differences (426->450). This results in the differences in the path data length I've seen. The values seem correct, though.

@MoKob MoKob changed the title [WIP] Move PostProcessing onto Route Steps Move PostProcessing onto Route Steps Mar 23, 2016
@MoKob MoKob force-pushed the fix/guidance/postprocessing branch from 2ad5b42 to 7a925da Compare March 23, 2016 11:05
@MoKob
Copy link
Author

MoKob commented Mar 23, 2016

@TheMarex this one is all yours

@TheMarex
Copy link
Member

@MoKob looks great! Going to rebase and merge.

@TheMarex TheMarex force-pushed the fix/guidance/postprocessing branch from 7a925da to b5b5dbc Compare March 23, 2016 16:14
@TheMarex TheMarex merged commit b5b5dbc into rewrite/new-api Mar 23, 2016
@TheMarex TheMarex deleted the fix/guidance/postprocessing branch March 23, 2016 17:07
@1ec5 1ec5 mentioned this pull request May 4, 2016
6 tasks
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