Implementation of simplify script #7
Conversation
Element was incorrectly being created with a path tag
scripts/simplify.py
Outdated
| sort_element_attr(new_layer) | ||
|
|
||
| # Sub elements of current layer | ||
| if layer.getAttribute('inkscape:label') in ['Board Bottom', 'Board Top']: |
There was a problem hiding this comment.
We should not depend on inkscape features as some people might want to use other tools (we don't know how large this project will eventually get, especially if we might do other revisions of the motherboard or other PCBs in general).
Making the tool generic will also help with making a generic workflow that other communities can re-use.
A solution would be to have 1 file per layer, or check if SVG has some layer support itself.
We could also have some preprocessor / postprocessor which splits or combines the different files into one file for inkscape. However, I don't think that's necessary anyway, as people can work top and bottom independently.
There was a problem hiding this comment.
We should not depend on inkscape features as some people might want to use other tools (we don't know how large this project will eventually get, especially if we might do other revisions of the motherboard or other PCBs in general).
Making the tool generic will also help with making a generic workflow that other communities can re-use.
Fixed in f327736
This commit removes all of the namespaces expect for xlink which is used for linking an external image to an element.
Instead of using Inkscape labels we are now using the element ID attribute. This appears to work correctly in Inkscape as the label for the object.
Also, removed 'inkscape:groupmode'. The side-effect of this that there are no longer 'Layers' in Inkscape, but the same functionality exists in 'Objects'.
The attribute 'inkscape:connector-curvature' was removed. I did not spend a lot of time investing this, but it appears to have no ill-effect on the output.
A solution would be to have 1 file per layer, or check if SVG has some layer support itself.
We could also have some preprocessor / postprocessor which splits or combines the different files into one file for inkscape. However, I don't think that's necessary anyway, as people can work top and bottom independently.
I feel like this outside of the scope this pull request and should be addressed in #8. I will keep this into consideration though.
scripts/simplify.py
Outdated
|
|
||
| # Add UUID if needed | ||
| if not element.getAttribute('pcbre:uuid'): | ||
| element.setAttribute('pcbre:uuid', str(uuid.uuid4())) |
There was a problem hiding this comment.
I'm worried about some tools eating custom tags (I'm not sure how/if SVG enables extensions and if all tools support it properly?) and would have abused the ID tag with a suffix (also because editors will display it then, so we can find it in the file if we generate a warning).
There was a problem hiding this comment.
Using the id attribute appears to be the correct way to implement this functionality.
https://www.w3.org/TR/SVG2/struct.html#Core.attrib
Tested with Inkscape and it appears to work correctly.
Fixed as a side effect of f327736 though not finalized as it's more directly addressed in another comment.
scripts/simplify.py
Outdated
| # Sort elements | ||
| new_layer.childNodes.sort(key=lambda x: x.getAttribute('pcbre:uuid')) | ||
|
|
||
| def process_zones_layer(xml, layer, new_layer): |
There was a problem hiding this comment.
There's a bunch of common code that could be factored out into something like new_element = process_svg_element(old_element, allowed_attribs, default_attribs, sorted=True, ensure_uuid=True)
There was a problem hiding this comment.
I think you should keep wrappers, to make the code more readable:
def process_zones_layer(xml, layer):
return format_group_elements(xml, layer, element_format_rules['zones'])Maybe also process_zone_elements(xml) (as I'm still against keeping layers at all).
You could even check if the path is filled or not (to be fair: a bit tricky due to loose SVG definition of the "style" attribute) to decide if it has to be a zone or a path.
Then you could even turn this into simplify_path_elements(xml).
scripts/simplify.py
Outdated
| new_image = xml.createElement('image') | ||
|
|
||
| # Recreate each allowed attribute in the newly created element | ||
| for allowed_attr in image_allowed_attrs: |
There was a problem hiding this comment.
It should probably warn about dropped attributes (even if there might be a lot of log spam).
scripts/simplify.py
Outdated
|
|
||
| # Add UUID if needed | ||
| if not new_trace.getAttribute('pcbre:uuid'): | ||
| new_trace.setAttribute('pcbre:uuid', str(uuid.uuid4())) |
There was a problem hiding this comment.
Instead of uuid, I'd just pick the first N comma-separated symbols of the d attribute, and hash them or spit out their numbers in some form. Usually a path starts with m <x>,<y>, so this would be the start location of the path.
If the path has been re-imported into the users SVG editor, this is irrelevant, because it will already have it's uuid (wether it's a hash, or some location we once chose).
But if the users working copy does not have the hashes (because they never re-imported after our tool generated hashes), the generated hashes should still match, unless the trace was modified (like movement). If you use the position (instead of a random hash), then it would even be stable if it was moved, as long as it wasn't moved too much. If you quantize the position to some integer then this would probably improve stability and readability of the SVG.
There was a problem hiding this comment.
We probably don't even have to embed the hash/uuid/location in the file explicitly. We'd just have to specify this as sorting criteria (and use the existing d attribute).
(Edit: This actually wouldn't resolve conflicts. However, we could still do this to separate the conflict and sorting issue)
There was a problem hiding this comment.
Ignore this; UUID is probably fine and less error prone.
I'll work on visual conflict resolution in compare.py which will tell us which path of svg_new overlaps with svg_old. If there's too much overlap we can throw away the new trace, even if it has a different name or a different path.
|
Looks like a very good start; in addition to my feedback above (already communicated via Discord):
|
scripts/simplify.py
Outdated
| new_image = xml.createElement('image') | ||
|
|
||
| # Recreate each allowed attribute in the newly created element | ||
| for allowed_attr in image_allowed_attrs: |
There was a problem hiding this comment.
In addition to warning, we probably want to explicitly error on certain attributes that we recognize as problematic:
- allowed attribs: will be kept, anything else is at least a warning.
- disallowed attribs: will be error.
(Could be extended with something where we set allowed defaults, so we'd tolerate some tags if they have a no-op argument; thereby avoiding errors or warnings)
Matches other elements
Forces us to track the attributes on the SVG element and allows to remove easily remove unneeded namespaces in the future.
scripts/simplify.py
Outdated
| 'opacity': '0.5', | ||
| 'fill': '#00ffff', | ||
| 'stroke': '#000000', | ||
| 'stroke-width': '0.99999994px', |
scripts/simplify.py
Outdated
| default_trace_style = { | ||
| 'fill': 'none', | ||
| 'stroke': '#ff0000', | ||
| 'stroke-width': '1.25', |
There was a problem hiding this comment.
No unit? default_zone_style uses pixels.
For compatibility with different files I'd prefer if we could use metric units, then we would have a fixed coordinate system and would only have to scale the images to match it.
There was a problem hiding this comment.
I've changed this to use '1.25px' so it's not left without a unit of measurement. b84721f
I would rather address units of measurements and everything associated with that in a separate issue.
|
|
||
| def main(argv): | ||
| # Load and parse SVG | ||
| input_xml = minidom.parse(argv[0]) |
There was a problem hiding this comment.
Consider using argparse, or use sys.argv directly for readability.
I think argv[0] looks like a bug (implies sys.argv[0]), but it's correct because it's actually sys.argv[1] in this case.
There was a problem hiding this comment.
This was mostly a place holder until a better method for handling the command arguments was implemented. I see the need for additional arguments in the future and I would rather we go ahead and choose a library to handle that instead. At this time I don't have any recommendations on what would be the best way to do this.
There was a problem hiding this comment.
https://docs.python.org/3/library/argparse.html is standard in python
Better matches the usage as this part of the code is handling the SVG root groups.
|
Did a partial rewrite 5ff7a98 to address a lot of the outstanding comments by @JayFoxRox The main difference is the rebuild approach. Instead of creating two different XML/SVG objects, it seemed more straightforward to have a single object. The reason is it was becoming too difficult to track, especially since we'll be adding multiple passes. (Verification, validation, and other command-line options) |
| remove_none_element_nodes(node.childNodes) | ||
|
|
||
| def main(argv): | ||
| # Load and parse SVG |
There was a problem hiding this comment.
Bad indentation. Please check if other instances exist, too.
There was a problem hiding this comment.
Whoops. Stupid editor :P Fixed and checked the rest of the code.
Which 2 XML/SVG objects? Are we talking about layers here? The remainder of this post assuming you meant layers / groups.
Also consider this as a response for #7 (comment): I think splitting at least different views would make sense, even in this PR. It reduces complexity of this PR. The simplify script doesn't have to care about interactions between layers.
We'll need a converter to get a netlist anyway, or to connect different elements. Such tool could just load SVGs in a loop and put each found element into an output SVG / netlist / .. If we have a single file, we need the Keeping the layer stuff out of the simplification script will reduce complexity:
Edit: Because you can't differentiate zones and paths without the group (yet; see #7 (comment)), you'd have to add a CLI flag so the tool knows what it's processing. |
LoveMHz
changed the title
|
I'm pushing this PR forwards. even though I strongly agree with @JayFoxRox on many of his concerns that are still outstanding. I feat that if we don't push forwards with the project that it will become stall. |
Here's the initial go at creating a script to address the need of simplifying SVGs and preparing them for GIT consumption.
This should address issue #2
Sample output
The code is not ready to be merged at this time.