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.

Sign up for GitHub

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

Jump to bottom

Add support for multi-sprites in styles #1232

Merged
merged 22 commits into from
Apr 23, 2024
Merged

Add support for multi-sprites in styles #1232

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
eb978ad
fix: allow sprite array in styles
acalcutt Apr 20, 2024
a5f3b14
fix: add 'sprite/' to generated path
acalcutt Apr 20, 2024
92b5284
fix: allow old sprite endpoints to work
acalcutt Apr 20, 2024
80953f4
fix: lint
acalcutt Apr 20, 2024
1e50e17
docs: add multi-sprite name to endpoint
acalcutt Apr 20, 2024
c08f884
fix: duplicate styleJSON.sprite
acalcutt Apr 21, 2024
bae0d10
fix: try to fix some codeql errors
acalcutt Apr 21, 2024
6f91995
fix: try to fix some codeql errors
acalcutt Apr 21, 2024
4652b46
fix: try to fix some codeql errors
acalcutt Apr 21, 2024
24a7027
fix: try to fix some codeql errors
acalcutt Apr 21, 2024
b461396
fix: try to fix some codeql errors
acalcutt Apr 21, 2024
37857c3
fix: lint
acalcutt Apr 21, 2024
4bc80ee
fix: ci sprite issue
acalcutt Apr 21, 2024
ae933d5
fix: remove console.log's
acalcutt Apr 21, 2024
ee77371
fix: use sprite id instead of name
acalcutt Apr 23, 2024
c848107
fix: add suggestions
acalcutt Apr 23, 2024
6774f27
fix: remove unneeded array check
acalcutt Apr 23, 2024
361464f
fix: lint
acalcutt Apr 23, 2024
e9bb169
fix: don't force multi-sprites in style
acalcutt Apr 23, 2024
452f9f0
fix: move fixUrl and allowedOptions to utils
acalcutt Apr 23, 2024
ae13acb
fix: add test for default sprite
acalcutt Apr 23, 2024
99b2232
fix: lint
acalcutt Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/endpoints.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ Styles
======
* Styles are served at ``/styles/{id}/style.json`` (+ array at ``/styles.json``)

* Sprites at ``/styles/{id}/sprite[@2x].{format}``
* Sprites at ``/styles/{id}/sprite[/spriteID][@2x].{format}``
* Fonts at ``/fonts/{fontstack}/{start}-{end}.pbf``

Rendered tiles
Expand Down
44 changes: 34 additions & 10 deletions src/serve_rendered.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ import { renderOverlay, renderWatermark, renderAttribution } from './render.js';
const FLOAT_PATTERN = '[+-]?(?:\\d+|\\d+.?\\d+)';
const PATH_PATTERN =
/^((fill|stroke|width)\:[^\|]+\|)*(enc:.+|-?\d+(\.\d*)?,-?\d+(\.\d*)?(\|-?\d+(\.\d*)?,-?\d+(\.\d*)?)+)/;
const httpTester = /^(http(s)?:)?\/\//;
const httpTester = /^https?:\/\//i;

const mercator = new SphericalMercator();
const getScale = (scale) => (scale || '@1x').slice(1, 2) | 0;
Expand Down Expand Up @@ -1045,16 +1045,40 @@ export const serve_rendered = {
return false;
}

if (styleJSON.sprite && !httpTester.test(styleJSON.sprite)) {
styleJSON.sprite =
'sprites://' +
styleJSON.sprite
.replace('{style}', path.basename(styleFile, '.json'))
.replace(
'{styleJsonFolder}',
path.relative(options.paths.sprites, path.dirname(styleJSONPath)),
);
if (styleJSON.sprite) {
if (Array.isArray(styleJSON.sprite)) {
styleJSON.sprite.forEach((spriteItem) => {
if (!httpTester.test(spriteItem.url)) {
spriteItem.url =
Copy link
Contributor

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 = ...
    }
  }
}

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

@acalcutt acalcutt Apr 23, 2024 •
edited
Loading

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.

'sprites://' +
spriteItem.url
.replace('{style}', path.basename(styleFile, '.json'))
.replace(
'{styleJsonFolder}',
path.relative(
options.paths.sprites,
path.dirname(styleJSONPath),
),
);
}
});
} else {
if (!httpTester.test(styleJSON.sprite)) {
styleJSON.sprite =
'sprites://' +
styleJSON.sprite
.replace('{style}', path.basename(styleFile, '.json'))
.replace(
'{styleJsonFolder}',
path.relative(
options.paths.sprites,
path.dirname(styleJSONPath),
),
);
}
}
}

if (styleJSON.glyphs && !httpTester.test(styleJSON.glyphs)) {
styleJSON.glyphs = `fonts://${styleJSON.glyphs}`;
}
Expand Down
128 changes: 96 additions & 32 deletions src/serve_style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

import { getPublicUrl } from './utils.js';

const httpTester = /^(http(s)?:)?\/\//;
const httpTester = /^https?:\/\//i;

const fixUrl = (req, url, publicUrl) => {
if (!url || typeof url !== 'string' || url.indexOf('local://') !== 0) {
Expand Down Expand Up @@ -42,33 +42,76 @@
}
// mapbox-gl-js viewer cannot handle sprite urls with query
if (styleJSON_.sprite) {
styleJSON_.sprite = fixUrl(req, styleJSON_.sprite, item.publicUrl);
if (Array.isArray(styleJSON_.sprite)) {
styleJSON_.sprite.forEach((spriteItem) => {
spriteItem.url = fixUrl(req, spriteItem.url, item.publicUrl);
});
} else {
styleJSON_.sprite = fixUrl(req, styleJSON_.sprite, item.publicUrl);
}
}
if (styleJSON_.glyphs) {
styleJSON_.glyphs = fixUrl(req, styleJSON_.glyphs, item.publicUrl);
}
return res.send(styleJSON_);
});

app.get('/:id/sprite:scale(@[23]x)?.:format([\\w]+)', (req, res, next) => {
const item = repo[req.params.id];
if (!item || !item.spritePath) {
return res.sendStatus(404);
}
const scale = req.params.scale;
const format = req.params.format;
const filename = `${item.spritePath + (scale || '')}.${format}`;
return fs.readFile(filename, (err, data) => {
if (err) {
console.log('Sprite load error:', filename);
app.get(
'/:id/sprite(/:spriteID)?:scale(@[23]x)?.:format([\\w]+)',
(req, res, next) => {
const spriteID = req.params.spriteID || 'default';
const scale = req.params.scale || '';
const format = req.params.format;
const item = repo[req.params.id];

let spritePath;
if (item && item.spritePaths) {
for (const sprite of item.spritePaths) {
acalcutt marked this conversation as resolved.
Show resolved Hide resolved
if (sprite.id === spriteID) {
spritePath = sprite.path;
}
}
if (!spritePath) {
return res.sendStatus(404);
}
} else {
return res.sendStatus(404);
}

let spriteScale;
const allowedScales = ['', '@2x', '@3x'];
acalcutt marked this conversation as resolved.
Show resolved Hide resolved
for (const as of allowedScales) {
if (as === scale) {
spriteScale = as;
}
}

let spriteFormat;
const allowedFormats = ['png', 'json'];
for (const af of allowedFormats) {
if (af === format) {
spriteFormat = af;
}
}

if (spriteFormat) {
const filename = `${spritePath + spriteScale}.${spriteFormat}`;
return fs.readFile(filename, (err, data) => {
Fixed Show fixed Hide fixed
if (err) {
console.log('Sprite load error:', filename);
Fixed Show fixed Hide fixed
return res.sendStatus(404);
} else {
if (format === 'json')
res.header('Content-type', 'application/json');
if (format === 'png') res.header('Content-type', 'image/png');
return res.send(data);
}
});
} else {
if (format === 'json') res.header('Content-type', 'application/json');
if (format === 'png') res.header('Content-type', 'image/png');
return res.send(data);
return res.sendStatus(400);
}
});
});
},
Fixed Show fixed Hide fixed
);

return app;
},
Expand Down Expand Up @@ -135,27 +178,48 @@
}
}

let spritePath;

if (styleJSON.sprite && !httpTester.test(styleJSON.sprite)) {
spritePath = path.join(
options.paths.sprites,
styleJSON.sprite
.replace('{style}', path.basename(styleFile, '.json'))
.replace(
'{styleJsonFolder}',
path.relative(options.paths.sprites, path.dirname(styleFile)),
),
);
styleJSON.sprite = `local://styles/${id}/sprite`;
let spritePaths = [];
if (styleJSON.sprite) {
if (Array.isArray(styleJSON.sprite)) {
styleJSON.sprite.forEach((spriteItem) => {
if (!httpTester.test(spriteItem.url)) {
let spritePath = path.join(
options.paths.sprites,
spriteItem.url
.replace('{style}', path.basename(styleFile, '.json'))
.replace(
'{styleJsonFolder}',
path.relative(options.paths.sprites, path.dirname(styleFile)),
),
);
spriteItem.url = `local://styles/${id}/sprite/` + spriteItem.id;
spritePaths.push({ id: spriteItem.id, path: spritePath });
}
});
} else {
if (!httpTester.test(styleJSON.sprite)) {
let spritePath = path.join(
options.paths.sprites,
styleJSON.sprite
.replace('{style}', path.basename(styleFile, '.json'))
.replace(
'{styleJsonFolder}',
path.relative(options.paths.sprites, path.dirname(styleFile)),
),
);
styleJSON.sprite = `local://styles/${id}/sprite`;
spritePaths.push({ id: 'default', path: spritePath });
}
}
}

if (styleJSON.glyphs && !httpTester.test(styleJSON.glyphs)) {
styleJSON.glyphs = 'local://fonts/{fontstack}/{range}.pbf';
}

repo[id] = {
styleJSON,
spritePath,
spritePaths,
publicUrl,
name: styleJSON.name,
};
Expand Down
Loading