search

maplibre / flutter-maplibre-gl

Customizable, performant and vendor-free vector and raster maps, flutter wrapper for maplibre-native and maplibre-gl-js (fork of flutter-mapbox-gl/maps)
https://pub.dev/packages/maplibre_gl
Other
185 stars 106 forks source link

added setStyle method on map controller #431

Closed itheamc closed 3 weeks ago

smallTrogdor commented 1 month ago

I am not a maintainer, but I have updated the style simply by passing a new style into the map itself - does this not work for you?

itheamc commented 1 month ago

I am not a maintainer, but I have updated the style simply by passing a new style into the map itself - does this not work for you?

I understand that directly passing a new style to the map widget seems convenient. However, this approach requires rebuilding the entire map, which can cause a performance hit due to dismissal and re-initialization.

smallTrogdor commented 1 month ago

This is not only convenient, but the standard way of interacting with and building widgets in flutter. If you look closely at what happens under the hood, you will actually notice that it does in fact not rebuild the entire view from scratch.

The view for both iOS and Android is created the first time you build the map widget. Afterwards, passing in new styles just updates these views.

In profile mode, the first time the map builds one can observe raster times times up to 16ms on a Galaxy S20, when I refresh the style, the times do not go above ~4ms.

Please correct me (or the maintainers) if I have gotten something wrong.

I do see however the setStyleString method and it not being used anywhere ...

So up to the maintainers to decide 👍 😄

josxha commented 1 month ago

Thank you for your pull request @itheamc. I agree that MapboxMapController.java needs to get formatted but could you do this in a separate pull request please? That way we have a commit history that is more clear, especially because the formatting results in more than 2k changed lines. I see that you changed the maplibre dependencies in pubspec.yaml to path depedencies. This should not be required because of the pubspec_overrides.yaml files. Did you had any problems used it that way?

@smallTrogdor Thanks for joining the discussion. It a nice find that you can simply swap out the widget parameter and let flutter do the rest. Although there are two reasons I'd like to add this functionality.

itheamc commented 1 month ago

Thank you for your pull request @itheamc. I agree that MapboxMapController.java needs to get formatted but could you do this in a separate pull request please? That way we have a commit history that is more clear, especially because the formatting results in more than 2k changed lines. I see that you changed the maplibre dependencies in pubspec.yaml to path depedencies. This should not be required because of the pubspec_overrides.yaml files. Did you had any problems used it that way?

@smallTrogdor Thanks for joining the discussion. It a nice find that you can simply swap out the widget parameter and let flutter do the rest. Although there are two reasons I'd like to add this functionality.

  • While the flutter state management is really awesome, especially when if comes to rebuilds of the widget tree. I strongly prefer to rely on the MapLibre state underneath and have the flutter wrapper "as stupid as possible". That way we can benefit from potential performance tweaks and keep the state management inside flutter as little as possible.
  • setStyle is part of the maplibre-native and maplibre-gl-js API (see web or android API docs). Because I see it as a long term goal to support all native API, it's one step further that goal.

Sure, I'll create a new pull request with well-defined commit messages for better tracking.

smallTrogdor commented 4 weeks ago

Alright, thanks for the insight! 👍

josxha commented 3 weeks ago

Closing this pr in favor of you new pull request. #433