-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
Possible incorrect interpretation of Path close spec #137
Comments
P.S. Use of the |
I decided to fix this as part of #138 since it was a similar issue to other two. |
https://www.w3.org/TR/SVG/paths.html#PathDataClosePathCommand If the closepath is followed by a moveto then the moveto is the start of a new subpath. If the closepath is followed by anything else then the next subpath starts at the same point as the initial subpath. M0,0L1,1ZL2,2Z is one path, The Z in both cases returns to 0,0 based on the location of the move.
The Z command goes to the initial position of the last subpath which is always defined by the M command and not by the Z command. |
Actually rereading the spec I am not sure if that is 1 or 2 subpaths. But, in both cases the Z returns to 0,0 based on the initial move location. The second Z is not based on the position of the previous Z. Do note, however, there is no functional difference here since the first Z is by definition based on the location of the M so the second Z being based on the first Z or original M can have no functional difference. However, I'm rather certain that the it is entirely based on the M. I'll check implementations of markers at subpath ends locations in some of the robust implementations and run it by the SVG Working group if there's no agreement there. |
No software put <svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:ev="http://www.w3.org/2001/xml-events" width="310.000000mm" height="210.000000mm" viewBox="0 0 1172 794">
<defs>
<marker id="triangler" viewBox="0 0 10 10"
refX="1" refY="5"
markerUnits="strokeWidth"
markerWidth="10" markerHeight="10"
orient="auto">
<path d="M 0 0 L 10 5 L 0 10 z" fill="#f00"/>
</marker>
<marker id="triangleg" viewBox="0 0 10 10"
refX="1" refY="5"
markerUnits="strokeWidth"
markerWidth="10" markerHeight="10"
orient="auto">
<path d="M 0 0 L 10 5 L 0 10 z" fill="#0f0"/>
</marker>
<marker id="triangleb" viewBox="0 0 10 10"
refX="1" refY="5"
markerUnits="strokeWidth"
markerWidth="5" markerHeight="5"
orient="auto">
<path d="M 0 0 L 10 5 L 0 10 z" fill="#00f"/>
</marker>
</defs>
<path d="M 0,0 L 100,0 Z L 100,50 L 200,200 Z L 0 200" stroke="#000000" transform="translate(10,10)" stroke-width="1.0" marker-start="url(#triangleg)" marker-mid="url(#triangleb)" marker-end="url(#triangler)" fill="none" />
</svg> |
It's clear from some procedures that Inkscape decides that the existence of a Z means there are more than one subpath there. I think it's somewhat ambiguous but I also can't think of any way it could make a difference. The additional subpath can because it multiple things break subpaths and if Z marks a subpath end and implies a move for the next subpath it's an interesting value bit. I might have to ask W3C on that, but I am not sure of a place where the distinction should actively matter. |
Hmmm ... I read the spec again, and whilst it is potentially open to interpretation, on balance I think you are right. |
Hmmm ... I thought about this again whilst I was having a swim in the pool - and decided that there is no real difference between your interpretation (new subpath starts after a Close even if no Move - using the previous Move end point as the initial point of this new subpath) and mine (new subpath starts after a Close even if no Move - using the previous Close end point subpath) because they are actually the same point. However, neither |
Yeah, that seems to be the distinction I see there. I checked the various implementations and Inkscape agrees with that. Since breaking the path breaks at the Z values. Checking in with the SVG Working group. Because that does seem to change things for the implementation there. Inkscape divides the paths into 3 rather than 1, treating close paths and forced path ends with implied start at the previous start. I think the as_subpath() code is in error. It should divide the subpath at a M or at a Z. And generally violate the typical wysiwug path segments and insert the Now Subpath() are not Path() and could potentially point to path fragments, but it could be that path-fragments are just weird but somewhat valid paths. If we can very much have It would be a breaking change though not a serious one to retcon this choice, and display implied initial moves like real moves as was requested in #130, especially here where you really could end up with a subpath of That, or, if we have a Z followed by a non-move command we go ahead and just invent and shove the moveto in there so that we satisfy the That, or, we don't do shit or change shit, and just leave everything the way it currently works. |
Reading between the lines the spec generally suggests:
I think implementing this correctly requires that |
Yes. I agree. "If it begins with any other command" means that it is either the very first command or it follows a "Close" command. If it follows a Close then that Close will have gone back to the original Move (possibly recursively). |
|
zpoint should be able to look for either the previous |
zpoint should look back at the previous |
Yes - it would depend on the We should probably code the default of (0,0) if the path doesn't have any |
0,0 isn't necessarily correct, the correct answer is that the path is invalid. Now admittedly the starting with a lowercase m line makes it seem fairly origin based but it's really not, and all of that would be breaking changes. The as_subpaths() should likely get changed but that's also a breaking change and requires a major version switch but, is still clearly correct in ways the current methods aren't. I dunno about the move from 0,0 for clearly invalid paths. |
I don't have time to check right now, but I think I read in the spec that if there is no initial |
I've read the spec a bunch, if you do an |
Ah yes - that was it. So what happens if you don't have an initial Move then? |
The spec actually says: "A path data segment (if there is one) must begin with a "moveto" command." So a path d string without an initial move is actually invalid - and elsewhere it says A) that you should only render the parts of the path that are valid up to the first invalid point and B) that you should treat a path with no valid commands as None. |
So what should happen in svgelements when a path doesn't have an initial Whatever we do needs to be consistent. To be backwards compatible and not break existing calling code which creates broken paths, we probably need to assume that paths that do not start with a Move actually start at the origin and when looking back in the path for the previous move if we get to the beginning without finding one we need to assume the origin. |
It says you must have an initial move. There are some subpath things that matter. Subpath is off-spec, at least to the extent that the subpaths are not actually defined within the spec explicitly but there are implicit subpaths and these clearly do start at a move and end at a close. And does so for either so M0,0ZZZZZZZZ has a bunch of subpaths and these need to count and be respected. This is a breaking change and requires a version bump but this is closer to what the spec says. |
Yes - it does. So what happens in svgelements when you don't? OK - you are off-spec, so do we raise some sort of Exception or assume an origin start point? I have no real view either way - I just believe that svgelements should be predictable. |
Well, you get a fragment. The lines might not have a start point and there's an open question as to whether we should invent move point or not. This gets more weird obvious when you consider subpaths which do violate this requirement. You must have one for the original path, but these aren't needed for the subpaths. So it seems like it needs a robust and predictable solution. I am leaning toward #130 which suggested invented it. Since fragments were expected to not occur in reality but actually do appear as a real thing within subpaths. And I'm thinking the best solution would be to make a new Move() as part of the validation. If you do M0,0ZZZZ this is 5 subpaths, if you query them they would be Move(0,0)-Close, Close, Close, Close, Close. But, in theory if you took the subpath Close and you sent it to a So Path(Line(start=(0,0), end=(1,1)) would give you Sadly I'd likely need to think through most of these edge conditions and make sure it's consistent.
Etc. I can probably work out a few tests for these things that at are generally consistent with this stuff, but it does need to make sure all the test cases are taken care of, bumping the major version (since it's a breaking change), implementing the altered subpath() commands, and making sure we're consistent. And making sure previous code that might have worked before still works since Path() might actually introduce a Move() that wasn't there before so the equals code and add code etc all might call this code and it might after the change goes in cause a failure. |
I would counsel against inserting an actual I still believe that the choices are: a. Assume an implicit origin start point (non-breaking); or If we go with a. then we also need to decide whether
agree - when you convert a Path back to
agree - as previously stated in #130, IMO the start point of PathSegment objects should not exist or at least not exposed.
complicated - IMO
agreed
agreed
agreed
disagree |
We probably can't change this now, but for the future it might actually be beneficial to implement Subpaths as a subclass of slice object (because effectively that is what it is). You could then do:
|
Also, we can probably get around the whole
|
Yes, but if we take subpath(1) and then wrap that in a Path we took "z" and made it a Path. And paths must begin with an The subpath is absolutely a slice of the path. That's how it's implemented. The subpath objects can just be accessed like path objects but they reflect the underlying path list of the original. PathSegments can't maintain weak-references to the paths they are a part of, that's a lot of things and should maybe be a total of one thing, if they are already a member of a path, they would be better off cloning themselves. Also, pathsegments are supposed to be dumb by design, and I'm not sure garbage collection in python has weak references. And certainly can't and totally should not serve as links in multiple paths. |
Hmm - I may have misunderstood the example because I mis-read the brackets - or perhaps because the brackets are actually unmatched. 😃 But I still disagree. Because I do not believe we should add a missing move, but instead believe we should assume it exists if it is missing - except when creating d when we should add it. Here is an example why (based on a user - for some reason known only to themselves - creating it backwards):
Assuming that you create a Move whenever the first PathSegment is not a Move the "d" result would be Another counter example So IMO in your example |
The subpath code was changed to agree with the spec as the w3c issue confirmed is the expectation. |
Path::z-point determines which point the path close z/Z uses.
If I have read the code correctly, the current implementation looks for the previous Move and uses that point as the close point, however this appears to be the opposite of the specification referred to.
If I have interpreted the spec correctly, it seems to me that we should be searching for the previous Move or Close and using the end point of that object as the z-point.
The text was updated successfully, but these errors were encountered: