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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Remove the early exit to make sure the prepass textures are cleared #7891

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Remove the early exit to make sure the prepass textures are cleared #7891

wants to merge 1 commit into from

Conversation

geieredgar
Copy link
Contributor

@geieredgar geieredgar commented Mar 4, 2023 •
edited
Loading

Objective

Solution

  • Remove the early exit to make sure the prepass textures are cleared.

@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Mar 4, 2023
@JMS55
Copy link
Contributor

JMS55 commented Mar 4, 2023

The fix seems to work. I'm somewhat confused on the concept though. Say you have a camera with DepthPrepass, but don't have any meshes with prepass_enabled. What happens both in terms of what depth textures get generated, and how the main pass uses it?

@geieredgar
Copy link
Contributor Author

geieredgar commented Mar 4, 2023 •
edited
Loading

With this we always begin a RenderPass, which will clear the specified color and depth attachments (the prepass specific textures). If there are no meshes, there are no draw calls so the textures will stay in their cleared state. Previously, because we exited early, no render pass was started so the textures stayed in their old state from the previous frame.

@geieredgar geieredgar changed the title Remove the early exit to make sure the depth buffer is cleared Remove the early exit to make sure the prepass textures are cleared Mar 4, 2023
@JMS55
Copy link
Contributor

JMS55 commented Mar 4, 2023

That much makes sense to me. What confuses me is, what happens in the main pass then? Presumably it has an empty depth texture, which seems like it would cause problems?

@geieredgar
Copy link
Contributor Author

geieredgar commented Mar 4, 2023 •
edited
Loading

The depth texture is cleared with the value 0.0, which I assume is the depth value at the far plane. So fragments inside the frustum should always pass the depth test, assuming the depth test is set to greater or equal (if the depth of the near plane is positive). If that is the case, it should not cause any issues.

JMS55
JMS55 approved these changes Mar 4, 2023
View reviewed changes
Copy link
Contributor

@JMS55 JMS55 left a comment

Choose a reason for hiding this comment

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

After reading up on how depth testing works, this lgtm now :)

IceSentry
IceSentry approved these changes Mar 4, 2023
View reviewed changes
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.

I originally had the if there to avoid unnecessary work, but I guess it's not that much code that ends up running when these are empty anyway.

@JMS55
Copy link
Contributor

JMS55 commented Mar 4, 2023

The alternative to this would be to set a flag so that the main pass clears the depth texture itself. I don't think that's worth it, this is an unlikely situation to come up anyways.

robtfm
robtfm approved these changes Mar 4, 2023
View reviewed changes
Copy link
Contributor

@robtfm robtfm 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 is fine. the node won't get this far if no prepass_enabled materials are used as the query above will not match (no RenderPhase would be added), so there won't be a perf impact for no-prepass use cases.

@robtfm
Copy link
Contributor

robtfm commented Mar 4, 2023

set a flag so that the main pass clears the depth texture itself

yes updating the camera's depth load op would be better so we don't need to check for all possible prepass markers in the core_3d node. something to bear in mind for if we move the rendergraph to a pure schedule.

@superdump superdump added the A-Rendering Drawing game state to the screen label Mar 4, 2023
superdump
superdump approved these changes Mar 4, 2023
View reviewed changes
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot pushed a commit that referenced this pull request Mar 4, 2023
@geieredgar
Remove the early exit to make sure the prepass textures are cleared (#…
a213c8a 
…7891)

# Objective

- Fixes #7888.

## Solution

- Remove the early exit to make sure the prepass textures are cleared.
@bors
Copy link
Contributor

bors bot commented Mar 4, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Remove the early exit to make sure the prepass textures are cleared [Merged by Bors] - Remove the early exit to make sure the prepass textures are cleared Mar 4, 2023
@bors bors bot closed this Mar 4, 2023
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
@geieredgar @Shfty
Remove the early exit to make sure the prepass textures are cleared (b…
73f3df7 
…evyengine#7891)

# Objective

- Fixes bevyengine#7888.

## Solution

- Remove the early exit to make sure the prepass textures are cleared.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robtfm robtfm robtfm approved these changes

@JMS55 JMS55 JMS55 approved these changes

@IceSentry IceSentry IceSentry approved these changes

@superdump superdump superdump approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen
Projects
None yet
Milestone
0.10
Development

Successfully merging this pull request may close these issues.

prepass should run even when phases are empty
6 participants
@geieredgar @JMS55 @robtfm @superdump @IceSentry @alice-i-cecile