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 leisure=ice_rink #3330

Merged
merged 9 commits into from
Dec 17, 2018
Merged

Render leisure=ice_rink #3330

merged 9 commits into from
Dec 17, 2018

Conversation

jragusa
Copy link
Contributor

@jragusa jragusa commented Aug 5, 2018

Fixes #796

Changes proposed in this pull request:
Render leisure=ice_rink with glacier background and pitch colour text

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

Before
ice_rink_before

After
ice_rink_name_green

@jragusa jragusa changed the title render ice_rink Render leisure=ice_rink Aug 5, 2018
@kocio-pl
Copy link
Collaborator

kocio-pl commented Aug 5, 2018

It looks to me that we should also add the outline. Code for glaciers is here:

#icesheet-outlines {
[zoom >= 8] {
[ice_edge = 'ice_ocean'],
[ice_edge = 'ice_land'] {
line-width: 0.375;
line-color: @glacier-line;
[zoom >= 8] {
line-width: 0.5;
}
[zoom >= 10] {
line-dasharray: 4,2;
line-width: 0.75;
}
}
}

@kocio-pl
Copy link
Collaborator

kocio-pl commented Aug 6, 2018

And maybe the outline color could be different to make it more distinguishable from glaciers. I believe green could be good (the same as in label).

@jragusa
Copy link
Contributor Author

jragusa commented Aug 7, 2018

It looks better, what do you think ?
ice_rink_greenline

@kocio-pl
Copy link
Collaborator

kocio-pl commented Aug 7, 2018

Nice, it is clearly different enough from glacier:

screenshot_2018-08-07 openstreetmap

landcover.mss Outdated
@@ -270,6 +270,16 @@
}
}

[feature = 'leisure_ice_rink'] {
[zoom >= 10] {
Copy link
Collaborator

@kocio-pl kocio-pl Aug 7, 2018

Choose a reason for hiding this comment

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

I think we still need [way_pixels > 3000][is_building = 'no']condition here - we don't like to have just the outline without the fill.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like: [zoom >= 10][way_pixels > 3000][is_building = 'no'] { ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, most probably. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok done :)

@kocio-pl
Copy link
Collaborator

kocio-pl commented Aug 7, 2018

Did you test the code after the change? I suggest to always do it, no matter how small and innocent the change looks like. 😄

In this case I get error (shortened):

Postgis Plugin: ERROR:  column "is_building" does not exist
.
.
.
  ) AS landcover

Which means that landcover data layer is not aware of is_building ("virtual" column), because this hasn't been defined there. See for example:

CASE WHEN building = 'no' OR building IS NULL THEN 'no' ELSE 'yes' END AS is_building -- always no with the where conditions

and try to put it after this line inside landcover layer:

CASE WHEN religion IN ('christian', 'jewish', 'muslim') THEN religion ELSE 'INT-generic'::text END AS religion,

This might be not enough, but that's probably something to start digging. If you will have any problems with that, just tell here.

@HolgerJeromin
Copy link
Contributor

HolgerJeromin commented Aug 7, 2018

We should probably do something with
seasonal=winter/yes (or better !=no)

@kocio-pl
Copy link
Collaborator

kocio-pl commented Aug 7, 2018

Thanks. What do you propose? Would simply reusing water type of rendering work for you?

@jragusa
Copy link
Contributor Author

jragusa commented Aug 7, 2018

I removed the is_building condition and buildings are not affected by the PR:
https://www.openstreetmap.org/way/246399017
ice_rink_building

Edit: actually, the pitch is covered by the building. We can see a very slight green outline

@kocio-pl
Copy link
Collaborator

kocio-pl commented Aug 7, 2018

I believe this is not a good solution and proper solution (selecting only non-buildings for area+outline and maybe labels for all the cases) should be used instead.

@kocio-pl
Copy link
Collaborator

@jragusa Are you willing to deploy proposed solutions in this PR?

@jragusa
Copy link
Contributor Author

jragusa commented Aug 13, 2018

Sorry @kocio-pl for the delay, I was busy last week with manuscript corrections.

I tested your proposition and I get an error. I'm testing the suggestion of HolgerJeromin. Generally, this problem of building condition is not restricted to ice_rink and also concerns sports_centre for example. Actually, the outline width is 0.3 for the latter with respect to 0.5 for ice_rink which explains why there is no graphical glitch for sports_centre.

@kocio-pl
Copy link
Collaborator

OK, no need to hurry - I prefer people learning how to make a proper code in this style, so we could resolve more complex problems than just adding icons for shops. Do you need some help with this?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Sep 3, 2018

Hi, how are you with this code? Do you need some assistance or you want to go on with it yourself?

@jragusa
Copy link
Contributor Author

jragusa commented Sep 4, 2018

@kocio-pl I still have problem with the condition is_building. It's used for text but when I tried to define it for surface, I got an error from projet.mml. I will investigate this on Thursday.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Oct 6, 2018

Do you have any news about this problem?

project.mml Outdated Show resolved Hide resolved
@jragusa
Copy link
Contributor Author

jragusa commented Dec 12, 2018

@kocio-pl this PR is ready to merge

ice_rink_building_after

project.mml Outdated Show resolved Hide resolved
@jeisenbe
Copy link
Collaborator

jeisenbe commented Dec 16, 2018 via email

@kocio-pl kocio-pl merged commit 05757cf into gravitystorm:master Dec 17, 2018
@kocio-pl
Copy link
Collaborator

Thanks, it works! And now we have a 4 year old issue fixed... 😄

@jragusa jragusa deleted the ice_rink branch December 31, 2018 09:45
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.

4 participants