-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Add TrackedRenderPass::inner method #3595
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
Conversation
bors try |
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
/// | ||
/// Resets all tracked state in the process. | ||
pub fn inner(&mut self) -> &mut RenderPass<'a> { | ||
self.state = Default::default(); |
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.
How would this be used? It seems that this could be used to circumvent the checks that TrackedRenderPass
is designed to prevent. Maybe it would be better to extend TrackedRenderPass
.
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.
If I understand the rest of the code, the checks are specifically optimizations to avoid redundant calls on the underlying RenderPass. By clearing the tracked state, we prevent those checks from firing when they shouldn't and thereby force subsequent calls not to be deduplicated.
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.
If so, I think it’s worth adding a comment that TrackedRenderPass
is an optimization over RenderPass
, and resetting the state shouldn’t break anything.
I don't really have any context for this PR - can you give some example(s) of why this is necessary? I'm not saying it isn't, just that I don't know why it is. :) |
The goal is to enable non-bevy rendering libraries to draw objects in bevy scenes. Such a library wouldn't directly want to take a Does that make sense? |
Ah, interesting. It makes sense in that I understand what you're saying. I've just been adding proxies to wgpu API to the appropriate bevy types (which are just thin wrappers for the sake of Arc<>) whenever something is missing. Is there something that that kind of approach would not address that means your PR is necessary? |
Could you give an example of what one of those proxies might look like? Are you talking about a struct that internally contains a bevy type? |
I meant like if I need to be able to call |
Ah, so you're imagining only using the proxy type from within a bevy project. What I'd like to do is have a project structure along the lines of:
But this means that bevy_teapot cannot use proxy types, because wgpu_teapot only knows about wgpu types. (teapot being a standin for whatever rendering functionality might be provided) |
Ok, so you want to be able to plug in wgpu code that exists elsewhere without having to rewrite any of it? I think this is a question for @cart to decide whether this is something desirable to support or not. |
# Objective - Currently there is now way of making an indirect draw call from a tracked render pass. - This is a very useful feature for GPU based rendering. ## Solution - Expose the `draw_indirect` and `draw_indexed_indirect` methods from the wgpu `RenderPass` in the `TrackedRenderPass`. ## Alternative - #3595: Expose the underlying `RenderPass` directly
# Objective - Currently there is now way of making an indirect draw call from a tracked render pass. - This is a very useful feature for GPU based rendering. ## Solution - Expose the `draw_indirect` and `draw_indexed_indirect` methods from the wgpu `RenderPass` in the `TrackedRenderPass`. ## Alternative - #3595: Expose the underlying `RenderPass` directly
# Objective - Currently there is now way of making an indirect draw call from a tracked render pass. - This is a very useful feature for GPU based rendering. ## Solution - Expose the `draw_indirect` and `draw_indexed_indirect` methods from the wgpu `RenderPass` in the `TrackedRenderPass`. ## Alternative - bevyengine#3595: Expose the underlying `RenderPass` directly
@cart would be good to get clarification on whether bevy wants to be compatible with wgpu libraries that don't themselves depend on bevy. |
If we can support "raw" wgpu libraries in a way that doesn't compromise Bevy's rendering approach, im on board. Resetting the inner state does seem reasonable (other than forcing rebinds of anything that comes after). This could easily become a performance foot-gun, especially if done on a per-drawn-entity basis. The "draw a teapot" scenario feels like it would be a per-entity sort of thing, which would force rebinds of at least bind group zero across entity draws. On a related note, if the "teapot drawing" logic includes bevy-agnostic render pipelines, it would be incompatible with bevy's built in bindings for cameras / lights / materials / etc. In practice I expect the compatibility problems in that scenario would not be "worth it" for Bevy users relative to just downloading and using a teapot model with Bevy-native pipelines. Before committing to going down this path, I think we should actually try doing it / prove out this use case / see what exposing the inner pass enables in practice. The pass is tied to Render World lifetimes, so I expect that this will require storing any "raw wgpu state" in Bevy's Render World. Bevy also makes its own assumptions about how windows and render targets are represented, how objects are sorted and drawn relative to each other, how bevy app data is passed to shaders, etc. I suspect that there will be classes of "opinionated" raw wgpu logic that will not mesh with those patterns. If we can find a class of thing that works nicely and provides value to users, I think I'm on board. |
Closing as obsolete in accordance with the new PR Closing Guidance. This was implemented in PR #10722. |
The linked PR is equivalent to this one except that this one clears out the internal state and the other one doesn't. I haven't kept up with this part of the codebase, but not clearing out the same seems like it might be a bug? |
I think you understand it better than me. Would you mind writing up an issue? |
Objective
Enable implementations of
Draw
andRenderCommand
to directly work with wgpu types.Solution
TrackedRenderPass
into awgpu::RenderPass