-
Notifications
You must be signed in to change notification settings - Fork 369
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
Conversation
Neither extension has the `from_endpoints_and_radii` constructor that Rust does.
OK, I think this is ready for review now. Things to note:
|
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.
Looks good, but before we commit to length=Z
we should really look into what is the standard (if any)
/// 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. |
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.
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
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.
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.
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.
We've decided to make this configurable in a follow-up PR: #1361 (comment)
crates/store/re_types/definitions/rerun/archetypes/capsules3d.fbs
Outdated
Show resolved
Hide resolved
let length = direction.length(); | ||
let quaternion = glam::Quat::from_rotation_arc(glam::Vec3::Z, direction / length); |
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.
We should explicitly handle length = 0
here, preferably with a test too
crates/viewer/re_space_view_spatial/src/visualizers/utilities/entity_iterator.rs
Show resolved
Hide resolved
/* 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 | ||
); | ||
*/ |
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.
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
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.
Implementing from_endpoints_and_radii
anywhere will be easy once we have a MainAxis
component, as suggested in #1361 (comment)
Thank you for adding this feature. Can't wait for the tapered capsules support! |
let length = direction.length(); | ||
|
||
let quaternion = if length > 0.0 { |
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.
this is more robust against small non-zero lengths (e.g. denormals):
let length = direction.length(); | |
let quaternion = if length > 0.0 { | |
let direction = direction.normalize_or_zero(); | |
let quaternion = if direction != glam::Vec3::ZERO { |
What
Adds the archetype
Capsules3D
, componentLength
, 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.
Missing parts that should make it into this PR before it leaves draft:
Further work not included in this PR, that will add complications:
from_endpoints
constructors, because there wasn’t the quaternion math handy. Should we add an equivalent toglam::Quat::from_rotation_arc()
to the minimal Rerun quaternion types?FillMode
component). This is because it was recommended that I usere_math
’s capsules, butre_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.Checklist
main
build: rerun.io/viewernightly
build: rerun.io/viewerCHANGELOG.md
and the migration guideTo run all checks from
main
, comment on the PR with@rerun-bot full-check
.