From c78412a2ca8a6ca2a49429df4290519b75280bed Mon Sep 17 00:00:00 2001 From: Robert Swain Date: Sat, 13 Aug 2022 09:27:00 +0200 Subject: [PATCH 1/3] bevy_pbr: Fix tangent and normal normalization Morten Mikkelsen clarified that the world normal and tangent must be normalized in the vertex stage and the interpolated values must not be normalized in the fragment stage. --- .../bevy_pbr/src/render/mesh_functions.wgsl | 24 +++++++++++-------- crates/bevy_pbr/src/render/pbr_functions.wgsl | 4 ++-- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/crates/bevy_pbr/src/render/mesh_functions.wgsl b/crates/bevy_pbr/src/render/mesh_functions.wgsl index 20c763bd22d79..ee5f160549501 100644 --- a/crates/bevy_pbr/src/render/mesh_functions.wgsl +++ b/crates/bevy_pbr/src/render/mesh_functions.wgsl @@ -17,20 +17,24 @@ fn mesh_position_local_to_clip(model: mat4x4, vertex_position: vec4) - } fn mesh_normal_local_to_world(vertex_normal: vec3) -> vec3 { - return mat3x3( - mesh.inverse_transpose_model[0].xyz, - mesh.inverse_transpose_model[1].xyz, - mesh.inverse_transpose_model[2].xyz - ) * vertex_normal; + return normalize( + mat3x3( + 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, vertex_tangent: vec4) -> vec4 { return vec4( - mat3x3( - model[0].xyz, - model[1].xyz, - model[2].xyz - ) * vertex_tangent.xyz, + normalize( + mat3x3( + model[0].xyz, + model[1].xyz, + model[2].xyz + ) * vertex_tangent.xyz + ), vertex_tangent.w ); } diff --git a/crates/bevy_pbr/src/render/pbr_functions.wgsl b/crates/bevy_pbr/src/render/pbr_functions.wgsl index cd922bf74412c..5db9b61140ddc 100644 --- a/crates/bevy_pbr/src/render/pbr_functions.wgsl +++ b/crates/bevy_pbr/src/render/pbr_functions.wgsl @@ -15,7 +15,7 @@ fn prepare_normal( #endif is_front: bool, ) -> vec3 { - var N: vec3 = normalize(world_normal); + var N: vec3 = world_normal; #ifdef VERTEX_TANGENTS #ifdef STANDARDMATERIAL_NORMAL_MAP @@ -236,7 +236,7 @@ fn pbr( fn tone_mapping(in: vec4) -> vec4 { // tone_mapping return vec4(reinhard_luminance(in.rgb), in.a); - + // Gamma correction. // Not needed with sRGB buffer // output_color.rgb = pow(output_color.rgb, vec3(1.0 / 2.2)); From d3f69f46e4e94a62a926006d3dbaa1af510da54c Mon Sep 17 00:00:00 2001 From: Robert Swain Date: Sat, 13 Aug 2022 12:40:15 +0200 Subject: [PATCH 2/3] bevy_pbr: Add comments clarifying why tangent/normals are processed this way --- crates/bevy_pbr/src/render/mesh_functions.wgsl | 12 ++++++++++++ crates/bevy_pbr/src/render/pbr_functions.wgsl | 6 ++++++ 2 files changed, 18 insertions(+) diff --git a/crates/bevy_pbr/src/render/mesh_functions.wgsl b/crates/bevy_pbr/src/render/mesh_functions.wgsl index ee5f160549501..6c50c7369a4de 100644 --- a/crates/bevy_pbr/src/render/mesh_functions.wgsl +++ b/crates/bevy_pbr/src/render/mesh_functions.wgsl @@ -17,6 +17,12 @@ fn mesh_position_local_to_clip(model: mat4x4, vertex_position: vec4) - } fn mesh_normal_local_to_world(vertex_normal: vec3) -> vec3 { + // NOTE: The mikktspace method of normal mapping requires that the world normal is + // re-normalized in the vertex shader to match the way mikktspace bakes vertex tangents + // and normal maps so that the exact inverse process is applied when shading. Blender, Unity, + // Unreal Engine, Godot, and more all use the mikktspace method. Do not change this code + // unless you really know what you are doing. + // http://www.mikktspace.com/ return normalize( mat3x3( mesh.inverse_transpose_model[0].xyz, @@ -27,6 +33,12 @@ fn mesh_normal_local_to_world(vertex_normal: vec3) -> vec3 { } fn mesh_tangent_local_to_world(model: mat4x4, vertex_tangent: vec4) -> vec4 { + // NOTE: The mikktspace method of normal mapping requires that the world tangent is + // re-normalized in the vertex shader to match the way mikktspace bakes vertex tangents + // and normal maps so that the exact inverse process is applied when shading. Blender, Unity, + // Unreal Engine, Godot, and more all use the mikktspace method. Do not change this code + // unless you really know what you are doing. + // http://www.mikktspace.com/ return vec4( normalize( mat3x3( diff --git a/crates/bevy_pbr/src/render/pbr_functions.wgsl b/crates/bevy_pbr/src/render/pbr_functions.wgsl index 5db9b61140ddc..a77c065fca221 100644 --- a/crates/bevy_pbr/src/render/pbr_functions.wgsl +++ b/crates/bevy_pbr/src/render/pbr_functions.wgsl @@ -15,6 +15,12 @@ fn prepare_normal( #endif is_front: bool, ) -> vec3 { + // NOTE: The mikktspace method of normal mapping explicitly requires that the world normal NOT + // be re-normalized in the fragment shader. This is primarily to match the way mikktspace + // bakes vertex tangents and normal maps so that this is the exact inverse. Blender, Unity, + // Unreal Engine, Godot, and more all use the mikktspace method. Do not change this code + // unless you really know what you are doing. + // http://www.mikktspace.com/ var N: vec3 = world_normal; #ifdef VERTEX_TANGENTS From 7ef6eb672ee5df696f5ed4ab36993325019dfacf Mon Sep 17 00:00:00 2001 From: Robert Swain Date: Wed, 17 Aug 2022 23:00:41 +0200 Subject: [PATCH 3/3] bevy_pbr: Account for things like negative scaling If the sign of the determinant of the 3x3 model matrix is negative, this must be applied when calculating the bitangent from the cross-product of the interpolated world space normal and interpolated world space tangent. This information came from Morten Mikkelsen and can be understood from section 2 of the following paper, though I do not understand it well enough to be able to explain it: https://jcgt.org/published/0009/03/04/ --- crates/bevy_pbr/src/render/mesh.rs | 16 +++++++++++----- crates/bevy_pbr/src/render/mesh_functions.wgsl | 13 ++++++++++++- crates/bevy_pbr/src/render/mesh_types.wgsl | 2 ++ 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/crates/bevy_pbr/src/render/mesh.rs b/crates/bevy_pbr/src/render/mesh.rs index 082e20b733585..3d70cfd110eb5 100644 --- a/crates/bevy_pbr/src/render/mesh.rs +++ b/crates/bevy_pbr/src/render/mesh.rs @@ -9,7 +9,7 @@ use bevy_ecs::{ prelude::*, system::{lifetimeless::*, SystemParamItem, SystemState}, }; -use bevy_math::{Mat4, Vec2}; +use bevy_math::{Mat3A, Mat4, Vec2}; use bevy_reflect::TypeUuid; use bevy_render::{ extract_component::{ComponentUniforms, DynamicUniformIndex, UniformComponentPlugin}, @@ -117,6 +117,9 @@ bitflags::bitflags! { #[repr(transparent)] struct MeshFlags: u32 { const SHADOW_RECEIVER = (1 << 0); + // Indicates the sign of the determinant of the 3x3 model matrix. If the sign is positive, + // then the flag should be set, else it should not be set. + const SIGN_DETERMINANT_MODEL_3X3 = (1 << 31); const NONE = 0; const UNINITIALIZED = 0xFFFF; } @@ -143,13 +146,16 @@ pub fn extract_meshes( for (entity, _, transform, handle, not_receiver, not_caster) in visible_meshes { let transform = transform.compute_matrix(); - let shadow_receiver_flags = if not_receiver.is_some() { - MeshFlags::empty().bits + let mut flags = if not_receiver.is_some() { + MeshFlags::empty() } else { - MeshFlags::SHADOW_RECEIVER.bits + MeshFlags::SHADOW_RECEIVER }; + if Mat3A::from_mat4(transform).determinant().is_sign_positive() { + flags |= MeshFlags::SIGN_DETERMINANT_MODEL_3X3; + } let uniform = MeshUniform { - flags: shadow_receiver_flags, + flags: flags.bits, transform, inverse_transpose_model: transform.inverse().transpose(), }; diff --git a/crates/bevy_pbr/src/render/mesh_functions.wgsl b/crates/bevy_pbr/src/render/mesh_functions.wgsl index 6c50c7369a4de..40779b28d5c6e 100644 --- a/crates/bevy_pbr/src/render/mesh_functions.wgsl +++ b/crates/bevy_pbr/src/render/mesh_functions.wgsl @@ -32,6 +32,15 @@ fn mesh_normal_local_to_world(vertex_normal: vec3) -> vec3 { ); } +// Calculates the sign of the determinant of the 3x3 model matrix based on a +// mesh flag +fn sign_determinant_model_3x3() -> f32 { + // bool(u32) is false if 0u else true + // f32(bool) is 1.0 if true else 0.0 + // * 2.0 - 1.0 remaps 0.0 or 1.0 to -1.0 or 1.0 respectively + return f32(bool(mesh.flags & MESH_FLAGS_SIGN_DETERMINANT_MODEL_3X3_BIT)) * 2.0 - 1.0; +} + fn mesh_tangent_local_to_world(model: mat4x4, vertex_tangent: vec4) -> vec4 { // NOTE: The mikktspace method of normal mapping requires that the world tangent is // re-normalized in the vertex shader to match the way mikktspace bakes vertex tangents @@ -47,6 +56,8 @@ fn mesh_tangent_local_to_world(model: mat4x4, vertex_tangent: vec4) -> model[2].xyz ) * vertex_tangent.xyz ), - vertex_tangent.w + // NOTE: Multiplying by the sign of the determinant of the 3x3 model matrix accounts for + // situations such as negative scaling. + vertex_tangent.w * sign_determinant_model_3x3() ); } diff --git a/crates/bevy_pbr/src/render/mesh_types.wgsl b/crates/bevy_pbr/src/render/mesh_types.wgsl index e0bfd2c2899cd..d44adbc2bb13f 100644 --- a/crates/bevy_pbr/src/render/mesh_types.wgsl +++ b/crates/bevy_pbr/src/render/mesh_types.wgsl @@ -14,3 +14,5 @@ struct SkinnedMesh { #endif let MESH_FLAGS_SHADOW_RECEIVER_BIT: u32 = 1u; +// 2^31 - if the flag is set, the sign is positive, else it is negative +let MESH_FLAGS_SIGN_DETERMINANT_MODEL_3X3_BIT: u32 = 2147483648u;