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

Single tree and tree_row colors as forest #4448

Merged
merged 1 commit into from
Jul 16, 2022
Merged

Single tree and tree_row colors as forest #4448

merged 1 commit into from
Jul 16, 2022

Conversation

yvecai
Copy link
Contributor

@yvecai yvecai commented Aug 15, 2021

Fixes # (id of the issue to be closed)
#2736
Concerns:
#1753
#2789

Changes proposed in this pull request

  • Change natural=tree and natural=tree_row color from 'green' to the same color as forest
  • Change the trunk color of tree node to the same as the forest svg overlay
  • Add back trees up to z16 given the lower visibility

Rationale

#2736 is closed as integrated in #1753, however no work as been done on the latest for 3 years. This pull request at least solve the outstanding tree colors, I doubt the original #00ff00 was of particular cartographic significance.

The transparency of the canopy has been kept, I guess in urban areas some will be attached to this transparency. #1753 is slightly improved, but not solved. Switching layer order could solve while keeping transparency.

I decided to add back the trees up to zoom 16, this was removed by @kocio-pl in #2789, this is probably subject to discussion. I think the less saturated green dots does not affect so much the map visibility any more. Also, proposing greener towns in august, I hope this will find some support ;-)

Test rendering with links to the example places:

Before / After - Urban area, over various backgrounds Z18
urban_area_z18

Before / After - Transparency kept Z19
transparency_z19

Before / after image showing a tree_row, a hedge and single trees.
tree_row-hedge_intersects_crop

Before / After - In the countryside Z18
meadow_z18

Lower zoom (Z17)
This was change by #2789
Carto now
warsaw_z17_now

Proposed
warsaw_z17_proposed

Carto 2017
warsaw_z17_2017

Lower zoom (Z16)
This was change by #2789
Carto now
warsaw_z16_now

Proposed
warsaw_z16_proposed

Carto 2017
warsaw_z16_2017

@imagico
Copy link
Collaborator

imagico commented Aug 16, 2021

Revisiting the zoom level progression for trees/tree rows/hedges is definitely a good idea - as already indicated in #2789 (comment) #2789 left this in a very inconsistent state.

As far as colors are concerned - rendering individual trees and tree rows in the same color as wood/forest fill is definitely not a good idea - it would make trees within such areas invisible and it would be prone to confusion between tree symbols/tree row line signatures and mapped wood/forest polygon geometries. Of course you don't actually unify the colors, you just make them closer to the wood/forest fill.

Generally speaking - when you use colors with transparency as here you will get a large spectrum of color mixes depending on the color of the background so subtleties in color choice will often not be recognizable. That is why the relatively strong color for tree symbols is a fairly decent choice (though indeed at z16/17 it can be considered to be somewhat noisy). That of course does not mean there are no other and potentially better solutions. #1753 showed a number of ideas. I would in particular consider symbol and line pattern designs that do not use a larger area semitransparent fill worth exploring. Also interesting would be to differentiate by leaf_cycle/leaf_type (2.8M/1.7M combinations with natural=tree).

@yvecai
Copy link
Contributor Author

yvecai commented Aug 16, 2021

Color is why I jumped into this after a message on the french forum, in particular this example that look particularly odd :
tree_Rows_saturated (Z17)

The bare green become overly saturated over a background like meadow. This does not happen with the less vivid variation of the forest color proposed in the pull request.

Also, keeping transparency allows the singular trees to stand out from the forest. The user still have feedback, albeit less prominently:
Actual Z19:
tree_in_forest_actual

Proposed Z19:
trees_in_forest_proposed

