maplibre / martin

Blazing fast and lightweight PostGIS, MBtiles and PMtiles tile server, tile generation, and mbtiles tooling.
https://martin.maplibre.org
Apache License 2.0
2.33k stars 216 forks source link

feat: add `sdf` sprites via additional apis #1492

Closed CommanderStorm closed 1 month ago

CommanderStorm commented 3 months ago

Resolves https://github.com/maplibre/martin/issues/1075

I am not quite right if I have made everything correctly. The project has lots of places to configure things and it was not always clear where I need an Option.

~Another thing which is likely wrong: I'd assume as far as I read the blessing that should require new files... Something is really odd here => need to debug that before public review makes sense~ I fucked up the testcase a bit. This is now fixed

nyurik commented 1 month ago

Thanks, this is an awesome feature to have. One thing I kept thinking while reviewing: would it be possible to NOT have any of the associated config values, and instead simply allow both sdf and non-sdf usage? It's not very difficult to run through the spreet crate twice and generate two spritesheets, and this would simplify usage -- either use /sprite or /sdf (or some other prefix)?

TBD:

CommanderStorm commented 1 month ago

Honestly: I have not see a difference between the pngs and sdf images, except for the sdf: true metadata.

I need to look into if this is possible, but I think we could just activate this automatically if users are using monochrome icons.

Using a different url is fine too. You likely don't want to mix the monochrome, but color-configurable sdf images and the regularly colored pngs, but that solution achieves the goal. What do you think about /sprites/sdf/.. instead? They are still sprites, so having that under this url is likely better.

Will have to test if this would be a breaking change though ^^ => Will rework this today

Answering your questions:

In any case, I need to make this more clear in the docs, why one would choose the one or the other image format.

hiddewie commented 1 month ago

Hi 👋🏻, chipping into this nice PR because it would be very helpful for improving the symbol rendering one of my projects using the MapLibre stack (https://github.com/hiddewie/OpenRailwayMap-vector).

I have not see a difference between the pngs and sdf images, except for the sdf: true metadata.

Testing the output from the spreet commandline shows a difference for me when rendering the same directory

spreet symbols/general sprites

gives me sprites

while

spreet symbols/general sprites --sdf

gives me sprites

(colorless and with the distance field visible).

So the Martin SDF sprites should output something similar when using the spreet library.

One thing I kept thinking while reviewing: would it be possible to NOT have any of the associated config values, and instead simply allow both sdf and non-sdf usage?

It would be useful for me to have both normal and SDF sprites output and available for map rendering. The first thing I can think of is rendering the same symbol twice, one SDF with the icon outline, and the colorful icon on top to automatically generate a (fixed color) halo around a symbol.

And the other reason mentioned above, to render both colored and monochrome variants of the same image on the map is also useful.


The combination of a colorful image together with the SDF data would be ideal, but I don't think that is supported in a single sprite image by any of the libraries or SDF sprite consumers.

Maybe there could be a way to change the consuming map rendering libraries to accept two paths of the same sprite resource, one for a colorful image, and one for the SDF image, and the rendering library could combine the information to render colorful icons together with an icon halo.

CommanderStorm commented 1 month ago

I have not see a difference between the pngs and sdf images, except for the sdf: true metadata.

That was user error. Thanks for noticing, that feature would have not worked.

I have raised a PR documenting the behaviour upstream https://github.com/flother/spreet/pull/85 and fixed the bug in my code ^^

CommanderStorm commented 1 month ago

Have changed it to /sdf_sprite/.. as far as the choices prefered by you, that is the best imo.

The reason why I introduced it as /sprite/sdf/.. without any backwards compatibiltiy considerations is the following:

The default regular expression associated with a replacement marker [^/]+ matches one or more characters which are not a slash. For example, under the hood, the replacement marker {foo} can more verbosely be spelled as {foo:[^/]+}.

https://actix.rs/docs/url-dispatch/#generating-resource-urls

=> even if a sprite was previously named "sdf", it could not match the new routes

nyurik commented 1 month ago

Thx for changing - i do think it looks a bit better now, but if anyone has other thoughts, please chime in. @CommanderStorm could you add some tests to the test.sh? Make sure to run just bless afterwards.

CommanderStorm commented 1 month ago

could you add some tests to the test.sh?

Shure thing, have added it

hiddewie commented 1 month ago

Great work @CommanderStorm, this gives nice results on my maps!

CommanderStorm commented 3 weeks ago

We finally also had the time to finish the refactoring. A bit of eye candy:

image