Add support for multiple sprites in one style #1805

smellyshovel · 2022-11-02T17:48:06Z

Changed API

None.

Main classes that were changed (inner impl.)

Map, Style.

Updated API

`map.setStyle`

Now supports sprite value diffing. Namely, in case the sprites change with a call to setStyle, all the old sprites are removed and the new ones are added. So it's not exactly "diff"ing in its most accurate sense, but AFAIK changing sprite with setStyle previously was not possible at all.

New API

`style.json`'s sprite property

This PR adds support for multiple sprite files per one style file while keeping the backwards compatibility intact.

Previously, a sprite could only be defined as a single URL to fetch both .json and .png files from.
Now, one can also pass a JSON-array with {id: "id", url: "url"} objects to have more than one sprite loaded per one style file (i.e. one sprite per one map).

So both following declarations are now OK:

sprite: "https://example.com/sprite"

and

sprite: [{ id: "sprite1", url: "https://example.com/sprite1" }, { id: "sprite2", url: "https://example.com/sprite2" }]

(As previously, the sprite declaration can still be skipped allogether).

Resrtictions and validation.

All the id and all the urls must be unique. This is checked both initially on validating the style and using the new puclic API (see below), when validation is not explicitly disabled for them.

Namespacing

Each sprite's id is used as a namespace for its images' names. So, if you sprite declaration looks like this:

sprite: [{ id: "sprite1", url: "https://example.com/sprite1" }, { id: "sprite2", url: "https://example.com/sprite2" }]

then all the images from both sprites should use sprite1: and sprite2: prefixes when referenced. For example (for the above declaration):

// for a symbol layer
"icon-image": "sprite1:airport"

Images from the sprite with id "default" are not prefixed.

sprite: [{ id: "default", url: "https://example.com/sprite1" }, { id: "sprite2", url: "https://example.com/sprite2" }]

// ...

"icon-image": "airport"

// ... but you still need to prefix everything that's not "default"

"icon-image": "sprite2:toilet"

That's done for the backwards-compatibility reasons. So that you don't have to majorly change your style file in case you want to start using the multi-sprites feature.

Public methods

`map.getSprite()`

Returns the currently used sprite always as an array. When no sprite is set for the map, it's [].

`map.addSprite(id, url, options?)`

Adds a sprite to the map style.

id - The ID of the sprite to add. Must not conflict with existing sprites.
url - The URL to load the sprite from
options - optional StyleSetterOptions object. The same as for the other similar PI methods (like {validate: false} e.g.)

Fires an error event if validation fails.

`map.removeSprite(id)`

Removes a sprite with the given ID from the map. Fires an error event in case there's no sprite with the specified ID exist.

id - The ID of the sprite to add

If the sprite is declared as a single URL, you must use "default" as the ID.

`map.setSprite(spriteUrl, options?)`

Sets the value of the style's sprite property.

spriteUrl - Sprite URL to set.
options - optional StyleSetterOptions object. The same as for the other similar PI methods (like {validate: false} e.g.)

Only supports string-form declarations.

Glyphs

@ambientlight ???

github-actions · 2022-11-02T17:54:49Z

Bundle size report:

Size Change: +899 B
Total Size Before: 207 kB
Total Size After: 207 kB

Output file	Before	After	Change
maplibre-gl.js	197 kB	198 kB	+899 B
maplibre-gl.css	9.1 kB	9.1 kB	0 B

ℹ️ View Details

No major changes

HarelM · 2022-11-02T18:39:13Z

