Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation of multi-sprites in mbgl-core #1858

Merged
merged 35 commits into from
Feb 27, 2024

Conversation

geolives-contact
Copy link
Contributor

In order to solve #641.
Implemented in the same way as for Maplibre GL JS.

In detail

In Style JSON, sprite property can now be a string or an array. If this is an array that must contains object(s) with id and urls.
SpriteLoader can now manage multiple sprites in parallel.
Style::Impl now stores the information about how much sprites are loaded. Included a method areSpritesLoaded() to be adequate with the old boolean property that serves the same goal.

When using a string as sprite, that creates internally a Sprite object with id default and the url.
When using an array, there is a parsing to create adequate Sprite objects.

Usage

In images identifiers you need to prepend the sprite id to the image. Example :

"icon-image": "hiking:myimage",

That leads to the image identifier 'myimage' for the sprite object with id 'hiking'.
If id of your sprite is 'default', you don't need to prepend it :

"icon-image": "myimage",

Tip: If your icon-image is property driven, you can use this instead :

"icon-image": [
          "concat",
          "hiking:",
          [
            "get",
            "myproperty"
          ]
        ],

Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution.

Please add tests to style_parser.test.cpp to validate both the old (single string URL) and new (multiple sprites in array) are parsed correctly. Does an empty array get parsed correctly? What is the behavior when specifying the same sprite id twice? Some inspiration can be taken from https://github.com/maplibre/maplibre-gl-js/pull/1805/files#diff-3f25c81a5d56e4b9c90f8e63d1bdc10eeaf8e0de78818ff567d87c5ff5950710

We need to copy the render tests from MapLibre GL JS.

src/mbgl/style/parser.hpp Outdated Show resolved Hide resolved
@geolives-contact
Copy link
Contributor Author

I resolved some of the issues, but one big issue still exist. In the current commit version you can have multiple sprites with the same id. MapLibre GL JS doesn't allow that and I need to handle this case + unit tests.

@geolives-contact
Copy link
Contributor Author

I implemented checks about id uniqueness.
I added also 2 style parsing tests sprites-missing-fields and sprites-not-same-ids within the style_parser test directory to check attributes of sprites and id uniqueness.
4 additional tests (SpriteAsArrayEmpty,SpriteAsArraySingle, SpriteAsArrayMultiple and SpriteAsString) added to test compatibility with array of sprite or single sprite url inside style_parser.test.cpp.

Seems to be good for me. Tell me if something is wrong.

@louwers
Copy link
Collaborator

louwers commented Dec 4, 2023

@Geolives Will do a manual test today!

Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for getting back again to this PR so late.

Code looks good, but we still need to add a render test.

@wipfli
Copy link
Contributor

wipfli commented Dec 11, 2023

Since this feature already exists in MapLibre GL JS if I remember correctly, can we copy-paste the render test from there?

@louwers
Copy link
Collaborator

louwers commented Dec 11, 2023

@wipfli Yes I think so. @Geolives Let me know if you want me to take a look at this, because I have been meaning to do that anyway.

@geolives-contact
Copy link
Contributor Author

It's probably better if you can do it, as my understandings about the render tests are minor.
Thanks :)

Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience with this one. The new render tests passed locally. Thanks for this awesome contribution!

@louwers louwers enabled auto-merge (squash) February 15, 2024 19:00
Copy link

github-actions bot commented Feb 15, 2024

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.2% +34.5Ki  +0.2% +32.0Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-1858-compared-to-main.txt

@louwers
Copy link
Collaborator

louwers commented Feb 17, 2024

iOS render test passes locally.

On CI it complains about missing metrics.

@louwers louwers disabled auto-merge February 27, 2024 15:36
@louwers louwers merged commit f4c6fab into maplibre:main Feb 27, 2024
30 checks passed
acalcutt pushed a commit to WifiDB/maplibre-native that referenced this pull request Feb 27, 2024
Co-authored-by: Christophe Brasseur <christophe.brasseur@geolives.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Bart Louwers <bart.louwers@gmail.com>
Co-authored-by: Bart Louwers <bart@emeel.net>
acalcutt pushed a commit to WifiDB/maplibre-native that referenced this pull request Apr 11, 2024
Co-authored-by: Christophe Brasseur <christophe.brasseur@geolives.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Bart Louwers <bart.louwers@gmail.com>
Co-authored-by: Bart Louwers <bart@emeel.net>
acalcutt pushed a commit to WifiDB/maplibre-native that referenced this pull request Apr 16, 2024
Co-authored-by: Christophe Brasseur <christophe.brasseur@geolives.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Bart Louwers <bart.louwers@gmail.com>
Co-authored-by: Bart Louwers <bart@emeel.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants