-
Notifications
You must be signed in to change notification settings - Fork 373
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
Mesh3D
archetype
#3363
Mesh3D
archetype
#3363
Conversation
a78524c
to
a7910bc
Compare
|
/// Optional properties for the mesh as a whole (including indexed drawing). | ||
// | ||
// NOTE: We cannot have triangle indices here as that would break our instance key rules (either 0, 1 or N). | ||
mesh_properties: rerun.components.MeshProperties ("attr.rerun.component_recommended", nullable, order: 2000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not call this mesh_indices: rerun.components.MeshIndices
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels likely like we're gonna want to stash a few more things in there sooner than later (winding order, index format, ...) 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MeshTriangles
maybe? "properties" is very vague. I would expect there to be color stuff in there, not fundamental stuff like indices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not necessarily a fan of MeshProperties
but I like MeshTriangles
even less tbh.
- This really has nothing to do with triangles at this point (it's now
vertex_indices
) - Indices are not "fundamental", quite the contrary they're optional :/
Anyway, time to ship this; feel free to change it in another PR.
One thing to keep in mind: this will make large meshes (like hundreds of thousands of vertices and more) quite slower than before, since meshes are now more akin to point clouds: all vertex properties are joined etc according to their instance keys. We could easily bypass that join (this PR actually used to do that) to regain that performance, at the cost of violating our query model and therefore user expectations. |
f43cc9f
to
e57b1a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing!
crates/re_types/definitions/rerun/datatypes/mesh_properties.fbs
Outdated
Show resolved
Hide resolved
#[inline] | ||
fn try_to_arrow( | ||
&self, | ||
) -> crate::SerializationResult< | ||
Vec<(::arrow2::datatypes::Field, Box<dyn ::arrow2::array::Array>)>, | ||
> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have as_component_batches
, can't try_to_arrow
just call self.as_component_batches
and then call try_to_arrow()
on each component batch? A lot less code needs generating. I guess we get an additional dyn
call per component, but that seems worth it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#3377 wins us back a millisecond: The only way to do better at this point would be to not deserialize at all when we take the optimized path, instead working with raw |
42a22b5
to
ccad17e
Compare
Implements the
Mesh3D
archetype:An interesting facet of the Mesh3D archetype is that it's composed of two kinds of data:
Because the vertices becomes the instances, meshes cannot be batched.
There's a bit of a mess here and there because the ancestors of
Mesh3D
&Asset3D
used to be tightly coupled.This will be fixed in the next PR that implements the next-gen
Asset3D
archetype.No dedicated roundtrip tests: API examples already cover everything.
Fixes #2788
Here's an example of partial updates:
23-09-19_15.45.08.patched.mp4
Checklist