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

Render dashed lines for all railways under construction in the electrification layer #69

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

JonathanBittner
Copy link
Contributor

@JonathanBittner JonathanBittner commented Mar 2, 2022

This PR fixes #47 where certain lines under construction show as completed lines in the electrification layer. I also addressed as a quick fix (not) # 42 but it is not as comprehensive as PR #57.

This wasn't a straightforward fix and required a bit of tweaking. We may also want to address the color scheme: the dashed pattern of grey and color for lines under construction does not render well on thinly weighted light rail lines.

Some of these issues were caused by poor or ambiguous tagging where there is a mix of regular (electrified,voltage,frequency) and construction: tags. While some of these issues can be corrected in the project code, we should not attempt to become an AI bot and "do what the user meant." However, I will take advantage of certain fall throughs if they produce the desired result. An improper fall through in railway_electrification_state() sql/functions.sql triggered several of the issues.

Here are some of the behaviors I have documented:

  1. [OK] extant line, electrification upgrades under construction (railway=rail; construction:electrification=rail/contact_line/4th_rail => renders dashed line alternating between black and the assigned electrification color; example: Caltrain San Francisco to San Jose
  2. [OK] line under construction, properly tagged construction:electrified/construction:voltage/construction:frequency => renders dashed grey + assigned electrification color; example: here
  3. [BAD!] non-electrified line under construction, construction:electrified=no => renders as completed line/ambiguous electrification (solid grey); example: NJ Transit Lackawanna Cut Off Expansion; Desired behavior: render as dashed black.
  4. [BAD!] electrified line under construction, electrified/voltage/frequency specified without construction: prefix => renders as a solid line in the as if the line were completed; example: the Finland case in Desired behavior: render as broken line with electrification colors (anticipating user needs) OR render as a dashed line without colorization indicating line is under construction. This could be corrected by fixing the tagging but I imagine we'll see many other ways with this style of tagging. The example here was taken from Electrification Layer: Track which is under construction is displayed as "electrified" OpenRailwayMap#718
  5. [BAD!] line under construction, ambiguously tagged (e.g. electrified=yes) => renders as solid grey; example here We don't necessarily have a desired behavior for this, but the line should be dashed In this PR I propose interleaving the current grey fill color with an additional darkgrey fill color to mix grey/darkgrey
  6. around Greenboro light rail station renders solid black to the north of the station where it is fully tagged (construction:railway=light_rail; construction=light_rail; railway=construction) but is also tagged electrified=no. The line just south of the station has no electrification tagging and renders solid grey. Both lines should be dashed.

All 6 examples now render properly.

normal line

These changes cause all lines under construction to be displayed with
dashes in the electrification layer, regardless of electrification
state.
@DerDakon DerDakon requested a review from Nakaner March 3, 2022 06:05
@epimorphic
Copy link
Contributor

epimorphic commented Mar 3, 2022

I'm not sure about labeling example 2 as "OK" -- it seems to suggest the line already exists, just with incomplete tagging of the present electrification status. Having only the dashed line for the future electrification, without the background gray "no information" line, seems better to me. (The white casing around the fill lines might be excessive for half-transparent dashed lines though, so I've been playing around with opacity for casing as well.)

Also the PR as it stands turns Switzerland (and I'd imagine many of its neighbors) into a sea of yellow. (Which is something that also happened in the course of the US/Japan PR. Really, all the more reason to finally do something about that one.)

@JonathanBittner
Copy link
Contributor Author

Very good comments @epimorphic I think it might be better to go back into the CSS for the base layer and turn off rendering lines under construction entirely and allow the second layer (#electrification_future) to do all the rendering for those lines.

I agree with the Switzerland issue: I made the issue worse than before so it no longer matches the colors on the website. Really #57 needs to land first and then I can rebase and resubmit this.

@JonathanBittner JonathanBittner marked this pull request as draft March 3, 2022 20:43
@JonathanBittner
Copy link
Contributor Author

I agree with @epimorphic: going to a white background on new lines under construction is a good idea. The gauge layer already does this. By doing this, i can likely simplify both the SQL and CSS. Give me a few more days...

Lines under construction now properly render in the electrification
layer with dashing.  Corrects issue OpenRailwayMap#47.
@JonathanBittner JonathanBittner marked this pull request as ready for review March 7, 2022 17:53
@JonathanBittner
Copy link
Contributor Author

JonathanBittner commented Mar 7, 2022

I started this one anew. It will no longer attempt to correct issue 42, let PR #57 handle that.
I tried to simplify things but in the end things got a little more complicated.

I changed how the layers handle construction: lines under construction are handled in the railway_fill_layer and are set to dashed. Color is assigned based on electrification/no electrification/unknown electrification New lines under construction will no longer be handled by the electrification_future layer. This layer will exclusively handle existing lines that are being upgraded to electrified (or are proposed to be upgraded).

Lines under construction will appear on a white background, which is consistent with the Gauge layer.

While the CSS has become more simplified compared to my last submission, the SQL became a bit more complicated. New states were added to railway_electrification_state() to remove ambiguities. The states are "present," "no," null (undefined/ambiguous), "construction_now" (new, indicating a line under construction), "construction_future" (new, indicating a line being upgraded), and "proposed_future" (renamed from "proposed" for consistency. There is no "proposed_now" state as the previous version did not render proposed new lines at all.

In addition, I had to make some more changes to the SQL to ensure construction was rendered in oblique fonts and certain pass-through cases were handled accordingly and the voltages and frequencies were captured correctly. What do I mean by this? For lines being built that have electrification included, there is quite a bit of mistagging in the database. The present code handled this reasonably well: rather than cause more breakage and disruption to the map, I added some additional handling to continue to capture these values. For instance a light rail line is under construction. It was tagged railway=construction, construction=light_rail, and construction:railway=light_rail. On the electrification side, it was tagged: electrified=contact_line, voltage=750, and frequency=0. It would be more correct to tag construction:electrified=contact_line, construction:voltage=750, and construction:frequency=0, but as I mentioned there are many lines tagged this way and we should be able to handle them

electrification.mss Show resolved Hide resolved
electrification.mss Show resolved Hide resolved
@DerDakon
Copy link
Contributor

I think it's time to update this as at least #57 has already been merged. Maybe we can even get an example rendering for a place where this is currently be wrong and then fixed by this commit?

@DerDakon DerDakon added the style:electro electrification style label Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement style:electro electrification style
Projects
None yet
Development

Successfully merging this pull request may close these issues.

electrification layer renders railway=construction as normal line
3 participants