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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 10 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
93c05f6
Update the camera uniform to include the view matrix
lwansbrough Mar 10, 2021
8962b8c
Update shaders to include new uniform data
lwansbrough Jan 3, 2021
a7e84f2
Clean up
lwansbrough Mar 10, 2021
26654c7
Start on splitting camera bindings
mtsr Mar 10, 2021
8a22500
Finish splitting camera bindings
mtsr Mar 10, 2021
1e8c67a
Fix hot.vert
mtsr Mar 10, 2021
55910d3
Fix all other shaders
mtsr Mar 10, 2021
5501613
Fix test_reflection
mtsr Mar 10, 2021
081e0fd
Remove commented code
mtsr Mar 11, 2021
dde6079
Add comment describing buffers HashMap
mtsr Mar 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion assets/shaders/hot.vert
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

layout(location = 0) in vec3 Vertex_Position;

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.

mat4 ViewProj;
};

Expand Down
5 changes: 4 additions & 1 deletion crates/bevy_pbr/src/render_graph/forward_pipeline/forward.frag
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,12 @@ layout(location = 2) in vec2 v_Uv;

layout(location = 0) out vec4 o_Target;

layout(set = 0, binding = 0) uniform Camera {
layout(set = 0, binding = 0) uniform CameraViewProj {
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 •
edited
Loading

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.

mat4 View;
};

layout(set = 1, binding = 0) uniform Lights {
vec3 AmbientColor;
Expand Down
5 changes: 4 additions & 1 deletion crates/bevy_pbr/src/render_graph/forward_pipeline/forward.vert
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,12 @@ layout(location = 0) out vec3 v_Position;
layout(location = 1) out vec3 v_Normal;
layout(location = 2) out vec2 v_Uv;

layout(set = 0, binding = 0) uniform Camera {
layout(set = 0, binding = 0) uniform CameraViewProj {
mat4 ViewProj;
};
layout(set = 0, binding = 1) uniform CameraView {
mat4 View;
};

layout(set = 2, binding = 0) uniform Transform {
mat4 Model;
Expand Down
127 changes: 75 additions & 52 deletions crates/bevy_render/src/render_graph/nodes/camera_node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@ use crate::{
RenderResourceBindings, RenderResourceContext,
},
};
use bevy_core::AsBytes;
use bevy_core::{AsBytes, Byteable, Bytes};
use bevy_ecs::{
system::{BoxedSystem, IntoSystem, Local, Query, Res, ResMut},
world::World,
};

use bevy_transform::prelude::*;
use bevy_utils::HashMap;
use std::borrow::Cow;

#[derive(Debug)]
Expand Down Expand Up @@ -50,9 +52,8 @@ impl SystemNode for CameraNode {
config.0 = Some(CameraNodeState {
camera_name: self.camera_name.clone(),
command_queue: self.command_queue.clone(),
camera_buffer: None,
staging_buffer: None,
})
buffers: HashMap::default(),
});
});
Box::new(system)
}
Expand All @@ -62,8 +63,8 @@ impl SystemNode for CameraNode {
pub struct CameraNodeState {
command_queue: CommandQueue,
camera_name: Cow<'static, str>,
camera_buffer: Option<BufferId>,
staging_buffer: Option<BufferId>,
/// This mas from `binding_name` -> `(staging_buffer_id, buffer_id)`
buffers: HashMap<String, (BufferId, BufferId)>,
}

pub fn camera_node_system(
Expand All @@ -75,63 +76,85 @@ pub fn camera_node_system(
mut render_resource_bindings: ResMut<RenderResourceBindings>,
query: Query<(&Camera, &GlobalTransform)>,
) {
let render_resource_context = &**render_resource_context;
let camera_name = state.camera_name.clone();

let (camera, global_transform) = if let Some(entity) = active_cameras.get(&state.camera_name) {
let (camera, global_transform) = if let Some(entity) = active_cameras.get(&camera_name) {
query.get(entity).unwrap()
} else {
return;
};

let staging_buffer = if let Some(staging_buffer) = state.staging_buffer {
render_resource_context.map_buffer(staging_buffer, BufferMapMode::Write);
staging_buffer
} else {
let size = std::mem::size_of::<[[f32; 4]; 4]>();
let buffer = render_resource_context.create_buffer(BufferInfo {
size,
buffer_usage: BufferUsage::COPY_DST | BufferUsage::UNIFORM,
..Default::default()
});
render_resource_bindings.set(
&state.camera_name,
RenderResourceBinding::Buffer {
buffer,
range: 0..size as u64,
dynamic_index: None,
},
);
state.camera_buffer = Some(buffer);

let staging_buffer = render_resource_context.create_buffer(BufferInfo {
size,
buffer_usage: BufferUsage::COPY_SRC | BufferUsage::MAP_WRITE,
mapped_at_creation: true,
});
let view_matrix = global_transform.compute_matrix().inverse();
let view_proj_matrix = camera.projection_matrix * view_matrix;

state.staging_buffer = Some(staging_buffer);
staging_buffer
};
make_binding(
&mut *state,
&**render_resource_context,
&mut *render_resource_bindings,
&format!("{}ViewProj", camera_name),
view_proj_matrix.to_cols_array(),
);

make_binding(
&mut *state,
&**render_resource_context,
&mut *render_resource_bindings,
&format!("{}View", camera_name),
view_matrix.to_cols_array(),
);
}

fn make_binding<B>(
state: &mut CameraNodeState,
render_resource_context: &dyn RenderResourceContext,
render_resource_bindings: &mut RenderResourceBindings,
binding_name: &str,
bytes: B,
) where
B: Bytes + Byteable,
{
let buffer_size = bytes.byte_len();

let buffers_entry = state.buffers.entry(binding_name.to_owned());

let (staging_buffer, buffer) = buffers_entry
.and_modify(|(staging_buffer, _)| {
render_resource_context.map_buffer(*staging_buffer, BufferMapMode::Write);
})
.or_insert_with(|| {
let buffer = render_resource_context.create_buffer(BufferInfo {
size: buffer_size,
buffer_usage: BufferUsage::COPY_DST | BufferUsage::UNIFORM,
..Default::default()
});

let matrix_size = std::mem::size_of::<[[f32; 4]; 4]>();
let camera_matrix: [f32; 16] =
(camera.projection_matrix * global_transform.compute_matrix().inverse()).to_cols_array();
render_resource_bindings.set(
&binding_name,
RenderResourceBinding::Buffer {
buffer,
range: 0..buffer_size as u64,
dynamic_index: None,
},
);

let staging_buffer = render_resource_context.create_buffer(BufferInfo {
size: buffer_size,
buffer_usage: BufferUsage::COPY_SRC | BufferUsage::MAP_WRITE,
mapped_at_creation: true,
});
(staging_buffer, buffer)
});

render_resource_context.write_mapped_buffer(
staging_buffer,
0..matrix_size as u64,
*staging_buffer,
0..buffer_size as u64,
&mut |data, _renderer| {
data[0..matrix_size].copy_from_slice(camera_matrix.as_bytes());
data[0..buffer_size].copy_from_slice(bytes.as_bytes());
},
);
render_resource_context.unmap_buffer(staging_buffer);

let camera_buffer = state.camera_buffer.unwrap();
state.command_queue.copy_buffer_to_buffer(
staging_buffer,
0,
camera_buffer,
0,
matrix_size as u64,
);
render_resource_context.unmap_buffer(*staging_buffer);

state
.command_queue
.copy_buffer_to_buffer(*staging_buffer, 0, *buffer, 0, buffer_size as u64);
}
66 changes: 50 additions & 16 deletions crates/bevy_render/src/render_graph/nodes/pass_node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,15 +108,30 @@ impl<Q: WorldQuery> PassNode<Q> {

let camera_bind_group_descriptor = BindGroupDescriptor::new(
0,
vec![BindingDescriptor {
name: "Camera".to_string(),
index: 0,
bind_type: BindType::Uniform {
has_dynamic_offset: false,
property: UniformProperty::Struct(vec![UniformProperty::Mat4]),
vec![
BindingDescriptor {
name: "CameraViewProj".to_string(),
index: 0,
bind_type: BindType::Uniform {
has_dynamic_offset: false,
property: UniformProperty::Struct(vec![
UniformProperty::Mat4, // View Projection
]),
},
shader_stage: BindingShaderStage::VERTEX | BindingShaderStage::FRAGMENT,
},
shader_stage: BindingShaderStage::VERTEX | BindingShaderStage::FRAGMENT,
}],
BindingDescriptor {
name: "CameraView".to_string(),
index: 1,
bind_type: BindType::Uniform {
has_dynamic_offset: false,
property: UniformProperty::Struct(vec![
UniformProperty::Mat4, // View
]),
},
shader_stage: BindingShaderStage::VERTEX | BindingShaderStage::FRAGMENT,
},
],
);

PassNode {
Expand Down Expand Up @@ -192,18 +207,37 @@ where
.attachment =
TextureAttachment::Id(input.get(input_index).unwrap().get_texture().unwrap());
}
for camera_info in self.cameras.iter_mut() {
let camera_binding =
if let Some(camera_binding) = render_resource_bindings.get(&camera_info.name) {
camera_binding.clone()
} else {
continue;
};

'outer: for camera_info in self.cameras.iter_mut() {
let camera_name = &camera_info.name;

if render_context
.resources()
.bind_group_descriptor_exists(self.camera_bind_group_descriptor.id)
{
let camera_bind_group = BindGroup::build().add_binding(0, camera_binding).finish();
let mut camera_bind_group_builder = BindGroup::build();

for (index, binding_name) in self
.camera_bind_group_descriptor
.bindings
.iter()
.map(|binding| &binding.name)
.enumerate()
{
let camera_binding = if let Some(camera_binding) = render_resource_bindings.get(
&format!("{}{}", &camera_name, binding_name.replace("Camera", "")),
) {
camera_binding.clone()
} else {
continue 'outer;
};

camera_bind_group_builder =
camera_bind_group_builder.add_binding(index as u32, camera_binding);
}

let camera_bind_group = camera_bind_group_builder.finish();

render_context
.resources()
.create_bind_group(self.camera_bind_group_descriptor.id, &camera_bind_group);
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_render/src/shader/shader_reflect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ fn reflect_binding(

let name = name.to_string();

if name == "Camera" {
if let Some(0usize) = name.find("Camera") {
shader_stage = BindingShaderStage::VERTEX | BindingShaderStage::FRAGMENT;
}

Expand Down Expand Up @@ -325,7 +325,7 @@ mod tests {
layout(location = 2) in uvec4 I_TestInstancing_Property;

layout(location = 0) out vec4 v_Position;
layout(set = 0, binding = 0) uniform Camera {
layout(set = 0, binding = 0) uniform CameraViewProj {
mat4 ViewProj;
};
layout(set = 1, binding = 0) uniform texture2D Texture;
Expand Down Expand Up @@ -381,7 +381,7 @@ mod tests {
0,
vec![BindingDescriptor {
index: 0,
name: "Camera".into(),
name: "CameraViewProj".into(),
bind_type: BindType::Uniform {
has_dynamic_offset: false,
property: UniformProperty::Struct(vec![UniformProperty::Mat4]),
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_render/src/wireframe/wireframe.vert
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

layout(location = 0) in vec3 Vertex_Position;

layout(set = 0, binding = 0) uniform Camera {
layout(set = 0, binding = 0) uniform CameraViewProj {
mat4 ViewProj;
};

Expand Down
7 changes: 4 additions & 3 deletions crates/bevy_sprite/src/render/sprite.vert
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

layout(set = 2, binding = 0) uniform Transform {
Expand All @@ -25,7 +26,7 @@ void main() {

uint x_flip_bit = 1; // The X flip bit
uint y_flip_bit = 2; // The Y flip bit

// Note: Here we subtract f32::EPSILON from the flipped UV coord. This is due to reasons unknown
// to me (@zicklag ) that causes the uv's to be slightly offset and causes over/under running of
// the sprite UV sampling which is visible when resizing the screen.
Expand All @@ -41,4 +42,4 @@ void main() {

vec3 position = Vertex_Position * vec3(size, 1.0);
gl_Position = ViewProj * Model * vec4(position, 1.0);
}
}
9 changes: 4 additions & 5 deletions crates/bevy_sprite/src/render/sprite_sheet.vert
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ layout(location = 2) in vec2 Vertex_Uv;
layout(location = 0) out vec2 v_Uv;
layout(location = 1) out vec4 v_Color;

layout(set = 0, binding = 0) uniform Camera {
layout(set = 0, binding = 0) uniform CameraViewProj {
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

};

// TODO: merge dimensions into "sprites" buffer when that is supported in the Uniforms derive abstraction
Expand All @@ -25,7 +26,6 @@ layout(set = 1, binding = 1) buffer TextureAtlas_textures {
Rect[] Textures;
};


layout(set = 2, binding = 0) uniform Transform {
mat4 SpriteTransform;
};
Expand Down Expand Up @@ -75,11 +75,10 @@ void main() {
bottom_left,
top_left,
top_right,
bottom_right
);
bottom_right);

v_Uv = (atlas_positions[gl_VertexIndex]) / AtlasSize;

v_Color = color;
gl_Position = ViewProj * SpriteTransform * vec4(vertex_position, 1.0);
}
}
Loading