I like the sprite name in order to know from each sprite to take the icons from in order to avoid collision and the default key word.
I don't like the special characters string handling, it's error prone...
Would be nice to introduce an addSprite, removeSprite, updateSprite etc as part of this PR and effort (also worth looking into update style for the same reason as currently it doesn't support updating the sprite).
I think you can define the sprite as "string or (array of key and value)" - this won't break backward compatibility and will allow using the original string but also facilitate for something one can read too :-)
It might be a bit more effort, but relaying on \n and \t seems odd to me...
Overall, I think this feature is important, great and will make an excellent addition to this lib.

smellyshovel · 2022-11-02T18:55:59Z

@HarelM

I like the sprite name in order to know from each sprite to take the icons from in order to avoid collision and the default key word.

So we do need a key-value map of some sort.

I don't like the special characters string handling, it's error prone...

I also do have doubts about this part.

Would be nice to introduce an addSprite, removeSprite, updateSprite etc as part of this PR and effort (also worth looking into update style for the same reason as currently it doesn't support updating the sprite).

Thought of this as of another PR. Or at least later in this PR...

I think you can define the sprite as "string or (array of key and value)" - this won't break backward compatibility and will allow using the original string but also facilitate for something one can read too :-)

As a specified in the OP, I couldn't find how to do this. As far as I understood, it's done in v8.json file, right? But I don't see any existing "one or another type" declaration there to take it as an example. Could you please elaborate more on implementation details?

It might be a bit more effort, but relaying on \n and \t seems odd to me...
Overall, I think this feature is important, great and will make an excellent addition to this lib.

So, first we need to support both strings (for the BC) and maps of some sort. Either an array of form ["name1", "url1", "name2", "url2"], or {name1: "url1", name2: "url2"}. Right?
Second. We still need to support this on the level of style spec for other public methods like addSprite (and so on) to work, right? If I make the "sprite" property in-runtime-updatable, that won't be sufficient I mean.

HarelM · 2022-11-02T19:48:57Z

look for "type": "padding" or "type": "paint" both are "special" types to allow complex data structures.
paint seems to be one of other paint_* which sounds like there's an ability to have more than one type.
I'm not sure, but you can experiment with those.
I personally prefer [{id: "prefix1", url: "http://www.younameit.com/"}, {...}]

See this PR for more references:
#1289
Basically, you can change the generation code to do what you need in terms of type generation. Sadly, I'm not sure where the definition of how to write this json file is coming from...

pramilk · 2022-11-03T06:51:22Z

This PR is a prototype of an idea of how we can add support for multiple sprites per one style file. This has been an issue for quite a while already, and I believe it should be addressed somehow.

What is the use case/scenario for having multiple sprites?

HarelM · 2022-11-03T07:16:18Z

For me - if I allow users to have custom map layers and in general more than one layer of map (not maplibre layer but a "map layer" where you can stack different styles one on top of the other) then having more than one sprite allows the site to be agnostic of style file and allow users to create their own styles and present them in the site.
Currently, I have only one sprite and I can't have the users define a style that doesn't have their icons.
It might be an edge case only for me, but I totally see the value of this.
i.e. I can add layers and sources but I can't add icons using sprites. this is a limitation that this PR can solve.
This is the site I'm maintaining basically which will benefit from this PR:
https://israelhiking.osm.org.il/
In general, the font link also suffers from the same problem but fonts are more of a closed set as opposed to icons/sprite

test/integration/lib/localize-urls.ts

smellyshovel · 2022-11-03T23:02:43Z

@HarelM Any idea of why do integration tests fail? Where should I look into?

HarelM · 2022-11-04T03:55:44Z

I'm not sure honestly, you'll need to try and run then locally and see what change caused them to fail...
Another thought is around the callback stuff in the images and "maybe completed" code. It might prove beneficial to switch that part to promises...? IDK.

…sprites

smellyshovel · 2022-12-26T18:15:09Z

@HarelM
Changelog - done
Tests - pass
Build - passes
Feedback - addressed
Native repo issue - not created. Should I do it now or after this one's merged?

HarelM

Great work!! THANKS!!!

HarelM · 2022-12-26T19:36:00Z

You can open it now. I'll merge this shortly.
One last question, does this work with the style update/patch which was introduced latetly?

HarelM · 2022-12-26T19:37:31Z

We'll need to make sure this is also documented well.
Please open an issue in the docs repo to make sure we don't forget to check that once we release an official version the docs get updated with this new ability.

smellyshovel · 2022-12-27T06:03:30Z

One last question, does this work with the style update/patch which was introduced latetly?

Not sure what are you about. The setStyle method?
I think the diffing now works for it. But now as I thought of it I'm not sure that we've updated itst JSDoc comments accordingly...

Or are you talking about some other stuff?

HarelM · 2022-12-27T06:25:12Z

Yes, setStyle with diff is what I'm referring to as style update/patch.

smellyshovel · 2022-12-27T06:31:13Z

Yes, setStyle with diff is what I'm referring to as style update/patch.

I've updated the initial commen for the PR.
I think @ambientlight worked on that feature, but from what I've seen in the code, I think that in general updating sprites with setStyle now works. But diffing in its common sense is not exactly the diffing. As I understood, it anyway removes all the existing sprites and adds all from the updated style. As I understand, even if the value did not change. But I may be mistaken...

HarelM · 2022-12-27T06:39:33Z

It might be the current implantation, but since now there's a meaning to diff in terms of adding new sprites or removing non used, it needs to be addressed.
@ambientlight can you take a look at this and make sure we covered this use case?

ambientlight · 2022-12-27T08:11:52Z

I think @ambientlight worked on that feature, but from what I've seen in the code, I think that in general updating sprites with setStyle now works. But diffing in its common sense is not exactly the diffing. As I understood, it anyway removes all the existing sprites and adds all from the updated style. As I understand, even if the value did not change. But I may be mistaken...

To clarify, the diffing refers to style diffing that happens on map.setStyle(..., { diff: true }) is performed at

maplibre-gl-js/src/style-spec/diff.ts

Line 319 in 66e1009

function diffStyles(before, after) {

when sprite or glyph changes will result in calls to

maplibre-gl-js/src/style/style.ts

Line 1606 in 66e1009

    
           setSprite(sprite: SpriteSpecification, options: StyleSetterOptions = {}, completion: (err: Error) => void) {

or

maplibre-gl-js/src/style/style.ts

Line 1522 in 66e1009

setGlyphs(glyphsUrl: string | null, options: StyleSetterOptions = {}) {

in which case the behavior is as following:

for glyphs: previous glyph cache is dropped and new glyph ranges will be loaded
for sprites: _loadSprite() handles to logic of removing the old sprites in

maplibre-gl-js/src/style/style.ts

Line 353 in 66e1009

const imagesToRemove = this._spritesImagesIds[spriteId] ? this._spritesImagesIds[spriteId].filter(id => !(id in images)) : [];

, if we are loading a new image with the id already present in imageManager, we would perform the update instead of add.

smellyshovel · 2022-12-27T08:15:49Z

@ambientlight for sprites I believe it's the same.
@HarelM is it really worth the effort calculaing the real difference and add/remove sprites/glyphs instead of invalidating and re-adding all from scratch?

ambientlight · 2022-12-27T08:25:24Z

@smellyshovel: I think the current behavior is correct, there maybe some confusion regarding what diffing stands for. Diffing refers to style diffing. Here is the example:

<!DOCTYPE html>
<html lang="en">
<head>
    <title>MapLibre GL JS debug page</title>
    <meta charset='utf-8'>
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <link rel='stylesheet' href='../../dist/maplibre-gl.css' />
    <style>
        body { margin: 0; padding: 0; }
        html, body, #map { height: 100%; }
    </style>
</head>

<body>
<div id='map'></div>

<script src='../../dist/maplibre-gl-dev.js'></script>
<script>

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

map.showTileBoundaries = true;

map.on('load', () => {

    window.setTimeout(() => {
        // set glyphs and update symbol layers to new fonts
        map.setGlyphs('https://atlas.microsoft.com/styling/glyphs/{fontstack}/{range}.pbf?api-version=2.0', {
            validate: true
        });
        map.getStyle().layers
            .forEach(layer => {
                if (layer.type === 'symbol') {
                    map.setLayoutProperty(layer.id, 'text-font', ["StandardFont-Bold"]);
                }
            });

        // move to a new style without glyphs and symbol layers
        window.setTimeout(() => {
            map.setStyle('https://demotiles.maplibre.org/style.json', {
                transformStyle: (previousStyle, nextStyle) => {
                    const targetStyle = ({
                        ...nextStyle,
                        layers: nextStyle.layers.filter(layer => layer.type !== 'symbol')
                    });
                    delete targetStyle.glyphs;
                    return targetStyle;
                }
            });
        }, 10000);
    }, 5000);

});

</script>
</body>
</html>

In the above, if you map.setStyle to a style that has changed glyphs, previously, a full style reload would be performed, now the style would be updated (and thus diffed, as the set of operations would be computed in

maplibre-gl-js/src/style-spec/diff.ts

Line 319 in 66e1009

function diffStyles(before, after) {

) that would change the existing loaded style to the state that is expected to be per the style that is being set (loosely speaking). In the above case we will remove all symbol layers and also remove glyphs, with the later resulting in style.setGlyphs(null) being called.

ambientlight · 2022-12-27T08:29:34Z

@ambientlight can you take a look at this and make sure we covered this use case?

@HarelM: I've just verified the behavior to be as expected as per 66e1009, except only, on my side, I need to complete #1874 bug fix asap, since it may affect the sprite loading as discussed there.

HarelM · 2022-12-27T11:13:12Z

Thanks @ambientlight!
@smellyshovel, yes, style diffing is worth the effort when you have a dynamic map that you can add or remove layers, sources, sprites etc and you don't want to clear the map and add all the style for every change - for example when you want to switch a base layer and you have lines and markers drawn on the map.

ambientlight · 2022-12-27T11:20:49Z

for our case, where we have styles with ~1000 layers, the penalty of reloading the style from scratch is very high (x seconds) for just a sprite switch, which is a common case when switching between light and dark styles for example

smellyshovel added 9 commits November 2, 2022 02:17

Add debug-page

71599b9

Modify example

fc77ca4

First working proto

e56d652

Custom sprites' naming

7f6d375

Merge branch 'main' into multi-sprites

9e2100e

Revert

2111b72

Better test case

93c08f2

1st working prototype

d5c1ca8

Cancelation handling

0af5442

smellyshovel added 4 commits November 3, 2022 01:23

WIP on new Sprite type

e929b5e

New syntax proto

23381f3

WIP on validation

b3d2d9b

Working validation

f419b58

smellyshovel added 5 commits November 4, 2022 01:41

Strticter validation for duplicates

a476318

Cover validator with tests

fe77a35

WIP on unit tests

bc971b2

Fix ESlint

ed72a7d

Provide doc for the new API. Try to suppress failing tests

c4e0b0a

smellyshovel commented Nov 3, 2022

View reviewed changes

test/integration/lib/localize-urls.ts Outdated Show resolved Hide resolved

smellyshovel mentioned this pull request Nov 4, 2022

Remove mapbox localization tests #1810

Closed

Fix integration tests

d544706

smellyshovel marked this pull request as ready for review November 4, 2022 17:05

smellyshovel added 5 commits December 26, 2022 21:28

Merge remote-tracking branch 'smellyshovel/multi-sprites' into multi-…

35f73c4

…sprites

Try fix the build

83fc551

Fix unit tests

5de16fa

Test _load for sprite: undefined

5db0556

Add changelog entry

201ce14

Remove sprite better docs

452d37b

HarelM approved these changes Dec 26, 2022

View reviewed changes

HarelM merged commit 66e1009 into maplibre:main Dec 26, 2022

This was referenced Dec 27, 2022

Add documentation for the new multi-sprites feature maplibre/maplibre-gl-js-docs#358

Closed

Feature parity with the GL JS implementation (muli-sprites) maplibre/maplibre-native#641

Closed

smellyshovel deleted the multi-sprites branch December 27, 2022 08:16

This was referenced Feb 19, 2023

Use @maplibre/maplibre-gl-style-spec in MapLibre GL JS #2194

Closed

Update changelog for style-spec #2197

Merged

HarelM mentioned this pull request Jun 11, 2023

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

Open

HarelM mentioned this pull request Oct 11, 2023

Design Proposal: New 'sprites' property to handle multiple sprite urls within a style maplibre/maplibre-style-spec#378

Closed

louwers mentioned this pull request Oct 19, 2023

Sprite Documentation is Outdated maplibre/maplibre-style-spec#389

Closed

1ec5 mentioned this pull request Oct 31, 2023

Re-render shields after fonts load osm-americana/openstreetmap-americana#991

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple sprites in one style #1805

Add support for multiple sprites in one style #1805

smellyshovel commented Nov 2, 2022 •

edited

Loading

github-actions bot commented Nov 2, 2022 •

edited

Loading

HarelM commented Nov 2, 2022

smellyshovel commented Nov 2, 2022 •

edited

Loading

HarelM commented Nov 2, 2022

pramilk commented Nov 3, 2022

HarelM commented Nov 3, 2022 •

edited

Loading

smellyshovel commented Nov 3, 2022

HarelM commented Nov 4, 2022

smellyshovel commented Dec 26, 2022

HarelM left a comment

HarelM commented Dec 26, 2022

HarelM commented Dec 26, 2022

smellyshovel commented Dec 27, 2022

HarelM commented Dec 27, 2022

smellyshovel commented Dec 27, 2022

HarelM commented Dec 27, 2022

ambientlight commented Dec 27, 2022 •

edited

Loading

smellyshovel commented Dec 27, 2022

ambientlight commented Dec 27, 2022

ambientlight commented Dec 27, 2022

HarelM commented Dec 27, 2022

ambientlight commented Dec 27, 2022

Add support for multiple sprites in one style #1805

Add support for multiple sprites in one style #1805

Conversation

smellyshovel commented Nov 2, 2022 • edited Loading

Changed API

Main classes that were changed (inner impl.)

Updated API

map.setStyle

New API

style.json's sprite property

Resrtictions and validation.

Namespacing

Public methods

map.getSprite()

map.addSprite(id, url, options?)

map.removeSprite(id)

map.setSprite(spriteUrl, options?)

Glyphs

github-actions bot commented Nov 2, 2022 • edited Loading

HarelM commented Nov 2, 2022

smellyshovel commented Nov 2, 2022 • edited Loading

HarelM commented Nov 2, 2022

pramilk commented Nov 3, 2022

HarelM commented Nov 3, 2022 • edited Loading

smellyshovel commented Nov 3, 2022

HarelM commented Nov 4, 2022

smellyshovel commented Dec 26, 2022

HarelM left a comment

Choose a reason for hiding this comment

HarelM commented Dec 26, 2022

HarelM commented Dec 26, 2022

smellyshovel commented Dec 27, 2022

HarelM commented Dec 27, 2022

smellyshovel commented Dec 27, 2022

HarelM commented Dec 27, 2022

ambientlight commented Dec 27, 2022 • edited Loading

smellyshovel commented Dec 27, 2022

ambientlight commented Dec 27, 2022

ambientlight commented Dec 27, 2022

HarelM commented Dec 27, 2022

ambientlight commented Dec 27, 2022

smellyshovel commented Nov 2, 2022 •

edited

Loading

`map.setStyle`

`style.json`'s sprite property

`map.getSprite()`

`map.addSprite(id, url, options?)`

`map.removeSprite(id)`

`map.setSprite(spriteUrl, options?)`

github-actions bot commented Nov 2, 2022 •

edited

Loading

smellyshovel commented Nov 2, 2022 •

edited

Loading

HarelM commented Nov 3, 2022 •

edited

Loading

ambientlight commented Dec 27, 2022 •

edited

Loading