[Merged by Bors] - Update post_processing example to not render UI with first pass camera #6469
Conversation
examples/shader/post_processing.rs
Outdated
| @@ -42,11 +42,43 @@ fn setup( | |||
| ) { | |||
| let window = windows.primary_mut(); | |||
| let size = Extent3d { | |||
| width: window.physical_width(), | |||
| height: window.physical_height(), | |||
| width: window.width() as u32, | |||
There was a problem hiding this comment.
This will actually render this at a lower resolution when the scale factor is above 1, which isn't what we want generally. Scenes should still render at the native resolution. It is ui that needs to be scaled up.
examples/shader/post_processing.rs
Outdated
| ..default() | ||
| }; | ||
|
|
||
| commands |
There was a problem hiding this comment.
The point of this is example is to minimally illustrate a post processing effect. This UI serves no purpose other than to prove that a bug has been resolved. It introduces additional complexity that distracts from the purpose, so I think it should be removed.
examples/shader/post_processing.rs
Outdated
| @@ -147,6 +179,7 @@ fn setup( | |||
| ..Camera2dBundle::default() | |||
| }, | |||
| post_processing_pass_layer, | |||
| UiCameraConfig { show_ui: false }, | |||
There was a problem hiding this comment.
My personal take is that this is on the wrong camera. I would assert that UI should be added in the "final" pass without post processing effects applied (in the majority of cases). Moving this to the other camera also resolves the "wrong ui scale factor" problem, as it will inherit the window's scale factor (rather than assuming a scale factor of 1.0 for the Image rendertarget)
There was a problem hiding this comment.
But moving this to the other camera only resolves this in the case where the user doesn't want the post processing applied to the UI. What if the user wants the UI to have post processing applied? Seems that if rendering is done at the actual physical size with window.physical_size() the UI layout system won't take it into consideration and positions/sizes things based on the logical size instead.
There was a problem hiding this comment.
That is a much less common scenario, so I don't think we should cover it here, especially given the downsides I've already covered. That scenario will be covered by any of the following:
- Implementing post processing in the render graph, after UI is rendered (this is possible now using the new post processing primitives [Merged by Bors] - Rework ViewTarget to better support post processing #6415).
- Allowing users to define scale factor overrides for UI for a specific camera (not yet implemented, but it should be).
There is also the approach you employed here as a short term solution, but I don't want to encourage that pattern generally as it encourages the "wrong" outcomes (such as rendering at a resolution lower than you should).
There was a problem hiding this comment.
We also have plans for a higher level "post processing stack" which will likely subsume all of these scenarios.
There was a problem hiding this comment.
Okay makes sense, closing the PR then 👍
|
I have seen at least one other person get hit by the double-UI thing while trying to build off of this example. I think a PR to disable UI for the other camera would be welcome. |
Not sure if it makes sense to make a new PR for that, but just for simplicity I pushed the changes here since the original changes don't make sense anymore. The only change now is what you said, the disabling of the UI being rendered by the first camera. |
|
Just need to update the PR title and description if we're reusing this PR. |
darthdeus
changed the title
|
Okay PR title and description updated. |
|
Is it possible to make it so that UI rendering is only enabled by default for when there's a single camera? When multiple cameras are added, it's disabled by default and you need to add |
alice-i-cecile
added
A-Rendering
mockersf
added
the
S-Ready-For-Final-Review
|
bors r+ |
#6469) # Objective Make sure the post processing example won't render UI twice. ## Solution Disable UI on the first pass camera with `UiCameraConfig`
|
Pull request successfully merged into main. Build succeeded:
|
bors
bot
changed the title
bevyengine#6469) # Objective Make sure the post processing example won't render UI twice. ## Solution Disable UI on the first pass camera with `UiCameraConfig`
bevyengine#6469) # Objective Make sure the post processing example won't render UI twice. ## Solution Disable UI on the first pass camera with `UiCameraConfig`
bevyengine#6469) # Objective Make sure the post processing example won't render UI twice. ## Solution Disable UI on the first pass camera with `UiCameraConfig`
Objective
Make sure the post processing example won't render UI twice.
Solution
Disable UI on the first pass camera with
UiCameraConfig