Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Aug 24, 2023

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:

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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • 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.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@gaaclarke gaaclarke marked this pull request as ready for review August 24, 2023 23:54
@gaaclarke
Copy link
Member Author

@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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this syntax?

Copy link
Member Author

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.

Copy link
Contributor

@jonahwilliams jonahwilliams left a 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

@gaaclarke
Copy link
Member Author

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?

@jonahwilliams
Copy link
Contributor

The issue I saw with barriers was scrolling through the pull quote on wonderous would occassionally show incorrect tiling.

@jonahwilliams
Copy link
Contributor

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?

@gaaclarke
Copy link
Member Author

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);

@gaaclarke gaaclarke merged commit 66c75d8 into flutter:main Aug 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2023
gaaclarke added a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023
…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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants