Repository navigation
Fix square corner issue - #191096
Fix square corner issue#191096
Conversation
github-actions
Bot
added
engine
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors clip depth tracking and restoration logic in Impeller's Canvas, including fixes to clip restoration in EntityPassClipStack and the addition of corresponding unit tests. Feedback highlights a critical bug in Canvas::FlipBackdrop where mutating clip_depth to kMaxDepth permanently overwrites parent scope depth budgets. Additionally, the reviewer recommends addressing a style guide violation regarding the naming and encapsulation of the thread-local testing override, replacing std::min<uint64_t> with std::min<uint32_t> to avoid unnecessary type promotion, and removing verbose debug logging statements before merging.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modifies Impeller's Canvas clip depth management to preserve active parent clips across sibling restorations and backdrop filter splits, reducing kMaxDepth and updating EntityPassClipStack. The reviewer feedback identifies a critical issue where tracking and restoring pre-split clip depth can cause debug crashes (FML_DCHECK failures) in split passes. It is recommended to remove the pre_split_clip_depth tracking entirely, simplify FlipBackdrop to use kMaxDepth, and update the unit tests accordingly.
...redacted defense of the prior approach... Update: When I took this PR out of draft, the goldens bot notified me that there were untriaged goldens. It turns out, it was breaking some behavior. Resetting |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
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. |
|
Returning to draft because those golden diffs don't look right to me |
|
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. 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. |
418ee31 to
7d48f5d
Compare
andywolff
added
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/191096, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
andywolff
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
| auto entry = CanvasStackEntry{}; | ||
| entry.transform = transform_stack_.back().transform; | ||
| entry.clip_depth = current_depth_ + total_content_depth; | ||
| entry.clip_depth = std::min<uint32_t>(current_depth_ + total_content_depth, |
There was a problem hiding this comment.
This part of the fix concerns me. The current depth + the total content depth should never be greater than the clip depth. If that happens then something else consumed some depth values that we never accounted for. If the DCHECK is triggering then we need to adjust the clip accounting, not simply choose a lower depth.
|
Successfully created revert PR: #193189 |
flutteractionsbot
removed
the
revert
Reverts: [Fix square corner issue](flutter#191096) Initiated by: @andywolff Reason for reverting: seems to have caused a test order dependency flutter#192992 Original PR Author: @andywolff Reviewed By: @gaaclarke The original PR description is provided below: Fixes flutter#190602, see my [comment](flutter#190602 (comment)) on that issue for details about the problems I identified. Also fixes flutter#162172 ## What changed When a route contained rounded clips (such as `CupertinoSheetRoute`), intermediate child ink features, and a backdrop filter (such as a blurred app bar), the clip stack replay logic prematurely popped parent clips and exceeded the depth buffer budget across render pass splits, causing subsequent header draws to render with 90° square corners. This PR addresses both mechanisms: First, `EntityPassClipStack::RecordRestore` was popping replay entities unconditionally whenever any clip was restored. In a sheet containing ink decorations (like list item ripple effects), child clip restores at deeper heights would prematurely discard the root parent clip. We now track `clip_height` in `ReplayResult` so that restorations only pop entities when `clip_height > restore_height`, preserving parent clips across sibling restores. When replaying those clips in `ClipContents::Render`, we also supply the entity's model matrix to properly recompute the MVP matrix against the new pass's orthographic projection instead of reusing the stale matrix from the prior pass. Second, the pass depth budget was exhausting across backdrop splits. `Canvas::kMaxDepth` (`1 << 24`) was mismatched with `Entity::kDepthEpsilon` (`1.0 / 262144.0`, or `2^-18`), allowing integer depths to exceed the 18-bit precision of the depth buffer. Aligning `Canvas::kMaxDepth` to `262,143` and capping depth allocations in `Save`/`SaveLayer` to `min(current_depth_ + total_content_depth, parent_clip_depth)` keeps clip replay and subsequent draw operations within valid precision bounds. In `Canvas::Restore`, we also prevent unconditional depth jumps to `kMaxDepth` for unclipped saves, preserving the allocated budget for subsequent sibling draws. ## Testing Reproducing this interaction in automated tests required testing the onscreen pass-splitting branch (`IsOnscreen()`), which normal offscreen unit tests bypass. We added a `ScopedOnscreenOverride` test hook in `Canvas` and constructed a multi-layer display list recreating the route clip, child ink items, blurred backdrop bar, and trailing app bar: * `clip_stack_unittests.cc`: Added `EntityPassClipStackTest.RestoringChildClipPreservesParentReplayEntities` to isolate parent clip retention across sibling restores. * `canvas_unittests.cc`: Added `AiksTest.ClipDepthMaintainedAcrossBackdropFilterAndLayers` to verify depth invariants and precision bounds (`<= kMaxDepth`), and `AiksTest.ParentClipDepthBudgetPreservedAfterBackdropLayerRestore` to verify parent depth budget restoration. * `dl_golden_unittests.cc` (`impeller_golden_tests`): Added `DlGoldenTest.CanRenderClippedBackdropFilterWithSuperellipse` for pixel-accurate golden image verification in CI against Skia Gold. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md Co-authored-by: Andy Wolff <[email protected]>
5 participants
Fixes #190602, see my comment on that issue for details about the problems I identified.
Also fixes #162172
What changed
When a route contained rounded clips (such as
CupertinoSheetRoute), intermediate child ink features, and a backdrop filter (such as a blurred app bar), the clip stack replay logic prematurely popped parent clips and exceeded the depth buffer budget across render pass splits, causing subsequent header draws to render with 90° square corners. This PR addresses both mechanisms:First,
EntityPassClipStack::RecordRestorewas popping replay entities unconditionally whenever any clip was restored. In a sheet containing ink decorations (like list item ripple effects), child clip restores at deeper heights would prematurely discard the root parent clip. We now trackclip_heightinReplayResultso that restorations only pop entities whenclip_height > restore_height, preserving parent clips across sibling restores. When replaying those clips inClipContents::Render, we also supply the entity's model matrix to properly recompute the MVP matrix against the new pass's orthographic projection instead of reusing the stale matrix from the prior pass.Second, the pass depth budget was exhausting across backdrop splits.
Canvas::kMaxDepth(1 << 24) was mismatched withEntity::kDepthEpsilon(1.0 / 262144.0, or2^-18), allowing integer depths to exceed the 18-bit precision of the depth buffer. AligningCanvas::kMaxDepthto262,143and capping depth allocations inSave/SaveLayertomin(current_depth_ + total_content_depth, parent_clip_depth)keeps clip replay and subsequent draw operations within valid precision bounds. InCanvas::Restore, we also prevent unconditional depth jumps tokMaxDepthfor unclipped saves, preserving the allocated budget for subsequent sibling draws.Testing
Reproducing this interaction in automated tests required testing the onscreen pass-splitting branch (
IsOnscreen()), which normal offscreen unit tests bypass. We added aScopedOnscreenOverridetest hook inCanvasand constructed a multi-layer display list recreating the route clip, child ink items, blurred backdrop bar, and trailing app bar:clip_stack_unittests.cc: AddedEntityPassClipStackTest.RestoringChildClipPreservesParentReplayEntitiesto isolate parent clip retention across sibling restores.canvas_unittests.cc: AddedAiksTest.ClipDepthMaintainedAcrossBackdropFilterAndLayersto verify depth invariants and precision bounds (<= kMaxDepth), andAiksTest.ParentClipDepthBudgetPreservedAfterBackdropLayerRestoreto verify parent depth budget restoration.dl_golden_unittests.cc(impeller_golden_tests): AddedDlGoldenTest.CanRenderClippedBackdropFilterWithSuperellipsefor pixel-accurate golden image verification in CI against Skia Gold.Pre-launch Checklist
///).