[framework] remove usage and deprecate physical model layer #102274
Conversation
flutter-dashboard
bot
added
the
framework
flutter-dashboard
bot
added
f: material design
|
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. |
|
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. |
jonahwilliams
changed the title
| final Rect offsetBounds = bounds.shift(offset); | ||
| final RRect offsetClipRRect = clipRRect.shift(offset); | ||
| if (needsCompositing) { | ||
| if (clipBehavior == Clip.none) { |
There was a problem hiding this comment.
right now the context asserts if I push clip.none, but I think I should remove that and just make it do the right thing here
There was a problem hiding this comment.
Isn't the idea there to avoid compositing a layer unnecessarily?
There was a problem hiding this comment.
pushClip only conditionally composites, take a look at the implementation. Its why we pass in needsCompositing
There was a problem hiding this comment.
oh wait, this is the implementation lol. NVM. I added this change because otherwise the ClipRRectLayer would assert
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
Change LGTM.
Can we also add deprecation attributes to the PhysicalModelLayer?
We should also deprecate pushPhysicalShapeLayer in dart:ui.
| final Rect offsetBounds = bounds.shift(offset); | ||
| final RRect offsetClipRRect = clipRRect.shift(offset); | ||
| if (needsCompositing) { | ||
| if (clipBehavior == Clip.none) { |
There was a problem hiding this comment.
Isn't the idea there to avoid compositing a layer unnecessarily?
| // The drawShadow call doesn't add the region of the shadow to the | ||
| // picture's bounds, so we draw a hardcoded amount of extra space to | ||
| // account for the maximum potential area of the shadow. | ||
| // TODO(jsimmons): remove this when Skia does it for us. |
This comment was marked as outdated.
This comment was marked as outdated.
| final Rect offsetBounds = bounds.shift(offset); | ||
| final Path offsetClipPath = clipPath.shift(offset); | ||
| if (needsCompositing) { | ||
| if (clipBehavior == Clip.none) { |
There was a problem hiding this comment.
nit: I believe this will allow us to clean up some other call-sides that currently re-implement this branch and don't call pushClipPath if the behavior is none (e.g. RenderClipOval)
There was a problem hiding this comment.
Potentially the same for pushClipRRect.
| final Rect offsetBounds = bounds.shift(offset); | ||
| final Path offsetClipPath = clipPath.shift(offset); |
There was a problem hiding this comment.
This work is wasted if behaivor is none. Should we skip it in that case?
|
This improved frame times on the new gallery benchmark by ~20%: https://flutter-flutter-perf.skia.org/e/?begin=1651002847&end=1651037948&keys=X6e4aa961197b108bc4214fdad0791a7d&num_commits=50&request_type=1&xbaroffset=28573 |
6 participants
Removes usage of physical model layer and reverts back to paints and clips. The clips need not be composited. In addition this updates several tests that used Opacity or RenderOpacity to force compositing and instead switches those to use RepaintBoundary and RenderRepaintBoundary to make a future change to RenderOpacity easier to land
We expect this change should result in fewer layers in a given flutter application, with more complex operations rolled into those pictures. Thus for pictures which are raster cached, that raster caching should cache more operations.
This is a complimentary change to #101952, which makes scenarios where we must composite or where we perform animations with layers more efficient.
Partially reverts #29701
Partially fixes #91852
Will handle #102179 in a follow up (in addition to making the repaint compositing optional)