fix: svgparser creates polygon with no points#133
Open
Dexus wants to merge 2 commits into
Open
Conversation
Dexus
force-pushed
the
fix/svgparser-invalide-polygon-with-no-points
branch
from
32e324a to
7d8bb20
Compare
willmurnane
reviewed
May 21, 2025
| if(transformString) { | ||
| path.setAttribute('transform', transformString); | ||
| if (isNaN(r) || r === 0) { | ||
| console.warn('Degenerate circle encountered and will be removed or ignored:', element); |
Collaborator
There was a problem hiding this comment.
I think just "ignored" would be clearer
Comment on lines
+1115
to
1118
| var attr = element.attributes[k]; | ||
| if (attr.name !== 'cx' && attr.name !== 'cy' && attr.name !== 'r' && attr.name !== 'transform') { | ||
| path.setAttribute(attr.name, attr.value); | ||
| } |
Collaborator
There was a problem hiding this comment.
This copies all the attributes except the specified ones. What if we did for (const attr of ['style', 'fill', 'stroke', 'strokeWidth']) { instead, so only the attributes that were previously copied are copied by the new code?
Comment on lines
+1107
to
+1111
| // Create a path representing the circle | ||
| // M cx-r,cy A r,r 0 1,0 cx+r,cy A r,r 0 1,0 cx-r,cy Z | ||
| var d = `M ${cx - r},${cy} A ${r},${r} 0 1,0 ${cx + r},${cy} A ${r},${r} 0 1,0 ${cx - r},${cy} Z`; | ||
| path.setAttribute('d', d); | ||
| path.setAttribute('transform', transformString); // Apply the original transform to the new path |
Collaborator
There was a problem hiding this comment.
Instead of creating a string, parsing the string into Arcs, and applying a transform to those, could we create the Arcs directly using the constructor and transform them?
| uniquePolyForCheck.pop(); | ||
| } | ||
|
|
||
| if (uniquePolyForCheck.length >= 3 && this.isPolygonSelfIntersecting(uniquePolyForCheck)) { |
Collaborator
There was a problem hiding this comment.
- I think this is really two error cases: less than 3 points, not a polygon we can nest and self-intersecting polygon, which should generate different error messages.
- I don't think self-intersecting polygons should necessarily be skipped. If I want to cut out a pair of drop shapes 💧 by making a single 8, I think it's reasonable to cut that out.
Member
Author
There was a problem hiding this comment.
its more like make 2 triangles, with 4 lines, once you cross lines, its selfintersecting which is not allowed. Like an X.
| }; | ||
| } | ||
|
|
||
| SvgParser.prototype.isPolygonSelfIntersecting = function(uniquePolygonPoints) { |
Collaborator
There was a problem hiding this comment.
- Can we make this a member of the Polygon class?
- Can we add a unit test or two that demonstrates the correctness of the implementation? It looks reasonable to me, but there are some optimizations I can envision (I don't think we need to check for crossing of the lines formed from points (i, i+1) and (i+1, i+2) for example) and it'd be nice to have a test that shows whether those optimizations are valid.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request refactors the
SvgParserclass to improve handling of SVG elements and introduces new functionality for detecting self-intersecting polygons. The key changes include simplifying the replacement logic forcircleandrectelements, adding a self-intersection check for polygons, and handling degenerate shapes more robustly.Refactoring and Simplification:
circleandrectelements by removing redundant transformation code and directly applying transformations to the new elements. Degenerate shapes (e.g., circles with radius 0 or rectangles with width/height 0) are now identified and removed with a warning. (main/svgparser.js, main/svgparser.jsL1099-R1162)New Features:
isPolygonSelfIntersecting) for polygons. This checks if a polygon's edges intersect inappropriately, and skips processing of self-intersecting paths with a warning. (main/svgparser.js, main/svgparser.jsR1510-R1594)Debugging Enhancements:
console.logandconsole.warnstatements to provide insights into the processing of paths, including degenerate shapes and self-intersections. (main/svgparser.js, main/svgparser.jsR1510-R1594)