[Merged by Bors] - Add reusable shader functions for transforming position/normal/tangent #4901
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| #define_import_path bevy_pbr::mesh_functions | ||
|
|
||
| fn mesh_position_local_to_world(model: mat4x4<f32>, vertex_position: vec4<f32>) -> vec4<f32> { | ||
|
There was a problem hiding this comment. I'm pretty sure I've endorsed this idea in the past, but having thought about it a bit more, I think I'd like to push back on this general pattern a bit. Especially for the functions that just alias multiplication. Is introducing special-case methods for those operations "worth it"? On the one hand, they give descriptive (and consistent) names to the operations (mesh_position_local_to_world, mesh_position_world_to_clip), etc. On the other hand, teaching people to use this: Feels wrong to me. It is abstracting out a fundamental concept (vec/matrix multiplication) in favor of a custom case-specific concept. Worse, that case-specific concept / naming doesn't hold if you pass a non-model matrix in (or non mesh position). And on top of that, it requires you to pull in an import that you might not otherwise need. I think we should only provide helper functions that actually reduce boilerplate and/or fully abstract out concepts:
There was a problem hiding this comment. Note a complete response, just a quick one. The value I see for the local to world case is that it creates consistency and mental clarity that you’re doing it in the right way according to the engine regardless of whether your calculating the world or clip space versions. You could argue that someone should know that projection * inverse view * model * local position = clip position, and I would agree. But when doing a depth prepass (which I am expecting we will at some point) and using the equal depth comparison function in the main pass, float precision of the calculations will matter and I wouldn’t expect someone to know that. And then if you don’t have a function for local to world but do for local to clip, then there’s some inconsistency in the story. There’s nothing stopping people just using the matrices in the bindings, but if code examples use these functions then people will be encouraged to use them. When I move the mesh uniform into an instance buffer for performance reasons, you anyway won’t be able to just refer to mesh.model and will need to reconstruct the model matrix. Maybe we can use some ‘global variables’ to cache calculation of matrices somehow so that you only pass in the vector to be transformed…? I don’t know if wgsl would allow that. There was a problem hiding this comment. If we actually abstracted out the mesh matrix (enabling things like abstracting out the instance buffer access in the future) id be more convinced. I still think as it stands the "consistency" of There was a problem hiding this comment. Just verified that this is possible: It would require calling Another downside is that this requires doing the There was a problem hiding this comment. We could also do lazy calculations with a second There was a problem hiding this comment. We could also use the godot approach: treat user-defined vertex + fragment functions as "internal": [[stage(vertex)]]
fn internal_vertex(vertex: Vertex) -> VertexOutput {
/* do setup work here, such as setting MESH_MODEL */
return vertex(vertex);
}
// user defines this, which will implicitly get injected in to a template containing `internal_vertex` / whatever else is needed
fn vertex(vertex: Vertex) -> VertexOutput {
// user is free to assume MESH_MODEL is already valid and
// call things like mesh_position_local_to_world(vertex.position)
}Definitely more abstraction (and more "fixed"). Just throwing out options to get the juices flowing. But I still feel relatively strongly about the core principles outlined above in the bullet points of this message: #4901 (comment) There was a problem hiding this comment. It maybe doesn’t need to be global. But I don’t know if you like that. I was going to try a ptr with function scope. |
||
| return model * vertex_position; | ||
| } | ||
|
|
||
| fn mesh_position_world_to_clip(world_position: vec4<f32>) -> vec4<f32> { | ||
| return view.view_proj * world_position; | ||
| } | ||
|
|
||
| // NOTE: The intermediate world_position assignment is important | ||
| // for precision purposes when using the 'equals' depth comparison | ||
| // function. | ||
| fn mesh_position_local_to_clip(model: mat4x4<f32>, vertex_position: vec4<f32>) -> vec4<f32> { | ||
| let world_position = mesh_position_local_to_world(model, vertex_position); | ||
| return mesh_position_world_to_clip(world_position); | ||
| } | ||
|
|
||
| fn mesh_normal_local_to_world(vertex_normal: vec3<f32>) -> vec3<f32> { | ||
| return mat3x3<f32>( | ||
| mesh.inverse_transpose_model[0].xyz, | ||
| mesh.inverse_transpose_model[1].xyz, | ||
| mesh.inverse_transpose_model[2].xyz | ||
| ) * vertex_normal; | ||
| } | ||
|
|
||
| fn mesh_tangent_local_to_world(model: mat4x4<f32>, vertex_tangent: vec4<f32>) -> vec4<f32> { | ||
| return vec4<f32>( | ||
| mat3x3<f32>( | ||
| model[0].xyz, | ||
| model[1].xyz, | ||
| model[2].xyz | ||
| ) * vertex_tangent.xyz, | ||
| vertex_tangent.w | ||
| ); | ||
| } | ||
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.
nit /curious: What is the reasoning for this import statement to not be at the top of the file like the rest?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.
In my mind, the comment is directly about the position of this one import in relation to the others and the types and bindings in the file. As such, it made sense to me to add the note on it.
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.
Yes the comment I get. To clarify, why this:
Instead of this:
(coming from other languages, used to seeing all import statements at the top of the file, so I'm trying to understand the norms and patterns at play)
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.
That’s what the comment is supposed to explain. Types and bindings must come before functions that use them. The import statement that the comment is about is importing ‘bevy_pbr::mesh_functions’ which contains a bunch of helper functions and those functions refer to members of the view and mesh bindings.
If you understand ^ that explanation, do you have a suggestion for what could make the comment clearer?
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.
I think your comment is both clear and understandable :) Placing the comment + import statement and the usage of the function makes completely sense in this case.
This is more me trying to understand wgsl (new to me). I was curious that perhaps there was something in wgsl that required the import statement to placed close to where it was going to be used, but I understand now that as long as it is placed before, it could be placed anywhere.