Provide the camera's view matrix to shaders via the Camera uniform

[lwansbrough]

I'm writing a shader which depends on the camera position. This change adds the view matrix as a second mat4 on the Camera uniform which provides the camera's transform to the shader. Existing shaders in bevy's repository have been updated, but this is a breaking change for any shaders depending on the Camera uniform.

[cart]

I like everything about this but the fact that every shader needs to include both ViewProj and View. Logically it makes sense to group them inside the same uniform, but I'm not sure thats worth the added cost of needing to define both everywhere. Maybe we could split these out into separate bindings?

[lwansbrough]

I had a similar concern - a separate binding seems appropriate. If I get some time I'll update this PR.

[quarz]

I like everything about this but the fact that every shader needs to include both ViewProj and View. Logically it makes sense to group them inside the same uniform, but I'm not sure thats worth the added cost of needing to define both everywhere. Maybe we could split these out into separate bindings?

I think the root issue here is lack of #include support. There is always gonna be shared shader code, this View matrix case is just one of many.

Shaderc does have #include support (https://docs.rs/shaderc/0.7.0/shaderc/struct.CompileOptions.html#method.set_include_callback), but we don't use it currently. Do you think it would make sense to put some effort into this direction?

[cart]

I think its worth trying to add some sort of "importing", but I'm not sure I want to solve this type of problem with "pasting in bindings from other files". I'd rather just let people declare the bindings they want and have it all just work as expected.

[cart]

And I think we might want to build our own "post processing shader import system" that integrates directly with Bevy Asset. And that way we aren't directly tied to shaderc or glsl-to-spirv (when its ready we want to move to Naga).

[Neo-Zhixing]

I like everything about this but the fact that every shader needs to include both ViewProj and View. Logically it makes sense to group them inside the same uniform, but I'm not sure thats worth the added cost of needing to define both everywhere. Maybe we could split these out into separate bindings?

@cart Why is it that the Camera uniform is always using

layout(set = 0, binding = 0) uniform Camera

?

It seems that if I use any binding locations other than set 0 binding 0 the camera would not work. Searching through the code there are multiple locations where this was hard coded, for example here, here and here. Is there a reason for doing so? I have a feeling that if we want to allow users to choose View or ViewProj based on their binding we need to address this problem first.

I'm expecting that the user would be able to do this

layout(set = 0, binding = 2) uniform Camera3dViewProj
layout(set = 0, binding = 0) uniform Camera3dView
layout(set = 0, binding = 1) uniform Camera3dTransform
layout(set = 0, binding = 4) uniform PerspectiveProjection {
    float near, far, for, aspect_ratio;
}

with arbitrary set and binding numbers.

[cart]

Is there a reason for doing so?

Yup! RenderPassNodes can have different cameras assigned to each pass. The higher level RenderResourceBindings can currently be global, from entities, or from assets. None of those categories fits the "per pass" setup very well. And the current approach let us do the minimal amount of work required to bind the camera. We could consider giving each PassNode RenderResourceBindings instead, but off the top of my head im not sure thats the best path forward yet.

I definitely agree that with the addition of more camera properties, something needs to change.

[Neo-Zhixing]

We could consider giving each PassNode RenderResourceBindings instead, but off the top of my head im not sure thats the best path forward yet.

I understand that we can have different cameras assigned to each pass, but what's the problem with setting the camera bindings in the global RenderResourceBindings? If we require that the camera names must be unique, will it enable the cameras to be binded just like any other render resources?

@cart With wgpu 0.7 we will also have push constants. Maybe consider using that for cameras?

[mtsr]

@cart With wgpu 0.7 we will also have push constants. Maybe consider using that for cameras?

From a cursory reading it seems push constants can only accommodate 128 bytes reliably on all hardware. That's just about enough for View and ViewProj, but nothing else. I don't think that will cut it.

[Neo-Zhixing]

@cart With wgpu 0.7 we will also have push constants. Maybe consider using that for cameras?

From a cursory reading it seems push constants can only accommodate 128 bytes reliably on all hardware. That's just about enough for View and ViewProj, but nothing else. I don't think that will cut it.

Right. And on a second thought, using push constants require that we copy the camera data to the command buffer for every draw call. With a uniform buffer we'll be able to reuse this data every frame.

[Neo-Zhixing]

Can we close this PR now that 1689 was merged?

[cart]

Yup!