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

Add Mesh::merge #11456

Merged
merged 5 commits into from
Feb 12, 2024
Merged

Add Mesh::merge #11456

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cb2d848
Add `Mesh::merge`
Jondolf Jan 21, 2024
9ade3ee
Clean up match arm formatting
Jondolf Jan 21, 2024
c9bf410
Use vertex position count for index offset, document attribute mismatch
Jondolf Jan 21, 2024
4969e2e
Merge branch 'main' into merge-mesh
Jondolf Feb 12, 2024
3749ca8
Fix missing closing delimiters
Jondolf Feb 12, 2024
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
84 changes: 84 additions & 0 deletions crates/bevy_render/src/mesh/mesh/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,90 @@ impl Mesh {
Ok(self)
}

/// Merges the [`Mesh`] data of `other` with `self`. The attributes and indices of `other` will be appended to `self`.
///
/// # Panics
///
/// Panics if the vertex attribute values of `other` are incompatible with `self`.
/// For example, [`VertexAttributeValues::Float32`] is incompatible with [`VertexAttributeValues::Float32x3`].
pub fn merge(&mut self, other: Mesh) {
use VertexAttributeValues::*;

// The indices of `other` should start after the last vertex of `self`.
let index_offset = self.count_vertices();
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that (from count_vertices's documentation):

If the attributes have different vertex counts, the smallest is returned.

So it may be silently wrong (for your usecase) if that's the case. Might also be something to document.

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 changed it to just use the vertex position attribute length for now, although I'm not entirely sure if that's fully correct either


// Extend attributes of `self` with attributes of `other`.
for (id, values) in self.attributes_mut() {
let enum_variant_name = values.enum_variant_name();
if let Some(other_values) = other.attribute(id) {
match (values, other_values) {
Comment on lines +613 to +615
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can see, if self and other's attributes don't match up:

  • if self has an attribute but other doesn't, it is kept (note that this means future's self.count_vertices() will report the old vertices count);
  • if other has an attribute but self doesn't, it is silently ignored.

Which could be kinda surprising. I don't see much better ways to handle this (other than error), in both cases it should be documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I just added a short mention in the doc comment that attributes of other that don't exist on self will be ignored

(Float32(vec1), Float32(vec2)) => vec1.extend(vec2),
(Sint32(vec1), Sint32(vec2)) => vec1.extend(vec2),
(Uint32(vec1), Uint32(vec2)) => vec1.extend(vec2),
(Float32x2(vec1), Float32x2(vec2)) => vec1.extend(vec2),
(Sint32x2(vec1), Sint32x2(vec2)) => vec1.extend(vec2),
(Uint32x2(vec1), Uint32x2(vec2)) => vec1.extend(vec2),
(Float32x3(vec1), Float32x3(vec2)) => vec1.extend(vec2),
(Sint32x3(vec1), Sint32x3(vec2)) => vec1.extend(vec2),
(Uint32x3(vec1), Uint32x3(vec2)) => vec1.extend(vec2),
(Sint32x4(vec1), Sint32x4(vec2)) => vec1.extend(vec2),
(Uint32x4(vec1), Uint32x4(vec2)) => vec1.extend(vec2),
(Float32x4(vec1), Float32x4(vec2)) => vec1.extend(vec2),
(Sint16x2(vec1), Sint16x2(vec2)) | (Snorm16x2(vec1), Snorm16x2(vec2)) => {
vec1.extend(vec2);
}
(Uint16x2(vec1), Uint16x2(vec2)) | (Unorm16x2(vec1), Unorm16x2(vec2)) => {
vec1.extend(vec2);
}
(Sint16x4(vec1), Sint16x4(vec2)) | (Snorm16x4(vec1), Snorm16x4(vec2)) => {
vec1.extend(vec2);
}
(Uint16x4(vec1), Uint16x4(vec2)) | (Unorm16x4(vec1), Unorm16x4(vec2)) => {
vec1.extend(vec2);
}
(Sint8x2(vec1), Sint8x2(vec2)) | (Snorm8x2(vec1), Snorm8x2(vec2)) => {
vec1.extend(vec2);
}
Jondolf marked this conversation as resolved.
Show resolved Hide resolved
(Uint8x2(vec1), Uint8x2(vec2)) => {
vec1.extend(vec2);
}
(Unorm8x2(vec1), Unorm8x2(vec2)) => vec1.extend(vec2),
(Sint8x4(vec1), Sint8x4(vec2)) => {
vec1.extend(vec2);
}
(Snorm8x4(vec1), Snorm8x4(vec2)) => vec1.extend(vec2),
(Uint8x4(vec1), Uint8x4(vec2)) => {
vec1.extend(vec2);
}
Jondolf marked this conversation as resolved.
Show resolved Hide resolved
(Unorm8x4(vec1), Unorm8x4(vec2)) => vec1.extend(vec2),
_ => panic!(
"Incompatible vertex attribute types {} and {}",
enum_variant_name,
other_values.enum_variant_name()
),
Comment on lines +644 to +648
Copy link
Contributor Author

@Jondolf Jondolf Jan 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We might want to return some error instead? I'm not sure how common it would be to reach this panic since I'd expect e.g. position attributes to almost always be Float32x3

}
}
}

// Extend indices of `self` with indices of `other`.
if let (Some(indices), Some(other_indices)) = (self.indices_mut(), other.indices()) {
match (indices, other_indices) {
(Indices::U16(i1), Indices::U16(i2)) => {
i1.extend(i2.iter().map(|i| *i + index_offset as u16));
}
(Indices::U32(i1), Indices::U32(i2)) => {
i1.extend(i2.iter().map(|i| *i + index_offset as u32));
}
(Indices::U16(i1), Indices::U32(i2)) => {
i1.extend(i2.iter().map(|i| *i as u16 + index_offset as u16));
}
(Indices::U32(i1), Indices::U16(i2)) => {
i1.extend(i2.iter().map(|i| *i as u32 + index_offset as u32));
}
}
}
}

/// Compute the Axis-Aligned Bounding Box of the mesh vertices in model space
///
/// Returns `None` if `self` doesn't have [`Mesh::ATTRIBUTE_POSITION`] of
Expand Down