Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Add main_texture_other #7343

Closed

Conversation

torsteingrindvik
Copy link
Contributor

Objective

Use Case

A render node which calls post_process_write() on a ViewTarget multiple times during a single run of the node means both main textures of this view target is accessed.

If the source texture (which alternate between main textures a and b) is accessed in a shader during those iterations it means that those textures have to be bound using bind groups.

Preparing bind groups for both main textures ahead of time is desired, which means having access to the other main texture is needed.

Solution

Add a method on ViewTarget for accessing the other main texture.


Changelog

Added

  • main_texture_other API on ViewTarget

Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jan 23, 2023
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

The changes make sense and do seem useful. I'm not sure about the other terminology though. Unfortunately I can't think of something better

@JMS55
Copy link
Contributor

JMS55 commented Jan 23, 2023

I'm confused on how this would work. During the prepare render stage, when creating bind groups, you won't know which texture will be the last written to, so you can't bind that as read.

For instance, given nodes A -> B, if A does a post_process_write(), that affects which texture B will need to read from. How would B know which texture to bind?

Maybe I'm missing something about this.

@torsteingrindvik
Copy link
Contributor Author

torsteingrindvik commented Jan 24, 2023

@IceSentry agreed, I couldn't think of a good name either.

@JMS55
My use case is doing post_process_write() several times within the same node.
What I need is to prepare texture views for both main textures ahead of time.

Within the node I have a TextureViewId which I swap before the draw functions are called such that the correct bind group is bound.

@JMS55
Copy link
Contributor

JMS55 commented Jan 24, 2023

I see what you mean. I agree it fits your use case, but it's a very niche situation. I'll approve the PR, but I'd like the docs to indicate that it's probably not what you want 99% of the time. Name is fine imo, I can't think of anything better either.

Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>
@torsteingrindvik
Copy link
Contributor Author

@JMS55 I made the language in the docs even stronger to indicate that this method is only needed when you know you need it.

very niche situation

Yes I think so too.
But I'm thinking there will probably be others that have a render node with post_process_write within a loop, and then this is likely needed for them as well.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 27, 2023
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jan 27, 2023
# Objective

## Use Case

A render node which calls `post_process_write()` on a `ViewTarget` multiple times during a single run of the node means both main textures of this view target is accessed.

If the source texture (which alternate between main textures **a** and **b**) is accessed in a shader during those iterations it means that those textures have to be bound using bind groups.

Preparing bind groups for both main textures ahead of time is desired, which means having access to the _other_ main texture is needed.

## Solution

Add a method on `ViewTarget` for accessing the other main texture.

---

## Changelog

### Added

- `main_texture_other` API on `ViewTarget`
@bors bors bot changed the title Add main_texture_other [Merged by Bors] - Add main_texture_other Jan 27, 2023
@bors bors bot closed this Jan 27, 2023
@torsteingrindvik torsteingrindvik deleted the main-texture-other branch January 27, 2023 19:01
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

## Use Case

A render node which calls `post_process_write()` on a `ViewTarget` multiple times during a single run of the node means both main textures of this view target is accessed.

If the source texture (which alternate between main textures **a** and **b**) is accessed in a shader during those iterations it means that those textures have to be bound using bind groups.

Preparing bind groups for both main textures ahead of time is desired, which means having access to the _other_ main texture is needed.

## Solution

Add a method on `ViewTarget` for accessing the other main texture.

---

## Changelog

### Added

- `main_texture_other` API on `ViewTarget`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants