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

Rework of #1203: Provide the camera's view matrix to shaders #1611

Closed
wants to merge 10 commits into from

Conversation

mtsr
Copy link
Contributor

@mtsr mtsr commented Mar 10, 2021

I reworked the code from #1203 based on this comment by Cart. Particularly, I've split the Camera Uniform into separate bindings and made it easy to extend with more bindings in the future. Such as the CameraPos needed in #1554.

@lwansbrough I hope you don't mind. I also tried doing a PR against your fork, but github won't show/autocomplete your username or repositories in any fields for me.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible labels Mar 10, 2021
@mtsr mtsr force-pushed the global-transform branch from a9cef2d to 7e4c2a6 Compare March 10, 2021 21:42
@mtsr mtsr force-pushed the global-transform branch from 7e4c2a6 to 8a22500 Compare March 10, 2021 21:49
@mtsr
Copy link
Contributor Author

mtsr commented Mar 10, 2021

Rebased on main and fixed up commit author that was messed up during rebase.

layout(set = 0, binding = 0) uniform Camera {
layout(set = 0, binding = 0) uniform CameraViewProj {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This (across every shader) is the one part of this PR that feel problematic to me, because it will break pretty much every single shader people have. On the other hand it seems messy to let the name remain just Camera.

Copy link
Member

Choose a reason for hiding this comment

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

This seems to be a sensible change with an easy fix. My opinion is that it's better to make breaking changes like this sooner, in order to reduce the total code that must be fixed, and then add a note to our migration guides.

@@ -6,8 +6,9 @@ layout(location = 2) in vec2 Vertex_Uv;

layout(location = 0) out vec2 v_Uv;

layout(set = 0, binding = 0) uniform Camera {
layout(set = 0, binding = 0) uniform CameraViewProj {
Copy link
Member

Choose a reason for hiding this comment

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

This binding is now incorrect and sprites no longer render

mat4 ViewProj;
mat4 View;
Copy link
Member

Choose a reason for hiding this comment

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

This binding is now incorrect and sprite sheets no longer render

mat4 ViewProj;
mat4 View;
Copy link
Member

Choose a reason for hiding this comment

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

This binding is now incorrect and ui no longer renders

mat4 ViewProj;
};
layout(set = 0, binding = 1) uniform CameraView {
Copy link
Member

Choose a reason for hiding this comment

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

Neither of the forward shaders use CameraView, but removing it causes rendering to break (see the ui/sprite shaders, which are currently broken but can be "fixed" by adding CameraView).

As-is this implementation has the same problem I called out in #1203, but with the added downside of more boilerplate.

For this to be a solution, users need to be able to pull in only what they need.

Copy link
Contributor Author

@mtsr mtsr Mar 13, 2021

Choose a reason for hiding this comment

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

Thanks for the review.

I made a wrong assumption that led me think these things would work: I assumed that the reason this worked for render resources was due to a difference between a uniform struct and separate bindings. I am a complete beginner when it comes to shaders... I did fail to test all the examples, I will remember better next time.

Clearly there’s much more going on under water in the render resource bindings. Is it using shader reflection to create bindgroups that only have the bindings used in the shaders?

If so, that seems useful and I’d love to find out how. But I’m finding the related code to be both dense and somewhat spread across different implementations. Can you point me in the right direction?

But wouldn't that also mean the cost of not having to have these bindings in some shader code goes up yet again? There's the cost of using multiple buffers instead of one. Multiple bindings. But now we even have to set up the bind groups for the camera every time the pipeline changes, I think?

Copy link
Member

Choose a reason for hiding this comment

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

No worries! I can see why you would think that, given that the bindings provided by RenderResourceBindings are flexible in exactly that way. The reason it doesn't work here is that bind groups need to exactly match the shader layout. PassNode sets an explicit bind group for the camera (with two bindings), whereas RenderResourceBindings creates the exact bind group required to match the shader layout (if it doesn't already exist), which could have any number of bindings in any order.

Handling this case is non-trivial, especially if we want to do it generically. If every camera had a different name, we could just throw them into the global RenderResourceBindings and everything would magically work. However we dont want that for cameras.

I'm personally pretty attached to the idea of including pbr in 0.5, so I'm willing to consider short term solutions (such as the original impl: having a single combined binding/buffer and requiring that people include the camera position in every shader, or special-casing cameras to somehow allow both layouts).

I'll need to think a bit about this. Once I've wrapped up the other reviews i need to do, I'll probably spend some time on this. Feel free to explore the space too if you feel inclined!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I have a handle on this. Let me try to fix this PR.

@@ -32,8 +32,9 @@ const VERTEX_SHADER: &str = r#"
layout(location = 0) in vec3 Vertex_Position;
layout(location = 0) out vec4 v_Position;

layout(set = 0, binding = 0) uniform Camera {
layout(set = 0, binding = 0) uniform CameraViewProj {
mat4 ViewProj;
Copy link
Member

Choose a reason for hiding this comment

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

The example shaders are also broken.

@mtsr
Copy link
Contributor Author

mtsr commented Mar 13, 2021

I've taken a quick look at push constants, but they only reliably support 128 bytes, which is only enough for View and ViewProj.

See https://docs.rs/wgpu/0.7.0/wgpu/struct.Limits.html#structfield.max_push_constant_size

@cart
Copy link
Member

cart commented Mar 13, 2021

Yeah I'm also a little hesitant to use push constants as they aren't supported on the web: both WebGPU and (im assuming) bevy_webgl2. I don't want to support multiple render paths for those cases at this stage in bevy's development.

Copy link
Contributor Author

@mtsr mtsr 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 I get it. It's the RenderPipelines.bindings that holds all the bindings specific to an entity and DrawContext.set_bind_groups_from_bindings() holds the logic to only set the bindings used by the current pipeline.

I think I can reproduce this for cameras in PassNode, now.

@mtsr
Copy link
Contributor Author

mtsr commented Mar 16, 2021

Hah. Nope. I'd need Render(Resource)Context, the current PipelineDescriptor and the camera Bindings in one place. Of which Render(Resource)Context is problematic, I think.

bors bot pushed a commit that referenced this pull request Mar 19, 2021
Alternative to #1203 and #1611

Camera bindings have historically been "hacked in". They were _required_ in all shaders and only supported a single Mat4. PBR (#1554) requires the CameraView matrix, but adding this using the "hacked" method forced users to either include all possible camera data in a single binding (#1203) or include all possible bindings (#1611).

This approach instead assigns each "active camera" its own RenderResourceBindings, which are populated by CameraNode. The PassNode then retrieves (and initializes) the relevant bind groups for all render pipelines used by visible entities. 

* Enables any number of camera bindings , including zero (with any set or binding number ... set 0 should still be used to avoid rebinds).
* Renames Camera binding to CameraViewProj
* Adds CameraView binding
@mtsr mtsr closed this Mar 20, 2021
@mtsr
Copy link
Contributor Author

mtsr commented Mar 20, 2021

Solved by #1689

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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants