Conversation
| _ => panic!( | ||
| "Incompatible vertex attribute types {} and {}", | ||
| enum_variant_name, | ||
| other_values.enum_variant_name() | ||
| ), |
There was a problem hiding this comment.
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
| let enum_variant_name = values.enum_variant_name(); | ||
| if let Some(other_values) = other.attribute(id) { | ||
| match (values, other_values) { |
There was a problem hiding this comment.
From what I can see, if self and other's attributes don't match up:
- if
selfhas an attribute butotherdoesn't, it is kept (note that this means future'sself.count_vertices()will report the old vertices count); - if
otherhas an attribute butselfdoesn'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.
There was a problem hiding this comment.
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
| // The indices of `other` should start after the last vertex of `self`. | ||
| let index_offset = self.count_vertices(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
IMO we should just return a |
There was a problem hiding this comment.
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.
This is already kinda mentioned: /// Note that attributes of `other` that don't exist on `self` will be ignored.
|
Shatur
left a comment
There was a problem hiding this comment.
Ah, sorry, I somehow missed it.
|
Not a great milestone candidate at this stage (it's not vital), but I'm happy to merge it regardless. |
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
mergemethod toMesh. It appends the vertex attributes and indices ofothertoself, resulting in aMeshthat is the combination of the two.For example, you could do this:
This would result in
cuboidbeing aMeshthat 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.