css4j / echosvg

SVG implementation in the Java™ Language, fork of Apache Batik, supporting level 4 selectors and colors.
Apache License 2.0
40 stars 2 forks source link

incorrect relative move from getNormalizedPathSegList #50

Closed i-make-robots closed 2 years ago

i-make-robots commented 2 years ago
<?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 commented 2 years ago

this is with getNormalized...

carlosame commented 2 years ago

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.

i-make-robots commented 2 years ago

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

On Jul 24, 2022, at 3:19 AM, carlosame @.***> wrote:

 Closed #50 as not planned.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

carlosame commented 2 years ago

“ 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 commented 2 years ago

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

i-make-robots commented 2 years ago

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

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 commented 2 years ago

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 commented 2 years ago

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. 🙂

i-make-robots commented 2 years ago

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 commented 2 years ago

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.

i-make-robots commented 2 years ago
<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 commented 2 years ago

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 commented 2 years ago

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

i-make-robots commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

carlosame commented 1 year ago

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 a7adb2a2341e7a5197be8bd797f576d87d6296cf), 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 2ce93f813482db4c0e7849c55ae296e1a132fca7).

Coincidences are always amazing.