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

Keep track of when a texture is first cleared #10325

Merged
merged 41 commits into from
Dec 31, 2023

Conversation

JMS55
Copy link
Contributor

@JMS55 JMS55 commented Oct 31, 2023

Objective

  • Custom render passes, or future passes in the engine (such as Meshlet rendering (initial feature) #10164) need a better way to know and indicate to the core passes whether the view color/depth/prepass attachments have been cleared or not yet this frame, to know if they should clear it themselves or load it.

Solution

  • For all render targets (depth textures, shadow textures, prepass textures, main textures) use an atomic bool to track whether or not each texture has been cleared this frame. Abstracted away in the new ColorAttachment and DepthAttachment wrappers.

Changelog

  • Changed ViewTarget::get_color_attachment(), removed arguments.
  • Changed ViewTarget::get_unsampled_color_attachment(), removed arguments.
  • Removed Camera3d::clear_color.
  • Removed Camera2d::clear_color.
  • Added Camera::clear_color.
  • Added ExtractedCamera::clear_color.
  • Added ColorAttachment and DepthAttachment wrappers.
  • Moved ClearColor and ClearColorConfig from bevy::core_pipeline::clear_color to bevy::render::camera.
  • Core render passes now track when a texture is first bound as an attachment in order to decide whether to clear or load it.

Migration Guide

  • Remove arguments to ViewTarget::get_color_attachment() and ViewTarget::get_unsampled_color_attachment().
  • Configure clear color on Camera instead of on Camera3d and Camera2d.
  • Moved ClearColor and ClearColorConfig from bevy::core_pipeline::clear_color to bevy::render::camera.
  • ViewDepthTexture must now be created via the new() method

@JMS55 JMS55 added this to the 0.12 milestone Oct 31, 2023
@JMS55 JMS55 added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change labels Oct 31, 2023
@JMS55 JMS55 requested review from IceSentry and nicopap and removed request for nicopap October 31, 2023 02:40
@JMS55
Copy link
Contributor Author

JMS55 commented Oct 31, 2023

I can also maybe use Relaxed atomic ordering.

@superdump
Copy link
Contributor

I don’t have time to check right now but I think we need to be careful about removing all the prepass checks for whether to load or clear the depth buffer as they may not mean the same thing. For example, logically if something is the first write then we have to clear it and cannot load it. But if something is not the first write, perhaps then it still needs to be cleared in some cases. I don’t think so but I would also want to check to make sure.

@JMS55 JMS55 modified the milestones: 0.12, 0.13 Oct 31, 2023
@JMS55
Copy link
Contributor Author

JMS55 commented Oct 31, 2023

Oops this was supposed to be for milestone 0.13, fixed. There's no urgency on this PR.

@JMS55
Copy link
Contributor Author

JMS55 commented Nov 9, 2023

I also want to add flags for the prepass textures and any other render targets as well.

@JMS55
Copy link
Contributor Author

JMS55 commented Nov 10, 2023

TODO: Add support for the prepass outputs and the deferred gbuffer prepass outputs.

@JMS55 JMS55 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Nov 10, 2023
@alice-i-cecile
Copy link
Member

I'm not super familiar with this part of the renderer. Can you say more about why we should / need to use atomics here?

@JMS55
Copy link
Contributor Author

JMS55 commented Nov 20, 2023

I'm not super familiar with this part of the renderer. Can you say more about why we should / need to use atomics here?

Keeping track of whether or not to clear the texture is decided in the render nodes, which don't allow mutable access to components, so an atomic is needed. Otherwise it would just be a regular boolean.

@robtfm
Copy link
Contributor

robtfm commented Nov 20, 2023

i definitely think this should be done, but i'm not convinced about the way it is done here.

  • i'm not super happy with a function called is_first_write modifying the internal state
  • we have duplicated logic for working out what the clear color should be in each node that potentially needs to clear
  • the functions is_first_write, is_first_motion_vectors_write and friends all do basically the same thing

the last two points make me think we're missing an abstraction: it would be nicer i think to have something like a TrackedTexture which wraps a texture and a clear op (and an atomic), so the clear op is initialized once at the point the texture is created. then the nodes can just call texture.load_op() and get the right op for the texture without needing to know how to determine it themselves.

what do you think?

@JMS55
Copy link
Contributor Author

JMS55 commented Nov 20, 2023

I'm in favor of some kind of ClearOnceTexture wrapper, agreed.

i'm not super happy with a function called is_first_write modifying the internal state

Yeah it's not the best naming, but I couldn't think of anything better. I suppose moving the info into the type name helps, and then we can call the method just get_load_op() or something.

@JMS55 JMS55 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 Dec 30, 2023
@alice-i-cecile
Copy link
Member

This generally looks fine, I understand the motivation, and this is reviewed by folks with expertise. Merging now.

auto-merge was automatically disabled December 30, 2023 23:55

Head branch was pushed to by a user without write access

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 31, 2023
Merged via the queue into bevyengine:main with commit 70b0eac Dec 31, 2023
22 checks passed
@eerii eerii mentioned this pull request Jan 17, 2024
@JMS55 JMS55 mentioned this pull request Feb 5, 2024
6 tasks
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-Code-Quality A section of code that is hard to understand or change C-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants