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

[visualization] Add frame (aka triad) visualization function #19152

Merged
merged 1 commit into from Apr 10, 2023

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Apr 6, 2023

Closes #17170.


This change is Reviewable

Copy link
Contributor

@mitiguy mitiguy left a comment

Choose a reason for hiding this comment

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

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers


bindings/pydrake/visualization/_model_visualizer.py line 532 at r1 (raw file):

):
    """
    Adds illustration geometry representing the given coordinate frame, with

FYI It seems the word "coordinate" is unnecessary. In the context of Drake, it is unclear how a coordinate frame differs from a frame. Consider removing it.


bindings/pydrake/visualization/_model_visualizer.py line 547 at r1 (raw file):

      radius: the radius of each axis in meters.
      opacity: the opacity each axes, between 0.0 and 1.0.
      X_FT: the pose of the triad frame T in the frame_id frame F.

FYI consider:

X_FT: rigid transform relating frame T and frame F.

bindings/pydrake/visualization/_model_visualizer.py line 548 at r1 (raw file):

      opacity: the opacity each axes, between 0.0 and 1.0.
      X_FT: the pose of the triad frame T in the frame_id frame F.
      name: the added geometries will have names "{name} x-axis", etc.

FYI Wondering if x-axis associates with a line or a unit vector (a direction) -- or if there is sufficient context that it is irrelevant? If the x-axis is a unit vector and the body name is B, I typically use names such as Bx, By, Bz for the associated unit vectors -- or if enough unicode bandwidth 𝗕x 𝗕y 𝗕ᴢ.

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @jwnimmer-tri and @mitiguy)


bindings/pydrake/visualization/_model_visualizer.py line 564 at r1 (raw file):

        geom_name = f"{name} {char}-axis"
        p_TG = 0.5 * length * eye[i]
        R_TG = AngleAxis(angle=np.pi/2, axis=eye[1-i])

BTW am I interpreting eye[1-i] correctly: it maps x->y, y->x and z to itself? And is that the intent?

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @mitiguy and @sherm1)


bindings/pydrake/visualization/_model_visualizer.py line 564 at r1 (raw file):

Am I interpreting eye[1-i] correctly: it maps x->y, y->x and z to itself

Yes!

And is that the intent?

Yes. R_TG rotates a cylinder aligned with the z axis to instead align with the ith axis. When i == 2, it spins the cylinder around the z axis, but this is a no-op (for a uniformly textured cylinder).

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @jwnimmer-tri and @mitiguy)


bindings/pydrake/visualization/_model_visualizer.py line 564 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Am I interpreting eye[1-i] correctly: it maps x->y, y->x and z to itself

Yes!

And is that the intent?

Yes. R_TG rotates a cylinder aligned with the z axis to instead align with the ith axis. When i == 2, it spins the cylinder around the z axis, but this is a no-op (for a uniformly textured cylinder).

👍

@jwnimmer-tri jwnimmer-tri force-pushed the add-triad-public branch 2 times, most recently from e112f3f to 73718c8 Compare April 7, 2023 15:04
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @mitiguy and @sherm1)


bindings/pydrake/visualization/_model_visualizer.py line 548 at r1 (raw file):

Previously, mitiguy (Mitiguy) wrote…

FYI Wondering if x-axis associates with a line or a unit vector (a direction) -- or if there is sufficient context that it is irrelevant? If the x-axis is a unit vector and the body name is B, I typically use names such as Bx, By, Bz for the associated unit vectors -- or if enough unicode bandwidth 𝗕x 𝗕y 𝗕ᴢ.

The illustration geometry is not really a unit vector.

@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review April 7, 2023 15:04
@rpoyner-tri rpoyner-tri self-assigned this Apr 7, 2023
Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

+@rpoyner-tri feature.

Reviewable status: LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri)

a discussion (no related file):
Are there any recommended manual tests for visuals?


Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers (waiting on @rpoyner-tri)

a discussion (no related file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

Are there any recommended manual tests for visuals?

My nominal manual test is always just to run the model file given in the help string, in this case with triads turned on:

bazel run //tools:model_visualizer -- package://drake/manipulation/models/iiwa_description/iiwa7/iiwa7_with_box_collision.sdf --visualize_frames


Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

:lgtm: nittery

Reviewed 1 of 2 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers (waiting on @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

My nominal manual test is always just to run the model file given in the help string, in this case with triads turned on:

bazel run //tools:model_visualizer -- package://drake/manipulation/models/iiwa_description/iiwa7/iiwa7_with_box_collision.sdf --visualize_frames

Looks good. I pondered whether there was a way with current on-screen controls to conveniently turn the frames on/off as a layer or even individually, but didn't really succeed in finding one. Maybe can be addressed in follow-up work?



bindings/pydrake/visualization/_triad.py line 26 at r2 (raw file):

    radius: float = 0.005,
    opacity: float = 0.9,
    X_FT: RigidTransform = RigidTransform(),

nit recall that the default values are global lifetime and mutable. I can't see a problem with the current implementation here (no modifications of X_FT), but I will confess a shiver goes down my spine.

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: needs at least two assigned reviewers (waiting on @rpoyner-tri)

a discussion (no related file):

Maybe can be addressed in follow-up work?

Yup. We could do it the same way that we are going to do the inertia visualization. It requires one new capability from MeshcatVisualizer so I'm going to wait for inertia to land first.



bindings/pydrake/visualization/_triad.py line 26 at r2 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

nit recall that the default values are global lifetime and mutable. I can't see a problem with the current implementation here (no modifications of X_FT), but I will confess a shiver goes down my spine.

I had actually thought about that, but somehow misremembered that RigidTransform was immutable so dismissed it out of hand. Oops.

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

+@ggould-tri for Monday platform review.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri)

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r1, 4 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),ggould-tri(platform) (waiting on @jwnimmer-tri)

@jwnimmer-tri jwnimmer-tri merged commit 2bda29c into RobotLocomotion:master Apr 10, 2023
9 checks passed
@jwnimmer-tri jwnimmer-tri deleted the add-triad-public branch April 10, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: feature This pull request contains a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

geometry: Should expose public API for add_triad for frame visualization
5 participants