maplibre-gl-js

maplibre-gl-js copied to clipboard

1. extends the style swap options (previously as { diff?: boolean }) with an optional stylePatch function that allows performing a side effect during style switching: after a style is fetched but before it is committed to the map state. This allows to support a range of functionalities when incoming style requires modification based on external state or when previous style carries certain 'state' that needs to be carried over to a new style gracefully (without causing unnecessary visual transitions if changes are applied after a style change)

2. adds layer serialization on layout or paint property changes: previously serialized version of the layer was not updated when its paint or layout properties were changed. (maplibre keeps two copies of layers internally, a list of serialized layers and a list of 'live' layers that need to be evaluated against the current feature state (and other things) before they are used for rendering.)

Overview

Map's setStyle() and Style's loadURL() and loadJSON() options now may take an additional stylePatch function, which is exposed with a following signature:

stylePatch?: (
    previousStyle: StyleSpecification,
    nextStyle: StyleSpecification,
    preserveLayer: (layerId: string, before?: string) => void,
    updatePaintProperty: (layerId: string, name: string, value: any) => void,
    updateLayoutProperty: (layerId: string, name: string, value: any) => void,
    updateFilter: (layerId: string, name: string, value: any) => void
) => void;

previous and incoming style are passed, while 4 hooks that allow tampering with an incoming style:

1. preserving layer from previous style: this will either result in moveLayer or addLayer called on an incoming style. If an incoming style does not include layer's source (compared by the source's id), the source will be copied over as well. If an incoming style contains a layer with a same id that is getting preserved, it will be removed and replaced.
2. updating paint property on incoming style layer
3. updating layout property on incoming style layer
4. updating layer filter on incoming style layer

original author: @mbelt

Hypothetical Example

Preserving OSM base layer with opacity reduced when switching to a new style:

const style = {
    version: 8,
    sources: {
        osm: {
            type: 'raster',
            tiles: [
                'https://tile.openstreetmap.org/{z}/{x}/{y}.png'
            ],
            tileSize: 256
        },
        
    },
    layers: [{
        id: 'osm',
        type: 'raster',
        source: 'osm',
        minzoom: 0,
        maxzoom: 19
    }]
}

const map = new maplibregl.Map({
    container: 'map',
    zoom: 12.5,
    center: [-77.01866, 38.888],
    style,
    hash: true
});

map.setStyle('https://demotiles.maplibre.org/style.json', {
    stylePatch: (prevStyle, nextStyle, preserveLayer, updatePaintProperty, updateLayoutProperty, updateFilter) => {
        const prevRasterLayer = prevStyle.layers[0];
        preserveLayer(prevRasterLayer.id);
        updatePaintProperty(prevRasterLayer.id, 'raster-opacity', 0.75)
    }
})

Launch Checklist

• [x] Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
• [x] Briefly describe the changes in this PR.
• [x] Write tests for all new functionality.
• [x] Document any changes to public APIs.
• [x] Post benchmark scores.
• [x] Manually test the debug page. (added debug page)
• [X] Suggest a changelog category: feature
• [X] Add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog>Add style swap's stylePatch support (#1238)</changelog>.

Bundle size report:

Size Change: +653 B Total Size Before: 204 kB Total Size After: 204 kB

Output file	Before	After	Change
maplibre-gl.js	195 kB	195 kB	+653 B
maplibre-gl.css	9.03 kB	9.03 kB	0 B

ℹ️ View Details

No major changes

Benchmarks are at all.pdf, I have had several benchmarks broken or frozen (not sure why yet), which I had to skip: Layout, WorkerTransfer, SymbolLayout, HillshadeLoad, I also wonder if they might get affected by the fact of my CPU might exhibiting occasional thermal throttling.

Do the same ones fail in the browser, i.e., when you open the benchmark page in a browser?

@wipfli: correct, same ones fail. does the full bench suite runs fine for you?

The hillshade loading benchmark works for me. The other three broken ones should be fixed once #1297 is merged...

@wipfli: the comments should be resolved. Let me know if there is anything else needed here. The documentation of map.setStyle with inline example renders as:

We moved the debug folder in #1366. Can you merge main and move your debug file too?

@wipfli: done, thanks!

