[Impeller] Fixes stroke path geometry that can draw outside of path if the path ends at sharp turn. #45252

chunhtai · 2023-08-29T22:45:02Z

The issue is that when we draw a line we always try to draw a complete rectangle from point 1 to point2 before drawing the join and the next line

This becomes a problem if there is a sharp turn

Notice there will be a slight over drawing at the bottom.

The solution then is to not draw the rect first before drawing the join. It will be the join's responsibility to draw the complete the rect from the line start to the join.

This should also save a lot of repeatedly drawing during the join

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 Hixie said 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.

chunhtai · 2023-08-30T17:53:38Z

impeller/aiks/aiks_unittests.cc

+  builder.AddArc(rect, Degrees(0), Degrees(90), false);
+
+  canvas.DrawPath(builder.TakePath(), paint);
+  ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));


I am not too familiar with the code, let me know if this is enough to set up golden test.

Yup, this is the correct way to add a golden. 👍

chunhtai · 2023-08-30T21:11:23Z

impeller/entity/geometry/stroke_path_geometry.cc

      vtx.position = polyline.points[point_i - 1] + offset;
      vtx_builder.AppendVertex(vtx);
      vtx.position = polyline.points[point_i - 1] - offset;
      vtx_builder.AppendVertex(vtx);
-      vtx.position = polyline.points[point_i] + offset;


This was where the complete rect is drawn. This part is now handle by the join when then draw the first pair of inner miter point and outer

flutter-dashboard · 2023-08-30T21:12:54Z

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 #45252 at sha 018c24d

chunhtai · 2023-08-30T21:15:38Z

oops looks like the golden has failed

flutter-dashboard · 2023-08-30T21:18:53Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

flutter-dashboard · 2023-09-01T19:16:32Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #45252 at sha bff1fa2

flutter-dashboard · 2023-09-01T23:07:20Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #45252 at sha e273a81

chunhtai · 2023-09-01T23:09:33Z

The circle golden is missing a tiny tiny slide of pixels which i think won't affect the real world pixel, but let me know if this is a problem

@bdero This is ready for review now

chinmaygarde

LGTM but @bdero is more familiar with this code and may have thoughts.

Thanks so much!

bdero

This patch introduces regressions with most strokes (visible in EntityTest.StrokeCapAndJoinTest).

bdero · 2023-09-05T19:02:28Z

Screen.Recording.2023-09-05.at.12.01.38.PM.mov

chunhtai · 2023-09-12T21:53:26Z

I also added a new golden test that similar to the cubic component playground

flutter-dashboard · 2023-09-12T22:03:15Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #45252 at sha ef08968

bdero

EntityTest.StrokeCapAndJoinTest still doesn't look right. This is the line I use to build and run it:

autoninja -C ../out/host_debug_unopt/ -j 1000 flutter/impeller:impeller_unittests && ../out/host_debug_unopt/impeller_unittests --timeout=0 --gtest_filter="Play/EntityTest.StrokeCapAndJoinTest/Metal" --enable_playground

bdero · 2023-09-13T17:15:01Z

Ah hold on one sec, I think I screwed up the checkout locally.

bdero

LGTM! Thanks a lot for your work on this! :)

bdero · 2023-09-13T17:23:14Z

The goldens are looking good and I marked them as positive.

