map.SetStyle(...) conflicts with custom render layers

[measuredweighed]

maplibre-gl-js version: 3.0

browser: Safari, Chrome (likely all)

Steps to Trigger Behavior

1. Set a custom style using map.setStyle(...)
2. Add a custom render layer like so: maplibre.on("style.load", () => { maplibre.addLayer(customLayer); });
3. Change the map style using map.setStyle(...) again

Link to Demonstration

This behaviour can be seen live on https://planefinder.net. Tap the settings icon in the top right and toggle Map Appearance in the sidebar that appears (switching between "Road" and "Satellite"). Notice that the yellow aircraft marker layer is hidden by the appearance of the new style.

Expected Behavior

We'd expect the style to switch without impacting our custom render layer.

Actual Behavior

The custom render layer appears to be overlapped by the layers loaded from the new style. This approach has previously worked for MapLibre versions < 3.0 (and before that, Mapbox). I've attached a video showcasing the issue.

https://github.com/maplibre/maplibre-gl-js/assets/1234600/7ea68d7d-2a22-4f44-b7ae-9eb16396dd0c

Addendum

Thank you for such a wonderful 3.0 release. It's wonderful to see the MapLibre project thriving. You've all done incredible work.

[razorness]

Facing same issue. Looks like style.load is not getting fired on map instance.

https://github.com/maplibre/maplibre-gl-js/blob/bc64a2833e69f0cb1c10a717df3f82bf18e730f4/src/style/style.ts#L329

I have a small library for Vue bindings with a style switch. All custom sources and layers add them self on map on style.load event.

Here is a branch for reproduction: https://github.com/razorness/vue-maplibre-gl/tree/v3-style.load-not-fired

• Checkout branch
• exec: yarn && yarn dev

The magic happens in src/lib/composable/useSource.ts:

https://github.com/razorness/vue-maplibre-gl/blob/7731bd070468a505e55d9d85fff1cf9375b03b12/src/lib/composable/useSource.ts#L32

and

https://github.com/razorness/vue-maplibre-gl/blob/7731bd070468a505e55d9d85fff1cf9375b03b12/src/lib/composable/useSource.ts#L19-L25

I added a simple console.log to get some logging without disturbing tmings etc.

https://github.com/razorness/vue-maplibre-gl/blob/7731bd070468a505e55d9d85fff1cf9375b03b12/src/lib/composable/useSource.ts#L20

[HarelM]

Can you maybe try and narrow down the version in which this stopped working properly? We have released some pre-releases along the way in order to find those issues sooner, these can help narrow down when this issue started.

[razorness]

Well, yes. I will test this tomorrow.

[leearmstrong]

The 3.0.0-pre.3 release was where it first started to break

[HarelM]

I don't see anything that looks related to it on the changelog. Can you create a small reproduction in jsbin or stackblitz please?

[razorness]

@HarelM in v3.0.0-pre.3 it gets buggy. In v3.0.0-pre.5 it stops working execpt for initial style load.

Video is made with v3.0.0-pre.3:

https://github.com/maplibre/maplibre-gl-js/assets/684302/50bffa5f-6503-4e45-969b-8297d407f73b

[HarelM]

Can you send a jsbin please?

[razorness]

It's currently set on v3.0.0-pre.3.

https://jsbin.com/segawecico/1/edit?html,output

[acalcutt]

I also seem to have this issue. I can confirm v3.0.0-pre.2 works but beyond that it didn't seem trigger style.load when the style changed.

for example, in this v3 page you can see all the trails at the start, but when you switch styles at the bottom it looks like 'style.load' never triggers so the trails don't get reloaded. https://wifidb.net/demo/trails/testing/trails3.html

where if I do the same thing in v2.4.0 it works fine, the trails reload https://wifidb.net/demo/trails/testing/trails2.html

I added some console.logs for when the style.load triggers, but on the v3 map it only seems like it gets triggered on the first load

[HarelM]

It might be related to the changes in the ability to load multiple sprites, I'm not sure though, there aren't many changes between pre.2 and pre.3... cc: @smellyshovel @acalcutt if you can create a test here to simulate this it would be much easier to solve, I think.

[acalcutt]

Ya, I can work on a simpler example later tonight. I added the ones I provided in slack to my last post for now.

EDIT: actually a test... will have to look into that.

[HarelM]

It's less about creating a jsbin, because we already have that, but more of creating a unit test here and make sure this passes when the issue is solved.

[acalcutt]

I was trying to make a test for this but I haven't had any luck.

my last attempt was adding something like this to src\ui\map.test.ts in the v3.0.0-pre.2 commit where this was still working

        test('emits style.load event each time SetStyle', done => {
            const map = new Map({container: window.document.createElement('div')} as any as MapOptions);

            map.setStyle(createStyle());
            map.on('style.load', test1);

            function test1() {
                map.off('style.load', test1);
                map.on('style.load', test2);
                map.setStyle(createStyle());
            }

            function test2() {
                done();
            }
        });

I also tried something like the 'sets up event forwarding' test where it counted events, but i'm not quite sure how to make it wait for the first 'style.load' to finish before trying it again.

[HarelM]

I assume this kind of test already exists and probably passing, so there's probably something a bit more complicated that's causing this, if I needed to guess. I would try and reduce the style to a minimal style that still cause this behavior, once this is done coding it to a tests should be fairly easy, I hope...

[neodescis]

When is "style.load" actually supposed to fire? Is it possible that it's not being fired here because what used to cause a full style reload is now handled by just an incremental update?

[HarelM]

It might be possible, I'm not sure... Possible offenders: #1805 #1824 Might be related to: #2651

[neodescis]

When debugging through things, it appears that style.load is only fired during a full reload. Watching for the styledata event instead, it fires on any update, but it often fires before isStyleLoaded() returns true. This has been a pain point for me for a while (even before the fork from mapbox), and there have been various issues filed around it.

[neodescis]

I have another data point. I have some unit tests that depend on the styledata event that are failing after upgrading to 3.x. It seems that the event is also not fired when calling setStyle() with only a change to the glyphs definition.

[igal1c0de4n]

Greetings from https://mapme.com 🌍 🤓
Amazing work on V3. We can't wait to start using it 🙏🏼

Unfortunately this issue blocks our upgrade.

We serve maps with a style-toggle button, for example: https://viewer.mapme.com/56c92070-fbbe-44f5-8c12-98311016e368 https://viewer.mapme.com/c69deaf3-4575-40ac-9ed5-075590c4d8b2

When using maplibreglV3 - clicking this button no longer toggles the style due to the missing style.load event callback.

Is there any way we can help?

[HarelM]

In this thread there are the possible offenders that cause this regression. If you could take a look and maybe narrow down to the commit that caused this issue it could help a lot. Also if you could write a very simple test that can reproduce this issue in this repo it can also help to push things forward in terms of PR and help make sure this won't happen in the future. THANKS! :-)

[acalcutt]

I tested building a few of the commits after v3.0.0-pre.2, and it seems to break for me at https://github.com/maplibre/maplibre-gl-js/pull/1805 https://github.com/maplibre/maplibre-gl-js/commit/66e1009d30c24f83b280faad53dac3aed7cc5b8b https://wifidb.net/demo/trails/testing/trails3_1805.html (changing style does not trigger trails to be reloaded)

If I build the previous commit #1992 https://github.com/maplibre/maplibre-gl-js/commit/05096ba68769ec7ce1591a61de918359a4e7e52e it still works https://wifidb.net/demo/trails/testing/trails3_1992.html (changing style works)

[HarelM]

@smellyshovel any chance you could take a look at this?

[igal1c0de4n]

Adding code to reproduce the issue: https://codepen.io/igal1c0de4n/pen/MWzogJa

It displays a button to switch styles. The first setStyle sends style.load as expected. when using:

https://unpkg.com/maplibre-gl@3.0.0-pre.3/dist/maplibre-gl.js

The 2nd button click doesn't, and the test shows style.load missing

FYI after downgrade to "pre 2" version:

https://unpkg.com/maplibre-gl@3.0.0-pre.2/dist/maplibre-gl.js

the event is received multiple times as expected.

Credit to https://github.com/Hanzila12 thanks for creating this test!

[HarelM]

