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

Change label colour of private parking #3085

Merged
merged 2 commits into from
Mar 12, 2018
Merged

Change label colour of private parking #3085

merged 2 commits into from
Mar 12, 2018

Conversation

jragusa
Copy link
Contributor

@jragusa jragusa commented Feb 23, 2018

Resolves #2170.

Replace greenish hardcoded colour to opacity value.

@kocio-pl
Copy link
Collaborator

Thanks! If you change "Related to" into "Resolves", the issue will be closed automatically when merging this code.

@jragusa
Copy link
Contributor Author

jragusa commented Feb 23, 2018

Thank you :)

It's done

@polarbearing
Copy link
Contributor

polarbearing commented Feb 23, 2018

I assume it takes the colour from the block above, thus text-fill: @transportation-text;.
It would still be helpful to see a test rendering, to evaluate if it is readable enough. Preferably both on a building and on surface parking.

Edit: I see the test is in the original issue, thanks. Can we have the building, though?

@jragusa
Copy link
Contributor Author

jragusa commented Feb 24, 2018

The Garage Hélicoïdal of Grenoble (https://www.openstreetmap.org/way/32538552)

before (z=19)
before

after (z=19)
after

@kocio-pl
Copy link
Collaborator

That's what I expected. @polarbearing What's your opinion?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Mar 5, 2018

@polarbearing ping?

@polarbearing
Copy link
Contributor

Well, it looks nice on the parking-grey.
It is difficult to read on the building. It was difficult before but it gets even worse.

My explanation is that the brighter typesetting kind of blurs with the halo.

Maybe we could drop the halo when rendered on a building? though that might be difficult to code for the case that the parking is not tagged on the building outline but as a node.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Mar 5, 2018

@jragusa Could you test this approach?

@jragusa
Copy link
Contributor Author

jragusa commented Mar 5, 2018

current setting | text-opacity: 0.33 | text-opacity: 0.33 + text-halo-radius: 0

surface parking
surface

building parking
building

I note that the suggestion of @polarbearing is easiest to read and name remains of less importance that shop names. What do you think ?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Mar 5, 2018

I agree that his proposition works best. @polarbearing If you're happy with how does it look like, I would merge the altered code.

@polarbearing
Copy link
Contributor

The zero-halo approach is the best so far.

For a single node that does not bring its own parking/building background, I'd expect that to work fine on a bright background, such as 'empty' or amenity-yellow.

I'm wondering what mixture we have to expect on industrial/commercial.
Do we have any darker backgrounds to consider?

@jragusa
Copy link
Contributor Author

jragusa commented Mar 5, 2018

I don't find such association, so examples are welcome to test rendering

@jragusa jragusa closed this Mar 5, 2018
@jragusa jragusa reopened this Mar 5, 2018
@polarbearing
Copy link
Contributor

Feed the overpass wizard with
amenity=parking and access=private and name=* and type:node in YOURAREA.

However I just realise that names on parking are only rendered on polygons, not on nodes. I wonder if this is intentional or a bug?

The analysis shows that rendering the marker is split in the code:
The marker is set in line 370 for those >900waypixel, and in line 1096 in the .amenity-low-priority section for any.

At line 1279 the label is applied for polygons>900waypixel. There is no matching text label for the catch-all condition, thus apparently not rendered for nodes.

@polarbearing
Copy link
Contributor

It's intentional, it was introduced here #1364.

That means, we currently have no parking on nodes to investigate which might mix with a strange background, and can merge the PR.

Should we want labels on parking nodes eventually, we would need to reconsider the colour question.

@jragusa
Copy link
Contributor Author

jragusa commented Mar 5, 2018

@polarbearing
I tried and only found parking nodes in residential landuse

@kocio-pl
Copy link
Collaborator

So - what is the proposed solution now?

@jragusa
Copy link
Contributor Author

jragusa commented Mar 12, 2018

It's ok for me unless you want to add names on nodes.

I just have an additional question : why label and maker don't have the same colour ?

@kocio-pl
Copy link
Collaborator

It's ok for me unless you want to add names on nodes.

We're just fixing current issue, not adding anything new.

OK, so I'll merge this code now. Thanks for the code and discussion!

I just have an additional question : why label and maker don't have the same colour ?

I have difficulties trying to find when this line has been introduced:

text-fill: #66ccaf;

@kocio-pl kocio-pl merged commit c6b9f7a into gravitystorm:master Mar 12, 2018
@jragusa
Copy link
Contributor Author

jragusa commented Mar 12, 2018

Sorry, I used wrong expression :

@transportation-icon: #0092da and @transportation-text: #0066ff

Inversely, amenities use colour @amenity-brown for both icon and text.

Is there a specific reason to use two different colours for transportation and accommodation ?

@kocio-pl
Copy link
Collaborator

Oh, I see.

As far as I remember, this is because text is thin and the icon is thick usually, and it makes the illusion that the colors are very different (halo can also trigger this effect). It was designed before I came here, but that was my conclusion. You have to test it however, because it was long time ago when I checked this. I'm also not sure why amenity is different in this regard.

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.

3 participants