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

Fix incorrect geometry index after advancing #357

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

Conversation

ahmedre
Copy link
Contributor

@ahmedre ahmedre commented Nov 12, 2024

After advancing the step, the geometry index sometimes incorrectly refers to an index in the previous RouteStep instead of the current one. This patch recomputes the geometry index when the RouteSteps change.

if let Some(current_route_step) = remaining_steps.first() {
// TODO: We could move this to the Route struct or NavigationController directly to only calculate it once.
let current_step_linestring = current_route_step.get_linestring();
self.snap_user_to_line(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we be passing the original location here from the caller and using that instead of the snapped location?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also found this to be a struggle to understand as I built the snapping task.

@ianthetechie the core state logic feels like an area of the core where some substantial commenting may help us all down the road. Basically to:

  1. Explain what params produce the major state changes like step advancement.
  2. To outline cases where unexpected params/vars are used (e.g. the different location vars).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, agreed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I probably won't get around to commenting up this before my week ends, but I haven't forgotten about it. Alternately if @ahmedre ends up putting more time into this and identifies missing comments, adding them to this PR would be great too. Otherwise I can tackle improving docs here next week.

@ahmedre
Copy link
Contributor Author

ahmedre commented Nov 12, 2024

I have no idea if this is the ideal fix or not, but I saw this case while testing, where, while logging:

               "metadata: " +
                    navigatingState.tripState.remainingSteps.size +
                    " remaining steps, current index; " +
                    navigatingState.tripState.currentStepGeometryIndex +
                    " geometry size " +
                    navigatingState.tripState.remainingSteps.firstOrNull()?.geometry?.size

I saw a case where the geometry index was greater than the geometry size (and, at that point, the current remaining steps size had decreased by 1). Looking at the code, it seems that there's no re-computation of the current step geometry index after the current step changes, causing this to be incorrect.

After advancing the step, the geometry index sometimes incorrectly
refers to an index in the previous RouteStep instead of the current one.
This patch recomputes the geometry index when the RouteSteps change.
@ahmedre ahmedre marked this pull request as ready for review November 12, 2024 19:26
@ahmedre
Copy link
Contributor Author

ahmedre commented Nov 13, 2024

CleanShot 2024-11-12 at 22 45 13@2x

Copy link
Contributor

@ianthetechie ianthetechie left a comment

Choose a reason for hiding this comment

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

It looks like this doesn't actually solve the problem 😅 I've added some tests.

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