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

Cache depth texture based on usage #9565

Merged
merged 3 commits into from
Sep 4, 2023

Conversation

IceSentry
Copy link
Contributor

Objective

  • Currently, the depth textures are cached based on the target. If multiple camera have the same target but a different depth_texture_usage bevy will just use the same texture and ignore that setting.

Solution

  • Add the usage as a cache key

@IceSentry IceSentry added A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior labels Aug 24, 2023
@mockersf mockersf 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 Aug 24, 2023
@superdump
Copy link
Contributor

Looks like some cargo fmt is needed?

@IceSentry
Copy link
Contributor Author

yeah, this is caused by rust 1.72.0 now formatting let-else. This PR should fix it #9562

@IceSentry
Copy link
Contributor Author

@superdump I discussed about this with @Neo-Zhixing and they suggested to just OR all the usages per render target. This would mean we'll only create 1 texture per target like it's done currently. The only unknown is whether or not some of the usage flags could be incompatible with each other. I'm curious if you have any opinions on this.

@Neo-Zhixing
Copy link
Contributor

Yeah so my recommendation is that we do it over two passes, the first pass collect the usage flags per render target, the second pass creates the textures. That way you create the same number of textures with the flag reflecting what all of the cameras requested.

The problem we're trying to solve here is that if multiple cameras specify the same render target but with different flags, it'll just use the flag for the first camera and ignore the rest.

I don't think texture usages can conflict with one another. People are generally more concerned about "if this texture usage was supported by the texture format", and usage flags should always be compatible i think.

@IceSentry
Copy link
Contributor Author

I've implemented the two pass suggestion in my latest commit

@alice-i-cecile alice-i-cecile added this to the 0.12 milestone Aug 28, 2023
@alice-i-cecile
Copy link
Member

@superdump @mockersf this is well-reviewed, but I don't fully understand what's going on here (docs on Camera3dDepthTextureUsage would have been useful), so I'm going to defer to one of you to merge this.

@alice-i-cecile
Copy link
Member

It's been a week, so I'm going to merge this on the basis of your approvals to unblock this fix.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 4, 2023
Merged via the queue into bevyengine:main with commit d516a27 Sep 4, 2023
20 checks passed
Neo-Zhixing pushed a commit to ForesightMiningSoftwareCorporation/bevy that referenced this pull request Oct 26, 2023
# Objective

- Currently, the depth textures are cached based on the target. If
multiple camera have the same target but a different
`depth_texture_usage` bevy will just use the same texture and ignore
that setting.

## Solution

- Add the usage as a cache key
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- Currently, the depth textures are cached based on the target. If
multiple camera have the same target but a different
`depth_texture_usage` bevy will just use the same texture and ignore
that setting.

## Solution

- Add the usage as a cache key
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-Bug An unexpected or incorrect behavior 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.

6 participants