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

Water slide #3346

Merged
merged 2 commits into from
Aug 21, 2018
Merged

Water slide #3346

merged 2 commits into from
Aug 21, 2018

Conversation

johsin18
Copy link
Contributor

Fixes #3335

Changes proposed in this pull request:

  • Render attraction=water_slide similar to a (water) stream
  • Render a marker for water slide
  • Render the name of the water slide

This is more a proof of goodwill, probably both the rendering and the code have to be further improved, as I'm a complete newbie to this.
In particular, the marker is hidden unter the slide, how can I fix that? However, I'm not so sure the marker makes sense, anyway.

Test rendering with links to the example places:
https://www.openstreetmap.org/way/353735844

Before
waterslide_before

After
waterslide19
waterslide18
waterslide17
waterslide16
waterslide15

water.mss Outdated
[attraction = 'water_slide'] {
[zoom >= 15] {
[zoom >= 17] {
bridgecasing/line-color: saturate(darken(@water-color, 40%), 30%);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'm not sure what the prefix "bridgecasing" references to here. Is it really needed?

water.mss Outdated
[zoom >= 18] { bridgecasing/line-width: 3.5; }
[zoom >= 19] { bridgecasing/line-width: 7; }
}
water/line-color: @water-color;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'm not sure what the prefix "water" references to here. Is it really needed?

project.mml Outdated
AND (bridge IS NULL OR bridge NOT IN ('yes', 'aqueduct'))
WHERE (waterway IN ('river', 'canal', 'stream', 'drain', 'ditch', 'wadi')
AND (bridge IS NULL OR bridge NOT IN ('yes', 'aqueduct')))
OR attraction IN ('water_slide')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it okay after all to use the "water-lines" for the water slides?

@polarbearing
Copy link
Contributor

I wonder why you place the symbol at the beginning of the slide? Usually here, symbols are in the middle of the feature.

@lakedistrictOSM
Copy link

Thanks for this PR.

I'm not sure what the icon is supposed to be but it's probably unnecessary anyway.

What zoom level are you starting from? If I'm reading the code correctly z15 might be too early.

@sommerluk
Copy link
Collaborator

Hi @johsin18

Thanks for making a Pull Request. You’re welcome!

While I didn’t do neither a cartographic nor a code review (and don’t have time to do it currently), as a first impression I feel this is a feature for starting rather with zoom level 18 or 19.

Best regards and thanks again for the PR!

@Tomasz-W
Copy link

Tomasz-W commented Aug 15, 2018

@johsin18 My remarks:

  • icon (?) is unnecessary here
  • for me this way looks just like a stream flowing out of water body (more like a natural feature than a human-made one), can you try with leisure=pitch green way fill and dark-blue outline?

Thanks for this PR and good luck! :)

PS. There should be a way of highway=steps connected with attraction=water_slide added at the end, then it would be more intutive.

@HolgerJeromin
Copy link
Contributor

perhaps we need a bridge like border. In fact a slide is kind of a bridge

@johsin18
Copy link
Contributor Author

Thanks for all your comments.

  • I will drop the icon, nobody (including me) likes it ;-)
  • about the zoom level: let's start at 17, otherwise it pops up too suddenly IMHO. After all, the feature is quite large usually
  • about the color: I will try with some that is more consistent with man-made features
  • @Tomasz-W Once water slides are actually rendered, people will have a motivation to map the corresponding steps. Right now, it would look like a stairway to heaven ;-) And actually, this problem will stay for zoom levels where the slide is not yet shown, but just the steps (so should we go to 16 then maybe?)
  • @HolgerJeromin How should it become more similar to a bridge border? Different outline color? Some white "casing" in between (how would I achieve that)?

Can you please comment on the code (and my questions added as comments there)?
Is the code in the right place? Does it not cause efficiency problems maybe?

@johsin18
Copy link
Contributor Author

So I have made some of the proposed changes, simplified the code, and moved everything to amentiy-points.mss, which seems to make more sense. It is rendered from zoom level 16 now, due to the usual size of such slides.
waterslide19
waterslide18
waterslide17
waterslide16

I show screenshots on are more complex example:
https://www.openstreetmap.org/#map=19/47.53818/7.62098

How could I make the junction at the very top smoother? It's common to assemble as slide from multiple ways in multiple layers, as you would have self-intersecting ways otherwise. Should/could they be combined with some kind of relation?

@@ -14,6 +14,7 @@ node,way admin_level text linear
node,way aerialway text linear
node,way aeroway text polygon
node,way amenity text polygon
node,way attraction text polygon
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if and how this works. Typically we don't change this file, since it would require database reload, which is very time consuming and is rarely done on OSMF servers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In cases when the column is not defined when creating database, we use the hstore column with construction tags->'foobar', see for example this very short PR with icao and iata value check: https://github.com/gravitystorm/openstreetmap-carto/pull/2674/files

@kocio-pl
Copy link
Collaborator

I'm not sure what works better - water with casing or pitch with casing.

@johsin18
Copy link
Contributor Author

Ok I have removed the change to openstreetmap-carto.style.

@Tomasz-W
Copy link

Tomasz-W commented Aug 16, 2018

@kocio-pl Notice that function of water slides is to allow people to fastly flow down by water and gravity, not to just move water from one place to another. It makes water slides closer to running track than a typical water stream ;)

@matthijsmelissen
Copy link
Collaborator

Thanks for your proposal, @johsin18! However, I still think this is too specific for a general purpose map, and I don't think we should accept this PR.

@matthijsmelissen
Copy link
Collaborator

It's also not clear to me what the usecase of rendering waterslides is.

@dieterdreist
Copy link

dieterdreist commented Aug 16, 2018 via email

@kocio-pl
Copy link
Collaborator

After some time (a year?) contemplating use case idea, I'm starting to think this is not the best tool for making general map using grass root data.

For me it's rather about reflecting the reality, especially "on the ground" truth (with some exceptions like tunnels), secondary aim is the quick check what is the state of database (rough feedback loop on completeness and quality). Maybe I would see use cases as a third level - if we have some problem with too much data, we can think how to filter them.

In case of waterslides (1) this is what's visible on the ground, (2) it helps to check if the tagging is there and (3) it probably never competes with some other objects in such places, so no need to filter it other than using scaling rule (initial zoom level should be proportional to the typical size).

@dieterdreist
Copy link

dieterdreist commented Aug 16, 2018 via email

@HolgerJeromin
Copy link
Contributor

HolgerJeromin commented Aug 16, 2018

How should it become more similar to a bridge border? Different outline color? Some white "casing" in between

All bridges have a black stroke. Even water bridges:

[bridge = 'yes'] {
[zoom >= 14] {
bridgecasing/line-color: black;
bridgecasing/line-join: round;
bridgecasing/line-width: 6;
[zoom >= 15] { bridgecasing/line-width: 7; }
[zoom >= 17] { bridgecasing/line-width: 11; }
[zoom >= 18] { bridgecasing/line-width: 13; }
}
}

@meased
Copy link
Contributor

meased commented Aug 17, 2018

Use cases :

  • Orientation (just about everything has this usecase).
  • Discovering details about an attraction.
    • Not all usecases are using the map to assist with travel, sometimes people just want to browse around to see what's there, which is in itself a usecase.
  • Providing feed back for people mapping water parks.
    • Water parks are much more likely to be micro mapped.
  • Shows off the richness of OSM.

Really, pretty close to the usecases of rendering swimming pools or pitches.

I like blue better. Swimming pools aren't for holding water, they're for swimming in. Why aren't they pitch green?

Also, I think

line-join: round;
line-cap: round;

would look good.

@kocio-pl
Copy link
Collaborator

So maybe the blue line with a black (bridge-like) outline?

BTW: it's amazing to me that this PR attracts so much attention. I don't understand why, but I'm happy with that.

@polarbearing
Copy link
Contributor

this PR attracts so much attention. I don't understand why

Because this summer is so hot?

I prefer the pitch colour. As said before, the prime function is to slide. Water is just the lubricant, not the medium as in the pool. The pitch colour would also apply to dry slides where people sit on a rag.

@johsin18
Copy link
Contributor Author

johsin18 commented Aug 17, 2018

Ok so I changed the casing color to black, as there seems to be a consensus on that.
For the interior color (pitch or pool) I don't care too much.
Adding line-cap is not a good idea, as it puts a break between two connected slide segments. Also, a water slide is always open at both ends, so it does not match reality well.
There is still a narrow black line between the two segments, I'm afraid, is there a way to avoid it completely?

@kocio-pl
Copy link
Collaborator

Could you render this version? It makes a lot of sense to make visualization of every change.

@johsin18
Copy link
Contributor Author

These are the renderings of the latest code (inner pitch, outer black)

waterslide19
waterslide18
waterslide17
waterslide16

@Tomasz-W
Copy link

Is it possible to not render black outline on ways ends, but only on its sides? Water slides are open at start point and end point, so lack of outline there would be better visualisation of reality.

@johsin18
Copy link
Contributor Author

line-cap: round;
(just for the inner) helps here, but I'm not sure it's a clean solution:
waterslide19

@polarbearing
Copy link
Contributor

keeping the ends open is also important to have no lines between segments (probably the segments are ther for different layer tags). Thus this is better now in this aspect also.

@johsin18
Copy link
Contributor Author

So would please somebody merge this PR? In the upcoming weeks I will have little time to further work on this.

@kocio-pl
Copy link
Collaborator

text-halo-fill: @standard-halo-fill;
text-placement: line;
text-vertical-alignment: middle;
text-repeat-distance: @waterway-text-repeat-distance;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel this is not needed for such short features.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to remove it, but then I got the name twice even on quite short slides.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see - surprising side effect. 😄

@polarbearing
Copy link
Contributor

looks visually good to me

@kocio-pl kocio-pl merged commit fd473eb into gravitystorm:master Aug 21, 2018
@kocio-pl
Copy link
Collaborator

Thanks for the code and nice tuning, it made people really interested. 😄

@Tomasz-W
Copy link

Tomasz-W commented Aug 21, 2018

@johsin18 Big thanks for this PR! If you will have some time and will in future, please look at task lists in #3298 (comment) and #3298 (comment). Maybe there is something which you could help us with :)

@kocio-pl
Copy link
Collaborator

I've just found a place with many water slides mistagged as streams:

https://www.openstreetmap.org/#map=19/37.97477/-122.05156

@polarbearing
Copy link
Contributor

We should wait til the release is through, so we can tell him how much better they would have looked if he hadn't tagged for the renderer so blatantly ;-)

@kocio-pl
Copy link
Collaborator

He was smarter and used double tagging... 😄

aakr8tzq

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.

Add rendering for water slides (attraction=water_slide)
10 participants