[Impeller] Fix interpolation error in Rect::TransformAndClipBounds#181420
Merged
auto-submit[bot] merged 2 commits intoflutter:masterfrom Jan 24, 2026
Merged
Conversation
github-actions
bot
added
engine
Contributor
Author
|
I will also work on breaking the TransformAndClipBounds unit test into multiple tests. |
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses a critical interpolation error in the Rect::TransformAndClipBounds method, which previously caused incorrect handling of points under perspective transforms. The fix correctly applies the interpolation factor t to linearly interpolate between a clipped point and its unclipped neighbor. Additionally, the unit tests for TransformAndClipBounds have been significantly enhanced with detailed comments explaining the complex calculations for expected bounds, greatly improving the test's clarity and maintainability.
Comment on lines
+3181
to
+3221
| // Exactly two of these homogenous results should have a W<0 | ||
| // | ||
| // When W<0 we interpolate the point back towards the adjacent points | ||
| // that have W>0 to a location just greater than the W=0 half-plane. | ||
| // We interpolate them to W=epsilon where epsilon == 2^-14. | ||
|
|
||
| EXPECT_VECTOR3_NEAR(matrix.TransformHomogenous(src.GetLeftTop()), | ||
| Vector3(200.0f, 200.0f, 0.9f)); | ||
| // Contributes (200, 200) / 0.9 == (222.2222, 222.2222) to bounds | ||
|
|
||
| EXPECT_VECTOR3_NEAR(matrix.TransformHomogenous(src.GetRightTop()), | ||
| Vector3(400.0f, 200.0f, -0.6f)); | ||
| // Interpolates at epsilon against LeftTop to produce: | ||
| // t = (epsilon - -.6) / (.9 - -.6) | ||
| // = (epsilon + .6) / 1.5 | ||
| // = 0.4000407 | ||
| // Lerp(RightTop, LeftTop, 0.4000407) = (319.9919, 200, epsilon) | ||
| // = (5242747, 3276800) | ||
| // Cannot interpolate against RightBottom because it also has W<0 | ||
|
|
||
| EXPECT_VECTOR3_NEAR(matrix.TransformHomogenous(src.GetLeftBottom()), | ||
| Vector3(200.0f, 400.0f, 0.3f)); | ||
| // Contributes (200, 400) / 0.3 == (666.6667, 1333.3333) to bounds | ||
|
|
||
| EXPECT_VECTOR3_NEAR(matrix.TransformHomogenous(src.GetRightBottom()), | ||
| Vector3(400.0f, 400.0f, -1.2f)); | ||
| // Interpolates at epsilon against LeftBottom to produce: | ||
| // t = (epsilon - -1.2) / (.3 - -1.2) | ||
| // = (epsilon + 1.2) / 1.5 | ||
| // = 0.8000407 | ||
| // Lerp(RightBottom, LeftBottom, 0.8000407) = (239.9919, 400, epsilon) | ||
| // = (3932026.667, 6553600) | ||
| // Cannot interpolate against RightTop because it also has W<0 | ||
|
|
||
| // Min/Max X and Y of all the points generated above are: | ||
| // Min X == 222.2222 | ||
| // Min Y == 222.2222 | ||
| // Max X == 5242747 | ||
| // Max Y == 6553600 | ||
|
|
||
| Rect expect = Rect::MakeLTRB(222.2222f, 222.2222f, 5242747.f, 6553600.f); |
Contributor
There was a problem hiding this comment.
The added comments provide excellent clarity on the complex logic of perspective transformation, clipping, and interpolation. This significantly improves the maintainability and understanding of this test case, especially for future debugging or modifications. This aligns with the repository's style guide principle of making documentation useful and explaining the 'why' and 'how' (Repository Style Guide line 55).
Comment on lines
+3238
to
+3280
|
|
||
| // Exactly three of these homogenous results should have a W<0 | ||
| // | ||
| // When W<0 we interpolate the point back towards the adjacent points | ||
| // that have W>0 to a location just greater than the W=0 half-plane. | ||
| // We interpolate them to W=epsilon where epsilon == 2^-14. | ||
|
|
||
| EXPECT_VECTOR3_NEAR(matrix.TransformHomogenous(src.GetLeftTop()), | ||
| Vector3(200.0f, 200.0f, 0.4f)); | ||
| // Contributes (200, 200) / 0.4 == (500, 500) to bounds | ||
|
|
||
| EXPECT_VECTOR3_NEAR(matrix.TransformHomogenous(src.GetRightTop()), | ||
| Vector3(400.0f, 200.0f, -1.6f)); | ||
| // Interpolates at epsilon against LeftTop to produce: | ||
| // t = (epsilon - -1.6) / (.4 - -1.6) | ||
| // = (epsilon + 1.6) / 2 | ||
| // = 0.8000305 | ||
| // Lerp(RightTop, LeftTop, 0.8000305) = (239.9939, 200, epsilon) | ||
| // = (3932060, 3276800) | ||
| // Cannot interpolate against RightBottom because it also has W<0 | ||
|
|
||
| EXPECT_VECTOR3_NEAR(matrix.TransformHomogenous(src.GetLeftBottom()), | ||
| Vector3(200.0f, 400.0f, -0.2f)); | ||
| // Interpolates against LeftTop to produce: | ||
| // t = (epsilon - -.2) / (.4 - -.2) | ||
| // = (epsilon + .2) / .6 | ||
| // = 0.333435 | ||
| // Lerp(LeftBottom, LeftTop, .333435) = (200, 333.31299, epsilon) | ||
| // = (3276800, 5461000) | ||
| // Cannot interpolate against RightBottom because it also has W<0 | ||
|
|
||
| EXPECT_VECTOR3_NEAR(matrix.TransformHomogenous(src.GetRightBottom()), | ||
| Vector3(400.0f, 400.0f, -2.2f)); | ||
| // Cannot interpolate against either RightTop or LeftBottom because | ||
| // both of those adjacent points transformed to a W<0 homogenous point. | ||
|
|
||
| // Min/Max X and Y of all the points generated above are: | ||
| // Min X == 500 | ||
| // Min Y == 500 | ||
| // Max X == 3932060 | ||
| // Max Y == 5461000 | ||
|
|
||
| Rect expect = Rect::MakeLTRB(500.0f, 500.0f, 3932060.f, 5461000.f); |
Contributor
There was a problem hiding this comment.
Similar to the previous block, these detailed comments are highly valuable for understanding the edge cases of perspective transforms where multiple corners are clipped. They clearly outline the interpolation steps and the resulting bounds, which is crucial for verifying the correctness of the TransformAndClipBounds method. This is a great example of useful documentation within tests.
b-luk
approved these changes
Jan 23, 2026
flutter-dashboard
bot
removed
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 24, 2026
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 24, 2026
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 25, 2026
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jan 25, 2026
auto-submit bot
pushed a commit
to flutter/packages
that referenced
this pull request
Jan 25, 2026
flutter/flutter@bfc9041...def9ca9 2026-01-25 [email protected] Roll Skia from f1433eb44a50 to 2830fbe8bafe (1 revision) (flutter/flutter#181464) 2026-01-25 [email protected] Roll Fuchsia Linux SDK from 6xoKGIry6Y8T8x5Sa... to T4qTEa3T5CCSCIoJY... (flutter/flutter#181458) 2026-01-24 [email protected] Roll Skia from b6d396a151bc to f1433eb44a50 (1 revision) (flutter/flutter#181449) 2026-01-24 [email protected] Roll Dart SDK from 29918a54dd5c to 60553fc4c04f (1 revision) (flutter/flutter#181437) 2026-01-24 [email protected] Roll Fuchsia Linux SDK from n7NohL9DPpEuPjNt9... to 6xoKGIry6Y8T8x5Sa... (flutter/flutter#181438) 2026-01-24 [email protected] [Impeller] Fix perspective clips with a large perspective bias (flutter/flutter#181434) 2026-01-24 [email protected] Roll Dart SDK from e82d7ad1855e to 29918a54dd5c (4 revisions) (flutter/flutter#181435) 2026-01-24 [email protected] Roll Skia from 32b52343e757 to b6d396a151bc (4 revisions) (flutter/flutter#181431) 2026-01-24 [email protected] [Impeller] Fix interpolation error in Rect::TransformAndClipBounds (flutter/flutter#181420) 2026-01-23 [email protected] Roll Skia from 6d438894c2a8 to 32b52343e757 (2 revisions) (flutter/flutter#181419) 2026-01-23 [email protected] [Material] modernize Typography._withPlatform with Dart 3 switch expression (flutter/flutter#181398) 2026-01-23 [email protected] CupertinoSheetRoute with scrolling and dragging (flutter/flutter#177337) 2026-01-23 [email protected] Adds contents of keys file when a skia gold error occurs. (flutter/flutter#181401) 2026-01-23 [email protected] Roll Skia from e4bd0a355e68 to 6d438894c2a8 (3 revisions) (flutter/flutter#181405) 2026-01-23 [email protected] bump KGP and AGP max known versions (flutter/flutter#181325) 2026-01-23 [email protected] Roll Skia from db10db8bd55f to e4bd0a355e68 (3 revisions) (flutter/flutter#181391) 2026-01-23 [email protected] Roll Packages from 9010299 to 5af5f50 (4 revisions) (flutter/flutter#181388) 2026-01-23 [email protected] Look for project root for FeatureFlags manifest (flutter/flutter#180689) 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: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
flutter-zl
pushed a commit
to flutter-zl/flutter
that referenced
this pull request
Feb 10, 2026
…lutter#181420) We had been interpolating the points backwards in the `Rect::TransformAndClipBounds` code. When we transform a rectangle under a perspective transform, some of the points may generate a homogenous bias (W) that is less than 0. This is a special condition that means that the point was transformed "behind the camera" (this is one way of looking at it and helps explain why we avoid such values). In order for it to contribute properly to the bounds in front of the "camera", it needs to be moved back into the positive (W>0) space. We do this by interpolating from that out of bounds coordinate to either of its adjacent neighbors which are in bounds (i.e. their W>0) to move the point to a bias value that is just slightly in the "in bounds" half-space - we choose W=(1/2^14) as our "in bounds" bias value. Unfortunately, the code to interpolate the 2 coordinates was written with conflicting definitions of which direction we computed the `t` value and which direction we applied it to the 2 coordinates - thus generating nonsensical values. This turned up while debugging a fix for flutter#173104 where we fixed the transform logic in the clip code, but the rectangle was still partially clipped due to a bad result from the `GetCoverage` method of the rect geometry code. Note that this fix alone does not correct the problem in that Issue, it merely fixes a needed utility method that the fix will rely on.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We had been interpolating the points backwards in the
Rect::TransformAndClipBoundscode.When we transform a rectangle under a perspective transform, some of the points may generate a homogenous bias (W) that is less than 0. This is a special condition that means that the point was transformed "behind the camera" (this is one way of looking at it and helps explain why we avoid such values). In order for it to contribute properly to the bounds in front of the "camera", it needs to be moved back into the positive (W>0) space. We do this by interpolating from that out of bounds coordinate to either of its adjacent neighbors which are in bounds (i.e. their W>0) to move the point to a bias value that is just slightly in the "in bounds" half-space - we choose W=(1/2^14) as our "in bounds" bias value.
Unfortunately, the code to interpolate the 2 coordinates was written with conflicting definitions of which direction we computed the
tvalue and which direction we applied it to the 2 coordinates - thus generating nonsensical values.This turned up while debugging a fix for #173104 where we fixed the transform logic in the clip code, but the rectangle was still partially clipped due to a bad result from the
GetCoveragemethod of the rect geometry code. Note that this fix alone does not correct the problem in that Issue, it merely fixes a needed utility method that the fix will rely on.