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:
|
emilk
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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_endpointsconstructors, because there wasn’t the quaternion math handy. Should we add an equivalent toglam::Quat::from_rotation_arc()to the minimal Rerun quaternion types?FillModecomponent). This is because it was recommended that I usere_math’s capsules, butre_mathhas 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
mainbuild: rerun.io/viewernightlybuild: rerun.io/viewerCHANGELOG.mdand the migration guideTo run all checks from
main, comment on the PR with@rerun-bot full-check.