fix(convertPathData): fix some weird behavior

[KTibow]

• Move converting to z to the end of the loop. This makes sure the removing useless paths thing runs before it tries to convert to z, preventing extra zs from being added.
• Round relative data before using it to get a length estimate. This fixes some cases where absolute was used instead of relative, leading to possible size increasing and data loss.
• Use error when checking if curves are convertible or not. This fixes some false negatives caused by 0.1 + 0.2-esque situations.

See tests for examples of expected behavior

Results

This PR is just fixing some unexpected behavior that might be already resolved with other methods. However, with Isometric Madness it subtracts 927b without multipass and 915b with.

[KTibow]

@SethFalco would you mind leaving a review?

[SethFalco]

@KTibow Yeah sure, sorry to keep you waiting.

All the changes make sense, but the reason I wanted more time with this is because of a test like this, which is visually different with default settings:

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 0.05 0.05">
    <path d="M 0 0 h 0.0005" stroke="black"/>
</svg>

@@@

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 0.05 0.05">
    <path d="M0 0" stroke="black"/>
</svg>

This PR does not cause this behavior, it's always been there, even in v3.0.2. I was just thinking about it after seeing one of the tests you added. So I was thinking it could be worth improving our regression testing first, which may catch this kind of thing as well as improve our QA overall.

But we can come back to that later, we have a ticket for it already too.

However, with Isometric Madness it subtracts 927b without multipass and 915b with.

Plugins should execute all optimizations it would want upfront without the need of multipass.

In other words, a pipeline with a single plugin should not benefit from consequent executions.

However, I can see that this was already doing this prior to this PR, so this should be addressed separately. For now, happy to approve. 👍🏽

[KTibow]

Plugins should execute all optimizations it would want upfront without the need of multipass.

Not as in additional, I was comparing previous SVGO to current SVGO - how much better is this normally than previous normally? how much better is this with multipass than previous with multipass?

[KTibow]

This PR does not cause this behavior, it's always been there, even in v3.0.2.

Actually the default behavior wouldn't do that, it's always been checking for stroke. If this sounds good, could you a. tell me what "ticket we have already" you were referring to b. consider merging?

[SethFalco]

Actually the default behavior wouldn't do that, it's always been checking for stroke.

The default behavior does remove it, I tested manually. I didn't check which optimization within convertPathData did it, though.

ticket we have already

#1363

consider merging

It will be merged soon, similarly your other pull requests I've reviewed.

I was just saving the merge for next time I'm actively working on SVGO and reviewing pull requests. Just leaves more time for others to chime or spot potential issues.

In general, we'll get around to reviewing/merging/triaging when we can. Keep in mind that none of the maintainers work on SVGO full-time, so the pace just varies. The occasional nudge is fine, but I'd appreciate it if you'd be more patient between interactions.

[KTibow]

The default behavior does remove it, I tested manually. I didn't check which optimization within convertPathData did it, though.

I just realized that maybeHasStrokeAndLinecap also required the linecap - excuse my brain fart.

In general, we'll get around to reviewing/merging/triaging when we can. Keep in mind that none of the maintainers work on SVGO full-time, so the pace just varies. The occasional nudge is fine, but I'd appreciate it if you'd be more patient between interactions.

Right, I understand that. The motivation for "could you leave a review" was "it had been more than a week since this PR was last updated and nothing acknowledged that you had seen this at all", and the motivation for "could you merge" was a bit of confusion as there were some opposing signals (while it was an approval, it wasn't merged and it was talking about some blockers), but I'll take this into account in the future.

[KTibow]

On the problem with the test you sent, I think a better approach would be to change the method for calculating the default floatPrecision. eg, if this SVG was upscaled to [size], what's the minimum precision needed so that any change is less than [real error]? This lets us fix bugs with small viewboxes and get better reductions on large viewboxes.

A precision of 3 (error of 0.001) on a 48x48 SVG shows up as an error as only 0.038px if you upscaled the SVG to fill the height of my laptop. This lets us to reduce the precision to 2, while still staying under 0.5px error.
A precision of 3 (error of 0.001) on a 0.05x0.05 SVG shows up as an error of 36px on my screen (upscaled to fill the height). This means we should increase the precision to 5 to get it down to 0.36px error.

(my laptop's display is 2736 x 1824 (267 PPI), which means pixels are pretty small.)

Thoughts on this idea? Should I open a issue/PR for it?

[SethFalco]

I just realized that maybeHasStrokeAndLinecap also required the linecap

No worries, we'll figure all of it out later. SVGO has done that since <=v3.0.2, so while it'd be nice to address, there's no rush compared to the myriad of other long-standing issues.

as there were some opposing signals (while it was an approval, it wasn't merged and it was talking about some blockers), but I'll take this into account in the future.

Ahh, I see! I'll be clearer in future, sorry for the confusion. And for explicitness, I'm happy to merge this.

I was just making notes on issues I noticed while reviewing this PR that and wanted to be mindful of later.

• The issue is an actual problem, but not caused by this PR. It's likely related to how the default value for floatPrecision is calculated.
• The second issue is just a misunderstanding, which you've cleared up already.

Just to be clear, I had no qualms with a nudge when this went unreviewed for two weeks. While it does add pressure, I can appreciate it's frustrating when you invest your time to open a PR, and it doesn't get reviewed. I was just busy, so I kept to simpler tasks the past 1–2 weeks.

My message namely applied to the follow-up requesting a merge.

---

I think a better approach would be to change the method for calculating the default floatPrecision

Agreed, though I'm not sure what the best approach for this is yet. We have a similar issue with other numbers in SVGO too:

• Plugin 'convertTransform' corrupts image #1810
• altered paths #1858

I'd like if the default value was determined based on the SVG content rather than something we arbitrarily set, for example, maybe something like the highest between:

• 3
• The number of decimal places or significant digits in the viewBox, or if it's not present, the height and width of the closest parent.
• The number of decimal places or significant digits in the transform applied, if applicable.

That definition probably has flaws, so it needs more thought. It's why I was thinking that maybe it was worth improving our regression tests first.

Should I open a issue/PR for it?

If you have an idea, contributions are welcome!

[SethFalco]

The code looks good, and I ran regression tests with the larger dataset locally with no impact on mismatches. Happy to merge, and thanks for being patient!