maplibre / maplibre-gl-js

MapLibre GL JS - Interactive vector tile maps in the browser
https://maplibre.org/maplibre-gl-js/docs/
Other
6.74k stars 726 forks source link

Paint property changed via setPaintProperty is well reflected right after via getPaintProperty but not via getStyle().layers. #3373

Closed DanielForniessoria-TomTom closed 7 months ago

DanielForniessoria-TomTom commented 1 year ago

maplibre-gl-js version: 3.6.1

browser: Chrome 119.0.6045.123 MacOS

Steps to Trigger Behavior

  1. Draw a line with a blue color using a GeoJSON source.
  2. Change e.g. a line-color property of a line layer rendering a GeoJSON LineString via setPaintProperty(YOUR_LAYER, "line-color", "red")
  3. Verify the change visually (line appears red)
  4. Verify the new color property via map.getPaintProperty(YOUR_LAYER, "line-color")
  5. [FAILS] Then verify that line-color property in map.getStyle().layers.filter(layer => layer.id == YOUR_LAYER) -> The line-color inside the paint object still shows "blue".

Link to Demonstration

The screenshot below showcases the property inconsistency:

Screenshot 2023-11-16 at 11 41 14

Expected Behavior

getPaintProperty should be consistent with map.getStyle().layers for the same layer for a paint property changed via setPaintProperty. The same should apply to getLayoutProperty, and setLayoutProperty (but didn't test that yet)

Actual Behavior

When setting a paint property via setPaintProperty, it's reflected back via getPaintProperty but not in map.getStyle().layers for that layer.

HarelM commented 1 year ago

Yes, I understand the issue now, and it does look like a bug. It's worth trying previous versions as one of the versions introduced some caching change to the style which might have caused this bug.

hutec commented 11 months ago

I'm unfortunately not confident enough to fix this bug, but noticed some differences between setFilter (where this bug does not appear) and setPaintProperty:

https://github.com/maplibre/maplibre-gl-js/blob/99d946a4993650db35f4668bc36927ee5214872e/src/style/style.ts#L1038-L1063

compared to

https://github.com/maplibre/maplibre-gl-js/blob/99d946a4993650db35f4668bc36927ee5214872e/src/style/style.ts#L1105-L1123

Especially the last two lines https://github.com/maplibre/maplibre-gl-js/blob/99d946a4993650db35f4668bc36927ee5214872e/src/style/style.ts#L1061-L1063 seem like they might be related to the bug.

hutec commented 11 months ago

It seems like a second (?) call to this._updateLayer is missing in setPaintProperty, because without touching the package code, I could get it work as expected by manually calling _updateLayer:

const layer = map.getLayer("Route");
map.style._updateLayer(layer);
HarelM commented 11 months ago

I'm not super familiar with this code as well, but according to the variable name, it looks like it checks if there's a need to relayout and only if so it calls update, so it might be a performance consideration, IDK...

ericboucher commented 9 months ago

We had the same problem on https://github.com/WFP-VAM/prism-app/pull/1101. thanks @hutec for the _updateLayer tip. This helped us simplify our logic quite a bit! I agree that this should be the default behavior though and seems like a bug.

kumilange commented 7 months ago

Hi, I looked into the code and it appears that the root cause lies in the style's _serializedLayers not being reset when setPaintProperty is called. It retains the old value when accessed by style serialization triggered by map.getStyle().

The _updateLayer function includes the reset, which explains why @hutec's suggestion works. I'm not entirely sure about the intention of specifying the cases(DataDriven and CrossFadedProperty) to call this._updateLayer with the requiresRelayout flag.

To address this issue without altering the original code, I think we can just add this line at the end: this._serializedLayers = null; This will reset the serialized layers.

I can quickly create a PR for this. Please let me know your thoughts.

HarelM commented 7 months ago

I believe this was introduced in

And possible related to

Feel free to open a PR with the relevant fix after understanding the above two linked items. Thanks!

kumilange commented 7 months ago

@HarelM I reviewed https://github.com/maplibre/maplibre-gl-js/pull/2306 https://github.com/maplibre/maplibre-gl-js/pull/3133, too, and considered possible scenarios where resetting _serializedLayers might be necessary, but within my understanding, I couldn't identify any such cases. The Style class's _validate method also calls serialize similarly to setState(https://github.com/maplibre/maplibre-gl-js/pull/3133), but I couldn't find a scenario that could potentially cause this bug.

So, I'll stick with my initial suggestion of adding code to reset _serializedLayers in setPaintProperty and have opened a PR!

However, as mentioned in this(https://github.com/maplibre/maplibre-gl-js/issues/2651), I'm not 100% sure that all possible cases have been addressed so far. If we encounter more bugs related to this lazy serialization(https://github.com/maplibre/maplibre-gl-js/pull/2306), it might be better to roll back to the original approach, which seems straightforward and less prone to bugs.

HarelM commented 7 months ago

I'll close this in favor of #2651 as I believe they are describing the same problem.