[Impeller] Simplify convex tessellation#47957
Conversation
|
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. |
|
Golden file changes are available for triage from new commit, Click here to view. |
bdero
left a comment
bdero
left a comment
There was a problem hiding this comment.
LGTM! Suggestion about testing.
impeller/tessellator/tessellator.cc
Outdated
| for (auto j = 0u; j < polyline.contours.size(); j++) { | ||
| auto [start, end] = polyline.GetContourPointBounds(j); | ||
| auto center = polyline.GetPoint(start); | ||
| auto origin = polyline.GetPoint(start); |
impeller/aiks/aiks_unittests.cc
Outdated
| TEST_P(AiksTest, CanDrawMultiContourConvexPath) { | ||
| PathBuilder builder = {}; | ||
| for (auto i = 0; i < 10; i++) { | ||
| builder.AddCircle(Point(100 + 10 * i, 100 + 10 * i), 100); |
There was a problem hiding this comment.
nit: Interlace triangles here too (so the appended contours will be Circle, Triangle, Circle, Triangle, etc.) that way we know we're testing the a == b branch for TessellateConvex, which will only trigger for contours with an odd number of points.
There was a problem hiding this comment.
Ahh good idea.
| output.emplace_back(polyline.GetPoint(b)); | ||
| } | ||
| if (a == b) { | ||
| output.emplace_back(polyline.GetPoint(a)); |
There was a problem hiding this comment.
I think this checks out... for 3 points it should append indices 0, 1, 2, and for 4 points it should append 0, 1, 3, 2, etc.
There was a problem hiding this comment.
Ahh but there is a bug here, on L268 b should start at end and not end -, probably ...
There was a problem hiding this comment.
or maybe not, let me double check my assumptions here
There was a problem hiding this comment.
YEah, there was an issue in how I was doing the strip breaks, I needed to remove an extra point, now the triangles show up. Good idea, once again
…ms/engine into simplify_convex_tessellation
|
|
|
Golden file changes are available for triage from new commit, Click here to view. |
|
@bdero PTAL |
jonahwilliams
added
the
autosubmit
flutter/engine@d7ca057...bc5bbd3 2023-11-15 [email protected] [web] JSConfig: Add multiViewEnabled value. (flutter/engine#47939) 2023-11-15 [email protected] [Impeller] Simplify convex tessellation (flutter/engine#47957) 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
2 participants
Removes usage of index buffer and adds zig-zagging triangle strip. Could also be adapted emplace directly into host
buffer.
Benchmark is https://flutter-engine-perf.skia.org/e/?queries=test%3DBM_Convex_rrect_convex
Umbrella issue: flutter/flutter#138004