… of path if the path ends at sharp turn. (flutter/engine#45252)

…134683) flutter/engine@b71b366...154d6fd 2023-09-13 [email protected] Auto update dependencies for web_ui. (flutter/engine#45754) 2023-09-13 [email protected] Revert "[Impeller] Patch the compiler to account for subpass inputs and PSO metadata." (flutter/engine#45777) 2023-09-13 [email protected] Roll Skia from 471216072c74 to e39cf360ea93 (4 revisions) (flutter/engine#45776) 2023-09-13 [email protected] [Impeller] Remove dependency on //flutter/vulkan and use a single proc table. (flutter/engine#45741) 2023-09-13 [email protected] Roll Skia from 78d18d509475 to 471216072c74 (4 revisions) (flutter/engine#45774) 2023-09-13 [email protected] [Impeller] Fixes stroke path geometry that can draw outside of path if the path ends at sharp turn. (flutter/engine#45252) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…f path if the path ends at sharp turn. (flutter#45252)" This reverts commit 343dfe8.

#46334) â�¦f path if the path ends at sharp turn. (#45252)" This reverts commit 343dfe8. introduced a regression flutter/flutter#135225 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…lutter#134683) flutter/engine@b71b366...154d6fd 2023-09-13 [email protected] Auto update dependencies for web_ui. (flutter/engine#45754) 2023-09-13 [email protected] Revert "[Impeller] Patch the compiler to account for subpass inputs and PSO metadata." (flutter/engine#45777) 2023-09-13 [email protected] Roll Skia from 471216072c74 to e39cf360ea93 (4 revisions) (flutter/engine#45776) 2023-09-13 [email protected] [Impeller] Remove dependency on //flutter/vulkan and use a single proc table. (flutter/engine#45741) 2023-09-13 [email protected] Roll Skia from 78d18d509475 to 471216072c74 (4 revisions) (flutter/engine#45774) 2023-09-13 [email protected] [Impeller] Fixes stroke path geometry that can draw outside of path if the path ends at sharp turn. (flutter/engine#45252) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

#46334) â�¦f path if the path ends at sharp turn. (#45252)" This reverts commit 343dfe8. introduced a regression flutter/flutter#135225 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

github-actions bot added the e: impeller label Aug 29, 2023

chinmaygarde changed the title ~~Fixes stroke path geometry that can draw outside of path if the path …~~ Fixes stroke path geometry that can draw outside of path if the path ends at sharp turn. Aug 29, 2023

chinmaygarde assigned chunhtai Aug 29, 2023

chunhtai force-pushed the issues/133032 branch from 47a0133 to 018c24d Compare August 30, 2023 17:46

chunhtai commented Aug 30, 2023

View reviewed changes

chunhtai requested a review from chinmaygarde August 30, 2023 20:45

chunhtai marked this pull request as ready for review August 30, 2023 21:11

chunhtai commented Aug 30, 2023

View reviewed changes

flutter-dashboard bot added the will affect goldens label Aug 30, 2023

chunhtai marked this pull request as draft August 30, 2023 21:15

chunhtai force-pushed the issues/133032 branch 2 times, most recently from 7b9dc01 to bff1fa2 Compare September 1, 2023 18:38

chunhtai marked this pull request as ready for review September 1, 2023 18:38

chunhtai changed the title ~~Fixes stroke path geometry that can draw outside of path if the path ends at sharp turn.~~ [WIP]Fixes stroke path geometry that can draw outside of path if the path ends at sharp turn. Sep 1, 2023

chunhtai removed the request for review from chinmaygarde September 1, 2023 18:38

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Sep 1, 2023

chinmaygarde requested a review from bdero September 1, 2023 20:42

chunhtai force-pushed the issues/133032 branch from bff1fa2 to 8e4f30d Compare September 1, 2023 22:25

chunhtai removed the Work in progress (WIP) Not ready (yet) for review! label Sep 1, 2023

chunhtai changed the title ~~[WIP]Fixes stroke path geometry that can draw outside of path if the path ends at sharp turn.~~ Fixes stroke path geometry that can draw outside of path if the path ends at sharp turn. Sep 1, 2023

chinmaygarde changed the title ~~Fixes stroke path geometry that can draw outside of path if the path ends at sharp turn.~~ [Impeller] Fixes stroke path geometry that can draw outside of path if the path ends at sharp turn. Sep 5, 2023

chinmaygarde approved these changes Sep 5, 2023

View reviewed changes

bdero suggested changes Sep 5, 2023

View reviewed changes

chunhtai added 6 commits September 12, 2023 14:05

comment

be3e636

update

201d9de

format

1e38853

update

0ed9510

format

ca72e9b

Fix more curve

87a0e2d

chunhtai force-pushed the issues/133032 branch from 8023852 to 87a0e2d Compare September 12, 2023 21:06

addressing comments

ef08968

chunhtai requested review from bdero and chinmaygarde September 12, 2023 21:52

bdero reviewed Sep 13, 2023

View reviewed changes

bdero approved these changes Sep 13, 2023

View reviewed changes

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 13, 2023

auto-submit bot merged commit 343dfe8 into flutter:main Sep 13, 2023

engine-flutter-autoroll mentioned this pull request Sep 13, 2023

Roll Flutter Engine from b71b366e3de3 to cf1c3a17ca1b (3 revisions) flutter/flutter#134680

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 13, 2023

343dfe85c [Impeller] Fixes stroke path geometry that can draw outside…

10f4a5a

… of path if the path ends at sharp turn. (flutter/engine#45252)

engine-flutter-autoroll mentioned this pull request Sep 13, 2023

Roll Flutter Engine from b71b366e3de3 to 154d6fd601a3 (6 revisions) flutter/flutter#134683

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 13, 2023

343dfe85c [Impeller] Fixes stroke path geometry that can draw outside…

fe6201b

… of path if the path ends at sharp turn. (flutter/engine#45252)

HansMuller mentioned this pull request Sep 15, 2023

Flutter Framework Team: Notable Commits flutter/flutter#121415

Closed

chunhtai added a commit to chunhtai/engine that referenced this pull request Sep 27, 2023

Revert "[Impeller] Fixes stroke path geometry that can draw outside o…

5930fde

…f path if the path ends at sharp turn. (flutter#45252)" This reverts commit 343dfe8.

chunhtai mentioned this pull request Sep 27, 2023

Revert "[Impeller] Fixes stroke path geometry that can draw outside o… #46334

Merged

8 tasks

[Impeller] Fixes stroke path geometry that can draw outside of path if the path ends at sharp turn. #45252

[Impeller] Fixes stroke path geometry that can draw outside of path if the path ends at sharp turn. #45252

Uh oh!

Conversation

chunhtai commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

chunhtai Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

bdero Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

chunhtai Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Aug 30, 2023

Uh oh!

chunhtai commented Aug 30, 2023

Uh oh!

flutter-dashboard bot commented Aug 30, 2023

Uh oh!

flutter-dashboard bot commented Sep 1, 2023

Uh oh!

flutter-dashboard bot commented Sep 1, 2023

Uh oh!

chunhtai commented Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

bdero left a comment

Choose a reason for hiding this comment

Uh oh!

bdero commented Sep 5, 2023

Uh oh!

chunhtai commented Sep 12, 2023

Uh oh!

flutter-dashboard bot commented Sep 12, 2023

Uh oh!

bdero left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdero commented Sep 13, 2023

Uh oh!

bdero left a comment

Choose a reason for hiding this comment

Uh oh!

bdero commented Sep 13, 2023

Uh oh!

Uh oh!

chunhtai commented Aug 29, 2023 •

edited

Loading

chunhtai Aug 30, 2023 •

edited

Loading

chunhtai commented Sep 1, 2023 •

edited

Loading

bdero left a comment •

edited

Loading