@igal1c0de4n would you like to try and fix this issue as well?

[igal1c0de4n]

Hi We started to look into it, not yet close to submitting a fix we're confident about.

I can share that after analyzing #1805 we discovered the issue involves this code:

    _updateDiff(style: StyleSpecification, options?: StyleSwapOptions & StyleOptions) {
        try {
            if (this.style.setState(style, options)) {
                this._update(true);
            }
        } catch (e) {
            warnOnce(
                `Unable to perform style diff: ${e.message || e.error || e}.  Rebuilding the style from scratch.`
            );
            this._updateStyle(style, options);
        }
    }

It seems that style.load is rcvd when the catch is triggered. However, for the optimal execution path: when the catch isn't entered and the style isn't forcefully loaded - the line:

        this.fire(new Event('style.load'));

is simply never reached. We were thinking about adding it at the end or after

this._update(true);

However we didn't formulate a change we're confident enough to merge.

@HarelM do you have any recommendation or preferences re our direction/process?

[HarelM]

Looking at the code a bit more with this insight in mind, it seems that it has a brain split. Some part of the diff is handled in the map area of the code and some is handled at the style part of the code. So no wonder there's not a single point of responsibility. I would recommend refactoring it so either the map is responsible for raising this event or the style, but not both - meaning - who ever is responsible for the process end-to-end should raise the event when the process is completed. I'm not sure the above helps solving this issue. Let me know if there is more clarification needed.

[igal1c0de4n]

@HarelM Are you saying that style.load firing needs to be moved from current location: https://github.com/maplibre/maplibre-gl-js/blob/main/src/style/style.ts#L368 to the end of setStyle: https://github.com/maplibre/maplibre-gl-js/blob/main/src/ui/map.ts#L1685 ? If so, we can prepare and test this change

[HarelM]

I think so, but I don't have all the context of the flow of setStyle so you'll need to keep me honest here...

[igal1c0de4n]

Looking at the setStyle code:

    setStyle(style: StyleSpecification | string | null, options?: StyleSwapOptions & StyleOptions) {
        options = extend({},
            {
                localIdeographFontFamily: this._localIdeographFontFamily,
                validate: this._validateStyle
            }, options);

        if ((options.diff !== false && options.localIdeographFontFamily === this._localIdeographFontFamily) && this.style && style) {
            this._diffStyle(style, options);
            return this;
        } else {
            this._localIdeographFontFamily = options.localIdeographFontFamily;
            return this._updateStyle(style, options);
        }
    }

JS Promises aren't used - instead it relies on internal callbacks. Firing style.load from this function once the all the callbacks complete is... well, difficult (if to be gentle).

@HarelM Did you mean it's necessary to refactor the called functions _diffStyle and _updateStyle to return Promises? If not, I don't quite follow how style.load can be fired exclusively from the setStyle function.

btw, we still need to fire style.load on map initialization - so at least one other location which fires style.load is needed.

[HarelM]

Well, we would like to move to a more promise based code, but I don't know if this is the scope of this issue. The public API (i.e the map API) should still use callbacks as I don't want to change it as part of this fix as it will require a major version change.

I don't know if promises are needed or if it's not possible to do it with the current way things work, I just commented on a brief look at the code and where the event is fired vs who manages the flow.

If you think it's too big of a change for this bug fix and you think a patch is in order here that's fine too, but I would still want to know how to properly solve this even if we do not do it at this point in time.

[neodescis]

@igal1c0de4n, it sounds like you've confirmed my suspicion that "style.load" never fires for a differentially-updated style. Pre-fork, at one time the mapbox developers asserted that "style.load" was a "private" event and that "styledata" should be used instead (reference, reference), but eventually it became part of the API surface (reference). Still, they haven't really landed on how to truly notify when the style is actually loaded (reference).

I've been able to successfully watch for style loads and changes using the "styledata" event (which originates in the style and is re-broadcast by the map), with the caveat that I then turn around and wait (using setInterval()) for map.isStyleLoaded() to return true. It's certainly not great, but it works.

