Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

fintelia
Copy link
Contributor

@fintelia fintelia commented Jan 8, 2022

Objective

Enable implementations of Draw and RenderCommand to directly work with wgpu types.

Solution

  • A method that converts a TrackedRenderPass into a wgpu::RenderPass

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 8, 2022
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Jan 8, 2022
@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jan 8, 2022
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();
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@superdump
Copy link
Contributor

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. :)

@fintelia
Copy link
Contributor Author

fintelia commented Jan 16, 2022

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 bevy::TrackedRenderPass since that would mean it could only work with bevy. Instead it would only want to work with a wgpu::{Device, Queue, RenderPass, ...}. The first two of those are directly accessible now. But to make an object actually 'fit' into a scene it should be rendered while executing a render graph. That means drawing onto the RenderPass that is in use at the time.

Does that make sense?

@superdump
Copy link
Contributor

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?

@fintelia
Copy link
Contributor Author

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?

@superdump
Copy link
Contributor

I meant like if I need to be able to call wgpu::Device::create_texture_with_data() then I implement RenderDevice::create_texture_with_data() that calls wgpu::Device::create_texture_with_data(). Similarly if I needed to do something on wgpu::RenderPass then I'd add the necessary API to TrackedRenderPass.

@fintelia
Copy link
Contributor Author

fintelia commented Jan 17, 2022

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:

  • wgpu_teapot: teapot rendering library that uses pure wgpu. Can be used by non-bevy projects.
  • bevy_teapot: bevy plugin that lets you insert a teapot into your bevy scene, internally relying on wgpu_teapot.

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)

@superdump
Copy link
Contributor

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.

bors bot pushed a commit that referenced this pull request Feb 27, 2022
# 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
bors bot pushed a commit that referenced this pull request Feb 28, 2022
# 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
kurtkuehnert added a commit to kurtkuehnert/bevy that referenced this pull request Mar 6, 2022
# 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
@fintelia
Copy link
Contributor Author

@cart would be good to get clarification on whether bevy wants to be compatible with wgpu libraries that don't themselves depend on bevy.

@cart
Copy link
Member

cart commented Apr 12, 2022

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.

@NthTensor
Copy link
Contributor

Closing as obsolete in accordance with the new PR Closing Guidance. This was implemented in PR #10722.

@NthTensor NthTensor closed this May 2, 2024
@fintelia
Copy link
Contributor Author

fintelia commented May 3, 2024

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?

@NthTensor
Copy link
Contributor

I think you understand it better than me. Would you mind writing up an issue?

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-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants