exportSVG produces svgs with incorrect size. #1317
Conversation
The exportSVG function produced incorectly scaled svgs. Also the UnitOfMeasure is not something that should be guessed based on a heuristic but something that should be passed along by the user with the default being mm. The UnitScale has been set to 3.7795 as per the svg spec and creates svgs with the correct size.
Remove print functions
Fix formatting errors
There was a problem hiding this comment.
Took me a while to realize that the original implementation scales the projection to fit inside the dimensions of the SVG. This PR changes that behaviour so that the projection is scaled to 1:1. However, if we're doing 1:1 SVG's then IMO the dimensions of the SVG should be automatically calculated.
With the current state of the PR, if the part is too big to fit the default dimensions it will just get cropped and the user needs to start guessing the right dimensions (in pixels!) to get it to fit.
However, before pouring more effort on this, you might want to get more comments/thoughts on the topic. Normally it's preferable to open an issue before opening a PR. ( https://github.com/CadQuery/cadquery/blob/master/README.md#contributing-code )
| @@ -125,7 +98,7 @@ def getPaths(visibleShapes, hiddenShapes): | |||
| return (hiddenPaths, visiblePaths) | |||
|
|
|||
|
|
|||
| def getSVG(shape, opts=None): | |||
| def getSVG(shapes, opts=None): | |||
There was a problem hiding this comment.
Since accidentally having the wrong unit could lead to unnecessary end user headache, I would consider moving unitOfMeasure all the way as a separate kwarg to make it more obvious to the user.
| def getSVG(shapes, opts=None): | |
| def getSVG(shapes, unitOfMeasure=UNITS.MM, opts=None): |
| @@ -150,9 +123,10 @@ def getSVG(shape, opts=None): | |||
|
|
|||
| # Available options and their defaults | |||
| d = { | |||
| "unitOfMeasure": UNITS.MM, | |||
There was a problem hiding this comment.
| "unitOfMeasure": UNITS.MM, | |
| "unitOfMeasure": unitOfMeasure, |
Also, document in the docstring :param unitOfMeasure: above :param opts: (line 107)
If the suggestion is not implemented, unitOfMeasure needs to be still added to the opts documentation.
| @@ -125,7 +98,7 @@ def getPaths(visibleShapes, hiddenShapes): | |||
| return (hiddenPaths, visiblePaths) | |||
|
|
|||
|
|
|||
| def getSVG(shape, opts=None): | |||
| def getSVG(shapes, opts=None): | |||
There was a problem hiding this comment.
shape -> shapes is a breaking change (and in fact breaks 18 tests). Backwards compatibility could be maintained by allowing both single and iterable inputs
| @@ -236,12 +210,15 @@ def getSVG(shape, opts=None): | |||
| bb = Compound.makeCompound(hidden + visible).BoundingBox() | |||
|
|
|||
| # width pixels for x, height pixels for y | |||
There was a problem hiding this comment.
comment not relevant anymore
| @@ -51,36 +46,14 @@ | |||
| --> | |||
| </g>""" | |||
|
|
|||
| PATHTEMPLATE = '\t\t\t<path d="%s" />\n' | |||
| PATHTEMPLATE = ' <g stroke-opacity="1" stroke-linejoin="bevel" font-style="normal" stroke-linecap="square" font-family="MS Shell Dlg 2" stroke="rgb(%(strokeColor)s)" fill="none" font-weight="400" transform="scale(%(unitScale)s, %(unitScale)s) translate(%(xTranslate)s,%(yTranslate)s)" stroke-width="%(strokeWidth)s" font-size="55.5625">\n <path vector-effect="none" d="%(path)s" fill-rule="evenodd"/>\n </g>\n' | |||
There was a problem hiding this comment.
transform="scale(%(unitScale)s, %(unitScale)s)
Y axis should (must?) be flipped here (like in the old implementation). CQ/OpenCascade has Y axis moving upwards whereas SVG has Y axis moving downwards. Without flipping it the resulting SVG will be a horizontal mirror of the actual projection.
|
|
||
| # compute amount to translate-- move the top left into view | ||
| (xTranslate, yTranslate) = ( | ||
| (0 - bb.xmin) + marginLeft / unitScale, | ||
| (0 - bb.ymax) - marginTop / unitScale, | ||
| (0 + bb.ymax) + marginTop / unitScale, |
There was a problem hiding this comment.
Shouldn't be necessary if the Y axis is flipped. (ln 49)
Yeah sorry about the quality of the PR 🥲 I changed the code to fit my needs and didn't go through the rest of the codebase. I will close this issue for now, create an issue and break this into multiple PRs as this changes too many things. I really appreciate the feedback though and I will take it into account for the next PRs! |
The exportSVG function produced incorectly scaled svgs. Also the UnitOfMeasure is not something that should be guessed based on a heuristic but something that should be passed along by the user with the default being mm. The UnitScale has been set to 3.7795 as per the svg spec and creates svgs with the correct size.