Unfortunately the way the style is loaded and updated is very convoluted. It would be great to centralize the logic, but I believe the scope of doing so is large, and there are many different scenarios to consider given its asynchronous nature. To me, it would make sense for the style to emit a "load" event and have the map re-broadcast it, similar to how "styledata" works.

[HarelM]

That's interesting, thanks for sharing. When I worked on the event in the docs I found out that style.load is not really documented, so this might explain why. We did improve the situation around isStyleLoaded recently in a PR, so it might be worth continuing the notion of keeping the style.load private, or remove it complety. IDK... These event are so complicated and the callback hell is a nightmare to follow. I think the best approach is to continue the effort of converting the code to promises or introducing some state manager to solve this mess... But that's a bit off topic...

[igal1c0de4n]

I reviewed only the setStyle and sytle.load associated internals, these are my 2c: Firstly I agree it's a massive callback spaghetti.

The code can be greatly simplified and maintainability can be improved if refactored to use the js Promises.

From my pov switching to promises doesn't mean changes are necessary in the official API. The API can remain callback-based while the async operations under the hood are implemented with promises. Thou as a side note, Mapme developers (and probably others) will appreciate a new promises-friendly API.

Re style.load: I'm honestly amazed this was and still is an internal/unofficial event. I see style.load as one of the most useful and beneficial events for us at Mapme. We'd be toast without it. We'd need to revert to using complex app heuristics which wait for the somewhat irrelevant styledata event then poll for isStyleLoaded, as @neodescis described. Even then, the approach fails to guarantee all parts of the style really did complete loading: certain API calls throw an error, which doesn't happen if we wait for style.load before executing those APIs.

From Mapme's pov fixing sytle.load is high priority. Furthermore - we request it becomes an official part of the API.

Moving forward: Mapme might be able to produce a change which gets the style.load event to fire at the right timing after each setStyle is called. At the same time, given the unintuitive state of the internals - I would relate to such a change as a very dirty workaround/hack and consider it very temporary.

@HarelM This might be a "cart before the horse" question, still: Is there an option to leverage this thread and influence internal maplibregl dev team milestones so that asetStyle code refactor is planned?

[HarelM]

I'm not sure I fully understand the question. I would prefer a fix which involves big refactoring instead of a temporary fix/hack. If this event is critical to you I believe you should invest time in solving it properly. I don't mind making it official as part of this effort. There was an initial attempt to introduce promises here #2121 but it wasn't completed unfortunately. Bottom line, the ball is in your court. 😀

[igal1c0de4n]

Update We've tried to assign an internal developer to work on this issue, however due to being short-staffed this is a challenge.

As a result, despite having a better understanding of the issue and a general idea re the fix - we actually haven't made much progress.

Mapme is still willing to contribute resources to solve this issue.

We offer to work with an experienced maplibregl developer to solve it and cover the development cost.

If anyone is interested - please reply or contact at igal@mapme.com

[HarelM]

Thanks for the generous offer @igal1c0de4n! CC: @birkskyum @neodescis @rotu @astridx @kajkal @chippieTV

[neodescis]

I would love to see this done. I can take a look at it and see if I can come up with a strategy, but not for about a week. If anyone else wants to take it before I can get to it, feel free.

[chippieTV]

hi there.. so just reading through the issue, there was an error that was mentioned a few comments ago relating to it trying and failing the diff..

Unable to perform style diff: ${e.message || e.error || e}. Rebuilding the style from scratch.

(I also saw this in the console when loading the jsbin)

if you just set the diff option to false it will do a full reload and trigger style load.. this works unless I'm missing something?

like this (from the earlier jsbin):

map.setStyle(
  'https://api.maptiler.com/maps/streets/style.json?key=cQX2iET1gmOW38bedbUh',

  // add this to the options object to force a full update
  {diff: false}
);

Possibly refactoring and changing a bunch of stuff might make it a little cleaner but this seems like a reasonable workaround to try if you want to do something on style.load

(the above diff false in context from the jsbin)

