gravitystorm / openstreetmap-carto

A general-purpose OpenStreetMap mapnik style, in CartoCSS
Other
1.55k stars 822 forks source link

Add rule to render landuse flowerbed #4889

Closed MeonStudios closed 1 year ago

MeonStudios commented 1 year ago

Fixes #4564

Changes proposed in this pull request:

I saw in the discussion of issue #4564 that it was generally accepted that this should be rendered, but that no right design has been choses yet. I came across PR https://github.com/gravitystorm/openstreetmap-carto/pull/4251, but I saw it was abandoned & closed so I created a new PR.

I would like to thank @daganzdaanda for providing some nice inspiration of what pattern I could use.

Test rendering

Before

The surrounding area is tagged with leisure=garden, there is no other tagging for the flowerbeds so the big surrounding polygon leisure=park took over the rendering

image

After

landuse=flowerbed is rendered

image

Before

landuse=flowerbed in grass & residential area, the residential area background is rendered

image

After

landuse=flowerbed in grass & residential area, the rendered flowerbeds blend in nicely with the grass and are clearly visible

image

This is my first PR, so please let me know if you need anything more.

jxpsert commented 1 year ago

Looks nice! Quite subtle, but not invisible. Looks like a good addition to Carto to me.

MeonStudios commented 1 year ago

Another before and after:

Before

Here we see a slightly less "dense" part with places where there are no rendered tags

image

After

Now we see that the flowerbeds are rendered

image
imagico commented 1 year ago

Thanks for working on this.

I think this is going in a good direction. But you might want to try if making the pattern a bit less coarse would make it work better on small grained mapping at the lower zoom levels where many flower beds will not be large enough to show even a single full flower symbol with your design. This might negatively affect readability of the individual symbols - but it would still be worth trying IMO.

Another option is to use a non-pictorial structure pattern (examples were shown in #4564) at the mid zoom levels and only use pictorial symbols at the highest levels (like z16/z17+). We have no precedent for this design approach in the style so far but we also so far are not rendering any similarily micromapping oriented features with a pattern.

And you should include instructions how to generate the pattern used to facilitate potential changes in the future.

MeonStudios commented 1 year ago

Hi thanks for the pointers, I like the idea to use structure patterns at mid zoom levels and only pictorial symbols at the highest zoom levels. I have one question, what exactly do you mean by "making the pattern a bit less coarse"?

I will add some instructions on how to generate the pattern. I also found that the process of creating a svg pattern to use was quite hard to know how to get it all to work properly and I wanted to add some documentation for that. I can add that to this PR or create a new PR for that.

MeonStudios commented 1 year ago

I added a new non-pictorial structure pattern for lower zoom levels and it looks like this Zoom level 15

image

Zoom level 16

image

Zoom level 17

image

Zoom level 18

image

Another option I was thinking about is only rendering the symbol on zoom level 17+ and just the grass colour polygon fill before

imagico commented 1 year ago

I have one question, what exactly do you mean by "making the pattern a bit less coarse"?

That means smaller symbols in a combination with a denser grid.

MeonStudios commented 1 year ago

I updated the PR, please review again

The only thing I'm doubting a bit about is if I should render the symbols starting from layer 17 or 18. For larger flowerbeds it isn't really an issue, but smaller flowerbeds could benefit from it.

Example

zoom 17

image

zoom 18

image
MeonStudios commented 1 year ago

Example for rendering symbols only on layer 18+

Zoom level 17 smaller flowerbeds

image

Zoom level 17 bigger flowerbeds

image

My personal preference is to render symbols for layer 17+

MeonStudios commented 1 year ago

I would suggest to change the layout of the mid zoom pattern to match that of the high zoom pattern, not in density, but in principal arrangement. The high zoom pattern uses a grid that is not axis aligned (by rotating a triangle pattern by 45 degrees) - the mid zoom pattern uses an axis aligned triangle grid. This is non-ideal for communicating that both are depicting the same feature to the map user. You could use something like:

https://imagico.de/map/jsdotpattern.php#x,128,jdp90565;gt,4,32,32;rd,0,0,0,dot,0.125,4,4,0,jdp8462,eef6c0,cdebb0;

The link you sent is exactly the same pattern that I used in the mid zoom so I don't understand what you mean there.

When I try to rotate that pattern it also brings the dots closer somehow. I fiddled a bit and I think this is quite close: https://imagico.de/map/jsdotpattern.php#x,128,jdp47459;gv,6,32,32;tr;ts;rd,0,0,0,dot,0.125,4,4,0,jdp75205,eef6c0,cdebb0;

I could also just remove the angle from the symbol rendering but personally I like the organic (but orderly) look

imagico commented 1 year ago

Sorry, copy and paste error, i meant to link to

https://imagico.de/map/jsdotpattern.php#x,128,jdp57983;gv,6,32,32;tr;rd,0,0,0,dot,0.125,4,4,0,jdp91827,eef6c0,cdebb0;

MeonStudios commented 1 year ago

Ah that's funny, you came up with exactly the same pattern 😄. Great minds think alike I guess.

I updated the PR per your review

imagico commented 1 year ago

Merged, thanks for bringing this over the finish line, thanks also to @liotier for the initial attempt at rendering this is 2020 and to @hungerburg and @daganzdaanda for their design work in #4564.

MeonStudios commented 1 year ago

Great! Thanks for your quick feedback and reviews, motivates me to pick up more issues.

Question: how long do we need to wait (approx.) to see the effects of this PR on https://www.openstreetmap.org/ with the Standard Carto layer enabled?

imagico commented 1 year ago

That depends on when we are going to have the next release, which we have not done very regularly more recently. Right now we definitely would need to fix #4881 before.