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

Restored rendering for railway=platform + covered=yes #4797

Merged
merged 2 commits into from
Oct 14, 2023

Conversation

map-per
Copy link
Contributor

@map-per map-per commented Mar 27, 2023

Fixes #4008

Changes proposed in this pull request:

  • Restored rendering for railway=platform + covered=yes

Test rendering with links to the example places:
https://www.openstreetmap.org/way/160042477 (left before, right after)
platform19
platform18

@pnorman pnorman requested a review from jeisenbe March 28, 2023 01:31
@map-per
Copy link
Contributor Author

map-per commented May 7, 2023

Is there anything I can do to move this pullrequest forward?

@map-per
Copy link
Contributor Author

map-per commented May 28, 2023

@pnorman & @imagico: Is @jeisenbe still active and doing reviews?

@map-per
Copy link
Contributor Author

map-per commented Jun 30, 2023

Can someone please review this (@pnorman & @imagico)

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 sorry it took us so long to review this.

The main reason is probably that the style is quite a mess w.r.t. the whole subject of multilayered transportation infrastructure complexes and none of us seems very keen to touch that.

As @jeisenbe commented in #4008 (comment) the change hiding railway=platform + covered=yes was an ad-hoc change made without consensus on a bigger strategy.

I would be fine in principle with restoring specifically display of railway=platform + covered=yes, even if that is kind of another ad-hoc change on top of this because it indeed seems that hiding based on this tag specifically is highly problematic for mapper feedback.

However, i think this should fix at least the main inconsistencies in selective hiding of features and not introduce any new inconsistencies. Here is how current master renders the various variants:

master

And here how this change affects this:

this

The inconsistencies between linear way and polygon mapping should be resolved. I also would prefer it if we would treat highway=platform and railway=platform identically - but i am not tied to that.

Beyond that - we really need to develop an overall strategy how to handle multilevel transportation complexes and mapping of multi-level and underground features in those. How such a strategy can look like will depend on how much additional complexity we are willing to accept to accommodate that in the style and i am not sure if we are able to achieve consensus on that.

@map-per
Copy link
Contributor Author

map-per commented Jul 12, 2023

Thanks for the review

@map-per
Copy link
Contributor Author

map-per commented Aug 15, 2023

Ok, resolved the inconsistencies for railway=platform. Now linear platforms with covered=yes are displayed and platforms with location=underground are hidden.

Examples

Platform with covered=yes and without tunnel=yes or location=underground is shown (https://www.openstreetmap.org/way/166482810)
platform1

Platform with location=underground is not shown (https://www.openstreetmap.org/way/432808388)
Bildschirmfoto_2023-08-15_20-27-53

Platform with location=underground and tunnel=yes not shown (https://www.openstreetmap.org/way/432808388)
Bildschirmfoto_2023-08-15_20-29-52

@map-per map-per requested a review from imagico October 1, 2023 09:27
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.

Works as intended now:

this

And as said above i see this as a definitive improvement.

@map-per
Copy link
Contributor Author

map-per commented Oct 14, 2023

Great, can you merge it? Or are you waiting for others to review the PR as well?

@imagico imagico merged commit eec4e9b into gravitystorm:master Oct 14, 2023
2 checks passed
@map-per
Copy link
Contributor Author

map-per commented Oct 14, 2023

🎉

@imagico imagico mentioned this pull request Nov 10, 2023
@map-per map-per deleted the platform branch November 11, 2023 15:29
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.

covered tagging/rendering for railway=platform
2 participants