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

Add aboriginal areas #3521

Merged
merged 7 commits into from
Jan 29, 2019
Merged

Add aboriginal areas #3521

merged 7 commits into from
Jan 29, 2019

Conversation

almccon
Copy link
Contributor

@almccon almccon commented Nov 22, 2018

Fixes #3520

Changes proposed in this pull request:

  • Add rendering for boundary=aboriginal_area and for boundary=protected_area + protect_class=24.

Test rendering with links to the example places:

Before
screen shot 2018-11-22 at 22 nov 9 16 32

After
screen shot 2018-11-22 at 22 nov 10 25 33

More samples:
screen shot 2018-11-22 at 22 nov 11 09 40
screen shot 2018-11-22 at 22 nov 11 08 00
screen shot 2018-11-22 at 22 nov 11 04 23
screen shot 2018-11-22 at 22 nov 10 35 28
screen shot 2018-11-22 at 22 nov 11 36 45

Copy link
Collaborator

@pnorman pnorman left a comment

Choose a reason for hiding this comment

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

👍 in principle, I haven't done a detailed code review.

@kocio-pl
Copy link
Collaborator

As I have written in the issue ticket, it's not easy to find good color - this rendering might be too close to the zoo. So maybe we need some dark orange border; of course the colors can be switched (brown-orange for aboriginal area and orange for zoo) if that would work better. That needs testing. Also wiki documentation is needed, following discussion on the Tagging list to make sure if we really need 2 different schemes (maybe we do).

Another, smaller issue would be to rename data layer into something like protected-areas-boundaries instead of creating another similar layer called aboriginal-lands-boundaries.

@jeisenbe
Copy link
Collaborator

@almccon, if the orange doesn't work, another option would be to switch tourism boundaries (eg zoos, theme parks) to a shade of violet or purple, because we are planning to change administrative boundaries to gray, thus making purple available for tourism features.

@sommerluk
Copy link
Collaborator

If violet becomes available, it should not be a minor feature like this one to occupy it again (blocking a hole coulor).

@jeisenbe
Copy link
Collaborator

jeisenbe commented Nov 28, 2018 via email

@Adamant36
Copy link
Contributor

Isn't violet/purple being used for shops currently? I think it works good there. The new gastronomy icon is sort of orangish already also. Maybe the dark blue color that's currently being used for railway stations would work. Since its the only thing that I know of with the color.

There was a discussion somewhere around here about changing that and a few other colors around that never went anywhere. Maybe it would be worth revisiting and putting this stuff/that stuff under one umbrella. Since they are all sort of related. As far as zoos, amusement parks etc goes. I think it would be the wrong color for this issue though.

Orange might actually work here despite its food association because I doubt people think aboriginal areas are related to restaurants. Plus, its kind of an earth tone. Which sorta fits.

Copy link
Contributor

@matkoniecz matkoniecz left a comment

Choose a reason for hiding this comment

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

Thank you very much for a PR! Especially such well formatted one (with example images etc).

Unfortunately currently proposed rendering is far too close to rendering of zoos/theme parks. I understand that with style rendering so many features it is hard to find style that works is not already used for something else but in this case implication of rendering this feature in style very close to zoos are quite problematic.

Can you experiment with some other stylings?

@matkoniecz
Copy link
Contributor

@almccon Would it be OK to mark this PR as assigned to you for now?

@almccon
Copy link
Contributor Author

almccon commented Dec 7, 2018

@matkoniecz Yes, please go ahead and assign it to me, thanks. I will try some alternative color treatments to avoid the zoo conflict. I just haven't had time to do it yet.

@kocio-pl
Copy link
Collaborator

We have now boundary=aboriginal_lands documented:

https://wiki.openstreetmap.org/wiki/Tag:boundary%3Daboriginal_lands

and i agree that we should now render both schemes.

So, we just wait for solving rendering problems.

@jeisenbe
Copy link
Collaborator

@almccon, here are some options for changing the tourism color (currently brown, used for tourism=zoo and tourism=theme_park). Tourism=attraction uses #660033, but I would like to change this to be the same.

Here's the current rendering of this test area that I created in JOSM (which shows province and national borders as well as several landuse colors that may be relevant):
Current style
z15
z15-theme-park-master
z16
z16-theme-park-master
z17
z17-theme-park-master

#660033

  • this uses the current tourism=attraction color for zoos and theme parks as well
  • It may work even without changing the administrative boundary colors
    z15
    z15-theme-park-660033
    z16
    z16-theme-park-660033
    z17
    z17-theme-park-660033

Orange

  • This test is with #C77400,
    z15
    z15-theme-park-orange
    z16
    z16-theme-park-orange
    z17
    z17-theme-park-orange

Dark violet #864784

  • Too similar to current administrative boundary color; so this might need to be changed first.
    z15
    z15-theme-park-darkviolet
    z16
    z16-theme-park-darkviolet
    z17
    z17-theme-park-darkviolet

@kocio-pl
Copy link
Collaborator

#660033 for attractions looks great for me. But I think this should be discussed in separate PR.

@jeisenbe
Copy link
Collaborator

jeisenbe commented Dec 20, 2018 via email

@kocio-pl
Copy link
Collaborator

I guess #3045 covers it already and it's time to move to the next level - real code.

@kocio-pl
Copy link
Collaborator

@almccon Quoting myself:

Another, smaller issue would be to rename data layer into something like protected-areas-boundaries instead of creating another similar layer called aboriginal-lands-boundaries.

More layers create additional performance penalty and all these objects are equal in rendering (none of them should have rendering priority over the other), so it does not make sense to create additional data layer.

This is probably the last thing to be done here, the rest is just waiting for #3582 to be merged.

@kocio-pl
Copy link
Collaborator

@almccon Ping?

Now the #3582 is merged, so merging this depends only on your action.

@kocio-pl kocio-pl changed the title add aboriginal areas [WIP] add aboriginal areas Jan 3, 2019
@jeisenbe
Copy link
Collaborator

@almccon - do you have time to update this PR to fix the merge conflicts and per the suggestion in #3521 (comment)?

If not, I could finish it for you (by submitting a new PR)

@kocio-pl
Copy link
Collaborator

kocio-pl commented Jan 23, 2019

@jeisenbe I believe you can make your own PR now to fix this.

@almccon
Copy link
Contributor Author

almccon commented Jan 23, 2019

Hi all, sorry for the delay. I fixed the merge conflict, and I'll try to respond to the other comments tomorrow. But if you want to jump on any of it, be my guest @jeisenbe!

@jeisenbe
Copy link
Collaborator

jeisenbe commented Jan 23, 2019 via email

@almccon
Copy link
Contributor Author

almccon commented Jan 28, 2019

Okay, @kocio-pl, I combined the two layers into a single layer as per the suggestion in #3521 (comment)

In this branch, the aboriginal areas are #82643a, and thanks to #3582 zoos and other tourism are #660033. The aboriginal brown is lighter than the original tourism brown #734a08, which is still used for amenity points.

screen shot 2019-01-27 at 27 jan 4 33 19

screen shot 2019-01-27 at 27 jan 4 31 44

This feels like a good outcome to me. I don't think there are any other outstanding issues. Did I miss anything?

@kocio-pl
Copy link
Collaborator

Did you actually test the code (for both cases) after the changes? It is a common thing to do after each change to avoid errors.

I get SQL error when trying to render anything with this code:

Postgis Plugin: ERROR: column "boundary" does not exist

If you need some help, please talk with @jeisenbe probably.

@almccon
Copy link
Contributor Author

almccon commented Jan 28, 2019

I did test the code, and I did see that error happen initially after I made the code change, but then when I restarted docker I thought I stopped seeing the error. Let me check again.

@almccon
Copy link
Contributor Author

almccon commented Jan 28, 2019

You're right, that error is showing up for me at z13+, due to the protected-areas-text layer not including the boundary and class column. My fault. Should be fixed now.

@kocio-pl
Copy link
Collaborator

Thanks, now it works, but the text label color is wrong here for example:

https://www.openstreetmap.org/relation/2740289#map=10/9.9377/-72.9568

screenshot_2019-01-29 openstreetmap carto kosmtik

@kocio-pl
Copy link
Collaborator

Looks like the problem exists only for boundary=protected_area + protect_class=24 but not for boundary=aboriginal_lands:

https://www.openstreetmap.org/way/383485661#map=14/20.6472/-103.2405

screenshot_2019-01-29 openstreetmap carto kosmtik 1

@almccon
Copy link
Contributor Author

almccon commented Jan 29, 2019

Ugh, sorry. I thought I had tested the label color on boundary=protected_area + protect_class=24, but apparently I did not. I just pushed another commit that should fix the problem.

screen shot 2019-01-28 at 28 jan 5 20 23

screen shot 2019-01-28 at 28 jan 5 22 36

project.mml Outdated Show resolved Hide resolved
amenity-points.mss Outdated Show resolved Hide resolved
amenity-points.mss Outdated Show resolved Hide resolved
project.mml Outdated Show resolved Hide resolved
admin.mss Outdated Show resolved Hide resolved
@kocio-pl kocio-pl changed the title [WIP] add aboriginal areas Add aboriginal areas Jan 29, 2019
@kocio-pl
Copy link
Collaborator

It works now, it just needs some code cleaning (I have not repeated it for every occurrence).

more code cleanup

more code cleanup
@almccon
Copy link
Contributor Author

almccon commented Jan 29, 2019

Thanks for the feedback, all of those code cleanup recommendations are good, and I think I've applied them all.

@kocio-pl kocio-pl merged commit eba988f into gravitystorm:master Jan 29, 2019
@kocio-pl
Copy link
Collaborator

Now it looks OK for me. Thanks for all this work!

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.

7 participants