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 support for mesh vertex colors #1671

Merged
merged 26 commits into from
Mar 23, 2023
Merged

Add support for mesh vertex colors #1671

merged 26 commits into from
Mar 23, 2023

Conversation

emilk
Copy link
Member

@emilk emilk commented Mar 22, 2023

Works with rr.log_mesh and with any vertex colors found n GLB/GLTF or OBJ files.

Test: just py-build && examples/python/api_demo/main.py --demo raw_mesh

image

mesh-vertex-colors

Checklist

@emilk emilk added 🐍 Python API Python logging API 🔺 re_renderer affects re_renderer itself 🦀 Rust API Rust logging API 📺 re_viewer affects re_viewer itself labels Mar 22, 2023
@emilk emilk marked this pull request as ready for review March 22, 2023 15:21
@Wumpf Wumpf self-requested a review March 22, 2023 19:17
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you so much for going the extra mile to add all these robustness & error handling improvements, as well as the splitting up of the vertex buffer. Loving all of it!

Haven't found any substantial errors/todos, most comments are "conversational" ;)

This thing is at least worth a dev-pulse post, if not more :).

Haven't run it yet, will do so tomorrow and then check what's up with the color interpolation on the triangle as discussed at the office (the color gradient seems to be a bit off, might be something about the perspective interpolation though).

crates/re_log_types/src/component_types/color.rs Outdated Show resolved Hide resolved
crates/re_renderer/shader/instanced_mesh.wgsl Outdated Show resolved Hide resolved
@@ -243,13 +240,19 @@ fn import_mesh(
anyhow::bail!("empty mesh");
Copy link
Member

Choose a reason for hiding this comment

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

Really should get rid of all anyhow errors in all of re_renderer. One of the many things I never seem to get to :/

Copy link
Member Author

Choose a reason for hiding this comment

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

I found that copilot was really good at writing thiserror enums, so just do it

crates/re_renderer/src/mesh.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/mesh.rs Outdated Show resolved Hide resolved
Comment on lines +341 to +343
let vb_colors_start = vb_positions_size;
let vb_normals_start = vb_colors_start + vb_color_size;
let vb_texcoord_start = vb_normals_start + vb_normals_size;
Copy link
Member

Choose a reason for hiding this comment

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

[not actionable] I wonder if staging_buffer.extend_from_slice should return these ranges as a convenience feature. What do you think? Helpful or too weird for an extend_from_slice method? It's technically also different buffer ofc.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is tedious and error prone, so some helper would be nice

Comment on lines +46 to +47
#[error("Invalid mesh given as input")]
InvalidMesh(#[from] crate::mesh::MeshError),
Copy link
Member

Choose a reason for hiding this comment

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

The other errors are all about resource managers in general, so this should go to the mesh manager specifically.
However, the abstracted resource manager idea broke down quite a bit - e.g. I think I'll soon remove the "temp vs permanent resource" distinction; this didn't play out as I originally envisioned. So I'm ok if we punt on this :)

crates/re_viewer/src/misc/mesh_loader.rs Outdated Show resolved Hide resolved
rerun_py/src/python_bridge.rs Outdated Show resolved Hide resolved

// ----------------------------------------------------------------------------

fn slice_from_np_array<'a, T: numpy::Element, D: numpy::ndarray::Dimension>(
Copy link
Member

Choose a reason for hiding this comment

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

How did we manage without this so far Oo

emilk and others added 4 commits March 23, 2023 08:13
Co-authored-by: Andreas Reich <andreas@rerun.io>
Co-authored-by: Andreas Reich <andreas@rerun.io>
Co-authored-by: Andreas Reich <andreas@rerun.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐍 Python API Python logging API 🔺 re_renderer affects re_renderer itself 📺 re_viewer affects re_viewer itself 🦀 Rust API Rust logging API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants