Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

incorrect relative move from getNormalizedPathSegList #50

Closed
i-make-robots opened this issue Jul 22, 2022 · 18 comments
Closed

incorrect relative move from getNormalizedPathSegList #50

i-make-robots opened this issue Jul 22, 2022 · 18 comments
Assignees
Labels
bug Something isn't working spec compliance Some behavior may not be spec-compliant
Milestone

Comments

@i-make-robots
Copy link

i-make-robots commented Jul 22, 2022

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<!-- Created with Inkscape (http://www.inkscape.org/) -->
<svg width="210mm" height="297mm" viewBox="0 0 210 297" version="1.1" id="svg5" inkscape:version="1.1.1 (c3084ef, 2021-09-22)" sodipodi:docname="anniversaire.svg" xmlns:inkscape="http://www.inkscape.org/namespaces/inkscape" xmlns:sodipodi="http://sodipodi.sourceforge.net/DTD/sodipodi-0.dtd" xmlns="http://www.w3.org/2000/svg" xmlns:svg="http://www.w3.org/2000/svg">
  <sodipodi:namedview id="namedview7" pagecolor="#ffffff" bordercolor="#666666" borderopacity="1.0" inkscape:pageshadow="2" inkscape:pageopacity="0.0" inkscape:pagecheckerboard="0" inkscape:document-units="mm" showgrid="false" inkscape:zoom="0.77771465" inkscape:cx="7.7149119" inkscape:cy="576.04676" inkscape:window-width="2560" inkscape:window-height="1330" inkscape:window-x="1680" inkscape:window-y="25" inkscape:window-maximized="1" inkscape:current-layer="layer1"/>
  <g inkscape:label="Layer 1" inkscape:groupmode="layer" id="layer1">
    <g id="g888" transform="matrix(0.11652319,0,0,0.12098981,5.0275387,231.79878)">
      <path d="
M 118.5,102.5
C 118.5,80.168 136.668,62 159,62 
h 144
c 22.332,0 40.5,18.168 40.5,40.5
v 48
c 0,8.239 -16.171,16.553 -30.019,21.169
C 291.56,178.976 262.268,183 231,183 199.732,183 170.439,178.976 148.519,171.669 134.671,167.053 118.5,158.739 118.5,150.5
Z
m 0,80
v -8.518
c 5.971,4.143 14.148,8.208 25.275,11.918 23.41,7.803 54.387,12.1 87.225,12.1 32.838,0 63.815,-4.297 87.225,-12.101 11.128,-3.709 19.304,-7.775 25.275,-11.918
v 8.518
c 0,22.332 -18.168,40.5 -40.5,40.5 
H 159
C 136.668,223 118.5,204.832 118.5,182.5
Z
"/>
    </g>
  </g>
</svg>

Raw data I log receiving from EchoSVG 0.2 is

M 118.5 102.5
C 118.5 80.168 136.668 62.0 159.0 62.0
L 303.0 62.0
C 325.332 62.0 343.5 80.168 343.5 102.5
L 343.5 150.5
C 343.5 158.739 327.329 167.053 313.481 171.669
C 291.56 178.976 262.268 183.0 231.0 183.0
C 199.732 183.0 170.439 178.976 148.519 171.669
C 134.671 167.053 118.5 158.739 118.5 150.5
z
closing to (118.5, 102.5, 0.0)
M 118.5 230.5 // <-- UH OH should be 118.5, 182.5
L 118.5 221.982
C 124.471 226.125 132.648 230.19 143.775 233.9
C 167.185 241.70299 198.16199 246.0 231.0 246.0
C 263.838 246.0 294.815 241.703 318.225 233.899
C 329.353 230.19 337.529 226.12401 343.5 221.981
L 343.5 230.49901
C 343.5 252.83101 325.332 270.99902 303.0 270.99902
L 159.0 270.99902
C 136.668 223.0 118.5 204.832 118.5 182.5
z
closing to (118.5, 230.5, 0.0)
@i-make-robots
Copy link
Author

this is with getNormalized...

@carlosame carlosame added the invalid Not a bug, or lacks enough information label Jul 24, 2022
@carlosame
Copy link
Member

Hello Dan,

You do not mention what are you finding wrong here, I assume that it is the same concern as in #48 (although I should not have to look at another issue to figure out this one). Again, I see nothing wrong here. The description of the method is the following:

(as found in https://github.com/css4j/web-apis/blob/343b24c8474f39e95016b2ce88da48254810300e/svgom-api/src/main/java/org/w3c/dom/svg/SVGAnimatedPathData.java#L36-L65 )

SVGPathSegList getNormalizedPathSegList()

Gets the normalized contents of the d attribute.

The base (i.e., static) contents of the d attribute are provided in a form where
all path data commands are expressed in terms of the following subset of
SVGPathSeg types: PATHSEG_MOVETO_ABS (M), PATHSEG_LINETO_ABS (L),
PATHSEG_CURVETO_CUBIC_ABS (C) and PATHSEG_CLOSEPATH (z).

Thus, if the d attribute has an "absolute moveto (M)" and an "absolute arcto (A)"
command, then normalizedpathSegList will have one PATHSEG_MOVETO_ABS entry
followed by a series of PATHSEG_LINETO_ABS entries which approximate the arc. This
alternate representation is available to provide a simpler interface to developers
who would benefit from a more limited set of commands.

The only valid SVGPathSeg types are PATHSEG_MOVETO_ABS (M), PATHSEG_LINETO_ABS (L),
PATHSEG_CURVETO_CUBIC_ABS (C) and PATHSEG_CLOSEPATH (z).

Returns:
    the normalized contents of the d attribute.

And IMO the values returned by the method are correct. Therefore, I'm closing.

@carlosame carlosame closed this as not planned Won't fix, can't repro, duplicate, stale Jul 24, 2022
@i-make-robots
Copy link
Author

i-make-robots commented Jul 24, 2022 via email

@carlosame
Copy link
Member

“ M 118.5 230.5 // <-- UH OH should be 118.5, 182.5”

Unless you give more details about what you are exactly doing and why, this issue is invalid. Saying "Raw data I log receiving from EchoSVG 0.2" is not enough.

If you provide, for example, a well-commented unit test that fails, there would be more chances that any issue would be identified and fixed.

@carlosame
Copy link
Member

For example, where does the "closing to" comes from...

@i-make-robots
Copy link
Author

i-make-robots commented Jul 24, 2022

Hmm... well, I'll try to give more detail. https://github.com/MarginallyClever/Makelangelo-software/blob/7ca9c7a6f867731ac0fe95a84253a85560a6ea80/src/main/java/com/marginallyclever/makelangelo/makeart/io/vector/LoadSVG.java#L308

I

  • load all path elements,
  • ignore polylines,
  • ignore stroke=none,
  • get the matrix for the path item,
  • get the normalized path seg list,
  • loop to log each path seg using getValueAsString().

in PATHSEG_CLOSEPATH I also print the point to which I am returning (start of sub path), so that I know where the "cursor" is at before the next move command.
That is why I'm confident that the value received from SVGPathSeg.getItem() for the relative move is incorrect. I know it is at
118.5, 102.5 and the next move in the SVG file is m 0,80, aka M 118.5, 182.5. instead, what I get is M 118.5, 230.5. I don't know why it's adding an extra Y50.

@carlosame
Copy link
Member

I finally wrote the test that I mentioned and I believe that reproduces the results above, although mine prints the C coordinates in a different order. I thought that you were doing something in addition to the getNormalizedPathSegList but that's not the case, sorry.

I'll look at it.

@carlosame carlosame reopened this Jul 24, 2022
@carlosame carlosame added spec compliance Some behavior may not be spec-compliant and removed invalid Not a bug, or lacks enough information labels Jul 24, 2022
carlosame added a commit that referenced this issue Jul 27, 2022
@carlosame
Copy link
Member

I made an ugly fix. With that fix, I get the following:

M 118.5 102.5
C 159 62,118.5 80.17,136.67 62
L 303 62
C 343.5 102.5,325.33 62,343.5 80.17
L 343.5 150.5
C 313.48 171.67,343.5 158.74,327.33 167.05
C 231 183,291.56 178.98,262.27 183
C 148.52 171.67,199.73 183,170.44 178.98
C 118.5 150.5,134.67 167.05,118.5 158.74
z
M 118.5 182.5
L 118.5 173.98
C 143.77 185.9,124.47 178.12,132.65 182.19
C 231 198,167.18 193.7,198.16 198
C 318.23 185.9,263.84 198,294.82 193.7
C 343.5 173.98,329.35 182.19,337.53 178.12
L 343.5 182.5
C 303 223,343.5 204.83,325.33 223
L 159 223
C 118.5 182.5,136.67 223,118.5 204.83
z

Does this sound better to you? If it does, I'll wait a bit in case I can figure out a more elegant fix, otherwise I'll commit it.

getNormalizedPathSegList is not used during the rendering and was deprecated (removed from the spec) by W3C. I would not be surprised if you are the only user of that method in the entire world. So if you are happy with this, I'll be too. 🙂

@carlosame carlosame self-assigned this Jul 28, 2022
@carlosame carlosame added this to the 0.2.1 milestone Jul 28, 2022
@i-make-robots
Copy link
Author

Really? It's so much easier to implement than the alternatives! The new Z move looks right. Please let me know when I can try it out.

@carlosame
Copy link
Member

Please let me know when I can try it out.

I just pushed the commit. If you plan to test the current 0.2.1-SNAPSHOT I'll hold this issue open until I hear from you.

@carlosame carlosame added the bug Something isn't working label Jul 28, 2022
@i-make-robots
Copy link
Author

<dependency>
	<groupId>io.sf.carte</groupId>
	<artifactId>echosvg-all</artifactId>
	<version>0.2.1-SNAPSHOT</version>
</dependency>

cannot resolve io.sf.carte:echosvg-all:0.2.1-SNAPSHOT

@carlosame
Copy link
Member

cannot resolve io.sf.carte:echosvg-all:0.2.1-SNAPSHOT

This project has no snapshot repository (the Maven release repo is just a hack over Github Pages). The suggested procedure is to build from source and deploy to local Maven repo (the README explains how to).

@i-make-robots
Copy link
Author

I am very busy this weekend. I'll make a note to try it monday.

@i-make-robots
Copy link
Author

i-make-robots commented Jul 30, 2022

trying to build it now... I don't see a branch for 0.2.1. I'm new to gradle and i don't see a gradle.properties item for version number. Am I missing something? Do I just build the latest (IDEA > Gradle > echosvg-all > tasks > build > build)?

@i-make-robots
Copy link
Author

ok, i found the readme for publishToMavenLocal and got the SNAPSHOT installed. now I get a pile of module not found and I'm out of time to try this further today.

@carlosame
Copy link
Member

As we are coming close to the 0.2.1 release date, @i-make-robots can this be closed? You can always open a new issue if you find another bug.

@i-make-robots
Copy link
Author

Hi! I have been unable to test it. I will try again after the release.

@carlosame
Copy link
Member

Apache Batik has had this bug for 17 years (since AbstractSVGNormPathSegList.java was added in 2005), but about a month after EchoSVG 0.2.1 was released with a fix for it (commit a7adb2a), BATIK-1342 was opened with a patch containing a very similar fix (with a null check instead of a boolean variable).

The next day BATIK-1344 was created, asking for a limited support of the transparent keyword (support for transparent was one of the new features in 0.2.1, see #47 and 2ce93f8).

Coincidences are always amazing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working spec compliance Some behavior may not be spec-compliant
Projects
None yet
Development

No branches or pull requests

2 participants