-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] Refactor: Create attachment descriptions without setting layouts #45088
[Impeller] Refactor: Create attachment descriptions without setting layouts #45088
Conversation
|
@jonahwilliams I wanted to land this groundwork for recycling alone to make sure I'm not missing anything. It also cleans things up a bit anyway. |
| bool resolve_texture = false) { | ||
| const auto& texture = | ||
| resolve_texture ? attachment.resolve_texture : attachment.texture; | ||
| const std::shared_ptr<Texture> Attachment::*texture_ptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this syntax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a pointer to a field. It allows us to specify if we want to grab Attachment::texture or Attachment::resolve_texture instead of using an optional bool argument. The pointers are const static. Its easier to check out the calling site.
jonahwilliams
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks right, so as long as its validating fine LGTM
|
I don't get validation layer errors when scrolling around in the gallery, let me know if you think there is something else we should test, like advanced blends? |
|
The issue I saw with barriers was scrolling through the pull quote on wonderous would occassionally show incorrect tiling. |
|
But I'm trying to do the math in my head here. I think this looks right. I don't really understand why we need to split this into two functions, I would probably implement the recycling in the part of this method that actually constructs the object description and leave the side effect heavy wrapper? |
|
Wonderous looks good. The logic will eventually look something like this: std::optional<vk::RenderPass> render_pass = cache.Fetch(vk_context, render_target_);
if (!render_pass) {
render_pass = CreateVKRenderPass(vk_context, render_target_);
cache.add(vk_context, render_target_, render_pass.value());
}
SetTextureLayouts(render_target_, command_buffer); |
…133305) flutter/engine@1382d6d...9bcefc7 2023-08-25 [email protected] Revert ios cpu changes (flutter/engine#45095) 2023-08-25 [email protected] [Impeller] Refactor: Create attachment descriptions without setting layouts (flutter/engine#45088) 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
…ayouts (flutter#45088) This refactor makes recycling render passes easier since it starts to split out setting the layout of textures with creating the render pass description. I didn't fully implement the split since it would technically be slower. Until we get caching of render passes working it would require us to calculate the attachment descriptions twice, once to make the render pass and once to set the texture layout. teases out some of the work from: flutter#44527 in the service of implementing: flutter#44861 issue: flutter/flutter#133182 The final split will have this function for recycled render passes: ```c++ void SetTextureLayouts(const RenderTarget& render_target, const std::shared_ptr<CommandBufferVK>& command_buffer) { for (const auto& [bind_point, color] : render_target.GetColorAttachments()) { SetTextureLayout(color, CreateAttachmentDescription(color, &Attachment::texture), command_buffer, &Attachment::texture); if (color.resolve_texture) { SetTextureLayout( color, CreateAttachmentDescription(color, &Attachment::resolve_texture), command_buffer, &Attachment::resolve_texture); } } if (auto depth = render_target.GetDepthAttachment(); depth.has_value()) { SetTextureLayout( depth.value(), CreateAttachmentDescription(depth.value(), &Attachment::texture), command_buffer, &Attachment::texture); } if (auto stencil = render_target.GetStencilAttachment(); stencil.has_value()) { SetTextureLayout( stencil.value(), CreateAttachmentDescription(stencil.value(), &Attachment::texture), command_buffer, &Attachment::texture); } } ``` ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
This refactor makes recycling render passes easier since it starts to split out setting the layout of textures with creating the render pass description.
I didn't fully implement the split since it would technically be slower. Until we get caching of render passes working it would require us to calculate the attachment descriptions twice, once to make the render pass and once to set the texture layout.
teases out some of the work from: #44527
in the service of implementing: #44861
issue: flutter/flutter#133182
The final split will have this function for recycled render passes:
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.