-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Adds GlobalRenderResourcesNode to easily bind Resources #2043
Conversation
crates/bevy_render/src/render_graph/nodes/render_resources_node.rs
Outdated
Show resolved
Hide resolved
crates/bevy_render/src/render_graph/nodes/render_resources_node.rs
Outdated
Show resolved
Hide resolved
I might be a bit biased since I was somewhat involved in this. But in terms of general ergonomics, using a Resource felt like the most natural way to go and was the first thing I've tried when attempting to use "global" uniforms. |
2114b57
to
d4a9975
Compare
This seems like a really neat feature. For clarity and ease of comprehension, may I suggest renaming GlobalRenderResourcesNode into ResourcesNode and RenderResourcesNode into ComponentsNode? It would match their source object and I think the "Render" is superfluous since this is all already in the render_graph crate. Edit: I just saw there was an AssetRenderResourcesNode too and the naming I just proposed might be confusing in the global context. I also find that RenderResource might be confused with ECS Resources while they are in fact completely unrelated. So may I actually suggest:
|
mut render_resource_bindings: ResMut<RenderResourceBindings>, | ||
) { | ||
// TODO add support for textures | ||
// No need to do anything if no changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_changed() is also true for added. Although I guess it doesn't hurt to add it for clarity.
How does this relate to the modern |
I believe this is too outdated to be relevant. ExtractResource covers most of this. |
To expose a ECS Resource as a custom uniform requires reimplementing most of the stuff that
camera_node.rs
orlights_node.rs
do. My idea here was to see if this can be significantly reduced for Resources that implement RenderResources.One of the use cases is being able to easily expose values to FullscreenPassNode shaders, which do not use any entities.
It currently works, although it's quite possible I missed some edge cases. Also I'm aware it doesn't currently support Textures.
Before I put in the work polishing this, I'd like to hear what people think.