flutter-mapbox-gl / maps

A Mapbox GL flutter package for creating custom maps
Other
1.04k stars 502 forks source link

Consider using the TextureRegistry in stead of PlatformViews #1098

Closed dnfield closed 2 years ago

dnfield commented 2 years ago

See some discussion on https://github.com/flutter/engine/pull/33599#discussion_r904324862

This plugin currently uses platform views to render a component which ends up basically just rendering to a texture. It should be more efficient (and generally provide better UX for your developers) to instead use the texture registry, which does not have as many restrictions or rough edges as platform views (e.g. thread merging requirements, weird bugs/edge cases around input and a11y, etc).

The linked bug points to some of how video_player does this. From a quick look at your Android code, this is not a super trivial refactor but it also doesn't look like a huge awful refactor either.

In particular, Flutter would like to drop support for the virtual display mechanism used in platform views that embed a texture or surface view, since it is error prone and difficult to maintain. This plugin was identified as one of the contributors/reasons why we need to continue to support it.

dnfield commented 2 years ago

Hmm. I'm going to look into why google_maps_flutter stopped using the texture registry a bit more. It looks like mapbox_gl is copying what google_maps_flutter did. I'm guessing the texture registry didn't support something necessary.

dnfield commented 2 years ago

Ok.

The google_maps_flutter plugin did originally use a texture, but because the Google Maps SDK never exposed a way to render to a surface, it was using a really slow method of capturing and rendering the map frame.

I'm not familiar enough with the API surface for mapbox_gl, but it seems like it should be possible to still supply the controller interface over to Flutter and use the texture API. This should result in better performance and fewer quirks, but it is probably not a simple experiment to do either.

Some extra considerations from the linked discussion:

MapView uses SurfaceView and TextureView https://github.com/mapbox/mapbox-maps-android/blob/main/sdk/src/main/java/com/mapbox/maps/MapView.kt

The SDK also includes a lot of other functionality which we would have to implement ourselves instead if we either rendered the MapView directly to a surface or used MapSurface, outside of an activity, I believe? Other Flutter Mapbox plugin projects don't necessarily use flutter-mapbox-gl. Navigation apps have their own implementations of flutter-mapbox-gl's functionality mainly because the Mapbox Navigation SDK implements the MapView directly and handles navigation activity through the view and expects other components to be available in the activity.

https://docs.mapbox.com/android/navigation/examples/turn-by-turn-experience/

Mapbox is pushing developers in the direction of using their activity as a drop in UI completely and not directly implementing individual components at all: https://docs.mapbox.com/android/navigation/guides/drop-in-ui/

Looking at the list of things which would need to be reimplemented if we used a texture registry separately and reimplemented the remaining components of the Navigation SDK, we might as well just write native applications because we would be doing the same amount of work either way. Maybe less work.

Am I reading this wrong or will we actually have to bypass the activity and all the other Mapbox Navigation widgets (which are not drawn on the GL surface) when pushing the GL surface, which is only part of the SDK functionality, through a texture view?

These aren't a play button and a seek bar. They're turn by turn directions (this thing is complcated with lane guidance and road signs and more) and distances and mute buttons and speeding indicators and navigation/overhead view switchers where the underlying code isn't directly exposed to us.

/cc @josephcrowell

AAverin commented 2 years ago

This would be something to be addressed by the core Mapbox team. Flutter Mapbox is a community effort bringing mapbox support to Flutter and we do not have any direct connection with the core mapbox development team. @tobrun should provide a bit more light to this topic. New version of Mapbox SDK that was rebuilt from ground up with Kotlin might work differently under the hood.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.