-
Notifications
You must be signed in to change notification settings - Fork 638
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
feat: option to translate labels for static maps #999
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Martin d'Allens <[email protected]>
Code wise this looks good to me. I see several fixes included too. It may take a bit for me to test this. I do worry about compatibility with more complex styles. for example, in my style I have this ugly ordered preference of different names, with a preference towards english. |
The translation code from Maptiler (linked above) seems generic, but I didn't try to understand it. I have loaded your WDB_OSM style to test it. I used my own test mbtiles (the same as for the above screenshots) in place of your sources, and removed all layers that had problems (ex: your hillshading needed a raster source). You can find below static maps I generated with your style. Translations seem to work great. http://localhost:8080/styles/WDB_OSM/static/1.15,47.03,5.7/500x500.png http://localhost:8080/styles/WDB_OSM/static/1.15,47.03,5.7/500x500.png?language=fr http://localhost:8080/styles/WDB_OSM/static/1.15,47.03,5.7/500x500.png?language=en |
It looks like the lint part of CI is failing. could you run "npm run lint:js:fix" on this. |
Signed-off-by: Martin d'Allens <[email protected]>
for (const language of languages) { | ||
map.renderers_static[`${s}${language}`] = createPool( | ||
s, | ||
'static', | ||
language ? 1 : minPoolSize, | ||
language ? 1 : maxPoolSize, | ||
language, | ||
); | ||
} |
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 see why each language needs its own rendering pool. shouldn't the generic static renderer be able to handle any style you sent to it with renderer.load(styleJSON);
.
If you wanted more pools due to supporting more languages, you could always still set the maxPoolSizes option.
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'd caution again adding to many more pools than the user specifies. I think I already pushed that a bit doubling the pool size for tile/static mode,
Sometimes that maxPoolSizes is important for lower power systems, which need to limit the number of cores that can be used.
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.
Yeah, pool handling was the main point where I had doubts. I suspected the lower power constraint, but needed confirmation and details, so thanks.
I haven't tried calling renderer.load()
before each render. That's what you are suggesting, right? I was afraid of the performance penalty. That's the only reason we have separate pools for scale ratios and for 'tile' vs 'static', right? Instead of one big pool with dynamic configuration before each render.
If I remember correctly, renderer.load()
causes requests of some assets.
What should I investigate first in your opinion?
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.
The separate pools for static vs tile mode have two do with two issues.
when used as a tile server, maplibre-native tile mode is important to prevent label clipping. when in tile mode, maplibre considers information beyond the edges of the tile, which helps with labels that may go beyond the edge from the next tile (note that the source has to support this bit of extra data, like plantiler and openmaptiles tools do , but tilemaker does not)
maplibre/maplibre-native#284
However we have found that recent changes in maplibre break static images when in tile mode. the reason is that a change has been made when it was still mapbox-native that limited tile mode to only return a single tile. since a static image can span multiple tiles, the resulting image ends up with missing data. to work around this I made static images render in maplibre 'static' mode, which can span multiple tiles. (but does not deal with label clipping in any way)
#608
acalcutt#5
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.
Yes, I don't doubt we need to have different static and tile modes.
But why can't we have a single pool that constructs a new mlgl.Map
for each render? Is that really slow?
I see now that the situation is different for the style (.load()
method) than it is for the mode and the scale (which have to be set in the Map
constructor). But if it's slow to build new mlgl.Map
dynamically, is calling .load()
slow too?
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 not sure on the performance question about mlgl.Map each render. I do know from basic testing on a raspberry pi that lowering the pool value helped a lot. tileserver-gl defaults to a pool value of 8, but that little pi ran better with a pool of 4, because without that when someone when to a page and had to render a bunch of images, the 4 core cpu was pegged beyond 100% and just decreased the speed of the rendering for all images. at least set to 4 the pi could handle it, and it actually resulted in it rendering faster. I always assumed the pooling was meant as a limiter.
As for callling load, that is what i does now so it seems fine. i guess the question would be does putting the translate down in front of it really slow it down much?
The pooling may also have to do with the way maplibre-native in very linier in the way it operates. I know I had done some basic testing at https://github.com/acalcutt/maplibre-node-test/ and just getting it to do multiple images in a row was a pain. I was working with @tdcosta100 just trying to understand how it worked.
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.
The pool is needed because you cannot render multiple images on the same map, that is, each map can render ony one image at a time. And you need different map instances for each resolution, because it's something you configure when instantiating the map and cannot change that later. That said, there are some strategies for rendering different styles (or the same style with different language). Creating lots of instances (using several pools) can lead to consuming lots of memory. One solution is loading the style before the rendering pass, like @acalcutt suggested. This will lead to cache to be cleared everytime, so you will may have performance issues. But I think it's better than creating lots of map instances, unless you specify minPoolSize as 0, so you will create map instances only if you really use them.
3d55982
to
5b24379
Compare
I have paused work on this PR. |
This is a new feature for static maps that allows translating the labels to a language that is specified in the URL.
This is optional, it won't affect existing URLs. Also, if a language is not allowed or if no translation is available, it should fall back on the previous behavior.
Requirements:
languages: ["fr", "it", "de"]
name:fr
keys). If you use planetiler to generate MBtiles, it works.?language=fr
It works by transforming the selected style to replace label expressions.
The styles are transformed during startup because that is when the renderers currently load the style. I didn't have enough knowledge of the codebase to change that.
The translation code was imported from another Maptiler with a compatible license:
https://github.com/maptiler/maptiler-sdk-js/blob/c5c7a343dcf4083a5eca75ede319991b80fcb652/src/Map.ts#L579
Possible future improvements:
languages
config and increase efficiency. Currently there is only one worker for each (scale, language) pair.Accept-Language
header to select a language automatically.Miscellaneous improvements included:
--verbose
is passed on the command lineExamples
/styles/basic-preview/static/1.53979,46.33373,6/500x400.png (previous behavior)
/styles/basic-preview/static/1.53979,46.33373,6/500x400.png?language=it