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

Unpaved no access #4137

Closed
wants to merge 2 commits into from
Closed

Conversation

sommerluk
Copy link
Collaborator

Fixes #110

Closes #3399 (alternative version)

Changes proposed in this pull request:

  • Render paved/unpaved differently: Unpaved roads with a pattern

This PR is an alternative version of #3399 that does not render access on roads any more, to simplify the overall rendering of roads. (It does not change, however, the access rendering for tracks, footways and so on, which follows a completely different approach than access rendering on roads.)

@sommerluk sommerluk mentioned this pull request May 1, 2020
@imagico imagico mentioned this pull request Jul 10, 2020
@jeisenbe jeisenbe added the roads label Jul 26, 2020
@kocio-pl
Copy link
Collaborator

Are there any opinions about this code? If I understand it right, it removes the only thing that has been disputed lately, so I guess it could be merged soon.

@kocio-pl
Copy link
Collaborator

Since I'm again able to run development environment on my computer with new Ubuntu, there are test images of unpaved roads in the wild:

residential [ https://www.openstreetmap.org/#map=19/52.23962/20.93215 ]

Screenshot_2020-09-25 OpenStreetMap Carto — Kosmtik

tertiary [ https://www.openstreetmap.org/way/204584341#map=18/52.09157/21.37662 ]

Screenshot_2020-09-25 OpenStreetMap Carto — Kosmtik(1)

@kocio-pl
Copy link
Collaborator

primary [ https://www.openstreetmap.org/#map=18/11.15067/119.39736 ]

Screenshot_2020-09-25 OpenStreetMap Carto — Kosmtik(3)

secondary [ https://www.openstreetmap.org/#map=16/14.2736/120.7760 ]

Screenshot_2020-09-25 OpenStreetMap Carto — Kosmtik(4)

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.

I do not find residential, tertiary or primary easily distinguishable, and find secondary marginal.

I also don't feel that this cartography fits with the rest of the style.

I haven't done a line-by-line review of the MSS but am concerned about adding this much code and complexity to a difficult part of the codebase.

@kocio-pl
Copy link
Collaborator

I do not find residential, tertiary or primary easily distinguishable, and find secondary marginal.

I also have the hard time seeing the difference, but fixing this problem would be quite simple.

I also don't feel that this cartography fits with the rest of the style.

I'm surprised to hear that. In my opinion pattern rendering on areas is well-established and consistent with the whole style.

I haven't done a line-by-line review of the MSS but am concerned about adding this much code and complexity to a difficult part of the codebase.

That is a valid concern. On the other hand there is high demand for this feedback, which could help mitigate roads mistagging - see #110 (comment).

@sommerluk
Copy link
Collaborator Author

Rebased to current master.

@sommerluk
Copy link
Collaborator Author

I would really like to move that forward.

Regarding the MSS file: Of course, this PR adds code, but the code is not complex. Remember that Mapnik has kindly implemented pattern renderings on line strings because of our request, so that now we have straightforward code.

Regarding the pattern being distinguishable, I could provide a version with more contrast, if desired. Should I do so?

@imagico
Copy link
Collaborator

imagico commented Aug 7, 2022

I would really like to move that forward.

I very much support that, either this or #3399.

Regarding the pattern being distinguishable, I could provide a version with more contrast, if desired. Should I do so?

I think this as now is a good balance between clear readability and not being too bold to illustrate a secondary distinction and not messing too much with the overall balance of the map.

@sommerluk
Copy link
Collaborator Author

Closing this in favour of #3399, to centralize discussion and not have too many open PR:

@sommerluk sommerluk closed this Aug 19, 2022
@sommerluk sommerluk deleted the unpavedNoAccess branch August 24, 2022 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Render paved/unpaved
5 participants