Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Fix for issue flutter/#66502. #21372

Merged
merged 1 commit into from
Oct 2, 2020
Merged

Fix for issue flutter/#66502. #21372

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/web_ui/lib/src/engine/canvaskit/path.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ class CkPath implements ui.Path {

@override
void reset() {
_fillType = ui.PathFillType.nonZero;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@luigi-rosso luigi-rosso Sep 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_skPath.reset();
}

Expand Down