[Merged by Bors] - Rework ViewTarget to better support post processing #6415
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cart
added
A-Rendering
dataphract
approved these changes
Oct 30, 2022
DGriffin91
approved these changes
Oct 30, 2022
robtfm
reviewed
Oct 30, 2022
| main_textures: MainTargetTextures, | ||
| main_texture_format: TextureFormat, | ||
| /// 0 represents `texture_a`, 1 represents `texture_b` | ||
| main_texture: AtomicUsize, |
There was a problem hiding this comment.
should we force post processing to use &mut ViewTarget? i'm not sure but it feels like the atomic here could mask issues with concurrent writers, where there's no guarantee the render passes would be in the same order as the post_process_write requests.
edit: looks like node processing is single threaded, so that's not a problem. then i guess it's atomic so that it can be edited without a mut ref. all good.
There was a problem hiding this comment.
Yeah this is currently valid. I would like to revisit what mutability might look like here though. There is a world where we choose to make node processing parallel. Thats one of the reasons we're restricting to &World in the first place (to make graph execution read-only).
I know a number of people (including myself) have explored the idea to move this stuff into systems. Still not sure how I feel about it, but it would allow more granular read/writes in a parallel context (force linear command encoding for post processing effects, by nature of the mutable access to ViewTarget, allow parallel command encoding for most other things, as long as the systems are read only).
IceSentry
approved these changes
Oct 31, 2022
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Oct 31, 2022
# Objective Post processing effects cannot read and write to the same texture. Currently they must own their own intermediate texture and redundantly copy from that back to the main texture. This is very inefficient. Additionally, working with ViewTarget is more complicated than it needs to be, especially when working with HDR textures. ## Solution `ViewTarget` now stores two copies of the "main texture". It uses an atomic value to track which is currently the "main texture" (this interior mutability is necessary to accommodate read-only RenderGraph execution). `ViewTarget` now has a `post_process_write` method, which will return a source and destination texture. Each call to this method will flip between the two copies of the "main texture". ```rust let post_process = render_target.post_process_write(); let source_texture = post_process.source; let destination_texture = post_process.destination; ``` The caller _must_ read from the source texture and write to the destination texture, as it is assumed that the destination texture will become the new "main texture". For simplicity / understandability `ViewTarget` is now a flat type. "hdr-ness" is a property of the `TextureFormat`. The internals are fully private in the interest of providing simple / consistent apis. Developers can now easily access the main texture by calling `view_target.main_texture()`. HDR ViewTargets no longer have an "ldr texture" with `TextureFormat::bevy_default`. They _only_ have their two "hdr" textures. This simplifies the mental model. All we have is the "currently active hdr texture" and the "other hdr texture", which we flip between for post processing effects. The tonemapping node has been rephrased to use this "post processing pattern". The blit pass has been removed, and it now only runs a pass when HDR is enabled. Notably, both the input and output texture are assumed to be HDR. This means that tonemapping behaves just like any other "post processing effect". It could theoretically be moved anywhere in the "effect chain" and continue to work. In general, I think these changes will make the lives of people making post processing effects much easier. And they better position us to start building higher level / more structured "post processing effect stacks". --- ## Changelog - `ViewTarget` now stores two copies of the "main texture". Calling `ViewTarget::post_process_write` will flip between copies of the main texture.
bors
bot
changed the title
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective Post processing effects cannot read and write to the same texture. Currently they must own their own intermediate texture and redundantly copy from that back to the main texture. This is very inefficient. Additionally, working with ViewTarget is more complicated than it needs to be, especially when working with HDR textures. ## Solution `ViewTarget` now stores two copies of the "main texture". It uses an atomic value to track which is currently the "main texture" (this interior mutability is necessary to accommodate read-only RenderGraph execution). `ViewTarget` now has a `post_process_write` method, which will return a source and destination texture. Each call to this method will flip between the two copies of the "main texture". ```rust let post_process = render_target.post_process_write(); let source_texture = post_process.source; let destination_texture = post_process.destination; ``` The caller _must_ read from the source texture and write to the destination texture, as it is assumed that the destination texture will become the new "main texture". For simplicity / understandability `ViewTarget` is now a flat type. "hdr-ness" is a property of the `TextureFormat`. The internals are fully private in the interest of providing simple / consistent apis. Developers can now easily access the main texture by calling `view_target.main_texture()`. HDR ViewTargets no longer have an "ldr texture" with `TextureFormat::bevy_default`. They _only_ have their two "hdr" textures. This simplifies the mental model. All we have is the "currently active hdr texture" and the "other hdr texture", which we flip between for post processing effects. The tonemapping node has been rephrased to use this "post processing pattern". The blit pass has been removed, and it now only runs a pass when HDR is enabled. Notably, both the input and output texture are assumed to be HDR. This means that tonemapping behaves just like any other "post processing effect". It could theoretically be moved anywhere in the "effect chain" and continue to work. In general, I think these changes will make the lives of people making post processing effects much easier. And they better position us to start building higher level / more structured "post processing effect stacks". --- ## Changelog - `ViewTarget` now stores two copies of the "main texture". Calling `ViewTarget::post_process_write` will flip between copies of the main texture.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Post processing effects cannot read and write to the same texture. Currently they must own their own intermediate texture and redundantly copy from that back to the main texture. This is very inefficient.
Additionally, working with ViewTarget is more complicated than it needs to be, especially when working with HDR textures.
Solution
ViewTargetnow stores two copies of the "main texture". It uses an atomic value to track which is currently the "main texture" (this interior mutability is necessary to accommodate read-only RenderGraph execution).ViewTargetnow has apost_process_writemethod, which will return a source and destination texture. Each call to this method will flip between the two copies of the "main texture".The caller must read from the source texture and write to the destination texture, as it is assumed that the destination texture will become the new "main texture".
For simplicity / understandability
ViewTargetis now a flat type. "hdr-ness" is a property of theTextureFormat. The internals are fully private in the interest of providing simple / consistent apis. Developers can now easily access the main texture by callingview_target.main_texture().HDR ViewTargets no longer have an "ldr texture" with
TextureFormat::bevy_default. They only have their two "hdr" textures. This simplifies the mental model. All we have is the "currently active hdr texture" and the "other hdr texture", which we flip between for post processing effects.The tonemapping node has been rephrased to use this "post processing pattern". The blit pass has been removed, and it now only runs a pass when HDR is enabled. Notably, both the input and output texture are assumed to be HDR. This means that tonemapping behaves just like any other "post processing effect". It could theoretically be moved anywhere in the "effect chain" and continue to work.
In general, I think these changes will make the lives of people making post processing effects much easier. And they better position us to start building higher level / more structured "post processing effect stacks".
Changelog
ViewTargetnow stores two copies of the "main texture". CallingViewTarget::post_process_writewill flip between copies of the main texture.