add playground to test SDF primitive rendering under transforms#185010
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces unit tests and a playground for SDF primitives in Impeller, specifically for skewed rectangles, and adds an IsGoldenTest helper to differentiate test environments. Feedback suggests using using for type aliases, improving const-correctness for function parameters and global variables, simplifying matrix transformations, and removing a demonstration test case.
|
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. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds SDF primitive unit tests and a playground for Impeller, including a new test file and updates to playground headers to distinguish golden tests. Review feedback suggests explicitly setting the paint draw style to stroke when a stroke width is provided, replacing an unsafe reinterpret_cast on an enum class with safe casting for ImGui compatibility, and removing a demonstration test case.
…dd stroke width control
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds unit tests and a playground for SDF primitives in Impeller to verify stroke consistency under transforms. It introduces aiks_dl_sdf_primitive_unittests.cc and an IsGoldenTest helper to distinguish between playground and golden test environments. Feedback includes correcting an ImGui::ListBox signature to avoid a compilation error, initializing struct members, using float literals for consistency, and marking static methods as constexpr.
| ImGui::ListBox( | ||
| "Shape Type", &render_type_index, | ||
| [](void* data, int index) { | ||
| switch (static_cast<RenderType>(index)) { | ||
| case RenderType::kSquare: | ||
| return "Square"; | ||
| case RenderType::kRectangle: | ||
| return "Rectangle"; | ||
| case RenderType::kCircle: | ||
| return "Circle"; | ||
| case RenderType::kOval: | ||
| return "Oval"; | ||
| case RenderType::kLine: | ||
| return "Line"; | ||
| case RenderType::kCount: | ||
| FML_UNREACHABLE(); | ||
| } | ||
| }, | ||
| nullptr, static_cast<int>(RenderType::kCount), -1); |
There was a problem hiding this comment.
The current ImGui::ListBox usage with a lambda getter is incorrect because the standard ImGui signature for the getter overload is bool (*)(void* data, int idx, const char** out_text). The provided lambda returns a const char* and only takes two arguments, which will cause a compilation error. Using a static array of strings is a simpler and more idiomatic approach for a fixed list of items.
static const char* const items[] = {"Square", "Rectangle", "Circle", "Oval", "Line"};
ImGui::ListBox("Shape Type", &render_type_index, items, IM_ARRAYSIZE(items));There was a problem hiding this comment.
You used the wrong override. This is a newer version of that function. and has different argument requirements.
…-primitive-playground-and-goldens
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
@flar The example golden you added shows a hairline being too thin for the SDF renderer. But the hairline also looks wrong for the non-SDF renderers too. It is noticeably thicker than 1 pixel at the pointy ends of the skewed circle, and at the flatter sides it seems thinner than 1 pixel. Is that a current bug too? 
|
Correct, that's why I included it. At the time I added it the SDF renderers were not showing anything for any of the hairlines and that was because the hairlines weren't yet implemented in most of them. The test case was for future work, but I also noticed that the non-SDF renderer was producing a bad value for that one "example" test case which is why I removed the "included just as an example" text in the Description since it actually demonstrates a bug. It's harder to fix for the non-SDF renderers but we can create a custom tessellation for this case. |
SGTM. I don't think fixing the non-SDF version is a priority right now, but we should make sure there's an issue created for it so it's tracked somewhere. |
|
Filed #185222 We should add tests as necessary when we get done implementing hairlines across all shape types. |
flutter/flutter@8e8a194...2844af6 2026-04-19 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from bWoigpGIb60B6C7hD... to aDbXQm6WA0wFCAUp-... (flutter/flutter#185253) 2026-04-18 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from di3JdYrdE9OFu8Iyl... to bWoigpGIb60B6C7hD... (flutter/flutter#185231) 2026-04-18 rmolivares@renzo-olivares.dev Fix: text selection context menu should reappear after a non-fling scroll on Android and iOS (flutter/flutter#185054) 2026-04-18 flar@google.com Rect constructors for circle bounds (MakeCircle/EllipseBounds) (flutter/flutter#185110) 2026-04-17 97480502+b-luk@users.noreply.github.com Change uber SDF to get pixel size (for AA) using euclidean distance rather than manhattan distance. (flutter/flutter#184984) 2026-04-17 engine-flutter-autoroll@skia.org Roll Dart SDK from 7c2564c18770 to 9648f446f131 (7 revisions) (flutter/flutter#185223) 2026-04-17 47866232+chunhtai@users.noreply.github.com Rewrites no-response workflow to work with pr as well (flutter/flutter#185163) 2026-04-17 15619084+vashworth@users.noreply.github.com Only use LLDB breakpoint in debug mode (flutter/flutter#185158) 2026-04-17 flar@google.com add playground to test SDF primitive rendering under transforms (flutter/flutter#185010) 2026-04-17 30870216+gaaclarke@users.noreply.github.com Adds sdf rrects (and fixes opacity + color source) (flutter/flutter#184999) 2026-04-17 matt.kosarek@canonical.com Implementation of popup windows for Win32 (flutter/flutter#184516) 2026-04-17 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from R2EllDf4DgBXVNuiN... to dQ4PjIJB5kZFU8Y32... (flutter/flutter#185207) 2026-04-17 chris@bracken.jp [iOS] Update previousCompositionOrder to return Obj-C type (flutter/flutter#185136) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC boetger@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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
…r#11531) flutter/flutter@8e8a194...2844af6 2026-04-19 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from bWoigpGIb60B6C7hD... to aDbXQm6WA0wFCAUp-... (flutter/flutter#185253) 2026-04-18 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from di3JdYrdE9OFu8Iyl... to bWoigpGIb60B6C7hD... (flutter/flutter#185231) 2026-04-18 rmolivares@renzo-olivares.dev Fix: text selection context menu should reappear after a non-fling scroll on Android and iOS (flutter/flutter#185054) 2026-04-18 flar@google.com Rect constructors for circle bounds (MakeCircle/EllipseBounds) (flutter/flutter#185110) 2026-04-17 97480502+b-luk@users.noreply.github.com Change uber SDF to get pixel size (for AA) using euclidean distance rather than manhattan distance. (flutter/flutter#184984) 2026-04-17 engine-flutter-autoroll@skia.org Roll Dart SDK from 7c2564c18770 to 9648f446f131 (7 revisions) (flutter/flutter#185223) 2026-04-17 47866232+chunhtai@users.noreply.github.com Rewrites no-response workflow to work with pr as well (flutter/flutter#185163) 2026-04-17 15619084+vashworth@users.noreply.github.com Only use LLDB breakpoint in debug mode (flutter/flutter#185158) 2026-04-17 flar@google.com add playground to test SDF primitive rendering under transforms (flutter/flutter#185010) 2026-04-17 30870216+gaaclarke@users.noreply.github.com Adds sdf rrects (and fixes opacity + color source) (flutter/flutter#184999) 2026-04-17 matt.kosarek@canonical.com Implementation of popup windows for Win32 (flutter/flutter#184516) 2026-04-17 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from R2EllDf4DgBXVNuiN... to dQ4PjIJB5kZFU8Y32... (flutter/flutter#185207) 2026-04-17 chris@bracken.jp [iOS] Update previousCompositionOrder to return Obj-C type (flutter/flutter#185136) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC boetger@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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
Adds a new (extensible) playground test that draws an SDF primitive under a configurable transform that includes rotation, (non-uniform) scaling, and skewing. If there is an anomaly detected, the values can be copied and a new golden-capable test case can be easily constructed to test just those parameters. One such example which was observed by adjusting values in the playground is included as a demonstration of a real bug in the current code.
This PR also adds a new method that will indicate if the test is being run as a golden test so that a "playground-only" test can opt out of the golden system. The main playground version of the test which contains the UI for managing the transform performs this check and opt-out.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on [Discord].
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.