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

feat: option to translate labels for static maps #999

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Caerbannog
Copy link
Contributor

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:

  • Add a new line in config.json: languages: ["fr", "it", "de"]
  • Use a data source that contains translated labels (e.g. name:fr keys). If you use planetiler to generate MBtiles, it works.
  • Add a new URL parameter to static map URLs: ?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:

  • Allow translating labels on raster tiles.
  • It might be possible to delay style loading, loading only for each requests. This would allow removing the languages config and increase efficiency. Currently there is only one worker for each (scale, language) pair.
  • Use the Accept-Language header to select a language automatically.

Miscellaneous improvements included:

  • On the homepage, add a link to the static map endpoint
  • Show more logs when --verbose is passed on the command line

Examples

/styles/basic-preview/static/1.53979,46.33373,6/500x400.png (previous behavior)
image

/styles/basic-preview/static/1.53979,46.33373,6/500x400.png?language=it
image

@acalcutt
Copy link
Collaborator

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.
https://github.com/acalcutt/wifidb-tileserver-gl/blob/master/tileserver-gl/styles/WDB_OSM/style.json#L4747-L4783

@Caerbannog
Copy link
Contributor Author

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.
Alternatives if we find problems : mapbox-gl-language, maybe osm-americana.

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.
Now, all these tests were base on my own test mbtiles. We still need to test with other mbtiles structures, including yours.

http://localhost:8080/styles/WDB_OSM/static/1.15,47.03,5.7/500x500.png
image

http://localhost:8080/styles/WDB_OSM/static/1.15,47.03,5.7/500x500.png?language=fr
image

http://localhost:8080/styles/WDB_OSM/static/1.15,47.03,5.7/500x500.png?language=en
image

@acalcutt
Copy link
Collaborator

acalcutt commented Oct 1, 2023

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]>
Comment on lines +1533 to +1541
for (const language of languages) {
map.renderers_static[`${s}${language}`] = createPool(
s,
'static',
language ? 1 : minPoolSize,
language ? 1 : maxPoolSize,
language,
);
}
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

@acalcutt acalcutt Oct 2, 2023

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

Copy link
Contributor Author

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?

Copy link
Collaborator

@acalcutt acalcutt Oct 3, 2023

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.

Copy link
Contributor

@tdcosta100 tdcosta100 Oct 3, 2023

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.

@Caerbannog Caerbannog force-pushed the translate-style branch 3 times, most recently from 3d55982 to 5b24379 Compare October 11, 2023 14:40
@Caerbannog
Copy link
Contributor Author

Caerbannog commented Oct 13, 2023

I have paused work on this PR.
I'm trying to contribute to maplibre-native so that we can update the ratio and the mode of an existing map.
This would reduce the size of our render pool.

@Caerbannog Caerbannog marked this pull request as draft November 24, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants