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 #27: navigation puck falls off route when panning #53

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

michaelkirk
Copy link
Collaborator

@michaelkirk michaelkirk commented Jun 3, 2024

Description

The bug was especially noticeable when panning quickly or zooming in
while navigating.

The issue is that the upstream method signature changed in
maplibre/maplibre-native@76469a0 and then again in
maplibre/maplibre-native@8ad5f9c

So our "overridden method" wasn't actually being called.

It's not the commit authors fault that it broke - this is a private
method we're calling.

A longer term fix might be to include some kind of public stable
delegate method rather than an override.

Before

Simulator.Screen.Recording.-.Screenshots.5.5.iPhone.8+.-.2024-06-03.at.12.25.39.mp4.mp4

After

Simulator.Screen.Recording.-.Screenshots.5.5.iPhone.8+.-.2024-06-03.at.12.26.44.mp4.mp4

Open Tasks

Infos for Reviewer

The bug was especially noticeable when panning quickly or zooming in
while navigating.

The issue is that the upstream method signature changed in
76469a0c2227927f03a15fb95b3a20f34becc78c and then again in
8ad5f9c1fd58c60b93d26c2819637fd8f2144fd1, which first appeared in the
ios-v6.0.0 tag.

So our "overridden method" wasn't actually being called.

It's not the commit authors fault that it broke - this is a private
method we're calling.

A longer term fix might be to include some kind of public stable
delegate method rather than an override.
@michaelkirk michaelkirk marked this pull request as draft June 3, 2024 20:11
@michaelkirk
Copy link
Collaborator Author

I just noticed there already is a delegate method: mapViewDidFinishRenderingFrame maybe we can use that instead?

Converted to a draft while I look a bit more.

@michaelkirk
Copy link
Collaborator Author

michaelkirk commented Jun 3, 2024

Since NavigationMapView is a subclass of MLNMapView, NavigationMapView.delegate is just MLNMapView.delegate. We probably can't utilize that delegate functionality within NavigationMapView because we want downstream users of NavigationMapView to be able to set their own mapView.delegate.

It's often a bit tricky to mix inheritance and delegation. We want some kind of interceptor pattern.

I'm sure we could come up with something, but it seems like the solution will be less than trivial, so let's start by just updating the private header for now. (re-opening the PR as is)

@boldtrn
Copy link
Collaborator

boldtrn commented Jun 7, 2024

Thanks for looking into this @michaelkirk. @mindthefish as you saw serious issues with this, would you like to give this a try if this also fixes #27? I will also give this a try 👍

Copy link
Collaborator

@boldtrn boldtrn left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this, I can verify that this fixes the issue.

@boldtrn boldtrn merged commit eaee92b into maplibre:main Jun 11, 2024
2 checks passed
michaelkirk added a commit to michaelkirk/maplibre-navigation-ios that referenced this pull request Aug 20, 2024
This header was copied in from MapLibre Native to expose a private
method.

Previously this got out of sync and caused a bug:
maplibre#53

Instead we can use the already public delegate method.

Note: The deleted header was importing Maplibre/Mapbox.h, but we are
using that elsewhere, so I moved that import up a level.
michaelkirk added a commit to michaelkirk/maplibre-navigation-ios that referenced this pull request Aug 20, 2024
This header was copied in from MapLibre Native to expose a private
method.

Previously this got out of sync and caused a bug:
maplibre#53

Instead we can use the already public delegate method.

Note: The deleted header was importing Maplibre/Mapbox.h, but we are
using that elsewhere, so I moved that import up a level.
hactar pushed a commit that referenced this pull request Sep 6, 2024
This header was copied in from MapLibre Native to expose a private
method.

Previously this got out of sync and caused a bug:
#53

Instead we can use the already public delegate method.

Note: The deleted header was importing Maplibre/Mapbox.h, but we are
using that elsewhere, so I moved that import up a level.
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.

Moving the map with active navigation leads to marker getting dragged off the route
2 participants