The current implementation of style editing does not allow changing glyphs and sprite, I think. It would be great if this patch style will allow this too (in case it doesn't already). i.e. I'm patching a style with layer that has a different font that is currently not "supported" in the current style and I need to change the font source to one with those fonts (a superset maybe or an array of font sources) Another one is the same with sprites - i.e. I change the style with a icon that is not in the current sprite and I want to change the sprite source to have either multiple sources (which is unfortunately currently not supported in the spec) or a superset sprite image.

Maybe it's not related to this PR, not sure. i.e. there might be a need to change the spec to have an array for glyphs and sprites and the ability to add/remove glyphs/sprites from the map or from the style...

Any thoughts are welcome...

@HarelM: it seems based on https://github.com/maplibre/maplibre-gl-js/blob/964b2a7405a7508c3cd4be4eb1da7f03a7a14f67/src/style/style.ts#L80-L82 that diffing of sprites / glyphs were expected to be added at some point. We should look into that in a follow-up PR to this one.

Making the style spec optionally support array of urls for glyphs/sprites probably should have a discussion of its own like in github discussions?

@wipfli: I have resolved the comments the fixed the doc redundancy you mentioned in the following way:

1. map.setStyle() documentation contains a short brief description about StylePatchFunction and links to it's own documentation as part of Properties and Options section based on https://github.com/maplibre/maplibre-gl-js-docs/pull/217. (I have kept the stylePatch inside the example of map.setStyle to have it discoverable)
2. Each of the StylePatchFunction parameter callbacks have been defined separately with added examples to each of them after StylePatchFunction as part of Properties and Options section.

@wipfli asked me to review this, sorry for the extra effort and questions :-)

Making the style spec optionally support array of urls for glyphs/sprites probably should have a discussion of its own like in github discussions?

Sure, this sounds right. I don't think we need to break the style spec to do it, but rather expand it. Feel free to open a discussion.

that diffing of sprites / glyphs were expected to be added at some point. We should look into that in a follow-up PR to this one.

If this something you can look into it would be great! Unfortunately I'm not sure I fully understand the full flow related to patching a style when it comes to glyphs and sprites... :-(

Thanks for the review, @HarelM!

And cool that you opened the tracking issue or pull request in the docs repo...

@wipfli asked me to review this, sorry for the extra effort and questions :-)

no worries, thanks a lot for the review, I will try to address all of those soon

Since we are working on version 3.x now, if this need to introduce a breaking change, now is the time :-) Overall, I think this is a very nice addition, and also shows up which methods still needs to be implemented for style patch, which is great.

Since we are working on version 3.x now, if this need to introduce a breaking change, now is the time :-) Overall, I think this is a very nice addition, and also shows up which methods still needs to be implemented for style patch, which is great.

@HarelM: thanks, sorry, took some time for me to get back to this, thanks for great comments here also, I think all should be addressed.

@HarelM, @mbelt, @wipfli: Before getting this merged, I wanted to get your thoughts on possible alternative. There are 2 details of existing exposed stylePatch that I see:

stylePatch?: (
    previousStyle: StyleSpecification,
    nextStyle: StyleSpecification,
    preserveLayer: (layerId: string, before?: string) => void,
    updatePaintProperty: (layerId: string, name: string, value: any) => void,
    updateLayoutProperty: (layerId: string, name: string, value: any) => void,
    updateFilter: (layerId: string, name: string, value: any) => void
) => void;

1. there might be an impression that stylePatch looks like a constrained set of the functionalities for updating the next style (preserve, updatePaint/layout/filter), while actually next style can be mutated per current behavior. updatePaintProperty / updateLayoutProperty / updateFilter in this regard is just sugar, where preserveLayer is a special sugar with somehow opinionated move behavior.
2. preserveLayer move behavior with respect to layers and sources in case of id collisions is opinionated: (keep new source on source collision, move layer from old style and replace new on preserved layer id collision) might not be intuitive for all users

Possible alternative:

transformStyle?: (previousStyle: StyleSpecification, nextStyle: StyleSpecification) => StyleSpecification

A style patch could be simple pure function where users are responsible for composing a valid resulting style based on previous style and style that is being set, users would decide themselves how styles and sources are moved. The diff behavior of https://github.com/maplibre/maplibre-gl-js/blob/98a4aa901ceb7ed850e2aacc94dab6dcfd1c3292/src/style-spec/diff.ts#L319 would apply as is, without additional operations and logic needed to be added for buildStylePatch, this would be lighter, curious if anyone has any thoughts.

transformStyle sounds like a good idea to me. Since it is less opiniated than the current implementation, we should make sure to have some good examples how to use it.

I agree with @wipfli. If we can simplify the API and make it as generic as possible it will reduce the problem in the future of breaking it (by mistake or by purpose). I also agree that having a few example (and also tests) with how to preserve a layer, how to move a layer, how to keep a source that is being used are important and should allow better experience to cover for the fact that the API is generic.

@wipfli, @HarelM: thanks, in this case let me make another PR based on this and we can see how these two approaches compare

closed in favour of https://github.com/maplibre/maplibre-gl-js/pull/1632