-
Notifications
You must be signed in to change notification settings - Fork 823
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
Refurbished spring icon (fix #325 using @imagico first tests) #3189
Conversation
Is the ring shown on top of the stream? I think it should be placed under. |
And that is why @imagico created a layer for this; I'll correct that. |
z14 z15 z16 |
The color of blue seems odd. I think it would look more consistent with |
@meased : I tried that first, but the icon with that shade of blue seemed more difficult to see with some backgrounds, as you may see in #325. Besides, @kocio-pl was in favor of this version despite the different shade, and I felt that the favor of an experienced collaborator was enough to PR, particularly if the problem lay for years without resolution. |
Is there something I should improve before merging? |
I don't see anything like that at the moment. I just need some time to make additional test just in case, but basically that's all. |
symbols/spring.svg
Outdated
d="m 6,13 a 3,3 0 1 1 -6,0 3,3 0 1 1 6,0 z" | ||
id="path4333" | ||
style="fill:none;fill-opacity:1;stroke:#ffffff;stroke-width:0.60000002;stroke-miterlimit:4;stroke-dasharray:none;stroke-opacity:1" | ||
inkscape:connector-curvature="0" /> |
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.
These are some Inkscape leftovers in this file, as seen above - please make sure to save the file as plain SVG.
@kocio-pl: this way? I'm a newbie regarding SVG code, so there might still be some useless code. |
I haven't tested it, but probably yes. I guess that you've removed it by hand? Standard procedure is to just write the file in the Inkscape as a plain SVG instead of Inkscape SVG. Usually it leads to removing a lot of unnecessary stuff (probably helpful only for extensive editing). Could you try it? |
I tried that, but it merely added noise instead of removing it: <?xml version="1.0" encoding="UTF-8" standalone="no"?>
<svg
xmlns:dc="http://purl.org/dc/elements/1.1/"
xmlns:cc="http://creativecommons.org/ns#"
xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
xmlns:svg="http://www.w3.org/2000/svg"
xmlns="http://www.w3.org/2000/svg"
id="svg6"
version="1.1">
<metadata
id="metadata12">
<rdf:RDF>
<cc:Work
rdf:about="">
<dc:format>image/svg+xml</dc:format>
<dc:type
rdf:resource="http://purl.org/dc/dcmitype/StillImage" />
<dc:title></dc:title>
</cc:Work>
</rdf:RDF>
</metadata>
<defs
id="defs10" />
<g
transform="translate(0,-1036.3622)"
id="layer1">
<path
style="fill:none;fill-opacity:1;stroke:#ffffff;stroke-width:0.60000002;stroke-miterlimit:4;stroke-dasharray:none;stroke-opacity:1"
id="path4333"
d="m 6,13 a 3,3 0 1 1 -6,0 3,3 0 1 1 6,0 z"
transform="matrix(1.1666667,0,0,1.1666667,0,1033.6955)" />
<path
style="display:inline;fill:none;stroke:#ffffff;stroke-width:3.5;stroke-miterlimit:4;stroke-dasharray:none;stroke-opacity:1"
id="path4327-3"
d="m 6.875,1048.8622 a 3.375,3.3750069 0 1 1 -6.75,0 3.375,3.3750069 0 1 1 6.75,0 z" />
<path
style="display:inline;fill:none;stroke:#7abcec;stroke-width:1.5;stroke-miterlimit:4;stroke-dasharray:none;stroke-opacity:1"
id="path4327"
d="m 6,13 a 3,3 0 1 1 -6,0 3,3 0 1 1 6,0 z"
transform="matrix(1.125,0,0,1.1250023,0.125,1034.2372)" />
</g>
</svg> |
project.mml
Outdated
<<: *osm2pgsql | ||
table: |- | ||
(SELECT | ||
St_Centroid(way) AS way, "natural" |
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.
I'm not sure, but I suspect that this code makes test rendering on z14+ very long. Instead of 2-3 s it takes like 10+ s every time and there's heavy I/O load, which means big database query most probably.
This is nasty performance bug. Could you look closer and fix it?
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.
I think dropping St_Centroid should give a similar result?
Also this might result in the spring icon being displayed outside of the spring polygons in case of moon-shaped springs - do we want that? Dropping the St_Centroid should solve that too.
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.
This is beyond my knowledge, but sounds reasonable. I don't know how should the code look like after dropping it, could you show the proper code that I could test?
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.
Simply like this (like a few lines below):
way, "natural"
Make sure to test it on spring areas.
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.
It works somehow wrong then -
Example place: https://www.openstreetmap.org/way/436133353#map=18/50.43224/20.50234
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.
What looks wrong according to you? Note that the text in the second image is not the same as the text in the first image.
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.
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.
For all 6 such areas in Poland the icon is placed the same as before, so it looks like this is not even needed. I believe marker-placement: interior
in MSS file does the job.
I'm not sure if we should wait for @Penegal with this or just update this line somehow and merge it?
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.
BTW: I hope soon we will have exactly the same interior algorithm in Kosmtik and on the OMSF servers, we're really close:
- Update node-mapnik to 3.7.2 kosmtik/kosmtik#283 (and then a new Kosmtik release with npm package update, so our Docker image could be updated too...)
- Deploying new Mapnik version (3.0.19) openstreetmap/chef#155 (comment)
Thanks! This is a good update for probably the oldest leftovers that I hated the most... |
@kocio-pl: you're welcome; I'll be happy to see this non-intuitive S disappear. For information, why this additional commit? What's the matter with |
This was both not needed and resulted in serious performance loss. See: #3189 (comment) |
OK, I just saw the reviews; sorry for the noise. |
How does this look within a body of water? |
@polarbearing: as this has been merged and is part of v4.11.0 published 6 days ago, I would say that you'll see on the tiles on OSM.org. BTW, @kocio-pl, I'm surprised to not see the change on OSM.org tiles. I saw other style versions applied more quickly; is there a specific problem with this one? |
A version upgrade requires a manual action from the (voluntary) website maintainers. They likely have been busy with other stuff. |
Ok here is the case to watch, Big Spring Missouri mapped as a node in a body of water. |
Some forks have included last release already: https://tile.iosb.fraunhofer.de/#map=16/36.9517/-90.9932/3 I don't know why OSMF admins didn't push that change yet, you can ask them about it here: openstreetmap/chef#162. |
Looking at the Fraunhofer server, the rendering on water is strange. It is a pity that this was not tested beforehand, I had already mentioned that here: 325 comment. Should we reopen or make a new issue? |
First make sure that it's really style issue, not the server rendering issue. Cut ring suggests me the later (for example if you change the layer to OSMde you will see no halo at all), so it's always best to test in your own environment. |
ok but in the .de style you do not see any icon. So 'invisible in water' remains. |
I see. What fix would you propose for this? |
Maybe adding white dot on top of the water would be good? |
The release has been merged by Tom now, maybe we wait and check the rendering there. I'll do some analysis meanwhile: The SVG is constructed atypically compared to other icons. It seems to have no artboard size, thus git shows the icon in a large transparent square, opening with Illustrator presents it on an A4 sheet, partially hanging over the border. All new icons I have analysed so far use fill while the stroke is empty. This SVG is the opposite. Typically we have the icon black and define the colour in carto. The ring (# 7abcec) is darker than water (@water-color defined as # aad3df) IIRC, the order of the 'id:' elements in the 'Layer:' section in the project.mml defines the painting order? If so, 'water-areas' are painted earlier than 'springs' which are earlier than 'water-lines'. This explains why a river line is painted over the ring, but I don't understand why the white rings disappear over a water area? Finally I'd have the style for the spring rather in the water.mss with the natural things, while water-features.mss mostly has the man-made barrieres etc. |
Fresh tile is there, spring invisible: |
Fixes #325
Test rendering with links to the example places:
Thanks to @imagico for the starting point.