[Impeller] Fix new convex path shadow generation in perspective#183187
[Impeller] Fix new convex path shadow generation in perspective#183187auto-submit[bot] merged 5 commits intoflutter:masterfrom
Conversation
github-actions
bot
added
engine
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with convex path shadow generation under perspective transforms. The fix involves modifying PolygonInfo::CalculateConvexShadowMesh to perform shadow tessellation in a scale-only coordinate space by extracting scale components from the incoming transformation matrix. This approach avoids rendering artifacts that occurred when tessellating directly in a space with a perspective transform. A new visual test, CanDrawPerspectiveConvexShadow, has been added to verify the fix for both simple and general convex paths under perspective. The change is logical and the new test provides good coverage. I have one suggestion to simplify a conditional check by removing a redundant condition.
engine/src/flutter/impeller/entity/geometry/shadow_path_geometry.cc
Outdated
Show resolved
Hide resolved
|
It occurs to me that using a 4x4 matrix to transform a 2D point to a 2D answer and then trying to transform it by the inverse would not be reversible because there was information lost in the first operation that cannot be restored in the reverse. The new matrix being used should generally be sufficient because the original geometry is in the 2D (x,y,0) plane and so its shadow (while being offset in Z in theory, but not really implemented that way in practice) is just another 2D operation that will be geometrically computed in just the XY plane and then fully transformed by the full 4x4 matrix. Just extracting the 2D scales for transforming and un-transforming the geometry is thus the right way to apply this "device space" algorithm. |
|
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 modifies the convex path shadow generation to handle perspective transforms. The change involves extracting the 2D scale from the transformation matrix and performing shadow tessellation in a "device-scaled" space, rather than the fully transformed space. The resulting vertices are then transformed back to local space using an inverse scale transformation. This avoids issues with non-affine transformations during tessellation. New unit tests have been added to verify shadow rendering with perspective, rotation, and non-uniform scaling.
engine/src/flutter/impeller/display_list/aiks_dl_shadow_unittests.cc
Outdated
Show resolved
Hide resolved
engine/src/flutter/impeller/display_list/aiks_dl_shadow_unittests.cc
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new test cases for convex shadow generation under various transformations (perspective, rotation, and non-uniform scale) in aiks_dl_shadow_unittests.cc. The core fix addresses artifacts in perspective shadow generation by modifying shadow_path_geometry.cc to operate in a "2D device space scaled" coordinate system, extracting 2D scales from the transformation matrix instead of relying on full matrix inversions. New helper methods (GetBasisX2D, GetBasisY2D, GetBasisScaleXY) have been added to the Matrix class in matrix.h to facilitate this, along with corresponding unit tests in matrix_unittests.cc. Additionally, the GetLengthSquared and GetLength methods in point.h have been updated to return Scalar types for improved precision and type correctness.
| // and just use those measurements to guide our tessellation. | ||
| Vector2 scale_2d = matrix.GetBasisScaleXY(); | ||
|
|
||
| FML_DCHECK(scale_2d.x >= 0.0f && scale_2d.y >= 0.0f); |
There was a problem hiding this comment.
The FML_DCHECK on line 508 asserts that scale_2d.x and scale_2d.y are non-negative. The subsequent if condition !(scale_2d.x * scale_2d.y > 0.0f) correctly handles cases where either scale is zero or NaN. However, if scale_2d.x or scale_2d.y could theoretically be negative (e.g., due to a reflection), the FML_DCHECK would fail, but the if condition would still catch the zero case. If negative scales are possible and should be handled differently than zero scales, this logic might need refinement. If negative scales are truly impossible here, the FML_DCHECK is sufficient for debugging purposes.
There was a problem hiding this comment.
The values can't be negative because of the way that GetBasisScale2D performs its work. The (debug) check is there to make it more obvious to maintainers that we didn't forget this when we wrote the production condition. And just a little bit to make sure we understood that part correctly.
| constexpr Scalar GetLengthSquared() const { | ||
| return static_cast<Scalar>(static_cast<double>(x) * x + | ||
| static_cast<double>(y) * y); |
There was a problem hiding this comment.
Changing the return type of GetLengthSquared() to Scalar is a good improvement for type correctness, as the result of a squared length calculation (especially with floating-point inputs) is inherently a floating-point value. The explicit cast to double for intermediate calculation ensures precision before converting to Scalar.
|
|
||
| constexpr Type GetLength() const { return std::sqrt(GetLengthSquared()); } | ||
| constexpr Scalar GetLength() const { return sqrt(GetLengthSquared()); } |
There was a problem hiding this comment.
Similarly, changing the return type of GetLength() to Scalar is appropriate. The square root of a sum of squares will almost always be a floating-point number, so Scalar is the correct type.
…-convex-shadow-perspective
|
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. |
|
The golden files are all "off by 1" diffs in the blur output because of the slight change in scaling math. I'm not sure why the Google Testing step didn't generate a bunch of diffs, though. |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
LGTM, just some suggestions on making the test assertions more clear.
|
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. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
flutter-dashboard
bot
removed
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
Roll Flutter from d3dd7744e81f to d18214307703 (33 revisions) flutter/flutter@d3dd774...d182143 2026-03-06 [email protected] Roll Packages from 8d5c5cd to fe3de64 (2 revisions) (flutter/flutter#183308) 2026-03-06 [email protected] Roll Dart SDK from 1b51451cdb99 to 7c7c1e3d024d (2 revisions) (flutter/flutter#183294) 2026-03-06 [email protected] Roll Dart SDK from 9ac06cdd1801 to 1b51451cdb99 (9 revisions) (flutter/flutter#183289) 2026-03-06 [email protected] Add GitHub workflows to assist with release tasks (flutter/flutter#181978) 2026-03-06 [email protected] [Impeller] Fix new convex path shadow generation in perspective (flutter/flutter#183187) 2026-03-06 [email protected] Roll pub packages (flutter/flutter#183178) 2026-03-05 [email protected] fix: use double quotes in settings.gradle.kts template (flutter/flutter#183081) 2026-03-05 [email protected] Add fallbackColor for PredictiveBackPageTransitionBuilder and PredictiveBackFullscreenPageTransitionBuilder (flutter/flutter#182690) 2026-03-05 [email protected] Simplify TesterContextGLES (multithreading logic not needed), and enable some tests that now pass (flutter/flutter#183250) 2026-03-05 [email protected] Roll Skia from a94df1cdabb0 to a69ef43650ee (14 revisions) (flutter/flutter#183280) 2026-03-05 [email protected] Windowing implementation of `showDialog` that uses a native desktop window to display the content (flutter/flutter#181861) 2026-03-05 [email protected] Build CocoaPod plugin frameworks for Add to App FlutterPluginRegistrant (flutter/flutter#183239) 2026-03-05 [email protected] Extend the Linux web_skwasm_tests_1 timeout to 45 minutes (flutter/flutter#183247) 2026-03-05 [email protected] Update Dart to 3.12 beta 2 (flutter/flutter#183251) 2026-03-05 [email protected] Replace the rest of the references to `flutter/engine` with `flutter/flutter` (flutter/flutter#182938) 2026-03-05 [email protected] chore: convert android_verified_input to pub-workspace (flutter/flutter#183175) 2026-03-05 [email protected] Add await to flutter_test callsites (flutter/flutter#182983) 2026-03-05 [email protected] [iOS] Skip gesture recognizer reset workaround on iOS 26+ (flutter/flutter#183186) 2026-03-05 [email protected] Add warning for plugins not migrated to UIScene (flutter/flutter#182826) 2026-03-05 [email protected] Roll Fuchsia Linux SDK from JJw5EJ87vLGqFVl4h... to 8ay15_eQOEgPHCypm... (flutter/flutter#183255) 2026-03-05 [email protected] Roll Skia from ada0b7628c79 to a94df1cdabb0 (2 revisions) (flutter/flutter#183249) 2026-03-05 [email protected] Roll Packages from 82baf93 to 8d5c5cd (2 revisions) (flutter/flutter#183269) 2026-03-05 [email protected] Add `UnlabaledLeafNodeEvaluation` (flutter/flutter#182872) 2026-03-04 [email protected] Re-specify the ndk version in various test apps, to prevent ndk download (flutter/flutter#183134) 2026-03-04 [email protected] Eliminate rebuilds for Scaffold FAB animation (flutter/flutter#182331) 2026-03-04 [email protected] Add Michal Kucharski to AUTHORS (flutter/flutter#182366) 2026-03-04 [email protected] Merge changelog from 3.41.4 stable. (flutter/flutter#183243) 2026-03-04 [email protected] Allow stylus support on windows (flutter/flutter#165323) 2026-03-04 [email protected] Fix docs on SingletonFlutterWindow.supportsShowingSystemContextMenu (flutter/flutter#183142) 2026-03-04 [email protected] Roll Packages from 9083bc9 to 82baf93 (5 revisions) (flutter/flutter#183240) 2026-03-04 [email protected] Fixes FocusHighlightMode on Android when typing in software keyboard (flutter/flutter#180753) 2026-03-04 [email protected] Make compileShader() retry without sksl if it fails with sksl. (flutter/flutter#183146) 2026-03-04 [email protected] [web] Use pointer-events: auto for non-interactive leaf semantics nodes (flutter/flutter#183077) 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 [email protected],[email protected] 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: ...
…ter#183187) Fixes: flutter#182659 Shadows were generated in device space and then inverse transformed back into local space to interact with the rest of the pipelines well. Transforming by a perspective transforming, tessellating the shadow, and then transforming back by the inverse of the perspective transform seems to have artifacts. To simplify the process, we only need to work in "device space scaled" coordinate system so instead of using the entire transform we just extract the scales and work in that space instead - an operation which appears to inverse cleanly.
2 participants
Fixes: #182659
Shadows were generated in device space and then inverse transformed back into local space to interact with the rest of the pipelines well. Transforming by a perspective transforming, tessellating the shadow, and then transforming back by the inverse of the perspective transform seems to have artifacts.
To simplify the process, we only need to work in "device space scaled" coordinate system so instead of using the entire transform we just extract the scales and work in that space instead - an operation which appears to inverse cleanly.