-
Notifications
You must be signed in to change notification settings - Fork 822
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
Add aboriginal areas #3521
Conversation
There was a problem hiding this 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.
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 |
@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. |
If violet becomes available, it should not be a minor feature like this one to occupy it again (blocking a hole coulor). |
Lukas Sommer wrote:
If violet becomes available, it should > not be a minor feature like this
one to > occupy it again
Zoos an amusement parks are fairly minor, but we could use the same border
for many other tourist areas and accommodations: e.g. resorts, campsites,
caravan sites, etc.
Violet is a rather strong color an uncommon in nature, so I think it should
be used for less common but interesting features; tourism and
transportation are good options.
But if orange works, that could also be a good option for tourism.
|
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. |
There was a problem hiding this 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?
@almccon Would it be OK to mark this PR as assigned to you for now? |
@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. |
We have now 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. |
@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): #660033
Orange Dark violet #864784 |
|
Separate PR or separate issue?
I’m happy to make a PR, unless @almccon would like to do it.
…On Thu, Dec 20, 2018 at 11:19 AM kocio-pl ***@***.***> wrote:
#660033 for attractions looks great for me. But I think this should be
discussed in separate PR.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3521 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AoxshCDwwJUJ5fFwrnITgtIl64RJr4RWks5u6vO6gaJpZM4Yv4r6>
.
|
I guess #3045 covers it already and it's time to move to the next level - real code. |
@almccon Quoting myself:
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. |
@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) |
@jeisenbe I believe you can make your own PR now to fix this. |
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! |
Go ahead, I think you’ve got it.
…On Wed, Jan 23, 2019 at 2:36 PM Alan McConchie ***@***.***> wrote:
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 <https://github.com/jeisenbe>!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3521 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AoxshF3_Tw0Zva2LlkeCCipqIQEnsLxLks5vF_TPgaJpZM4Yv4r6>
.
|
Update with latest commits
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. This feels like a good outcome to me. I don't think there are any other outstanding issues. Did I miss anything? |
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:
If you need some help, please talk with @jeisenbe probably. |
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. |
You're right, that error is showing up for me at z13+, due to the |
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 |
Looks like the problem exists only for https://www.openstreetmap.org/way/383485661#map=14/20.6472/-103.2405 |
It works now, it just needs some code cleaning (I have not repeated it for every occurrence). |
more code cleanup more code cleanup
Thanks for the feedback, all of those code cleanup recommendations are good, and I think I've applied them all. |
Now it looks OK for me. Thanks for all this work! |
Fixes #3520
Changes proposed in this pull request:
boundary=aboriginal_area
and forboundary=protected_area
+protect_class=24
.Test rendering with links to the example places:
Before
After
More samples: