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.

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

merged 5 commits into from Feb 12, 2024

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Jan 21, 2024

Objective

It can sometimes be useful to combine several meshes into one. This allows constructing more complex meshes out of simple primitives without needing to use a 3D modeling program or entity hierarchies.

This could also be used internally to increase code reuse by using existing mesh generation logic for e.g. circles and using that in cylinder mesh generation logic to add the top and bottom of the cylinder.

Note: This is not implementing CSGs (Constructive Solid Geometry) or any boolean operations, as that is much more complex. This is simply adding the mesh data of another mesh to a mesh.

Solution

Add a merge method to Mesh. It appends the vertex attributes and indices of other to self, resulting in a Mesh that is the combination of the two.

For example, you could do this:

let mut cuboid = Mesh::from(shape::Box::default());
let mut cylinder = Mesh::from(shape::Cylinder::default());
let mut torus = Mesh::from(shape::Torus::default());

cuboid.merge(cylinder);
cuboid.merge(torus);

This would result in cuboid being a Mesh that also has the cylinder mesh and torus mesh. In this case, they would just be placed on top of each other, but by utilizing #11454 we can transform the cylinder and torus to get a result like this:

2024-01-21.14-31-30.mp4

This is just a single entity and a single mesh.

@Jondolf Jondolf added C-Enhancement A new feature A-Rendering Drawing game state to the screen labels Jan 21, 2024
Comment on lines +631 to +635
_ => panic!(
"Incompatible vertex attribute types {} and {}",
enum_variant_name,
other_values.enum_variant_name()
),
Copy link
Contributor Author

@Jondolf Jondolf Jan 21, 2024

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

crates/bevy_render/src/mesh/mesh/mod.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/mesh/mesh/mod.rs Outdated Show resolved Hide resolved
Comment on lines +589 to +591
let enum_variant_name = values.enum_variant_name();
if let Some(other_values) = other.attribute(id) {
match (values, other_values) {
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

Comment on lines 584 to 585
// 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

Copy link
Contributor

@Adamkob12 Adamkob12 left a comment

Choose a reason for hiding this comment

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

I think it would be better to panic if the attributes don't match (maybe only in debug mode?). Merging meshes seems very non-arbitrary, so catching the error will be easy.

Maybe a 'force_merge' method to ignore? Seems overkill though.

@alice-i-cecile
Copy link
Member

IMO we should just return a Result and then let the user decide how to handle the errors.

Copy link
Contributor

@Shatur Shatur left a comment

Choose a reason for hiding this comment

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

I applied this PR as a patch and tested in my game, works great!

I would add mention in the docs that if self have a missing attribute or indices, they will be ignored when merging.

Since it's a very small, yet useful PR, I added it to the milestone. Feel free to remove if you think it won't fit.

@Shatur Shatur added this to the 0.13 milestone Feb 12, 2024
@Jondolf
Copy link
Contributor Author

Jondolf commented Feb 12, 2024

I would add mention in the docs that if self have a missing attribute or indices, they will be ignored when merging.

This is already kinda mentioned:

/// Note that attributes of `other` that don't exist on `self` will be ignored.

Copy link
Contributor

@Shatur Shatur left a comment

Choose a reason for hiding this comment

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

Ah, sorry, I somehow missed it.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 12, 2024
@alice-i-cecile
Copy link
Member

Not a great milestone candidate at this stage (it's not vital), but I'm happy to merge it regardless.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 12, 2024
Merged via the queue into bevyengine:main with commit f1f83bf Feb 12, 2024
24 checks passed
@Jondolf Jondolf deleted the merge-mesh branch February 12, 2024 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Enhancement A new feature S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants