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

Add rule to render landuse flowerbed #4889

Merged

Conversation

MeonStudios
Copy link
Contributor

Fixes #4564

Changes proposed in this pull request:

  • Add rendering for tag landuse=flowerbed

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 #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
Copy link

jxpsert commented Oct 26, 2023

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

@MeonStudios
Copy link
Contributor Author

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
Copy link
Collaborator

imagico commented Oct 26, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

imagico commented Oct 26, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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+

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am mostly fine with this but i would encourage reviews from others for possible improvements in design. This is very close to some of the ideas i had presented on #4564 so i am a bit biased and there were other promising concepts tried there.

One of the SVGs contains stroke styling which should be removed:

style="fill:#eef6c0;stroke-width:0.125"

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;
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;

Here the current state in native resolution rendering:

z14:
z14
z15:
z15
z16:
z16
z17:
z17

project.mml Outdated Show resolved Hide resolved
style/landcover.mss Outdated Show resolved Hide resolved
@MeonStudios
Copy link
Contributor Author

MeonStudios commented Oct 27, 2023

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
Copy link
Collaborator

imagico commented Oct 27, 2023

@MeonStudios
Copy link
Contributor Author

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

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now from my perspective. Further reviews from others and ideas for improvements are still welcome.

@imagico imagico merged commit fe6fd98 into gravitystorm:master Nov 2, 2023
2 checks passed
@imagico
Copy link
Collaborator

imagico commented Nov 2, 2023

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
Copy link
Contributor Author

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
Copy link
Collaborator

imagico commented Nov 2, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please render landuse=flowerbed
4 participants