Started filtering out close line segments in rrect polylines.

[gaaclarke]

fixes flutter/flutter#156422

tests: AiksTest.GradientOvalStrokeMaskBlurSigmaZero

This filtering needs to happen somewheres. I opted for putting it inside of the AddLinearComponent instead of where it was affected me in the test:

engine/impeller/geometry/path_builder.cc

Line 180 in 31aaaaa

AddLinearComponent(

This seems preferable. A tiny line segment should never matter. The rub is that its only happening here. We might want to do this in other places as well. It's equally unimportant to have a tiny curve.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

[jonahwilliams]

LGTM

[flar]

Do we know what the transform is? The skippable distance should take that into account for when we zoom in on a shape.

[gaaclarke]

The transform doesn't matter because we are hitting the limits of the float32 format. So even if this is scaled up to be meaningful the points would be unstable.

[jonahwilliams]

Right, we don't practically care about rendering correctly with a 1000000000x scale.

[flar]

.001 is nowhere near a 1000000000x scale. Scaling up by 100 can affect pixelization. Scaling by 1000 could leave a pixel gap or miss a pixel entirely.

[flar]

And "the limits of float32 format" - how are you defining that? It can handle 10^(+/-37) and it can represent pixels up to +/- 16 million before it loses sub-pixel accuracy.

If you want to take float precision into account then you should use the built-in ulp-based tests.

Note that large scales aren't necessariy for large zooms. A developer could create geometry in the 0.0->1.0 coordinate space and then scale it into place. On a 4k monitor that could involve scales larger than 1000x.

[gaaclarke]

how are you defining that?

It's the standard definition for Impeller, ScalarIsNearly which uses the tolerance kEhCloseEnough which is 1e-3f. If that isn't sufficient we should consider changing kEhCloseEnough (and ScalarIsNearly).

[flar]

This solution is fixing a symptom of a deeper flaw in the logic of the join procs.

[gaaclarke]

This solution is fixing a symptom of a deeper flaw in the logic of the join procs.

Can you elaborate? From my debugging I don't believe it is related to the join procs. I have all my notes in the issue.

edit: Nevermind, I see you added that info to the issue, let me read through it.

[flar]

I tried my suggested fix in the join procs and it didn't improve the situation. My guess is that the bevel join is drawing the join in the wrong direction. I still see an issue here because nearly colinear lines can have outer corners that are far apart and the join proc just lets it go, which is not appropriate.

For now, I'll withdraw my "request changes" from this PR. I still believe it is not the right solution here, but until I can nail down the issues in the join procs, I don't have any other suggestions - I just wish it was based more on the transform and the line width as tiny lines at small angles can swing widely at the outer end of their stroked representaiton...

I don't have a better solution for now.

[gaaclarke]

For now, I'll withdraw my "request changes" from this PR. I still believe it is not the right solution here, but until I can nail down the issues in the join procs, I don't have any other suggestions - I just wish it was based more on the transform and the line width as tiny lines at small angles can swing widely at the outer end of their stroked representaiton...

I'll spend some time poking around in the miter join. Maybe we can tighten the epsilon here too if we land this. A scale of 1000x before seeing the difference isn't so big, 10,000x sounds more in the realm were we might omit something.

[flar]

For now, I'll withdraw my "request changes" from this PR. I still believe it is not the right solution here, but until I can nail down the issues in the join procs, I don't have any other suggestions - I just wish it was based more on the transform and the line width as tiny lines at small angles can swing widely at the outer end of their stroked representaiton...

I'll spend some time poking around in the miter join. Maybe we can tighten the epsilon here too if we land this. A scale of 1000x before seeing the difference isn't so big, 10,000x sounds more in the realm were we might omit something.

1000 is fine if we're talking pixel distances (perhaps even too generous). If we can factor the transform in (it would only need to be computed once at the start of the operation) then we just supply that computed number as the "nearly" metric in the function (it takes a 3rd arg that defaults to kEhCloseEnough).

But we also have to consider that if a bunch of really tiny lines are changing direction then the resulting widened line might have some noticeable curving to it that will look more like "spinning in place". That may be obscured by the widened parts next to it, but only if they are long enough to cover the turning of the widened pen. Any change in direction is like a person walking holding a really long ladder sideways and turning around and often the slapstick of the swinging ladder is visible outside the scene. And, if this is the only contour in the path, then we should treat it like a zero-length line and do just a giant square or circle per the cap (bevel typically does a big square even though it doesn't normally have any decoration).

Lots of considerations that I think are ignored by this simple fix, but so far I don't have any more targeted fix that gets rid of those dimples.

I'm still asking the question - even if only in my head - how does eliminating a segment cause the dimple to join?

[gaaclarke]

@flar here's a new thought I haven't explored yet. Do you think the fact the polyline is oscillating is messing it up? It goes up down up. My PR filters that out by virtue of the points being so close to eachother. I see we are calculating directions at some point in the math.

polyline.GetPoint(point_i-1)
{...}
x =
199.803528
y =
25.1365395
polyline.GetPoint(point_i)
{...}
x =
200
y =
30.0000019
polyline.GetPoint(point_i+1)
{...}
x =
200
y =
29.9999981
polyline.GetPoint(point_i+2)
{...}
x =
199.803497
y =
34.8634567

[gaaclarke]

I exhausted my timebox to debug the miter join. I didn't find anything. For this PR I can move the tolerance to 1e-5 which is a 100,000x scale which I think is within our acceptable limit. Some tests are failing though since there is code that banks on adding duplicate values to the path, like FillPathGeometry when representing points. We might have to pull on that thread a bit but might be well entrenched with how things work.

[flar]

What could be happening is that the lines are so small and their directions flip a lot and so we end up doing the miter join on the wrong side of that gap because it looks like the 2 adjacent lines are concave rather than convex?

[flar]

If we skip a join on one segment that is nearly colinear then that segment still becomes the basis for the join for the next line. We should probably maintain the previous segment for computing the next join?

[gaaclarke]

I'm switching this to just be applicable to rrect paths. I think there is probably a deeper bug in the polyline that gives it that minute squiggle in it. If it never manifests elsewhere though its probably not worth chasing down. I've added a unique interactive test for the rounded rect to verify further that this is good. (and discovered another error: flutter/flutter#157173)

[flutter-dashboard]

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #55929 at sha dd7be93