Fix #27: navigation puck falls off route when panning

michaelkirk commented 4 months ago

Description

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

The issue is that the upstream method signature changed in https://github.com/maplibre/maplibre-native/commit/76469a0c2227927f03a15fb95b3a20f34becc78c and then again in https://github.com/maplibre/maplibre-native/commit/8ad5f9c1fd58c60b93d26c2819637fd8f2144fd1

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

https://github.com/maplibre/maplibre-navigation-ios/assets/217057/6240dec0-7e48-41b4-be0b-2e3b281257cf

After

https://github.com/maplibre/maplibre-navigation-ios/assets/217057/9e6bcf5e-e25a-4fca-a241-1932bbf98f77

Open Tasks

Fixes #27

Infos for Reviewer

michaelkirk commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

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 👍

maplibre / maplibre-navigation-ios