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 Capsules3D archetype. #7574

Merged
merged 12 commits into from
Oct 29, 2024
Merged

Add Capsules3D archetype. #7574

merged 12 commits into from
Oct 29, 2024

Conversation

kpreid
Copy link
Collaborator

@kpreid kpreid commented Oct 2, 2024

What

Adds the archetype Capsules3D, component Length, and a matching visualizer.

Capsules are defined by their length and radius, and then transformed in the usual fashion to set their orientation. There is also a constructor for joining a pair of endpoints.

image

Missing parts that should make it into this PR before it leaves draft:

  • Needs an example image — I assume I don't have the permissions to upload one.

Further work not included in this PR, that will add complications:

  • C++ and Python have no from_endpoints constructors, because there wasn’t the quaternion math handy. Should we add an equivalent to glam::Quat::from_rotation_arc() to the minimal Rerun quaternion types?
  • There is no wireframe mesh support (expressed as having no FillMode component). This is because it was recommended that I use re_math’s capsules, but re_math has no support for generating wireframes. In the long run, we will want to support tapered capsules and cylinders and a new or substantially revised mesh generator to handle these cases will be needed anyway.
  • The support for setting the length of a capsule is a horrible kludge which is hard on the mesh cache — each length:radius ratio is cached separately. The idea I have for fixing this is adding “bone” support — the ability for selected vertices to be displaced by an externally controlled offset. This allows taking a single capsule mesh and stretching it without distorting the endcaps. (Tapered capsules will require multiple meshes to account for the angle at which the cone meets the endcap, but those can be fairly well approximated by a finite set of meshes.)

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!
  • If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide

To run all checks from main, comment on the PR with @rerun-bot full-check.

@kpreid kpreid added 📺 re_viewer affects re_viewer itself include in changelog labels Oct 2, 2024
kpreid added 2 commits October 6, 2024 21:12
Neither extension has the `from_endpoints_and_radii` constructor
that Rust does.
@kpreid
Copy link
Collaborator Author

kpreid commented Oct 7, 2024

OK, I think this is ready for review now. Things to note:

  • Someone needs to add an example image for the archetype docs — I assume I don't have the permissions to upload one myself.
  • There is no from_endpoints_and_radii() constructor in the C++ and Python Capsules3D archetypes yet, because there's no quaternion math function to compute the necessary rotation. Should I add such functions? If so, should I just translate glam's code that does the job, or do something else (possibly for licensing considerations)?
  • The CI check "Check if running codegen would produce any changes" is failing, but pixi run codegen --force locally doesn't change anything.

@kpreid kpreid marked this pull request as ready for review October 7, 2024 04:28
@emilk emilk self-requested a review October 7, 2024 14:06
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Looks good, but before we commit to length=Z we should really look into what is the standard (if any)

Cargo.toml Outdated Show resolved Hide resolved
/// 3D capsules; cylinders with hemispherical caps.
///
/// Capsules are defined by two endpoints (the centers of their end cap spheres), which are located
/// at (0, 0, 0) and (0, 0, length), that is, extending along the positive direction of the Z axis.
Copy link
Member

Choose a reason for hiding this comment

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

Did you check what other libraries use as their "length" axis? See #1361 (comment)

I'm especially interested in what is commonly used in the ROS ecosystem

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did some web searching and couldn’t easily find any ROS-related things that had actually implemented and documented capsules to compare. While I was looking I found a couple unrelated things that used Z or the two-points representation. But I am absolutely terrible at thorough research and wouldn’t know where to start with reliably determining what “other visualization tools” tend to use.

Copy link
Member

Choose a reason for hiding this comment

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

We've decided to make this configurable in a follow-up PR: #1361 (comment)

Comment on lines 41 to 42
let length = direction.length();
let quaternion = glam::Quat::from_rotation_arc(glam::Vec3::Z, direction / length);
Copy link
Member

Choose a reason for hiding this comment

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

We should explicitly handle length = 0 here, preferably with a test too

crates/viewer/re_space_view_spatial/src/proc_mesh.rs Outdated Show resolved Hide resolved
Comment on lines +24 to +36
/* TODO(kpreid): This should exist for parity with Rust, but actually implementing this
needs a bit of quaternion math.

/// Creates a new `Capsules3D` where each capsule extends between the given pairs of points.
//
// TODO(andreas): This should not take an std::vector.
//
static Capsules3D from_endpoints_and_radii(
const std::vector<datatypes::Vec3D>& start_points,
const std::vector<datatypes::Vec3D>& end_points,
const std::vector<float>& radii
);
*/
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if we could just call into Rust to do it for us, but adding FFI functions is tedious and error prone.

This could possibly help:

For now I'm fine with C++ missing the feature at the start

Copy link
Member

Choose a reason for hiding this comment

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

Implementing from_endpoints_and_radii anywhere will be easy once we have a MainAxis component, as suggested in #1361 (comment)

@kpreid kpreid requested a review from emilk October 11, 2024 00:18
@jeongseok-meta
Copy link

Thank you for adding this feature. Can't wait for the tapered capsules support!

Comment on lines 41 to 43
let length = direction.length();

let quaternion = if length > 0.0 {
Copy link
Member

Choose a reason for hiding this comment

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

this is more robust against small non-zero lengths (e.g. denormals):

Suggested change
let length = direction.length();
let quaternion = if length > 0.0 {
let direction = direction.normalize_or_zero();
let quaternion = if direction != glam::Vec3::ZERO {

@emilk emilk added this to the 0.20 - H.264, Undo milestone Oct 28, 2024
@emilk emilk merged commit 6599f87 into rerun-io:main Oct 29, 2024
32 of 33 checks passed
@emilk emilk added the 🪵 Log & send APIs Affects the user-facing API for all languages label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants