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

Turn instructions had wrong distance when using multiple segments #646

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tbsmark86
Copy link
Contributor

Split of from pull-request #641

The distance from the start for the a segment till the next turn is missing from the distance value of the last turn in the previous segment.
However it's not really that important right now because it's never visible to the user and only used when exporting to gpx in locus-style. (locus:rteDistance and locus:rteSpeed).

Your "distance-from-last" comment got me confused,

Yeah it's all really confusing :)

I've added more explanation to the code maybe this clears it up - or make it worse.

So voicehints of the expected test result brouterTotal.json

Ok. I've changed the test so the brouterTotal.json remains untouched.

should match the total download from the server (for this route).

Ah ok. I did not know that the server could handle multiple segments. So yes you're right it's also a backend bug. Consider this route without the first waypoint:
The distance for the first turn changes from 88 to 140. That's the almost the same as my calculated value of 141.

Is this important enough to also open a bug for brouter itself? Because unless someone is using the old export it should not affect a brouter-web user once this patch is merged?

@nrenner
Copy link
Owner

nrenner commented Oct 7, 2022

Thanks for splitting up and the clarifying explanation.

I think the goal should be that an exported route from brouter-web always produces basically the same result as the brouter server and app.

Therefore I would want to wait for the pending PR brouter#441, that changes segment-based handling to full route.

@nrenner nrenner added the wait-upstream Issue requiring upstream work label Oct 7, 2022
@tbsmark86
Copy link
Contributor Author

Hi again!

This is now fixed in upstream so it should be ready for merge now?
I've rebased my commit & updated the test export with a more recent brouter. Sadly there are now way more rounding errors then before.

Because the old test data had no time&energy field there is some new difference between client and brouter-app export.
Maybe also fix this in BR.Export._concatTotalTrack() ?

Don't know why github build is failing :/

As a reminder what this is about:

@afischerdev
Copy link

@tbsmark86
Thanks for your nice example, it gave me the chance to test it against the last library. I found a problem in voicehint calculation. Update is now running on test server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wait-upstream Issue requiring upstream work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants