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

Make tertiary road yellowish #2078

Merged
merged 1 commit into from
Mar 7, 2016

Conversation

kocio-pl
Copy link
Collaborator

@kocio-pl kocio-pl commented Mar 6, 2016

Resolves #1974.

Trivial patch with non-trivial consequences, please propose some testing places from different parts of the world to see. Some screenshot samples around Warsaw for the start (click to see the full resolution):

  1. City with residential, secondary and parking areas:
    zatiq0u3

  2. City with school and hospital area around:
    uody0wgv

  3. Outside the city with current (new) farmland color:
    soqg3vnv

@matthijsmelissen
Copy link
Collaborator

Looks good to me, at least in the screenshots.

How does it look on/near school areas?

I would also like to hear the opinion of @matkoniecz.

@Stalfur
Copy link

Stalfur commented Mar 6, 2016

Possible near school area to test http://www.openstreetmap.org/#map=17/64.10301/-21.89453

@pnorman
Copy link
Collaborator

pnorman commented Mar 6, 2016

Changing the colours needs to include changing the scripts that generate highway colours

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Mar 6, 2016

Remember to click to see a real size of those pictures!

  1. Botswana lonely road on a land color (z13/15/17):
    w2aqzku6
    ammrt9yb
    8m0ll d

Lonely secondary for comparison (z13):
inngd kd

  1. Botswana small city (Gakutio) with tertiary as main roads (z15):
    80r8b9 l

  2. Reykjavik
    z12:
    6gmxckj3
    z16:
    aiqd4wnj
    z17 (with school):
    rt367kp7

@Stalfur
Copy link

Stalfur commented Mar 6, 2016

This is great.

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Mar 7, 2016

@pnorman I guess all our scripts need at least basic documentation - what is their purpose, who maintains them etc. For example it's completely not clear for me what generate_road_colours.py does (aside from printing some color numbers on the screen). It would be good to include simple comment inside the code and let the script print some informations when called without arguments. Nice README.md would be also useful here. Should we open separate issue for this?

@matkoniecz
Copy link
Contributor

Should we open separate issue for this?

That would be a good idea.

@matthijsmelissen
Copy link
Collaborator

Tested this in Luxembourg, looks good as far as I'm concerned.

@matkoniecz What do you think?

@matthijsmelissen matthijsmelissen merged commit e041b31 into gravitystorm:master Mar 7, 2016
@kocio-pl kocio-pl deleted the tertiary-zibi branch March 7, 2016 22:57
@polarbearing
Copy link
Contributor

With the current discussion in #2071 to use the campus-yellow more widely, it would be good to see a test of a tertiary road running across such a campus.

@Stalfur
Copy link

Stalfur commented Mar 8, 2016

Campuses have tertiary roads?

@matthijsmelissen
Copy link
Collaborator

Yes, for example here. Unfortunately there are too much other colours around to compare the colour contrast well enough.

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Mar 8, 2016

Thanks for the example! On a micro scale (where there are not too much other elements and the roads are probably wider) it looks good enough for me:
z18:
c7v7p9bh
z19:
t xauvpz

If there are some not well micromapped examples I can test it too.

@polarbearing
Copy link
Contributor

Ok the hue is different enough, and the roads have their outlines, thus it works for me.

@mboeringa
Copy link

Well, I see @math1985 merged this, and I think there will be quite some happy OSM'ers once it makes it into the rendering.

@matthijsmelissen
Copy link
Collaborator

Whoops, didn't intend to merge this yet! However everybody seems happy with it so I guess merging was fine. I will roll back the merge if somebody still comes with areas where this change is problematic.

@mboeringa
Copy link

Whoops, didn't intend to merge this yet! However everybody seems happy with it so I guess merging was fine. I will roll back the merge if somebody still comes with areas where this change is problematic.

Yes, I think it will be fine, considering @kocio-pl's example renderings and responses here. This was already on a waiting list to be tackled anyway.

Only question that remains is if @pnorman's remark below still needs attention?...:

Changing the colours needs to include changing the scripts that generate highway colours

and whether you also meant to merge?...:
#2066

@pnorman
Copy link
Collaborator

pnorman commented Mar 9, 2016

Whoops, didn't intend to merge this yet! However everybody seems happy with it so I guess merging was fine. I will roll back the merge if somebody still comes with areas where this change is problematic

Because the changes will get done the next time someone repopulates the colours with the script, I think we need to roll it back

@matthijsmelissen
Copy link
Collaborator

I have no access to git right now, could somebody else roll it back?

@pnorman
Copy link
Collaborator

pnorman commented Mar 9, 2016

I have no access to git right now, could somebody else roll it back?

Done

@gravitystorm
Copy link
Owner

I've reopened the issue. I don't think there's a way to mark this as "unmerged". Unless there is some hidden option @kocio-pl can you please open a fresh PR? We can then discuss further the script changes that need to be made before merging again.

@pnorman
Copy link
Collaborator

pnorman commented Mar 9, 2016

I'd like to move road colours to their own file and have that entire file generated by the scripts - this would allow regeneration of the colours from the script without any copy/paste

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Mar 9, 2016

I can do a fresh PR, but it's a trivial one-liner and maybe we should wait until the script @pnorman is talking about is ready?

@pnorman
Copy link
Collaborator

pnorman commented Mar 9, 2016

I can do a fresh PR, but it's a trivial one-liner and maybe we should wait until the script @pnorman is talking about is ready?

The script already exists, what I was talking about would just make it easier to run.

@kocio-pl kocio-pl restored the tertiary-zibi branch March 9, 2016 21:48
@kocio-pl
Copy link
Collaborator Author

I meant "when it's done", but it can be also discussed and refined within the new PR scope, so I've opened it anyway.

@kocio-pl kocio-pl deleted the tertiary-zibi branch March 10, 2016 01:52
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.

8 participants