[Impeller] Fix up coverage hints for blending/vertices, fix TiledTextureContents::RenderToSnapshot #43017
Conversation
dnfield
changed the title
impeller/entity/contents/contents.cc
Outdated
| Entity sub_entity; | ||
| auto color_size = contents.GetColorSourceSize(); | ||
| if (color_size.has_value()) { | ||
| auto sx = coverage->size.width / color_size->width; |
There was a problem hiding this comment.
Nit: I believe sizes have operator/ already.
There was a problem hiding this comment.
So the origin correction is the stuff missing here right?
There was a problem hiding this comment.
Using the operator/ now.
Adding the origin correction was making things wrong on Metal. Do we still need it for Vulkan?
There was a problem hiding this comment.
Vulkan and Metal coordinate spaces should be configured to be the same (after the vk_khr_maintenance1).
dnfield
added
the
autosubmit
bdero
left a comment
bdero
left a comment
There was a problem hiding this comment.
LGTM. Would you happen to have a screengrab of the remaining issue? (No worries if not)
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/engine, pr: 43017, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label. |
I'll add screenshots to the issue I filed. Also looking into the CI failures |
dnfield
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/engine, pr: 43017, due to - The status or check suite Linux linux_clang_tidy has failed. Please fix the issues identified (or deflake) before re-applying this label. |
dnfield
added
the
autosubmit
|
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. |
|
I don't think those goldne changes are quite right :\ |
impeller/entity/contents/contents.cc
Outdated
| [&contents = *this, &entity, &coverage](const ContentContext& renderer, | ||
| RenderPass& pass) -> bool { | ||
| Entity sub_entity; | ||
| auto color_size = contents.GetColorSourceSize(); |
There was a problem hiding this comment.
I don't think this is quite right. Why would we need to perform a scale-up on the entity before rendering it here?
There was a problem hiding this comment.
Yes this is the problem.
There was a problem hiding this comment.
The idea was this would provide corrective scaling if the color source was down-scaled for coverage hinting we'd scale it to the right coordinate space again.
There was a problem hiding this comment.
This is gone now. Instead I'm overriding TiledTextureContents::RenderToSnapshot to pass through the original texture in cases where theeffect matrix is identity.
… Contents::RenderToSnapshot
dnfield
changed the title
| .transform = entity.GetTransformation(), | ||
| .sampler_descriptor = sampler_descriptor.value_or(sampler_descriptor_), | ||
| .opacity = GetOpacity(), | ||
| }; |
There was a problem hiding this comment.
This optimization will be a nice win for a lot of apps.
|
Golden file changes are available for triage from new commit, Click here to view. |
|
Golden changes look either small or expected to me. |
… TiledTextureContents::RenderToSnapshot (flutter/engine#43017)
…129334) flutter/engine@c6251a6...d9530e2 2023-06-22 [email protected] [Impeller] Fix up coverage hints for blending/vertices, fix TiledTextureContents::RenderToSnapshot (flutter/engine#43017) 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://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
3 participants
Fixes flutter/flutter#128329
Fixes flutter/flutter#128807
The panorama example app is still not quite visually on par with skia - the scaling or filtering or something isn't quite as nice as in the skia backend - but it does render now and no longer tries to create gigantical textures.