Fix <use> with no contents #279
Conversation
| @@ -80,7 +80,7 @@ export abstract class GeometryNode extends GraphicsNode { | |||
|
|
|||
| protected getBoundingBoxCore(context: Context): Rect { | |||
| const path = this.getCachedPath(context) | |||
| if (!path) { | |||
| if (!path || !path.segments.length) { | |||
| return [0, 0, 0, 0] | |||
There was a problem hiding this comment.
Is a bounding box with zero width and height "located at 0,0" handled properly in the rest of the code?
When we calculate the union of two bounding boxes do we filter "empty" bounding boxes like this?
What's the bounding box of a point at (0,0)?
E.g. in yFiles, a truely empty bounding box uses negative width and height as sentinel values, so that you can distinguish between non-existing and infinitely small bounding box rectangles...
There was a problem hiding this comment.
You're right, this is not 100% exact at this point and [0,0,0,0] isn't handled specially on the caller side. However, throughout the codebase we use [0,0,0,0] for empty bounding boxes and this hasn't been an issue so far. We could improve this in a separate PR, but let's keep it like this for the moment.
There was a problem hiding this comment.
Thanks for the PR.
- Please add a test case. You can find how to do that [here].(https://github.com/yWorks/svg2pdf.js/blob/master/README.md#testing)
- Please revert changes to the files in dist. They are only updated as part of a release.
- Please also revert yarn.lock
|
Sorry about the extra changes in Thanks for the README pointer. I added the test case from #278. If you'd rather a more interesting test (such as a larger output from MathJax where this occurs), or rather it integrated into the existing |
|
I published the bugfix: https://github.com/yWorks/svg2pdf.js/releases/tag/v2.2.3 |
|
I confirmed that the new release fixes my issues. Thank you!! |
Fixes #278 by correctly checking for empty paths in
GeometryNode.prototype.getBoundingBoxCore. Indeed, the line mentioned in #278 (comment) had a bug that wasn't correctly checking for empty paths.I've confirmed that this fixes #278 in my setting. I didn't add tests because I don't know how they're organized...