Fix for issue flutter/#66502.

[luigi-rosso]

Description

Fixes issue #66502 where the fillType is cached on the CkPath and doesn't get synced when the SkPath is reset.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the [contributor guide] and followed the process outlined there for submitting PRs.
• I signed the [CLA].
• I read and followed the [C++, Objective-C, Java style guides] for the engine.
• I read the [tree hygiene] wiki page, which explains my responsibilities.
• I updated/added relevant documentation.
• All existing and new tests are passing.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read [handling breaking changes].

• No, no existing tests failed, so this is not a breaking change.

[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 to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[yjbanov]

I think you want to do fillType = ui.PathFillType.nonZero (otherwise it won't send the value over to the WASM side; see line 37).

[yjbanov]

Ah, nvm. You explained it in the issue. _skPath.reset() does that already.

[luigi-rosso]

That was intentional as that's what the WASM side defaults it to (perhaps more future proof to set it as well).

[flar]

If this is implementing ui.Path.reset(), then it should not modify the winding rule. Instead of hard-coding the rule to nonZero, it should reset the WASM side to the current local value.

[luigi-rosso]

The underlying call to SkPath::reset does this: https://github.com/google/skia/blob/1b89eb742a66257cd780f119c537e833f4554f53/src/core/SkPath.cpp#L163

The stored _fillType needs to be synced with that or further rendering can use the wrong fillType as the CkPath believes its stored _fillType is still set on the backing path. This is the behavior we're seeing, where paths with evenOdd will start using nonZero after calling reset and cannot be set back to evenOdd without first changing them to nonZero.

Setting it to nonZero simply does the equivalent of what Skia is doing so that stored value is in sync. The alternative, more future proof way to do this would be to query the fill rule from _skPath and sync the value stored in CkPath that way. Unless the main flutter engine expects a ui.path.reset to force the fillType/Rule to some other value.

[flar]

Ignore my previous comment, I was looking at an old version of the sources (I had been investigating an old bug) and what I was reading implied that the fillType was not supposed to be changed on a reset(), but now I see that this is the correct behavior.

[yjbanov]

Is there a way to test this? The only thing I can think of is a screenshot test but we don't have that infra ready yet.

[yjbanov]

Ah, nvm. You explained it in the issue. _skPath.reset() does that already.

[flar]

I believe we need to send the local fill type along to the _skPath after it is reset, not the other way around.

[luigi-rosso]

More details: flutter/flutter#66502

[flar]

LGTM

[luigi-rosso]

Need anything further here? I don't think there's a good way to test this without screenshots or exposing some test-only getter for the backing SkPathFillType.

[flar]

I can see a possibility for a couple of tests:

• Set non-default value, reset path, check value was reset

• Check for correct behavior of contains(Offset) before/after reset with a path whose interior changes depending on fill type. After the reset, you'd have to insert the path segments back into it, but don't specifically change the fill type so it uses the value it got from the reset.

[yjbanov]

Merging. I will add a test in a follow-up PR. Thanks, @luigi-rosso !

[luigi-rosso]

Sorry for not coming back to this in time to submit a test, @flar had some really good examples. Let me know if that's something you'd like me to look into @yjbanov.