document.getElementById('map-select').addEventListener('change', (e) => {
  switch (e.target.value) {
    case 'streets':
      map.setStyle('https://api.maptiler.com/maps/streets/style.json?key=cQX2iET1gmOW38bedbUh', {diff: false});
      break;
    case 'basic':
      map.setStyle('https://api.maptiler.com/maps/basic/style.json?key=cQX2iET1gmOW38bedbUh', {diff: false});
      break;
    case 'bright':
      map.setStyle('https://api.maptiler.com/maps/bright/style.json?key=cQX2iET1gmOW38bedbUh', {diff: false});
      break;
  }
  console.log(e.target.value);
})

[leearmstrong]

This appears to work for us as a workaround.

[igal1c0de4n]

This workaround works well on our end too. Thanks @chippieTV !

[acalcutt]

I finally got to test this workaround and it works for me also. Once adding {diff: false}, switching the style actually triggers the custom layers to get recreated.

I'm so happy this works since my map at wifidb.net has a bunch of dynamically generated layers and it would have been a pain to get it working without style.load getting triggered. In the trailmap I was working on, the transformStyle way worked pretty well as an alternative.

One thing I had trouble with, that I doubt many people will see, is my setStyle call was in a smarty template. In smarty templates it the uses brackets for smarty commands. I had to surround {diff: false} with {literal} tags like {literal}{diff: false}{/literal} to make smarty happy and actually load the page again, because otherwise it thought it was a badly formatted command.

[HarelM]

Might have been fixed by :#3133 A new version will be released soon.

[sbachinin]

This bug is still reproducible with v 4.0.2

[Hooterr]

Does diff: false as a workaround have any performance implications?

[HarelM]

Yes, diff is checking which layers need to be added or removed and only apply those. I think it's not noticable in most cases though.

[sbachinin]

I have a humble design suggestion here.

Is it correct that style.load is used primarily for preservation of "extra" layers & sources, i.e those added by map.addLayers and map.addSources? (But even if it's not the ultimate goal of the current issue): I can imagine another, much less toxic way to preserve extra layers. Instead of re-adding layers/sources on style.load, could we just avoid removing them (optionally) on setStyle?

From the public API perspective, this solution can look like this: 1) user adds extra sources/layers once via map.on('load') 2) user calls setStyleand tells not to remove extra layers/sources by passing a flag { preserveExtraLayers: true }.

Internally, sources/layers added via map.add...() must be somehow marked as "extra". Then, when evaluating the style diff, such marked sources/layers should not be removed.

Also there can be a way to specify which layers/sources to preserve and which not to. (Though I'm not sure if it's a sensible use case): When calling map.addSomething(), a user can somehow flag a layer/source as "permanent" or "protected". Such layer/source will survive setStyle() and can be removed only by calling map.removeSomething().

This solution can be quite verbose of course and surely has its own complexities. But it will surely involve less (or no) async stuff.

One minor complication here is the ordering of layers. If an extra layer was added "after layer N", then it has to be (if possible) put in the same position within a new style. That kind of stuff. But I think it must be very manageable.

This solution must not break any existing code, it only adds new options.

Does it make any sense? )

[HarelM]

There is a delegate which you can pass to the diff method, is there a way to leverage it? I would like to keep the API as clean as possible and this use case is considered "advanced" from my point of view and should be handled in an "advanced way".

[eis-ioki]

If you came here looking for a way to keep existing layers and source in the map when switching the style, this is how it is possible:

map.setStyle(newStyleUrl, {
  transformStyle: (previousStyle, nextStyle) => {
    var custom_layers = previousStyle.layers.filter(layer => {
      return layer.id.startsWith('MyCustomPrefix')
    });
    var layers = nextStyle.layers.concat(custom_layers);

    var sources = nextStyle.sources;
    for (const [key, value] of Object.entries(previousStyle.sources)) {
      if (key.startsWith('MyCustomPrefix')) {
        sources[key] = value;
      }
    }
    return {
      ...nextStyle,
      sources: sources,
      layers: layers
    };
  }
});

Any source and layer that starts with MyCustomPrefix will still be in the new style without reloading.

[jessegpierce]

@eis-ioki Could you help me incorporating your solution with the style switcher control tutorial found at https://docs.maptiler.com/sdk-js/examples/control-style-switcher/?

[leoleonsio]

Is there an easier way to keep a set of custom loaded layers while freely switching the background map? Having to copy the once loaded layers for this kind of action sounds inefficient