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

added setStyle method on map controller #431

Closed
wants to merge 2 commits into from

Conversation

itheamc
Copy link

@itheamc itheamc commented May 30, 2024

No description provided.

@smallTrogdor
Copy link
Contributor

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
Copy link
Author

itheamc commented Jun 1, 2024

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
Copy link
Contributor

smallTrogdor commented Jun 1, 2024

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
Copy link
Collaborator

josxha commented Jun 1, 2024

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.

@itheamc
Copy link
Author

itheamc commented Jun 2, 2024

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
Copy link
Contributor

Alright, thanks for the insight! 👍

@josxha
Copy link
Collaborator

josxha commented Jun 5, 2024

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

@josxha josxha closed this Jun 5, 2024
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.

None yet

3 participants