-
Notifications
You must be signed in to change notification settings - Fork 829
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
Clean SVG symbols #4457
Clean SVG symbols #4457
Conversation
578790c
to
dfc722a
Compare
I would rather see this implemented in a language that can handle the format in an abstract level instead of line or text level, but as I can't do it myself, and the outputs are fine, I think it's OK. |
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.
Initial code review, have not yet tested the changes. But overall looking through the symbol SVGs and standardizing them better is a good idea.
Like @StyXman i think awk is not a very good choice for the task of making scripted modifications to SVG code here. In this PR the code is not pertinent to style use and editing, it is just a convenience tool for inspecting the point symbols. But in your other changes that is different. I am not tied to using python (we have precedent for SVG processing with python though: https://github.com/gravitystorm/openstreetmap-carto/blob/master/scripts/generate_shields.py) but cross platform availablility and familiarity among current and potential developers are an argument of course.
This is not a very important matter for me so if other maintainers are fine with using awk i won't oppose it. What i would oppose though is having symbol classifications and color assignments in several places to be kept manually in sync whenever someone changes them. That is not going to be feasible. A test script for visual inspection of symbols is only of value if it is actually suitable for testing what the style implements and not just replicates visually the rendering at the moment it is implemented.
dfc722a
to
c94f9be
Compare
@imagico Thank you for reviewing.
I can completely understand the concerns around using AWK. It is just me reaching for the most convenient tool I know well 😄. I am not married to this script, it is just quickly put together to get a preview. The relevant commit is now gone. I didn't even consider cross-platform availability. Just like Python and NPM for CartoCSS it is not available for Windows out of the box. But familiarity for developers is the most important consideration for a language.
Yes. that was a problem I also felt. Parsing the CartoCSS is just to hard to do for just this. But having two definitions that can get out of sync is worse. So, better to just remove the script and list. |
Added one more commit to remove the xml declaration from all files. It is not required, and really has no benefit. |
I have an initial version of a python version for the script. It's completely incomplete :))) and I didn't even had time to pick things up from the awk version. I will try to expand it, but my time is very limited. Maybe someone else can pick it up. #! /usr/bin/env python3
import sys
import lxml.etree as xml
old_doc = xml.parse(sys.argv[1])
old_root = old_doc.getroot()
if not old_root.tag.endswith('svg'):
raise ValueError(f"{sys.argv[1]} does not seem to contain an SVG.")
ns = 'http://www.w3.org/2000/svg'
name = 'svg'
new_root = xml.Element(name, nsmap={None: ns})
# start copying things
for attr in ('height', 'width', 'viewBox', 'xmlsns'):
try:
new_root.attrib[attr] = old_root.attrib[attr]
except KeyError:
pass
for old_child in old_root:
new_child = xml.Element(old_child.tag)
for attr in old_child.attrib:
if attr != 'style':
new_child.attrib[attr] = old_child.attrib[attr]
else:
style_strings = old_child.attrib['style'].split(';')
styles = dict([ style_string.split(':', 1) for style_string in style_strings ])
# fill:none;stroke:none;visibility:hidden
if styles.get('visibility', None) == 'hidden':
new_child = None
# get opacity, calculate
# fill-opacity:0.3;stroke-width:0.01526906;opacity:0.9;fill:#000000
form_opacity = float(styles.get('opacity', 1))
fill_opacity = float(styles.get('fill-opacity', 1))
opacity = form_opacity * fill_opacity
if opacity != 1.0:
new_child.attrib['opacity'] = f"{opacity:.2}"
if new_child is not None:
new_root.append(new_child)
xml.dump(new_root) |
@StyXman Wow, I didn't expect you to start porting the preview script to Python at all! If there is interest in having this I can try to help. Edit: if you lost access to the script because of my rebase, I have put a copy here. Parsing CartoCSS still seems like an obstacle to me, but parsing the generated Mapnik XML to get the symbols with color and opacity should be easy. Not sure what I want, but can we somehow keep the preview script a bit separated from this PR? So that what would be a new feature doesn't slow this one down unnecessary? |
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 tested the results in rendering now and the only thing i found at the first look was that for natural=spring the moved symbol blocks the labeling. You need to increase the label offset to avoid that.
Thank you for testing. And wow, that was an issue I didn't expect, good catch! Previously the spring symbol was drawn as a blue circle on top of a white one (and an invisible third circle). All had a radius of 3.375, and a stroke width. Because Mapniks size estimation doesn't take the stroke into account, it thought the symbol was 8px high when rounded. In reality it is a little more than 10, rounded to be 12px. The label was placed with its top 6px below the center of the symbol. Now that Mapnik knows the true size of the symbol, it sees the label is too close. Fixed, increased |
I agree in principle with this, but need to review it. It's a lot of symbols to check. |
Is there any way in which I can help? I hope you are not going to check every symbol line by line and in Mapnik, that would be a huge (and duplicated) effort. May I suggest you only look closely at the files that had 'real' modifications? And only do a spot check of the others? I pointed the out the tricky ones in the first post, but as a list: Centered
Cropped
Removed strokes
Added strokes
(Possibly) affected by invisible geometry
Akay, that is still a long list 😞. But more manageable! |
The test i used is simply analyzing amenity-points.mss and generating a grid of features with the tags used - you can find those here: poi_test_carto.osm These are not consistently sorted - with 250+ features you want to do geometry generation and tag assignment automatically and this is kind of hard to do in a way that lets you maintain a consistent order of features. But still this allows for a collective and comprehensive before-after comparison. |
@imagico Can you tell me how to run a test with these files? |
You run them through |
Thank you! A lot of stuff left to learn... Until now I have done all testing only on complete openstreetmap database imports of a country. |
This was not primarily meant for you but for anyone who wants to review this PR. |
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.
Having spot-checked some of the files and reviewed the non-SVG changes, I'm okay with this.
I have gone through all ±250 SVG symbols and cleaned them up. I know of no tool that can fully automate this.
fill
,stroke
,stroke-width
, andopacity
.fill-rule
attribute and a strangeviewBox
require changes to the shapes themselves. For this I have used Inkscape with various tricks.Besides its more limited SVG support, Mapnik differs in rendering in two important ways from image viewers and browsers:
Mapnik auto-cropping and centering
Description
Mapnik currently ignores the dimensions of the SVG, and crops the width and height to that of the shape within. What this mostly affects is the placement of the symbol: it is centered on the midpoint of the shape, not the midpoint of the svg. An extra complication is that the calculation is crude: it includes bezier control points, ignores strokes, and includes invisible geometry.
I am not entirely sure how mapnik still manages to correctly align symbols to pixel boundaries, despite the auto-cropping and centering.
The second thing that I think the dimensions of the auto-cropped SVG influence a little is the density of symbols on the map. Mapnik may make its calculations with a bounding box that is smaller (or in rare cases larger) than the SVGs dimensions.
I have a PR for Mapnik that tries to stop the auto-cropping, because it is not compatible with SVG patterns that have to extend a little outside their tile.
Actions
Centering: I have centered the shape in a few SVGs, to make the rendering in viewers match that of Mapnik more closely. Files:
nature/spring.svg
,barrier/toll_booth.svg
,shop/beauty.svg
. Note that if the behavior of Mapnik ever changes, these three files are the ones that would start to look different without this change.Cropping: I have cropped several symbols were the SVG was larger than the actual shape. For example
amenity/bar.svg
was a 12x12 symbol inside a 14x14 SVG, which is now just 12x12. This needed one adjustment to themss
-style wherehighway_bus_stop
used to set the size of the symbol, as an override to an earlier defined size of a square at a lower zoom level.Invisible geometry workaround: More than 100+ files included invisible geometry. Although they don't seem to be necessary to preserve pixel alignment, at kept them for
barrier/toll_booth
,shop/chemist.svg
andshop/houseware.svg
just in case.cliff.svg
,cliff2.svg
definitely need invisible geometry to center correctly.Finally I preserved the invisible geometries in the pattern files
ridge.svg
,ridge2.svg
,arete2.svg
andarete-mid.svg
. I suspect they are necessary, and didn't really want to touch those files much for now.Mapnik color override
The fill of all shapes that do not have
fill="none"
are overwritten by the color set inmarker-fill
properties in the.mss
styles. This ignores the regular inheritance design of properties that is build into SVG. Also, the color only affects fills, not strokes.Actions
Removing colors: For all symbols that are given a color in the
.mss
styles, I have removed the unneccessary colors in the file itself. Usually they didn't have any, or were black or dark gray.Removing strokes:
leisure/sauna.svg
had almost invisibly thin, dark gray strokes around the steam. Removed because I don't think they are intentional.Using strokes: Three symbols have a white casing around them,
natural/spring.svg
,barrier/level_crossing.svg
andbarrier/level_crossing2.svg
. By using a stroke for this, the symbol itself can be colored from the.mss
styles. This should make it a little easier to update the colors.Dual-color: A few symbols have two colors. Because of this they can't be styled from the stylesheets. Files:
amenity/bus_station.svg
,place/*.svg
,man_made/power_tower.svg
andman_made/power_tower_small.svg
.No color:
natural/cave.svg
andbarrier/gate.svg
have no color. I am not sure this is intentional?Preview script
In the
symbols
directory is ansymbols_index.awk
script (withsymbols_index.txt as input
) that can generate a preview of all symbols in one file, with the right size and color. There is no use for it except I found it very helpful while working on the symbols to get an overview. It is not perfect, but tries to imitate the rendering in Mapnik.Output on master: https://gist.githubusercontent.com/pitdicker/e971af0a90c3f7f66deb99c1c3dbd81f/raw/ea46959fbb23c99d369cc720bb2f13c3326bd109/symbols_index_old.svg
Output with this PR: https://gist.githubusercontent.com/pitdicker/8c70d8ca938ba57fa856f153145e783e/raw/5441c7c4e9348c285ccbad0af81caf13c48df6ba/symbols_index.svg
Advantages of this PR
To be honest, this was quite a bit of work for little benefit. Three advantages:
.mss
styles, where the couldn't before.Testing
symbols_index.awk
preview.svg2png
.Fixes #3755, #3393 (at least for now).