I deliberately let the line pattern for tree rows aside (#1753), because I'm not completely convinced a pattern is better than a simple line. See my comment #1753 (comment)

@MerleMoqueur
Copy link

I am the author of the "particulary odd" example of tree_row.

The rendering is ugly in this meadow context and I wonder if tree_row really fits a rural usage. I think it was initially devised for simple tree rows in town or along highways. I intended here to recreate the pattern of trees separating the fields and often joining the nearby forest. There are two problems here: the color and the width of the row.

On the wiki, it is stated that between fields the use of barrier=hedge instead is recommended, but the rendering is even thinner and the area=yes tag isn’t rendered on the standard layer, so it’s not really an option for now.

@yvecai
Copy link
Contributor Author

yvecai commented Oct 11, 2021

It seems there is no big support for this.
Yet, it only propose to have closer colors for forest and singular trees : here be trees.
It is also not revolutionary (transparency is kept where there was, single trees can still be spotted in wooded areas).
Do I miss something?

@imagico
Copy link
Collaborator

imagico commented Oct 11, 2021

I see issues with both the current choice of colors and with what you propose in that regard. With your proposal that is in particular the high risk that at z16/17 distinguishing tree and tree row symbols from mapped wood polygon geometries will often be fairly impossible. It is important for constructive mapper feedback to properly distinguish between individual trees, tree rows and unstructured wood polygons in a well readable form. Given the improvements in the zoom level progression i would in principle still be fine with your change.

As said in #4448 (comment) my preference would be trying out some different design that avoids the color mixing issues of larger area transparency used right now. But that is not something you have to explore, just an idea of what could be an improvement.

Again a reminder to all: Anyone is welcome to review changes like this - test them in different settings, discuss the pros and cons of the change and possibly suggest changes. The maintainers have the final say in if to merge a change or not, but a well argued review will often be quite influential on that.

Copy link
Collaborator

@jeisenbe jeisenbe left a comment

Choose a reason for hiding this comment

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

The color is an improvement because it uses half as much opacity (0.6 instead of 0.3) so there is less color mixing, though it is more similar to forest and wood.

The progression from 2.5 to 5 to 10 width of the tree symbol could give the impression that this is a real width, since it scales exactly with the change in zoom level from z15 to z17. However,that is a minor concern which can be fixed later.

Overall, this PR is providing a benefit and the remaining issues with tree rows can be addressed later. I am inclinded to merge it.

@imagico
Copy link
Collaborator

imagico commented Jun 8, 2022

I concur that if no consensus can be found on a fundamentally better solution not using layer based opacity the benefits of this (in particular also adding back trees at z16 and z17) outweighs the disadvantages.

However i would clearly like to mention that this definitely creates/massively increases the problem that tree/tree row symbols can be mistaken for mapped woodland polygon geometries (hence it would open an issue for that while closing another one).

Having recently worked quite a bit on tree rendering i am pretty sure that for a simple all-trees-the-same-symbol rendering of trees a solution that does not rely on layer based opacity would be possible without the need for the level of code complexity and the resulting performance implications of what i demonstrated here. But someone would need to put in the time and thought to make this work.

@jeisenbe
Copy link
Collaborator

http://blog.imagico.de/a-new-design-for-tree-rendering-in-digital-maps/

That’s pretty nice looking for the basic version:
Screen Shot 2022-06-10 at 11 25 46

How about we merge this since it will get us z16 and z17 again and reduce the opacity some, and then open a new issue suggesting changing the tree_row and tree rendering to try the outline based rendering shown there to avoid the problem of it looking like a forest/wood area. Or you could open a PR directly?

@imagico
Copy link
Collaborator

imagico commented Jun 11, 2022

The rendering i discuss in the blog post is much more complex - both design wise and technically. Adapting the idea on a simpler level with only a single, undifferentiated symbol but avoiding overlaps between symbols would require someone to invest in developing that. This is not rocket science but it is also not trivial.

I don't really have the time to do that but i also think that even if i did that would probably not be a sustainable approach. If there is not a broader developer base interested in working on innovative map design concepts on this style, me pushing for such changes on a few points (like here or in #4121) is not going to have a lot of long term benefit for the project.

Unfortunately this is kind of a hen-and-egg problem. Because OSM-Carto will only attract developers interested in substantially advancing map design if we show commitment and ambition in that field and value excellent and innovative map design work over 'good enough' solutions. And only if we did so we could attract developers with the ability to sustainably maintain a map style on that level of sophistication. At the moment it is not even clear if a more ambitious and innovative solution compared to this (or to #4121) would get consensus support from the maintainers.

So i see changes like this with both a happy and a sad eye - happy, because indeed this is an improvement and in particular rolls back the non-beneficial solutions to limit tree rendering to z18+, but sad because by accepting a design when we know there are much better solutions we are - as a project - in a way giving up on aiming for excellence and accepting to fall behind the state of the art of what is possible map design wise.

@jeisenbe
Copy link
Collaborator

Well, I started out making very small changes to obvious problems in this problem, but I slowly became more interested in developing solutions to bigger problems as I gained more experience. I now agree with you that we should aim for excellence.

But there is also a benefit to making incremental changes, as long as they have a net benefit. I would rather merge PRs which solve 2 significant problems but leave 1 significant problem, rather than wait for a perfect solution.

At the moment it is not clear if a more ambitious and innovative solution compared to this would get consensus support

That’s right, and we won’t find out what the consensus will be until someone submits a PR. I accept that most maintainers are busy with other projects and other parts of their lives, and it takes a real PR to get everyone’s attention.

@yvecai
Copy link
Contributor Author

yvecai commented Jun 19, 2022

I don't have the rendering tool chain ready as of now, so I'm not sure when I can check the trunk opacity thing. So if someone can have a look, you're welcome!

@yvecai
Copy link
Contributor Author

yvecai commented Jun 19, 2022

I must admit that I don't really get the problem with single tree or tree rows that are looking the same as areas.
It seems to me that 'here are tree(s)' is good enough for this style, but maybe I'm wrong.
Maybe it's worth opening a separate issue on this, at least to discuss if it is a problem for this style, or not.

@imagico
Copy link
Collaborator

imagico commented Jun 19, 2022

I must admit that I don't really get the problem with single tree or tree rows that are looking the same as areas.
It seems to me that 'here are tree(s)' is good enough for this style, but maybe I'm wrong.
Maybe it's worth opening a separate issue on this, at least to discuss if it is a problem for this style, or not.

This is not a codified principle so far but we have consensus among the maintainers that we try to avoid the design of point symbols, line signatures, patterns etc. to be easily mistaken as mapped geometries. The 'here are trees' message is not sufficient to serve our goal of constructive mapper feedback. It is important to communicate if those trees are mapped with a woodland polygon, with a tree row or with individual tree nodes. This is a distinction consciously made by mappers in millions of cases. And this in particular applies because individual trees and rows/strips of trees are quite commonly also mapped with woodland polygons - like:

https://www.openstreetmap.org/#map=14/44.7533/40.9303
https://www.openstreetmap.org/#map=14/45.7207/29.3329
https://www.openstreetmap.org/#map=15/52.3849/7.6988
https://www.openstreetmap.org/#map=17/47.45814/8.06097
https://www.openstreetmap.org/#map=16/47.6515/8.1674
https://www.openstreetmap.org/#map=16/47.0967/7.0066
https://www.openstreetmap.org/#map=16/46.0908/5.7856
https://www.openstreetmap.org/#map=15/50.8523/13.8723

Note this is not something that will prevent this change from being accepted, it is just that i liked to point out that this change will introduce/emphasize this as an issue. And - referring to what @jeisenbe rightfully pointed out that there is a gain of course of incremental changes even if the net benefit is small - it is an issue that will not be solved by incremental changes (unless those are essentially rolling back this change), solving this will require a thoroughly planned and executed design effort. This is fine with me but we should keep in mind that just doing incremental changes like this would over time accumulate significant design debt. We need to work also on larger design problems that require more thoroughly planned and executed design work and on creating a suitable environment for designers interested and talented in doing such work here.

@pnorman pnorman mentioned this pull request Jul 7, 2022
Copy link
Collaborator

@pnorman pnorman left a comment

Choose a reason for hiding this comment

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

This is an improvement, and these changes won't interfere with any future work that someone might want to do to redo tree and forest rendering, so there's no design debt we're taking on here.

@pnorman pnorman merged commit 9f00539 into gravitystorm:master Jul 16, 2022
@imagico
Copy link
Collaborator

imagico commented Jul 16, 2022

For the record - i like to explain the context of use of the term design debt here.

The change in color implemented here tries to solve a certain issue with the design of trees so far with an incremental adjustment of color. Doing so resolves the problem identified but by introducing a new problem (that trees and tree rows in their rendering can be confused with mapped woodland polygon geometries from which they are often only distinguishable by shape and not by color - and this being in conflict with the design paradigm to design point symbols and line patterns in a way that cannot easily be mistaken for mapped geometries).

The design concept we use for tree/tree row rendering to deal with the difficulty of handling overlapping symbols with layer level opacity inherently makes it impossible to solve both of these problems. That means this change is design wise a dead end. There are no incremental changes from here to solve the remaining problems without essentially reverting this change and re-introducing the problem it tries to solve.

In other words: What the color change here does is essentially refinancing design debt. Therefore @pnorman can be considered to be formally correct here that there is no additional design debt taken on here.

As i have pointed out on many occasions (like in #3951) however, we have for a long time not managed to find consensus on most larger design decisions and as a result almost every non-incremental design change is stalled. A large fraction of design work that is done (and also a large fraction of the scarce work time of designers and maintainers available) is spent on exactly that: Restructuring existing design debt and shifting existing problems from one side to the other with incremental changes without actually addressing the underlying causes.

So the consideration that this change does not take on any truly new debt is somewhat besides the point IMO. Like it is with debt - it will only ever increase, unless we accomplish to do a haircut (that is what #3977 is meant to facilitate) or we actually manage to clear off substantial debt (which requires working on consensus needed Indicates the lack of consensus among maintainers blocks a PR/issue ).

The change of re-introducing the rendering at lower zoom levels is unrelated to these considerations and is the main reason why i am fine with merging this.

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.

5 participants