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

Offset turn angle into +/-180 range correctly #2922

Closed
wants to merge 1 commit into from

Conversation

karenzshea
Copy link
Contributor

Issue

Fixes #2918. Turn angles were being subtracted from 180 before being passed into the lua turn_penalty function, which is possibly how the the old turn_penalty function expected them (essentially flipped, as an angle of 0 became 180 and an angle of 180 became 0). The new function expects values between -180 and 180.

h/t to @jakepruitt for help with debugging this

Tasklist

  • add regression test
  • review
  • adjust for for comments

Requirements / Relations

https://github.com/Project-OSRM/osrm-backend/milestone/16

Sorry, something went wrong.

@karenzshea
Copy link
Contributor Author

For the record, the graphed output of the new turn function looks more or less as expected; mirrored increase of penalties for sharper angles, slightly more expensive for left turns.

screenshot from 2016-09-15 17-05-22

const int32_t turn_penalty =
scripting_environment.GetTurnPenalty(180. - turn.angle);
scripting_environment.GetTurnPenalty(turn_angle);
Copy link

Choose a reason for hiding this comment

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

might I ask where this is coming from? As you can see in #2918, I have made tests using exactly 180-turn.angle and you get just the results we want.

From my tests an angle of 180 (resulting in 0 as call argument) is getting a turn penalty of zero.
From your new call value, we would get 7.4 seconds for going straight.
Osrm assumes 180 as angle for going straight. The turn function in the profile expects 0 for going straight.
The angle change is broken here.

@MoKob
Copy link

MoKob commented Sep 16, 2016

The obvious check here: https://travis-ci.org/Project-OSRM/osrm-backend/jobs/160304407#L3993 where you can see that https://github.com/Project-OSRM/osrm-backend/blob/master/features/bicycle/maxspeed.feature#L41-L48 is getting penalised now. Going straight didn't experience a penalty before, now it does.

@MoKob
Copy link

MoKob commented Sep 16, 2016

As described in #2918 (comment), we probably need a higher based penalty and probably additional penalties.

The turn function itself is correct though. OSRM simply has a different internal model than the external model to represent turn angles. A turn-angle in OSRM is modelled from 0 (u-turn) to 180 (going straight). Having the minimal penalty at 180, not at 0 due to 180-angle is wanted behaviour.

@MoKob MoKob closed this Sep 16, 2016
@MoKob MoKob deleted the fix/turn-angle branch September 16, 2016 06:13
@jakepruitt
Copy link
Contributor

Having the minimal penalty at 180, not at 0 due to 180-angle is wanted behaviour.

Just to clarify, is this 180 the number passed into the lua function? or the angle in the edge-based-graph-factory?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants