Complete support for PLY format (add mesh and 2D) also using more performant API of ply-rs-bw 4.0.0. - #12730
Complete support for PLY format (add mesh and 2D) also using more performant API of ply-rs-bw 4.0.0.#12730bourumir-wyngs wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Sorry for letting this sit here for so long! This looks really good overall and has a lot of great tests and even an example <3, but I think there's a missunderstanding of Mesh3D <-> Asset3D going on here:
Instead of loading the mesh SDK sided (which we'd need to expose to C++, Python and Rust) we load encoded meshes into Asset3D and unpack those at runtime using one of the importers in crates/viewer/re_renderer/src/importer/
I.e. what that means is that we'd rather like to
- change documentation of
Asset3Dto point out PLY as supported (that's incrates/store/re_sdk_types/definitions/rerun/archetypes/asset3d.fbs, running codegen will mirror that over to all SDKs) - have the dataloader branch to
Asset3Dif it finds any topology information - wire up
crates/viewer/re_view_spatial/src/mesh_loader.rsto a newre_renderer::importer::PLY
The reason we want to keep the raw mesh data as long as possible is that we generally like to preserve user input data as is, plus this delays the mesh parsing to the last moment meaning we don't have to "guard" all possible input paths (dataloader + 3x SDK)
there's ofc the future question whether Asset3D should just not render as point cloud if it sees a PLY without topology, but that's a design question that way beyond this iteration. Also, Asset3D is not very well inspectable in the Viewer today, but that's another problem entirely.
| // .ply is both point cloud and mesh and should not be in this list for detection to work. | ||
| pub const SUPPORTED_MESH_EXTENSIONS: &[&str] = &["glb", "gltf", "obj", "stl", "dae"]; | ||
|
|
||
| // TODO(#4532): `.ply` data loader should support 2D point cloud & meshes | ||
| pub const SUPPORTED_POINT_CLOUD_EXTENSIONS: &[&str] = &["ply"]; |
There was a problem hiding this comment.
[no action required] a bit odd, but given that so far our users use ply a lot for point clouds I think it makes sense
There was a problem hiding this comment.
Understand, working on this. .ply is very versatile format that can hold 2D or 3D mesh, and if without vertices, this becomes the point cloud. It is internally a bit like "binary XML".
f8ac64f to
355dfbe
Compare
|
Main changes since your review:
|
|
Could you please confirm the current status of this MR? I've implemented all requested changes. Before resolving the merge conflicts, I'd like to know whether the MR is still under consideration for merge. |
|
Sorry I've been first out on business travel and then for vacation and didn't hand this over to anyone else in the meantime! I can have a look at the conflicts and shepherd the rest in. |
|
Thanks for reply, would be great! |
header, and extend tests for path vs contents parsing, optional properties, ignored extra data, malformed payload diagnostics, and loader dispatch. Preparing to use ply-rs-bw 4.0.0
Keep topology-bearing PLY files as raw Asset3D data during import instead of parsing them into Mesh3D in the SDK. - Add a viewer-side PLY mesh importer - Wire the importer into Asset3D mesh loading - Update Asset3D media type documentation - Improve PLY media type guessing - Update tests for the new routing - Update the PLY/STL tetrahedron example: - Log PLY meshes as Asset3D - Run against the local viewer
Preserve Gaussian splat parsing, avoid retaining ignored payloads, and align the new example with the current workspace metadata.
There was a problem hiding this comment.
I did a mostly llm driven rebase & fixup and then went onto reviewing the latest state. The handling of asset3d in that whole machinery seems fine to me now. And most other things too! However, there's quite a few things that I found a bit too messy as well as some questions. You're still up for having a look at these? 🙏
There was a problem hiding this comment.
a whole new crate just for ply seems very heavy handed to me. I'm skeptical of whether this is actually needed, seems fine to inline most of these utilities? The non-test part is just 168 lines and most of it seem to be a bunch of standardized strings
Since you're the author of ply_rs, I wonder whether you'd be interested in instead up-streaming some of those constants?
| } else if crate::SUPPORTED_POINT_CLOUD_EXTENSIONS.contains(&extension.as_str()) { | ||
| re_log::debug!(?filepath, importer = self.name(), "Loading 3D point cloud…",); | ||
| self.send_chunks( | ||
| &tx, | ||
| &store_id, | ||
| load_point_cloud(timepoint, entity_path, &contents)?, | ||
| ) | ||
| re_log::debug!(?filepath, importer = self.name(), "Loading .ply geometry…",); | ||
| self.send_chunks(&tx, &store_id, load_ply(timepoint, entity_path, &contents)?) |
There was a problem hiding this comment.
this doesn't make any sense: as is the code states that if an extension is contained in SUPPORTED_POINT_CLOUD_EXTENSIONS then it has to be a ply
There was a problem hiding this comment.
The function was renamed to load_ply because it likely was not capable of loading any other kind of point cloud anyway. I currently see some more code there in the diff, maybe this came from some later contribution. Here something likely went wrong, trying to understand.
There was a problem hiding this comment.
Worst is that this re_importer seems missing completely in the current main branch. I need to untagle where all the code needs to go.
There was a problem hiding this comment.
eh sorry sounds like I might have messed up some stuff there as well
| // .ply is both point cloud and mesh and should not be in this list for detection to work. | ||
| pub const SUPPORTED_MESH_EXTENSIONS: &[&str] = &["glb", "gltf", "obj", "stl", "dae"]; | ||
|
|
||
| // TODO(#4532): `.ply` importer should support 2D point cloud & meshes | ||
| pub const SUPPORTED_POINT_CLOUD_EXTENSIONS: &[&str] = &["ply"]; |
There was a problem hiding this comment.
as per comment on importer_archetype: what's the point of saying it's a point cloud extension when it's not, it's a more general kind of extension that may yield to either.
So technically ply has to be indeed both on the mesh extensions and the point cloud extensions in case anyone probes from the outside what's supported (and they can since this is part of the public interface)
I'd do just that and then fix up the importer code
| fn example_ply_path() -> PathBuf { | ||
| Path::new(env!("CARGO_MANIFEST_DIR")).join("../../../examples/assets/example.ply") | ||
| } | ||
|
|
There was a problem hiding this comment.
misses mod tests
I don't quite get how you decided which tests to put in here versus in the higher level re_importer. Arguably what re_importer adds is fairly trivial integration and almost all the tests should be in here rather?
| } | ||
|
|
||
| impl ParsedMeshVertex { | ||
| fn into_parts(self) -> std::io::Result<(glam::Vec3, Option<glam::Vec3>, Option<Rgba32Unmul>)> { |
There was a problem hiding this comment.
labling of those parts via a struct would be good - have to know from context that it's position & normal. but also why not just do getter individually for position/normal/color?
| run(&rec)?; | ||
|
|
||
| rerun::native_viewer::show(main_thread_token, storage.take())?; | ||
| Ok(()) |
There was a problem hiding this comment.
this is very unidiomatic rerun usage: yes it makes the sample self-contained and doesn't need an external rerun viewer, but this means that the sample is the rerun viewer and has to compile the whole thing in. The normal Rerun execution model is that applications log to file/viewer/server and then consume data there.
Let's follow that here and use re_log::setup_logging();, rerun::clap::RerunArgs etc. like most other rust examples do
|
|
||
| [dependencies] | ||
| rerun = { path = "../../../crates/top/rerun", default-features = false, features = [ | ||
| "native_viewer", |
There was a problem hiding this comment.
as above, we only use native_viewer when demonstrating viewer extensions etc.
Related
What
Notes