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

use land-color as background for waterway tunnels #2594

Merged
merged 1 commit into from
May 8, 2017

Conversation

nebulon42
Copy link
Contributor

Ref #488.

Waterway tunnel dashes might disappear depending on background. By using an additional line with land-color as base this does not happen anymore.

Original location of #488:
before
pool_before

after
pool_after

This would also apply to intermittent waterways:
before
before_intermittent_river

after
after_intermittent_river

This can have side-effects when a intermittent river is tagged with a riverbank. Depends if we view that as valid tagging combination.
before
before_intermittent_with_riverbank

after
after_intermittent_riverbank

Also stream tunnels over forests look a bit strange but there is likely some fine-tuning possible:
stream_forest_strange

@nebulon42 nebulon42 requested a review from pnorman March 24, 2017 18:18
@imagico
Copy link
Collaborator

imagico commented Mar 26, 2017

I think for intermittent waterways this goes into the wrong direction.

Both tunnels and intermittent waterways are something less than a normal waterway in terms of visibility and importance for the typical map user. Therefore they should IMO be rendered less prominent than normal waterways. Using land color between the dashes will make them more prominent on many backgrounds. The difference is likely fairly small for tunnels so i consider the change acceptable there although both the current and the new proposed styling are not ideal in terms of relative prominence at the moment.

I am generally not really satisfied with the dashed rendering of intermittent waterways we have at the moment, especially at the low zoom levels. And at the high zooms we should probably revisit that when we implement intermittent water areas - see #996.

Note if we decide to change the water color in a way similar to what i proposed in #1781 (comment) the relative weight of surface waterways vs. tunnels would change (to the better).

The side effect you showed would be solved by the layer reordering i have envisaged for the move to water polygons and differentiated color water rendering (i.e. rendering waterways before water areas).

Copy link
Collaborator

@matthijsmelissen matthijsmelissen left a comment

Choose a reason for hiding this comment

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

Code looks good. Makes sense to do this for tunnels, can you provide a rationale (maybe example rendering where this is an improvement) for wadis?

water.mss Outdated
@@ -103,6 +103,20 @@
[waterway = 'canal'][zoom >= 12],
[waterway = 'river'][zoom >= 12],
[waterway = 'wadi'][zoom >= 13] {
// the additional line of land color is used to provide a background for dashed appearances
Copy link
Collaborator

Choose a reason for hiding this comment

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

tab in the middle of the line?

water.mss Outdated
[intermittent = 'yes'],
[waterway = 'wadi'],
[int_tunnel = 'yes'] {
line-color: @land-color;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe for clarity good to prefix these with land/ or background/ or so?

@nebulon42 nebulon42 force-pushed the water-tunnel-casing branch 2 times, most recently from bf896a9 to 39d45c7 Compare April 2, 2017 20:28
@nebulon42
Copy link
Contributor Author

I have adressed the points raised. For wadis the same as for intermittent waterways applies, therefore I have both removed from this change.

Wadis would have looked like this (likely not a wadi but tagged as one):
wadi

@nebulon42 nebulon42 changed the title use land-color as background for waterway dashes use land-color as background for waterway tunnels Apr 2, 2017
Copy link
Collaborator

@matthijsmelissen matthijsmelissen left a comment

Choose a reason for hiding this comment

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

A couple of technical issues

water.mss Outdated
[zoom >= 18] { background/line-width: 12; }
}

[int_tunnel = 'yes'] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This selector is the same as the selector above

@@ -113,6 +125,7 @@
[zoom >= 18] { bridgecasing/line-width: 13; }
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Spurious whitespace

@@ -166,24 +195,35 @@
[waterway = 'stream'][zoom >= 15] { bridgeglow/line-width: 3; }
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Spurious whitespace in diff

@pnorman
Copy link
Collaborator

pnorman commented Apr 21, 2017

Ping @nebulon42 about the technical comments above.

@nebulon42 nebulon42 force-pushed the water-tunnel-casing branch from 39d45c7 to 970df74 Compare May 1, 2017 14:38
@nebulon42
Copy link
Contributor Author

@math1985 I have merged the redundant filters, thanks for noticing. The additional newlines were intentional. I think they improve readability.

@pnorman pnorman merged commit 850c3fd into gravitystorm:master May 8, 2017
@nebulon42 nebulon42 deleted the water-tunnel-casing branch May 20, 2017 16:40
@HolgerJeromin
Copy link
Contributor

Does this solve / change #405 too?

@nebulon42
Copy link
Contributor Author

No, because the river line is still rendered above the tunnel.

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