[Impeller] EntityPass::Clone needs to clone harder #45313
Conversation
|
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. |
|
These goldens look really weird... |
|
I'm not sure if this is entirely right or if I'm missing something, goldens look very different locally... |
|
Goldens look like the pictures keep building up |
| pass->delegate_ = delegate_; | ||
| // Note: I tried also adding flood clip and bounds limit but one of the | ||
| // two caused rendering in wonderous to break. It's 10:51 PM, and I'm | ||
| // ready to move on. |
There was a problem hiding this comment.
Follow-up bug for investigating? The bounds limit might need to be transformed when the pass is applied with DrawPicture.
| pass->backdrop_filter_reads_from_pass_texture_ = | ||
| backdrop_filter_reads_from_pass_texture_; | ||
| pass->advanced_blend_reads_from_pass_texture_ = | ||
| advanced_blend_reads_from_pass_texture_; |
There was a problem hiding this comment.
(These read-counts are what's important for DrawPicture.)
There was a problem hiding this comment.
You need the readcounts, the delegate, and the backdrop proc or wonderous will crash.
There was a problem hiding this comment.
That shouldn't be the case if the cloned pass is being used for DrawPicture (the elements are copied inline and these values should get discarded).
There was a problem hiding this comment.
For the delegate and backdrop proc, that is. The read counts are important.
There was a problem hiding this comment.
I can confirm that wonderous does not work correctly without copying the backdrop prop and entity delegate. Maybe the entity delegate doesn't need to be copied but it does need to be set to a non-default value.
Let me double check that.
There was a problem hiding this comment.
Ah nevermind, we don't need these for the base pass/inline merge, but we definitely need to copy them for the recursive clone, otherwise Very Bad Things will happen:
engine/impeller/entity/entity_pass.cc
Line 1015 in 494fd7f
|
This still isn't right though, here is what the backdrop blur output is giving me on Vulkan:
|
jonahwilliams
added
the
autosubmit
…133879) flutter/engine@d00b69a...489c399 2023-09-01 matanlurey@users.noreply.github.com Update (flipping the default from false -> true) and deprecate Paint.enableDithering. (flutter/engine#44705) 2023-09-01 jonahwilliams@google.com [Impeller] EntityPass::Clone needs to clone harder (flutter/engine#45313) 2023-09-01 ychris@google.com Reland "ios: remove shared_application and support app extension build #44732" (flutter/engine#45351) 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 aaclarke@google.com,rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Fixes flutter/flutter#133731