[Impeller] Refactor clip stack into separate testable class. #51656
Conversation
…ine into fix_unbalanced_restore
jonahwilliams
requested review from
bdero and
matanlurey
and removed request for
bdero and
matanlurey
|
Wait this is still WIP! |
matanlurey
left a comment
matanlurey
left a comment
There was a problem hiding this comment.
Overall looks and reads much better, this is a great example of ✅ refactoring.
| if (!clip_stack_->AppendClipCoverage(clip_coverage, element_entity, | ||
| clip_depth_floor, | ||
| global_pass_position)) { | ||
| return true; |
There was a problem hiding this comment.
Maybe store the 1-line comment explaining why this state is terminal
| return !clip_coverage_.back().empty(); | ||
| } | ||
|
|
||
| void EntityPassClipStack::Initialize(const Rect& rect) { |
There was a problem hiding this comment.
nit: It would be nice if this was done in the constructor instead, but I am also not sure how feasible that is
|
I need to pass through the clip stack so it is shared across all entities, but its close! |
impeller/entity/entity_pass.cc
Outdated
| ClipCoverageStack clip_coverage_stack = {ClipCoverageLayer{ | ||
| .coverage = Rect::MakeSize(root_render_target.GetRenderTargetSize()), | ||
| .clip_depth = 0}}; | ||
| std::unique_ptr<EntityPassClipStack> clip_stack = |
There was a problem hiding this comment.
Actually this doesn't even need to be heap allocated
jonahwilliams
force-pushed
the
refactor_clip_stack
branch
from
ca80c6a to
769c95c
Compare
| .coverage = Rect::MakeSize(root_render_target.GetRenderTargetSize()), | ||
| .clip_depth = 0}}; | ||
| EntityPassClipStack clip_stack = EntityPassClipStack( | ||
| Rect::MakeSize(root_render_target.GetRenderTargetSize())); |
There was a problem hiding this comment.
This patch moves from per-pass tracking of replay clips to a single list of tracked replay clips reused across all passes. So when replaying clips we need to be careful that only clips which impact the current pass are replayed, otherwise there may be perf issues if StC is turned on and fidelity bugs if StC is turned off.
There was a problem hiding this comment.
Oh, taking a closer look, maybe you're already handling this correctly. I see the state is held in a per-pass vector.
There was a problem hiding this comment.
I made this mistake initially but I believe thatI've fully corrected it. The clip stacks and record/replay entities are stored per-subpass, and for each subpass we push/pop a new set of data.
bdero
left a comment
bdero
left a comment
There was a problem hiding this comment.
LGTM! I really like this change.
jonahwilliams
added
the
autosubmit
…145773) flutter/engine@b2d93a6...cffd1dc 2024-03-26 [email protected] Pass the flutter test font by default. (flutter/engine#51671) 2024-03-26 [email protected] [Impeller] revert usage of foreground blend optimization. (flutter/engine#51679) 2024-03-26 [email protected] [Impeller] Refactor clip stack into separate testable class. (flutter/engine#51656) 2024-03-26 [email protected] Roll buildroot to ba3ca696f4f95e998707523be755c15440c6bf3f (flutter/engine#51678) 2024-03-26 [email protected] Use RBE for Fuchsia CI builds (flutter/engine#51675) 2024-03-26 [email protected] Roll Skia from 7ffd936a66df to 90cfbf5fb91e (15 revisions) (flutter/engine#51677) 2024-03-26 [email protected] [macOS] Disable FlutterEngineTest.CanOverrideBackgroundColor (flutter/engine#51669) 2024-03-26 [email protected] Roll buildroot to 2a16784938d3be059014d4112f00ac70a386fa0c (flutter/engine#51674) 2024-03-26 [email protected] Roll Dart SDK from a783a4a043ec to 09e56db7d600 (2 revisions) (flutter/engine#51666) 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
3 participants
The clip coverage tracking has had some bugs, but its been difficult to test as it was mixed into the regular entity pass workflow. This change pulls this logic and the clip recorder logic into a new class that is responsible for managing the coverage stacks.
Adds an unbalanced restore unit test as well.