fix: useless path collapsing for segments that are not start and end #1755
Conversation
| if ( | ||
| params.removeUseless && | ||
| !hasMarkerMid && | ||
| (!maybeHasStrokeAndLinecap || !(isStart || isEnd)) | ||
| ) { |
There was a problem hiding this comment.
hasRoundLinejoinAndLinecap from #1751 could also be included.
| if ( | |
| params.removeUseless && | |
| !hasMarkerMid && | |
| (!maybeHasStrokeAndLinecap || !(isStart || isEnd)) | |
| ) { | |
| if ( | |
| params.removeUseless && | |
| !hasMarkerMid && | |
| (hasRoundLinejoinAndLinecap || !maybeHasStrokeAndLinecap || !(isStart || isEnd)) | |
| ) { |
There was a problem hiding this comment.
This doesn't make sense as these do fundamentally different things.
If it has a linecap, we can't remove segments off the end. We can only remove segments in between.
If it doesn't have a linecap, we can remove all useless segments.
What you're proposing is that if it has a round linejoin, we can change M5 5h0 to nothing. That would cause incorrect behavior, because the original shows up as a dot.
*I would personally write this as maybeHasStrokeAndLinecap ? !isStart && !isEnd : true as I find that way easier to read and understand, but style is personal.
| if (params.removeUseless && !maybeHasStrokeAndLinecap) { | ||
| // l 0,0 / h 0 / v 0 / q 0,0 0,0 / t 0,0 / c 0,0 0,0 0,0 / s 0,0 0,0 | ||
| // remove useless non-start/end path segments | ||
| var isStart = !path[index - 1] || path[index - 1].command === 'm'; |
There was a problem hiding this comment.
Shouldn't this be command === 'm', not testing if the previous one is m?
| if ( | ||
| params.removeUseless && | ||
| !hasMarkerMid && | ||
| (!maybeHasStrokeAndLinecap || !(isStart || isEnd)) | ||
| ) { |
There was a problem hiding this comment.
This doesn't make sense as these do fundamentally different things.
If it has a linecap, we can't remove segments off the end. We can only remove segments in between.
If it doesn't have a linecap, we can remove all useless segments.
What you're proposing is that if it has a round linejoin, we can change M5 5h0 to nothing. That would cause incorrect behavior, because the original shows up as a dot.
*I would personally write this as maybeHasStrokeAndLinecap ? !isStart && !isEnd : true as I find that way easier to read and understand, but style is personal.
| // l 0,0 / h 0 / v 0 / q 0,0 0,0 / t 0,0 / c 0,0 0,0 0,0 / s 0,0 0,0 | ||
| // remove useless non-start/end path segments | ||
| var isStart = !path[index - 1] || path[index - 1].command === 'm'; | ||
| var isEnd = !path[index + 1] || path[index + 1].command === 'm'; |
There was a problem hiding this comment.
If I understand what I'm reading correctly, this current version doesn't remove move statements at the very end of the path, which are completely useless. It should.
No description provided.