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 man made gray and text color, make text-dy uniform #3516

Merged
merged 5 commits into from
Nov 27, 2018

Conversation

jeisenbe
Copy link
Collaborator

@jeisenbe jeisenbe commented Nov 16, 2018

Fixes #3510
Related to #3512

Changes proposed in this pull request:

  • Change the text color of man-made feature's name labels to man-made-icon gray darkened by 15%
  • Make text-dy 10 for all of these man-made features
  • Remove redundant name label code for wastewater and water plants
  • Fix formating of natural_cave_entrance text-dy

Features affected
man_made = lighthouse, windmill, hunting_stand, bunker (currently gray)
historic = wayside_shrine (currently gray)
man_made = tower, mast, communications_tower, water_tower, chimney
power = generator with generator:sourece=wind - if this is merged (currently black)

Not affected: Religious items with black icon and text
man_made = cross
historic = wayside cross

Rational
All man_made features with icons in man-made-icon color (currently #555, dark gray) should have a name label in a matching color.

Currently, some man_made features have black text for the name label, others use man-made-icon gray. The text appears to be lighter than the corresponding icon when man-made-icon is used, but darker than the icon when black is used.

I would suggest darkening the man-made-icon color 10% or 15%

Also, the text-dy for lighthouses is currently excessive. I would also like to make the text-dy the same for all of these icons, which are all 14 x 14 pixels. As little as 8 is possible, but 10 looks best with extra-tall characters, as found in some languages.

Test rendering with links to the example places:

Test area 1:
Current rendering, z17
tower-labels-1-before
Man-made-icon 15% darker (except wayside cross)
tower-labels1-15darker

Test area 2:
Current
tower-labels-2-before
15% Darkened
tower-labels-2-15darker

Samphire tower:
https://www.openstreetmap.org/?mlat=51.1056&mlon=1.275&zoom=17
Current

15%
samphire-darken15

WRC and WKYS towers in Washington, DC
Current black
wrc-wkys-black
15%
wrc-wkys-darken15

WDCU in Washington, DC
Current black
wdcu-black
15%
wdcu-darken-15

Diamond Head Lighthouse, Hawaii
https://www.openstreetmap.org/?mlat=21.256944&mlon=-157.809444&zoom=17
man-made-gray (current rendering):
diamond-head-gray
15% darker
diamond-head-darken15

Test of text-dy: 10 with tall characters

@Tomasz-W
Copy link

@jeisenbe You firstly opened #3500, and then made this PR. But if you change man-made colour later, new darkened label also become lighter (maybe worse?). I totally don't get this order.
Please finish #3500 first, and then we will match label colour to lighter man-made shade.

@jeisenbe
Copy link
Collaborator Author

It appears we don't need to worry about tall characters; the text dy function does move the text down based on the top of the text characters.

text-dy-names

@jeisenbe
Copy link
Collaborator Author

You firstly opened #3500, and then made this PR. But if you change man-made colour later, new darkened label also become lighter (maybe worse?).

I was under the impression that #3500 was rejected.

@jeisenbe
Copy link
Collaborator Author

It looks like there may be enough support for changing man-made-icon color from the current #555 (same as #555555) dark gray to #666666, slightly lighter dark gray.

I believe darken 15% may still work well for the name labels, because the goal is to have the text look similar to the icon, but I will also show some tests with darken 20% and the new lighter icon color:

Test area 1,
Current (555 icon, text black or man-made icon)
1-test-master

Icon color (555) darkened 15% - this was the original PR suggestion
man-made-test1-555-darken15

666666 darkened 15% - slightly lighter icon color and text color
man-made-test1-666-darken15

666666 darkened 20% - slightly lighter icon color, text color darkened another 5%
man-made-test1-666-darken20

Test area 2
Current (555 icon, text black or man-made-icon)
2test-master

Icon color 555555, text darkened 15%
man-made-test2-555-darken15

Icon color 666666, text darkened 15%
man-made-test2-666-darken15

Icon color 666666, text darkened 20%
man-made-test2-666-darken20

Tower on construction area in Wamena
https://www.openstreetmap.org/#map=17/-4.09819/138.94287
Current
salib-icon555-black

Icon 555, darken text 15%
salib-icon555-darken15

Icon 666666, darken text 15%
salib-icon666-darken15

Icon 666666, darken text 20%
salib-icon666-darken20

Washington DC WOOK Radio tower
https://www.openstreetmap.org/#map=17/38.93970/-77.07902
Current
wook-master

555 darken 15%
wook-555-darken15

666666 darken 15%
wook-666-darken15

666666 darken 20%
wook-666-20

WKYS Radio Tower
https://www.openstreetmap.org/#map=17/38.93970/-77.07902
Current
wkys-master

555 darken 15%
wkys-555-darken15

666666 darken 15%
wkys-666-darken15

666666 darken 20%
wkys-666-20

Crane Milano

@jeisenbe
Copy link
Collaborator Author

Milan train station:
Worse-case scenario with gray bridge area and dark gray mainline rail tracks
Crane icon 555
milano-555

Crane icon 66666
milano-666

I believe changing man-made-icon color to #666666 will work well with darken 20%; changes pushed

@Tomasz-W
Copy link

Tomasz-W commented Nov 16, 2018

@jeisenbe Can you put few examples with text darken 5% and 10% to compare? (e.g. gastronomy-orange key has only 5% darken label)

@Adamant36
Copy link
Contributor

I like 666666 darken 15% and 666666 darken 20%. I think 20% is the sweet spot, but it looks a little to light in residential areas. So, maybe 15% would be better due to that. Id like to see @Tomasz-W's suggestion also though.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Nov 16, 2018 via email

@jeisenbe jeisenbe changed the title Change man made features text color to 15% darkened man-made gray and make text-dy uniform at 10 Change man made gray to #666666 and text color to 15% darkened, make text-dy uniform at 10 Nov 22, 2018
@jeisenbe
Copy link
Collaborator Author

Text at 5% and 10% darker than new man-made-icon gray (#666666), as requested.

I believe 15% makes the text look best with the icon. 5% appears to be lighter than the icon, and 20% appears to be darker.

Test 1
5%
test1-5

10%
test1-10

15%
test1-15

20%
test1-20

Test 2
5%
test2-5

10%
test2-10

15%
test2-15

20%
test2-20

Mew Island Light icon #666666
5%
mew-light-5
10%
mew-light-10
15%
mew-light-15
20%
mew-light-20

Black Mountain Mast
5%
black-mountain-5
10%
black-mountain-10
15%
black-mountain-15
20%
black-mountain-20

@jeisenbe
Copy link
Collaborator Author

Does anyone else have comments about the color of the text?
@Tomasz-W - do you agree with darken 15%?

@Tomasz-W
Copy link

@jeisenbe Yes

@kocio-pl
Copy link
Collaborator

Sorry for being late to the party... I like the changes, though I think 20% is good enough for readability, 15% is still a bit too light for me.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Nov 27, 2018 via email

@kocio-pl kocio-pl changed the title Change man made gray to #666666 and text color to 15% darkened, make text-dy uniform at 10 Change man made gray and text color, make text-dy uniform Nov 27, 2018
@kocio-pl kocio-pl merged commit 2767eb4 into gravitystorm:master Nov 27, 2018
@kocio-pl
Copy link
Collaborator

Frankly, this is so subtle change, that I just checked that nothing is broken, but I was not able to tell what has really changed.

Since testing multiple fixes is time consuming and error prone, I'd suggest (this is really just my personal suggestion, not a hidden warning 😄) to not focus too much on such fixes, because I will need a lot of time to focus to not let some fatal bug slip in. This is kind of code I will put on the bottom of my merge queue to keep my sanity and have more useful changes active.

@jeisenbe jeisenbe deleted the man-made-text branch December 1, 2018 01:39
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