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

invert face culling for negatively scaled gltf nodes #8859

Merged
merged 6 commits into from
Sep 18, 2023
Merged

invert face culling for negatively scaled gltf nodes #8859

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9a461df
invert face culling for negative scale nodes
robtfm Jun 16, 2023
a81084e
use global scale
robtfm Jun 16, 2023
f32b0a7
Merge remote-tracking branch 'upstream/main' into pr/robtfm/8859
robtfm Sep 13, 2023
6771a0f
fix merge
robtfm Sep 13, 2023
36dca92
add comment
robtfm Sep 14, 2023
2645d81
Update crates/bevy_gltf/src/loader.rs
robtfm Sep 17, 2023
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
35 changes: 28 additions & 7 deletions crates/bevy_gltf/src/loader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ async fn load_gltf<'a, 'b, 'c>(
let mut named_materials = HashMap::default();
// NOTE: materials must be loaded after textures because image load() calls will happen before load_with_settings, preventing is_srgb from being set properly
for material in gltf.materials() {
let handle = load_material(&material, load_context);
let handle = load_material(&material, load_context, false);
if let Some(name) = material.name() {
named_materials.insert(name.to_string(), handle.clone());
}
Expand Down Expand Up @@ -504,6 +504,7 @@ async fn load_gltf<'a, 'b, 'c>(
&mut node_index_to_entity_map,
&mut entity_to_skin_index_map,
&mut active_camera_found,
&Transform::default(),
);
if result.is_err() {
err = Some(result);
Expand Down Expand Up @@ -665,8 +666,12 @@ async fn load_image<'a, 'b>(
}

/// Loads a glTF material as a bevy [`StandardMaterial`] and returns it.
fn load_material(material: &Material, load_context: &mut LoadContext) -> Handle<StandardMaterial> {
let material_label = material_label(material);
fn load_material(
material: &Material,
load_context: &mut LoadContext,
is_scale_inverted: bool,
) -> Handle<StandardMaterial> {
let material_label = material_label(material, is_scale_inverted);
load_context.labeled_asset_scope(material_label, |load_context| {
let pbr = material.pbr_metallic_roughness();

Expand Down Expand Up @@ -712,6 +717,8 @@ fn load_material(material: &Material, load_context: &mut LoadContext) -> Handle<
double_sided: material.double_sided(),
cull_mode: if material.double_sided() {
None
} else if is_scale_inverted {
Some(Face::Front)
} else {
Some(Face::Back)
},
Expand All @@ -734,10 +741,19 @@ fn load_node(
node_index_to_entity_map: &mut HashMap<usize, Entity>,
entity_to_skin_index_map: &mut HashMap<Entity, usize>,
active_camera_found: &mut bool,
parent_transform: &Transform,
) -> Result<(), GltfError> {
let transform = gltf_node.transform();
let mut gltf_error = None;
let transform = Transform::from_matrix(Mat4::from_cols_array_2d(&transform.matrix()));
let world_transform = *parent_transform * transform;
Copy link
Contributor

Choose a reason for hiding this comment

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

Transform * Transform is a lossy operation. I'm concerned the determinant might end up incorrect in some transform hierarchies.
I suggest passing a GlobalTransform around instead to be certain.

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'm happy to do this. it'll be slower, but this isn't a hot code path.

in the gltf spec it says that you can either use a translation + scale + rotation, or a matrix which must decompose into those parts, and explicitly cannot skew or shear.

since we only care about the scale, and the scales are just Vec3-multiplied at each step, i'm pretty sure this is safe. i guess i could pass the scale down as a Vec3 instead of the whole transform if that would be clearer / avoid potential future misuse.

Copy link
Contributor

Choose a reason for hiding this comment

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

in the gltf spec it says that you can either use a translation + scale + rotation, or a matrix which must decompose into those parts, and explicitly cannot skew or shear.

I think that implementation note is referring purely to the local transform stored in the gltf format, but the note is unfortunately not making that very clear.

since we only care about the scale, and the scales are just Vec3-multiplied at each step, i'm pretty sure this is safe.

I don't have the math intuition to know whether the loss of skew and shear still keeps the determinant the same 🤷

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 that implementation note is referring purely to the local transform stored in the gltf format, but the note is unfortunately not making that very clear.

yes, but that's all we're dealing with here. at this point the gltf isn't part of the world heirarchy, we just initialize the transform to IDENTITY at the gltf root node.

// according to https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#instantiation,
// if the determinant of the transform is negative we must invert the winding order of
// triangles in meshes on the node.
// instead we equivalently test if the global scale is inverted by checking if the number
// of negative scale factors is odd. if so we will assign a copy of the material with face
// culling inverted, rather than modifying the mesh data directly.
let is_scale_inverted = world_transform.scale.is_negative_bitmask().count_ones() & 1 == 1;
let mut node = world_builder.spawn(SpatialBundle::from(transform));

node.insert(node_name(gltf_node));
Expand Down Expand Up @@ -812,13 +828,14 @@ fn load_node(
// append primitives
for primitive in mesh.primitives() {
let material = primitive.material();
let material_label = material_label(&material);
let material_label = material_label(&material, is_scale_inverted);

// This will make sure we load the default material now since it would not have been
// added when iterating over all the gltf materials (since the default material is
// not explicitly listed in the gltf).
// It also ensures an inverted scale copy is instantiated if required.
if !load_context.has_labeled_asset(&material_label) {
load_material(&material, load_context);
load_material(&material, load_context, is_scale_inverted);
}

let primitive_label = primitive_label(&mesh, &primitive);
Expand Down Expand Up @@ -948,6 +965,7 @@ fn load_node(
node_index_to_entity_map,
entity_to_skin_index_map,
active_camera_found,
&world_transform,
) {
gltf_error = Some(err);
return;
Expand Down Expand Up @@ -990,9 +1008,12 @@ fn morph_targets_label(mesh: &gltf::Mesh, primitive: &Primitive) -> String {
}

/// Returns the label for the `material`.
fn material_label(material: &gltf::Material) -> String {
fn material_label(material: &gltf::Material, is_scale_inverted: bool) -> String {
if let Some(index) = material.index() {
format!("Material{index}")
format!(
"Material{index}{}",
if is_scale_inverted { " (inverted)" } else { "" }
)
} else {
"MaterialDefault".to_string()
}
Expand Down