Conversation
yuecideng
added
bug
There was a problem hiding this comment.
Pull request overview
Updates Gizmo to the new Gizmo API by removing the deprecated .node accessor pattern and switching to direct method calls on the underlying _gizmo, including updating the transform flush callback pathway.
Changes:
- Replaced several
_gizmo.node.*calls with new direct_gizmo.*API methods (e.g.,follow,set_transform_flush_callback,detach_parent, pose/visibility accessors). - Updated proxy-based gizmo callback handling to consume the new
(node, local_pose, flag)signature and derive translation/rotation from a 4×4 matrix. - Added defensive error handling when registering the camera gizmo transform callback.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _proxy_gizmo_callback(self, *args): | ||
| """Generic callback for proxy-based gizmo. | ||
|
|
||
| def _proxy_gizmo_callback(self, node, translation, rotation, flag): | ||
| """Generic callback for proxy-based gizmo: only updates proxy cube transform, defers actual updates""" | ||
| Supports both old signature: (node, translation, rotation, flag) | ||
| and new signature: (node, local_pose, flag) where local_pose is a 4x4 matrix. | ||
| Updates the proxy cube transform and sets `_pending_target_transform`. | ||
| """ | ||
| # New API callback signature: (node, local_pose, flag) | ||
| if len(args) != 3: | ||
| return | ||
| node, local_pose, flag = args |
There was a problem hiding this comment.
_proxy_gizmo_callback docstring says it supports both the old (node, translation, rotation, flag) and new (node, local_pose, flag) signatures, but the implementation returns early unless len(args) == 3. This will silently drop callbacks from the old API. Either handle the 4-argument form (and map it into a 4x4 pose) or update the docstring/usage so the behavior matches the supported API.
| node.set_translation(translation) | ||
| elif flag == (TransformMask.TRANSFORM_LOCAL | TransformMask.TRANSFORM_R): | ||
| node.set_rotation_rpy(rotation) | ||
| # convert to numpy 4x4 matrix |
There was a problem hiding this comment.
Line 239 is indented inside the prior if ...: return block, making the comment unreachable and suggesting the conversion block is mis-indented. Dedent the comment (and ensure the conversion logic is aligned correctly) to avoid confusing dead code / future indentation mistakes.
| # convert to numpy 4x4 matrix | |
| # convert to numpy 4x4 matrix |
embodichain/lab/sim/objects/gizmo.py
Outdated
| if self._target_type == "rigidobject": | ||
| # RigidObject: direct node access through MeshObject | ||
| self._gizmo.node.update_gizmo_follow(self.target._entities[0].node) | ||
| self._gizmo.node.set_flush_transform_callback(create_gizmo_callback()) | ||
| # RigidObject: direct node access through MeshObject — use follow/attach | ||
| tgt_node = self.target._entities[0].node | ||
| self._gizmo.follow(tgt_node) | ||
| # set callback (localpose-style) | ||
| self._gizmo.set_transform_flush_callback(create_gizmo_callback()) |
There was a problem hiding this comment.
create_gizmo_callback() (from embodichain/lab/sim/utility/gizmo_utils.py) returns a callback with the old 4-argument signature (node, translation, rotation, flag), but here it’s registered via the new set_transform_flush_callback(...) API which (per this PR) now invokes callbacks as (node, local_pose, flag). This will raise a TypeError at runtime when manipulating rigid objects. Update create_gizmo_callback() to accept the new signature (or wrap/adapter it here).
| if ( | ||
| hasattr(self, "_gizmo") | ||
| and self._gizmo | ||
| and hasattr(self._gizmo, "node") | ||
| ): | ||
| self._gizmo.node.detach_parent() | ||
| self._gizmo.detach_parent() |
There was a problem hiding this comment.
destroy() still guards cleanup with hasattr(self._gizmo, "node"), but the new API removes/deprecates .node. This condition will likely be false, so detach_parent() won’t run and resources may not be cleaned up. Update the guard to check only _gizmo and update remaining .node.* calls in destroy() to the new direct gizmo API (e.g., clear callback via set_transform_flush_callback(None) if supported).
| # Detach gizmo using new API | ||
| self._gizmo.detach_parent() |
There was a problem hiding this comment.
detach() used to wrap detach_parent() in a try/except, but now calls self._gizmo.detach_parent() unguarded. If detach_parent() can raise during teardown (as implied by the previous code), this can crash callers. Consider restoring the try/except (or a capability check) for robustness consistent with the earlier behavior.
| # Detach gizmo using new API | |
| self._gizmo.detach_parent() | |
| # Detach gizmo using new API, but guard against failures for robustness | |
| try: | |
| detach_fn = getattr(self._gizmo, "detach_parent", None) | |
| if callable(detach_fn): | |
| detach_fn() | |
| except Exception as e: | |
| logger.log_error(f"Error detaching gizmo from parent: {e}") |
| def set_transform_callback(self, callback: Callable): | ||
| """Set callback for gizmo transform events (translation/rotation).""" | ||
| self._callback = callback | ||
| self._gizmo.node.set_flush_transform_callback(callback) | ||
| self._gizmo.set_transform_flush_callback(callback) |
There was a problem hiding this comment.
set_transform_callback’s docstring still describes a (translation, rotation) style callback, but this PR migrates to a (node, local_pose, flag) callback where local_pose is a 4x4 matrix. Update the docstring (and, if you intend to keep backward compatibility, consider adapting old-style callbacks before registering them).
| def _proxy_gizmo_callback(self, *args): | ||
| """Generic callback for proxy-based gizmo. | ||
|
|
||
| def _proxy_gizmo_callback(self, node, translation, rotation, flag): | ||
| """Generic callback for proxy-based gizmo: only updates proxy cube transform, defers actual updates""" | ||
| Supports both old signature: (node, translation, rotation, flag) | ||
| and new signature: (node, local_pose, flag) where local_pose is a 4x4 matrix. | ||
| Updates the proxy cube transform and sets `_pending_target_transform`. | ||
| """ | ||
| # New API callback signature: (node, local_pose, flag) | ||
| if len(args) != 3: | ||
| return | ||
| node, local_pose, flag = args | ||
| if node is None: | ||
| return | ||
|
|
||
| # Check if proxy cube still exists (not destroyed) | ||
| # Check if proxy cube still exists | ||
| if not hasattr(self, "_proxy_cube") or self._proxy_cube is None: | ||
| return | ||
|
|
||
| # Update proxy cube transform | ||
| if flag == (TransformMask.TRANSFORM_LOCAL | TransformMask.TRANSFORM_T): | ||
| node.set_translation(translation) | ||
| elif flag == (TransformMask.TRANSFORM_LOCAL | TransformMask.TRANSFORM_R): | ||
| node.set_rotation_rpy(rotation) | ||
| # convert to numpy 4x4 matrix | ||
| if isinstance(local_pose, torch.Tensor): | ||
| lp = local_pose.cpu().numpy() | ||
| else: | ||
| lp = np.asarray(local_pose) | ||
|
|
||
| if lp.shape != (4, 4): | ||
| return | ||
|
|
||
| trans = lp[:3, 3] | ||
| rot_mat = lp[:3, :3] | ||
| euler = R.from_matrix(rot_mat).as_euler("xyz", degrees=False) | ||
|
|
||
| # Mark that target needs to be updated, save target transform | ||
| proxy_pos = self._proxy_cube.get_location() | ||
| proxy_rot = self._proxy_cube.get_rotation_euler() | ||
| self._proxy_cube.set_location(float(trans[0]), float(trans[1]), float(trans[2])) | ||
| self._proxy_cube.set_rotation_euler( | ||
| float(euler[0]), float(euler[1]), float(euler[2]) | ||
| ) | ||
|
|
||
| # Build pending target transform (1,4,4) | ||
| target_transform = torch.eye(4, dtype=torch.float32) | ||
| target_transform[:3, 3] = torch.tensor( | ||
| [proxy_pos[0], proxy_pos[1], proxy_pos[2]], dtype=torch.float32 | ||
| ) | ||
| target_transform[:3, :3] = torch.tensor( | ||
| R.from_euler("xyz", proxy_rot).as_matrix(), dtype=torch.float32 | ||
| [trans[0], trans[1], trans[2]], dtype=torch.float32 | ||
| ) | ||
| # Ensure _pending_target_transform is (1, 4, 4) | ||
| if isinstance(target_transform, torch.Tensor) and target_transform.shape == ( | ||
| 4, | ||
| 4, | ||
| ): | ||
| target_transform = target_transform.unsqueeze(0) | ||
| self._pending_target_transform = target_transform | ||
| target_transform[:3, :3] = torch.tensor(rot_mat, dtype=torch.float32) | ||
| self._pending_target_transform = target_transform.unsqueeze(0) |
There was a problem hiding this comment.
The new callback parsing/building logic in _proxy_gizmo_callback is core to camera/robot gizmo control but there are no unit tests covering it (and the repo has an existing pytest suite under tests/). Consider adding a small test that feeds representative (node, local_pose, flag) inputs (torch tensor + numpy array) and asserts _pending_target_transform shape/value behavior, so future Gizmo API changes are caught quickly.
Description
This PR updates the
Gizmoclass inembodichain/lab/sim/objects/gizmo.pyto use the new Gizmo API, replacing the deprecated.nodeaccessor pattern with direct method calls on the_gizmoobject.Key changes:
_gizmo.node.update_gizmo_follow(node)→_gizmo.follow(node)_gizmo.node.set_flush_transform_callback(cb)→_gizmo.set_transform_flush_callback(cb)_gizmo.node.detach_parent()→_gizmo.detach_parent()set_world_pose,get_local_pose,set_visible, etc.) to call directly on_gizmoinstead of_gizmo.node_proxy_gizmo_callbacksignature to accept the new(node, local_pose, flag)format wherelocal_poseis a 4×4 matrix, replacing the old(node, translation, rotation, flag)signature with separate translation/rotation vectorsType of change
Screenshots
N/A
Checklist
black .command to format the code base.🤖 Generated with Claude Code