-
Notifications
You must be signed in to change notification settings - Fork 633
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
Add support for multi-sprites in styles #1232
Conversation
Signed-off-by: Andrew Calcutt <acalcutt@techidiots.net>
So it doesn't conflict with style json url Signed-off-by: Andrew Calcutt <acalcutt@techidiots.net>
Signed-off-by: Andrew Calcutt <acalcutt@techidiots.net>
Signed-off-by: Andrew Calcutt <acalcutt@techidiots.net>
Signed-off-by: Andrew Calcutt <acalcutt@techidiots.net>
Signed-off-by: Andrew Calcutt <acalcutt@techidiots.net>
Signed-off-by: Andrew Calcutt <acalcutt@techidiots.net>
Signed-off-by: Andrew Calcutt <acalcutt@techidiots.net>
Signed-off-by: Andrew Calcutt <acalcutt@techidiots.net>
Signed-off-by: Andrew Calcutt <acalcutt@techidiots.net>
Signed-off-by: Andrew Calcutt <acalcutt@techidiots.net>
Signed-off-by: Andrew Calcutt <acalcutt@techidiots.net>
Signed-off-by: acalcutt <acalcutt@techidiots.net>
src/serve_style.js
Outdated
app.get( | ||
'/:id/sprite(/:name)?:scale(@[23]x)?.:format([\\w]+)', | ||
(req, res, next) => { | ||
const name = req.params.name || 'sprite'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you wanted to you could do
const { name = 'sprite', scale = '', format, id } = req.params;
src/serve_rendered.js
Outdated
if (Array.isArray(styleJSON.sprite)) { | ||
styleJSON.sprite.forEach((spriteItem) => { | ||
if (!httpTester.test(spriteItem.url)) { | ||
spriteItem.url = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have an issue with how it is here, but I wonder if it might be worth having a helper method for doing this replacement?
Another option would be to start by normalizing sprites when initially loading styles?
if (styleJSON.sprite) {
if (!Array.isArray(styleJSON.sprite)) {
styleJSON.sprite = [{id: 'default', url: styleJSON.sprite }];
}
for (let spriteItem of styleJSON.sprite) {
if (!httpTester.test(spriteItem.url)) {
spriteItem.url = ...
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree this looks better and is simpler. My only concern my previous way preserved the existing sprite format, so if you were not using multi-sprites it would function as before and work with older versions of mapbox/maplibre that do not support multi-sprite.
Where this way basically forces multi-sprites on the users style whether they are using them or not. So it would only work with newer maplibre-js that supports that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence about reverting this change. I don't know if I want to break clients that doesn't support multi-sprite if the person making the style hasn't chosen to use them. it does look a lot nicer though.
Signed-off-by: acalcutt <acalcutt@techidiots.net>
Signed-off-by: acalcutt <acalcutt@techidiots.net>
Signed-off-by: acalcutt <acalcutt@techidiots.net>
'/:id/sprite(/:spriteID)?:scale(@[23]x)?.:format([\\w]+)', | ||
(req, res, next) => { | ||
const { spriteID = 'default', id } = req.params; | ||
const scale = allowedSpriteScales(req.params.scale) || ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super minor thing, you can move the || ''
in to make it the default:
const scale = allowedSpriteScales(req.params.scale) || ''; | |
const scale = allowedSpriteScales(req.params.scale, { defaultValue: '' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when I try this, scale ends up 'undefined' if no scale is set, so it ends up putting that into the file name. I see this at the console
Sprite load error: C:\Users\andrew.EIRI\Desktop\test_data\styles\klokantech-basic\spriteundefined.png
Signed-off-by: acalcutt <acalcutt@techidiots.net>
Signed-off-by: acalcutt <acalcutt@techidiots.net>
maplibre-gl-native node-v5.4.0 adds support for multi-sprites ( https://maplibre.org/maplibre-style-spec/sprite/#multiple-sprite-sources ).
This PR allows tileserver-gl to handle an array of local sprites, in a format like
you can also specify urls, like shown in the style spec
Example
I updated this ATV trail map to use two different sets of sprites, 'default' and 'colored'. You can see the default icons and colored icons show as expected
https://tiles.wifidb.net/styles/WDB_GPSTRAILS/?raster#15/45.0491/-69.8739