Skip to content

[Merged by Bors] - calculate flat normals for mesh if missing #1808

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.

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

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 1 addition & 0 deletions crates/bevy_gltf/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ bevy_render = { path = "../bevy_render", version = "0.4.0" }
bevy_transform = { path = "../bevy_transform", version = "0.4.0" }
bevy_math = { path = "../bevy_math", version = "0.4.0" }
bevy_scene = { path = "../bevy_scene", version = "0.4.0" }
bevy_log = { path = "../bevy_log", version = "0.4.0" }

# other
gltf = { version = "0.15.2", default-features = false, features = ["utils", "names", "KHR_materials_unlit"] }
Expand Down
11 changes: 11 additions & 0 deletions crates/bevy_gltf/src/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,17 @@ async fn load_gltf<'a, 'b>(
mesh.set_indices(Some(Indices::U32(indices.into_u32().collect())));
};

if mesh.attribute(Mesh::ATTRIBUTE_NORMAL).is_none() {
let vertex_count_before = mesh.count_vertices();
mesh.duplicate_vertices();
mesh.compute_flat_normals();
let vertex_count_after = mesh.count_vertices();

if vertex_count_before != vertex_count_after {
bevy_log::warn!("Missing vertex normals in indexed geometry, computing them as flat. Vertex count increased from {} to {}", vertex_count_before, vertex_count_after);
Copy link
Member

Choose a reason for hiding this comment

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

Calculating normals is a "feature" users might want to use. Warnings must be actionable, but users might not have the ability or inclination to add normals to a model (ex: they bought it on an asset store).

Additionally, some meshes / shaders, might not need normals, and calculating them could be "wasted effort". This is also true for #1803.

I think ideally we detect these cases only when they are relevant (ex: a mesh is missing normals and it is used by a shader that needs normals) and we warn or fail. Then users could somehow opt-in to "mesh transformations" like generating zero-ed out buffers or computing normals. The error/warning could provide a list of ways to resolve the error (ex: fix the mesh manually or opt in to a mesh transformation).

Opting in to these transforms would ideally be a part of the "asset configuration", but bevy_asset currently doesn't support asset config. Distill does (via .meta files), so long term I think thats our best bet.

Its hard to say what we should do in the short term. I'm curious to hear other peoples thoughts. Some ideas:

  1. Just warn like we do here and accept that there will be un-actionable warnings for some valid use cases (and open an issue calling this out so we dont forget).
  2. Use something like add a method on AssetServer to create an asset from an existing one #1665 to allow users to apply the transformation manually to suppress the warning.
  3. Add "asset config" to the current bevy_asset.

}
Copy link
Member

@mockersf mockersf Apr 11, 2021

Choose a reason for hiding this comment

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

we should log something even if there wasn't a vertex count change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}

let mesh = load_context.set_labeled_asset(&primitive_label, LoadedAsset::new(mesh));
primitives.push(super::GltfPrimitive {
mesh,
Expand Down
97 changes: 97 additions & 0 deletions crates/bevy_render/src/mesh/mesh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,13 @@ impl VertexAttributeValues {
self.len() == 0
}

fn as_float3(&self) -> Option<&[[f32; 3]]> {
match self {
VertexAttributeValues::Float3(values) => Some(values),
_ => None,
}
}

// TODO: add vertex format as parameter here and perform type conversions
/// Flattens the VertexAttributeArray into a sequence of bytes. This is
/// useful for serialization and sending to the GPU.
Expand Down Expand Up @@ -194,6 +201,29 @@ pub enum Indices {
U32(Vec<u32>),
}

impl Indices {
fn iter(&self) -> impl Iterator<Item = usize> + '_ {
match self {
Indices::U16(vec) => IndicesIter::U16(vec.iter()),
Indices::U32(vec) => IndicesIter::U32(vec.iter()),
}
}
}
enum IndicesIter<'a> {
U16(std::slice::Iter<'a, u16>),
U32(std::slice::Iter<'a, u32>),
}
impl Iterator for IndicesIter<'_> {
type Item = usize;

fn next(&mut self) -> Option<Self::Item> {
match self {
IndicesIter::U16(iter) => iter.next().map(|val| *val as usize),
IndicesIter::U32(iter) => iter.next().map(|val| *val as usize),
}
}
}

impl From<&Indices> for IndexFormat {
fn from(indices: &Indices) -> Self {
match indices {
Expand Down Expand Up @@ -369,6 +399,73 @@ impl Mesh {

attributes_interleaved_buffer
}

/// Duplicates the vertex attributes so that no vertices are shared.
///
/// This can dramatically increase the vertex count, so make sure this is what you want.
/// Does nothing if no [Indices] are set.
pub fn duplicate_vertices(&mut self) {
fn duplicate<T: Copy>(values: &[T], indices: impl Iterator<Item = usize>) -> Vec<T> {
indices.map(|i| values[i]).collect()
}

assert!(
matches!(self.primitive_topology, PrimitiveTopology::TriangleList),
"can only duplicate vertices for `TriangleList`s"
);

let indices = match self.indices.take() {
Some(indices) => indices,
None => return,
};
for (_, attributes) in self.attributes.iter_mut() {
let indices = indices.iter();
match attributes {
VertexAttributeValues::Float(vec) => *vec = duplicate(&vec, indices),
VertexAttributeValues::Int(vec) => *vec = duplicate(&vec, indices),
VertexAttributeValues::Uint(vec) => *vec = duplicate(&vec, indices),
VertexAttributeValues::Float2(vec) => *vec = duplicate(&vec, indices),
VertexAttributeValues::Int2(vec) => *vec = duplicate(&vec, indices),
VertexAttributeValues::Uint2(vec) => *vec = duplicate(&vec, indices),
VertexAttributeValues::Float3(vec) => *vec = duplicate(&vec, indices),
VertexAttributeValues::Int3(vec) => *vec = duplicate(&vec, indices),
VertexAttributeValues::Uint3(vec) => *vec = duplicate(&vec, indices),
VertexAttributeValues::Int4(vec) => *vec = duplicate(&vec, indices),
VertexAttributeValues::Uint4(vec) => *vec = duplicate(&vec, indices),
VertexAttributeValues::Float4(vec) => *vec = duplicate(&vec, indices),
VertexAttributeValues::Uchar4Norm(vec) => *vec = duplicate(&vec, indices),
}
}
}

/// Calculates the [`Mesh::ATTRIBUTE_NORMAL`] of a mesh.
///
/// Panics if [`Indices`] are set.
/// Consider calling [Mesh::duplicate_vertices] or export your mesh with normal attributes.
pub fn compute_flat_normals(&mut self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Normals should also be normalized/unit length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, normals should be actually normal 😄

if self.indices().is_some() {
panic!("`compute_flat_normals` can't work on indexed geometry. Consider calling `Mesh::duplicate_vertices`.");
}

let positions = self
.attribute(Mesh::ATTRIBUTE_POSITION)
.unwrap()
.as_float3()
.expect("`Mesh::ATTRIBUTE_POSITION` vertex attributes should be of type `float3`");

let normals: Vec<_> = positions
.chunks_exact(3)
.map(|p| face_normal(p[0], p[1], p[2]))
.flat_map(|normal| std::array::IntoIter::new([normal, normal, normal]))
.collect();

self.set_attribute(Mesh::ATTRIBUTE_NORMAL, normals);
}
}

fn face_normal(a: [f32; 3], b: [f32; 3], c: [f32; 3]) -> [f32; 3] {
let (a, b, c) = (Vec3::from(a), Vec3::from(b), Vec3::from(c));
(b - a).cross(c - a).normalize().into()
}

fn remove_resource_save(
Expand Down