search

maplibre / maputnik

An open source visual editor for the 'MapLibre Style Specification'
https://www.maplibre.org/maputnik
MIT License
2.02k stars 380 forks source link

Re-initialize map view after changing style #871

Closed zstadler closed 3 months ago

zstadler commented 5 months ago

Maputnik version: main Browser: Chrome, Firefox OS: Windows

Description of the bug: When changing style, the map view may contain features of the previous style

Steps to reproduce the behavior:

  1. Open Maplibre's maputnik in this location
  2. Open -> Gallery Styles -> Empty Style
  3. Reload the page to be on the safe side
  4. Open -> Open URL -> https://raw.githubusercontent.com/IsraelHikingMap/VectorMap/master/Styles/IHM.json
  5. The map view would look like this: image
  6. Open -> Gallery Styles -> OSM Liberty
  7. The map view would look like this, with a mix of both styles: image
  8. Reload the page
  9. The map view would look like this, without traces of the previous style: image
HarelM commented 5 months ago

Hmm... interesting, might be related to style diff capabilities or limitations.

zstadler commented 5 months ago

FWIW, cannot reproduce this with v1.7.0 at https://maputnik.github.io/editor/#13/31.27563/35.12306

HarelM commented 5 months ago

Looks like there's wrong handling of style update somewhere in the code. The following is reported in the console: image

In the past (1.7), the style diff was not able to perform the diff and the map was reloaded with the following warning: Unable to perform style diff: Unimplemented: setSprite.. Rebuilding the style from scratch. In recent version of maplibre-gl-js there is a more eager attempt to allow style diffing, but it might have bugs. So this might be a bug in maplibre and not maputnik. I'll need to see if I can create a minimal reproduction. Thanks for reporting this issue!

HarelM commented 5 months ago

Ok, so I tried to see if I can reproduce this on "plain" maplibre, and I was not able to. I also looked at the diff code in both maplibre-gl and maplibre-style-spec to see if I see some problems there and it is both covered with tests and runs as expected. So I dug deeper and it seems that the reason for this is the following:

  1. When replacing a style, the style diff method is called.
  2. This in turn removes all the layers and then removes the sources.
  3. After the first source is removed, an event of data is raised to let anyone who listens know that the source was removed.
  4. This apparently is picked up by mapbox-inspect which forces the original style back to the map.

mapbox-inspect is updating the map style here: https://github.com/lukasmartinelli/mapbox-gl-inspect/blob/dab82f2f83670bfeea93195da5db0f35dae99921/lib/MapboxInspect.js#L130

It might make sense since mapbox-inspect has its own style, but I'm not sure it should be applied when the mode in not "inspect" mode.

Bottom line, this is a maputnik related, I'm not sure I fully understand how to solve it though...

HarelM commented 4 months ago

Seems like https://github.com/lukasmartinelli/mapbox-gl-inspect is completely abandoned, not sure it's worth opening a PR there. @nyurik do you know if we can get access to that repo? The other option is simply copy the code into this repo and use it here, I'm sure it can be simplified dramatically to only support the use case of maputnik, maybe.

nyurik commented 4 months ago

We could ask @lukasmartinelli if he would be interested in transferring it here too - seems like a worthy component to maintain.

lukasmartinelli commented 4 months ago

Happy to transfer it!

nyurik commented 4 months ago

Awesome, thanks @lukasmartinelli! I think the easiest is to grant me or @HarelM admin rights on that repo, and we can do the rest. Note that we will probably want to continue publishing it under the original npm name, so may need to add harel as an admin there too. Thx!

nyurik commented 4 months ago

Actually, I just realized it has mapbox in its name - so perhaps we should rename it and/or fork it, and republish under maplibre name?

HarelM commented 4 months ago

Yeah, I was thinking about @maplibre/maplibre-inspect or something similar like we did with the other libraries. We can keep the original repo as is for legacy mapbox users.

HarelM commented 4 months ago

I can probably commit to maintaining it, as it is an integral part of maputnik, and I don't expect a lot of changes there (It's also like 5 files in total...). @nyurik will you do the honor of opening a PR to fork the repo under the maplibre umbrella?

nyurik commented 4 months ago

Yeah, perhaps it really does make sense to have an official fork that does not show up as a "github fork" (under the name at the top), but instead mentions in the readme that it was adapted from the other repo and renamed. There are not that many issues/pull requests it seems to do the transfer...

nyurik commented 3 months ago

I created https://github.com/maplibre/maplibre/issues/359 - I guess we should do a proper fork (without the link to upstream). Leaving it to @HarelM :)

HarelM commented 3 months ago

@zstadler let me know if you can reproduce this issue in latest version, I have deployed a temporary fix until we migrate maplibre-gl-inspect under maplibre org.

zstadler commented 3 months ago

@HarelM, The original steps to reproductions do not trigger this issue.

However, since inspect was mentioned above, I've tried the following steps, and the style does change as expected.

  1. Open Maplibre's maputnik in this location
  2. Open -> Gallery Styles -> Empty Style
  3. Reload the page to be on the safe side
  4. Open -> Open URL -> https://raw.githubusercontent.com/IsraelHikingMap/VectorMap/master/Styles/IHM.json
  5. Change to inspect view
  6. Open -> Gallery Styles -> OSM Liberty
  7. Change to map view
  8. The Israel Hiking style is shown rather than OSM Liberty
  9. Reload the page
  10. The OSM Liberty style map is shown
HarelM commented 3 months ago

The reason for the above, which is similar but different to the original issue is that the inspect library adds metadata to the style to indicate that this style is from the inspect library. When doing the diff, this flag is kept and the inspect library doesn't change the "original" style. So when turning off inspect, the original style is incorrect. Seems like there are a lot of assumptions in both maputnik and inspect that are preventing this from working correctly, I'm not sure I'll be able to solve those...

HarelM commented 3 months ago

After an archaeological digging I found some hacks that were added in order to fix some odd behavior between mapbox-gl and mapbox-gl-inspect. Here is the relevant link: https://github.com/maplibre/maputnik/blob/5f54dd0ccf34d205c8598186bd41eaae328f238b/src/components/MapMaplibreGl.tsx#L120L132

We now own maplibre-inspect and these should be fixed properly now in there, the problem is the issues they link to, and making sure everything is working as expected after the hacks are removed. In general there are some race conditions between setting the inspect style and regular style, and it causes a mayham.

Relevant issues to the above HACK comments: https://github.com/maplibre/maputnik/issues/576 https://github.com/maplibre/maputnik/issues/647

I'm not sure I know how to solve all this "properly" as it seems there's a fundamental problem with how maputnik works with the store and event drivern and how maplibre-inspect works - listening to map events.

HarelM commented 3 months ago

I hope both issues reported here are fixed now. I have tested both on the local dev machine and now the deployed site I can't reproduce the issues. Due to the complexity of the events and sync, the solution caused the transition to and from inspect is a bit more sluggish, but I'm OK with that assuming the data is correct.

zstadler commented 3 months ago

Well, almost...

The second scenario is leaves Maputnik in an inconsistent state

  1. Open Maplibre's maputnik in this location
  2. Open -> Gallery Styles -> Empty Style
  3. Reload the page to be on the safe side
  4. Open -> Gallery Styles -> OSM Liberty
  5. Change to inspect view
  6. Open -> Gallery Styles -> Positron
  7. The Positron style map is shown rather inspect view image
HarelM commented 3 months ago

In the above case, the sources are equal, so after the style change, which sets the "regular" map style, the inspect code, which listens to style changes compares the sources and doesn't apply the inspect style. I'll check what can be done, thanks for all the testing!!

HarelM commented 3 months ago

Hopefully the last patch I'm doing to fix these issues: #889

zstadler commented 3 months ago

LGTM!