Upstream svgpathtools improvement - v2.1 #236

NGExplorer · 2025-05-12T04:07:29Z

Fixes #232

Some GPU does not have 'arc' so we can use cubic bezier for such case.
When GPU does not support 'arc' command Circle can be rendered using 2 arcs

Incorporated review comments mentioned in PR #233

@mathandy Please review and let us know if any changes are required.

mathandy · 2025-05-12T21:42:37Z

Hi @NGExplorer, it looks like several of my requests / comments in #233 have not been taken care of yet. Please take a second look.

mathandy · 2025-05-12T21:47:55Z

Also, would it be possible for you to add some test?

NGExplorer · 2025-05-13T04:45:00Z

Hello @mathandy ,

On vglite-tools-2.1 branch we've incorporated following changes,
We rechecked your feedback on old PR #233 there were 4 review comments

Keep old behavior for 'arc' (keep use_cubics=False as default) to keep old toolkit behavior identical. (link)
We have incorporated this change. (fix)
Use is_polygon variable in call to polyline2pathd
We have incorporated this change. return polyline2pathd(polyline, is_polygon)
points = [(0,0)] (link)
Failing vector shapes-polygon-02-t.svg

Expected output: https://www.w3.org/Graphics/SVG/Test/20080912/htmlEmbedHarness/shapes-polygon-02-t.html
We have modified our application(gpu-vglite-toolkit) to not invoke polyline2pathd(), when there are no points.
Remove spaces around VG commands (link)
We have removed its original patch() which introduced this issue.

Let us know if we missed something to improve svgpathtools.

nicusorcitu · 2025-05-15T12:26:34Z

Hi @mathandy
Could you please review the changes incorporated by Nilam? If there is something else to be done just let us know.
Thank you!

mathandy · 2025-05-15T22:25:18Z

svgpathtools/svg_to_paths.py


-    return d + 'z'
+
+    return d + ' Z'


Does this change have any effect?
(if no, let's remove it to avoid unnecessarily changing output)

No, this changes does not have side-effect. We will remove it.

mathandy · 2025-05-15T22:26:06Z

svgpathtools/svg_to_paths.py

@@ -1,3 +1,6 @@
+#
+# Modified by NXP 2024


Let's remove this attribution -- many folks have worked on this file and your contributions are recorded in the git blame and likely the GitHub contributors view also

Hi @mathandy
This is the modification copyright according to our NXP policy.

For the newly created files we have to add:
"Copyright 2025 NXP"

and just for those kind of files that already exist, but without copyright, we do not want to take any extra credit so this is why it says just:
"Modified by NXP 2025"

Please let me know if staying as is looks acceptable to you.

mathandy · 2025-05-15T22:26:49Z

svgpathtools/svg_to_paths.py

@@ -68,7 +94,7 @@ def polyline2pathd(polyline, is_polygon=False):
    # The `parse_path` call ignores redundant 'z' (closure) commands
    # e.g. `parse_path('M0 0L100 100Z') == parse_path('M0 0L100 100L0 0Z')`
    # This check ensures that an n-point polygon is converted to an n-Line path.
-    if is_polygon and closed:
+    if is_polygon or closed:


What's the justification for this change?

I think this needs to be reverted, but if you have some reason for the change, we can talk about a solution

In SVG, a element is always rendered as a closed shape, regardless of whether the last point explicitly matches the first point. For example:

Test vector: shapes-polygon-01-t.svg

<polygon xml:id="polygon-01" fill="none" stroke="#000000" points="59,45,95,63,108,105,82,139,39,140,11,107,19,65" />
In this case, the first and last points differ, so a 'closed' flag computed from point equality would be false.

The original condition:

if is_polygon and closed:
points.append(points[0])

fails to append the starting point because closed flag is false, even though 'is_polygon' is True. This can lead to incorrect processing in cases where polygon closure needs to be explicitly handled in the logic.

To account for the SVG specification and ensure consistent behaviour, this condition should be updated to:

if is_polygon or closed:
points.append(points[0])

This change ensures that elements are always treated as closed paths, aligning with the SVG specification.

"This can lead to incorrect processing in cases where polygon closure needs to be explicitly handled in the logic."

@NGExplorer Can you give an example of this please?

"This can lead to incorrect processing in cases where polygon closure needs to be explicitly handled in the logic."

@NGExplorer Can you give an example of this please?

I got your point,
polygon() uses polyline2pathd() internally.

closure is implicit in polygon and is_polygon already sets to True in such cases.
Where is in case of polyline is_polygon is false.

In vector drawing close command can be specified using 'z' or Line (origin) command.
We are trying to force 'z' command where possible.

As a result if we compute closed by taking is_polygon in consideration, original condition can be retained.
Patch may look something like following, We will check this in our test-suite and update you by EOD.

closed = (float(points[0][0]) == float(points[-1][0]) and float(points[0][1]) == float(points[-1][1])) + closed = closed or (is_polygon == True) if is_polygon and closed: points.append(points[0]) + if closed: - if is_polygon or closed: d += 'z'

With this change existing tests are failing. So, we've used only point count related change in final fix.

We have extended test-suite to validate our fix.
Please check and approve changes.

mathandy · 2025-05-15T22:29:37Z

svgpathtools/svg_to_paths.py

    """converts the parameters from an ellipse or a circle to a string for a 
    Path object d-attribute"""

-    cx = ellipse.get('cx', 0)
-    cy = ellipse.get('cy', 0)
+    cx = (ellipse.get('cx', 0) or 0)


You've made several changes like this -- what's the justification? Is null a valid option here? If so, why change it to zero?

Reverted this change.

mathandy · 2025-05-15T22:34:26Z

Hi @mathandy Could you please review the changes incorporated by Nilam? If there is something else to be done just let us know. Thank you!

Done.
Also, please add unit tests for any changes to logic (assuming you have justification for them and want to keep them), as well as the new feature added.

NGExplorer · 2025-05-16T12:24:10Z

Hi @mathandy Could you please review the changes incorporated by Nilam? If there is something else to be done just let us know. Thank you!

Done. Also, please add unit tests for any changes to logic (assuming you have justification for them and want to keep them), as well as the new feature added.

Hello @mathandy ,

We have incorporated review comments. Please review.

Best Regards,

Nilam

NGExplorer · 2025-05-22T06:21:18Z

@mathandy Please review and let us know if any changes are required.

mathandy · 2025-05-22T22:57:50Z

svgpathtools/svg_to_paths.py

@@ -1,3 +1,6 @@
+#
+# Modified by NXP 2024, 2025


Did you leave this attribution in by accident or is there some reason you really want it?

hi @mathandy
I left an explanation here: #236 (comment)

Is this acceptable to stay in the code? It just says that we, NXP, modified some code on the offending file. It is NXP policy to enforce this upstream.

If you want to make it a single line and move into or above the if-statement you added, I'll accept this PR and leave it.

If you need to leave it where it is and you won't be angry if I remove it later, then I'll merge and likely remove it later. I can't guarantee that attribution will stay for long though.

OK, we moved our modified copyright on the if block.
Thanks!

mathandy · 2025-05-22T22:59:05Z

Looks great! I asked a question about the attribution, other than that, I think this is ready.

… option References: MGG-1481, MGG-1656 When H/W does not have 'arc' command, circrle or ellispe can be drawn using cubic curves. Start ellipse path from rightmost point and draw clockwise Signed-off-by: Harikrushna Parmar <[email protected]>

…if no dedicated end path is provided References: MGG-1402, MGG-1448 * Handle case when no points are specified in polygon. * Update is_polygon properly for rectangle, polygon and polyline. Signed-off-by: Harikrushna Parmar <[email protected]>

* Added polyline.svg and polygons_no_poits.svg to validate recent changes to svgpathtools Signed-off-by: Nilam Gaikwad <[email protected]>

mathandy · 2025-05-27T23:26:37Z

Thanks @nicusorcitu and @NGExplorer !

NGExplorer mentioned this pull request May 12, 2025

Upstream svgpathtools improvement #233

Closed

mathandy reviewed May 15, 2025

View reviewed changes

NGExplorer force-pushed the release/vglite-tools-2.1 branch from d422483 to dfac122 Compare May 16, 2025 12:05

NGExplorer force-pushed the release/vglite-tools-2.1 branch from dfac122 to 0e39e95 Compare May 19, 2025 11:16

mathandy reviewed May 22, 2025

View reviewed changes

NGExplorer force-pushed the release/vglite-tools-2.1 branch from 0e39e95 to b6370f7 Compare May 26, 2025 06:12

Harikrushna Parmar and others added 3 commits May 26, 2025 12:00

MGG-1656 [#ccc] svgpathtools: Adding test cases to validate fixes

Loading
Loading status checks…

aeb799b

* Added polyline.svg and polygons_no_poits.svg to validate recent changes to svgpathtools Signed-off-by: Nilam Gaikwad <[email protected]>

NGExplorer force-pushed the release/vglite-tools-2.1 branch from b6370f7 to aeb799b Compare May 26, 2025 06:31

mathandy merged commit 2c6c3b7 into mathandy:master May 27, 2025
21 checks passed

Upstream svgpathtools improvement - v2.1 #236

Upstream svgpathtools improvement - v2.1 #236

Conversation

NGExplorer commented May 12, 2025

Uh oh!

mathandy commented May 12, 2025

Uh oh!

mathandy commented May 12, 2025

Uh oh!

NGExplorer commented May 13, 2025

Uh oh!

nicusorcitu commented May 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NGExplorer May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathandy commented May 15, 2025

Uh oh!

NGExplorer commented May 16, 2025

Uh oh!

NGExplorer commented May 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathandy commented May 22, 2025

Uh oh!

Uh oh!

Uh oh!

mathandy commented May 27, 2025

Uh oh!

NGExplorer May 19, 2025 •

edited

Loading