Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds two features: (1) the ability to export a simulation scene to a USD file via a new export_usd method on SimulationManager, and (2) storing physical attributes read from USD-loaded entities back into the configuration objects (cfg) when use_usd_properties=True. A tutorial script demonstrates the export workflow, and the test file is updated to exercise the new export functionality.
Changes:
- Added
SimulationManager.export_usd()method that delegates to the underlying engine'sexport_to_usd_filewith error handling - When
use_usd_properties=True, rigid objects and articulations now write their USD-loaded physical properties back to their respectivecfgobjects (enabling round-trip export) - New tutorial script
export_usd.pyand test coverage for the export path
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
embodichain/lab/sim/sim_manager.py |
Adds export_usd() method to export scene to USD file |
embodichain/lab/sim/objects/rigid_object.py |
Reads USD entity properties back into cfg when use_usd_properties=True |
embodichain/lab/sim/objects/articulation.py |
Writes USD joint drive properties back into cfg.drive_pros when use_usd_properties=True |
tests/sim/objects/test_usd.py |
Adds export_usd helper method, updates init_pos values to avoid collisions, calls export from __main__ |
scripts/tutorials/sim/export_usd.py |
New tutorial demonstrating scene creation and USD export |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scoop_cfg = RigidObjectCfg( | ||
| uid="cup", | ||
| shape=MeshCfg( | ||
| fpath=get_data_path("MultiW1Data/paper_cup_2.obj"), | ||
| ), | ||
| attrs=RigidBodyAttributesCfg( | ||
| mass=0.3, | ||
| ), | ||
| max_convex_hull_num=1, | ||
| body_type="dynamic", | ||
| init_pos=[0.86, -0.76, 0.841], | ||
| init_rot=[0.0, 0.0, 0.0], | ||
| ) | ||
| scoop = sim.add_rigid_object(cfg=scoop_cfg) | ||
| return scoop |
There was a problem hiding this comment.
Same issue as create_table: the variable names scoop_cfg and scoop are misleading in a function that creates a cup. Consider renaming them to cup_cfg and cup to match the object being created.
| if not args.headless: | ||
| sim.open_window() | ||
| logger.log_info("Press Ctrl+C to exit.") | ||
| try: | ||
| while True: | ||
| sim.update(step=1) | ||
| except KeyboardInterrupt: | ||
| logger.log_info("\nExit") |
There was a problem hiding this comment.
All other tutorial scripts in this directory (e.g., create_scene.py, create_robot.py, import_usd.py) call sim.destroy() for cleanup when the simulation loop ends. This script is missing that cleanup call, which could leak resources. Consider adding sim.destroy() after the simulation loop (or in a finally block) to follow the established convention.
| cfg.attrs = RigidBodyAttributesCfg().from_dict( | ||
| first_entity.get_physical_attr().as_dict() | ||
| ) |
There was a problem hiding this comment.
RigidBodyAttributesCfg().from_dict(...) unnecessarily creates a throwaway instance before calling the classmethod from_dict. This should be RigidBodyAttributesCfg.from_dict(...) — calling the classmethod directly on the class rather than on a discarded instance.
| # Write the USD properties back to cfg | ||
| usd_drive_pros = self.cfg.drive_pros | ||
| usd_drive_pros.stiffness = ( | ||
| self.default_joint_stiffness[0].cpu().numpy().tolist() | ||
| ) | ||
| usd_drive_pros.damping = ( | ||
| self.default_joint_damping[0].cpu().numpy().tolist() | ||
| ) | ||
| usd_drive_pros.friction = ( | ||
| self.default_joint_friction[0].cpu().numpy().tolist() | ||
| ) | ||
| usd_drive_pros.max_effort = ( | ||
| self.default_joint_max_effort[0].cpu().numpy().tolist() | ||
| ) | ||
| usd_drive_pros.max_velocity = ( | ||
| self.default_joint_max_velocity[0].cpu().numpy().tolist() | ||
| ) |
There was a problem hiding this comment.
The JointDrivePropertiesCfg fields (stiffness, damping, friction, max_effort, max_velocity) are typed as Union[Dict[str, float], float]. However, .cpu().numpy().tolist() on a 1D tensor produces a list[float], which doesn't conform to either declared type. While this won't cause runtime issues in the current code path (since _set_default_joint_drive is only called in the not use_usd_properties branch), it could cause problems if code downstream of this point inspects the cfg and expects either a scalar or a dict. Consider updating the type annotation of JointDrivePropertiesCfg to include List[float] as a valid type, or documenting that after USD property readback, these fields contain per-joint lists.
| def export_usd(self): | ||
| self.sim.export_usd("test_export.usda") |
There was a problem hiding this comment.
The export_usd method is not prefixed with test_, so pytest will not discover or run it automatically as part of the test suite. It is only called when the file is executed directly via __main__. If this is intended to be a test, rename it to test_export_usd so pytest discovers it. If it's intentionally a helper method, consider adding a proper test_export_usd method that calls it and verifies the result (e.g., checking the return value of self.sim.export_usd() or verifying the file was created).
| return scoop | ||
|
|
||
|
|
||
| def create_caffe(sim: SimulationManager) -> Robot: |
There was a problem hiding this comment.
create_caffe has return type annotation -> Robot, but sim.add_articulation() returns Articulation, not Robot. Since Robot is a subclass of Articulation, this annotation incorrectly promises a more specific type than what is actually returned. The return type should be -> Articulation. You would also need to add Articulation to the imports from embodichain.lab.sim.objects.
| scoop_cfg = RigidObjectCfg( | ||
| uid="table", | ||
| shape=MeshCfg( | ||
| fpath=get_data_path("MultiW1Data/table_a.obj"), | ||
| ), | ||
| attrs=RigidBodyAttributesCfg( | ||
| mass=0.5, | ||
| ), | ||
| max_convex_hull_num=8, | ||
| body_type="kinematic", | ||
| init_pos=[1.1, -0.5, 0.08], | ||
| init_rot=[0.0, 0.0, 0.0], | ||
| ) | ||
| scoop = sim.add_rigid_object(cfg=scoop_cfg) | ||
| return scoop |
There was a problem hiding this comment.
The variable names scoop_cfg and scoop are misleading in a function that creates a table. These were likely copied from another function that created a scoop object. Consider renaming them to table_cfg and table (or similar) to match the object being created.
| self.default_joint_max_effort = self._data.qf_limits.clone() | ||
| self.default_joint_max_velocity = self._data.qvel_limits.clone() | ||
|
|
||
| # Write the USD properties back to cfg |
There was a problem hiding this comment.
Apart from joint drive properties, we also have link physics properties
There was a problem hiding this comment.
After discussion, this commit will not include this change
| bool: True if export is successful, False otherwise. | ||
| """ | ||
| try: | ||
| self._env.export_to_usd_file(fpath) |
There was a problem hiding this comment.
We should support exclusive uid list. (eg, remove ground plane from export)
There was a problem hiding this comment.
After discussion, this commit will not include this change
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Write the USD properties back to cfg | ||
| usd_drive_pros = self.cfg.drive_pros | ||
| usd_drive_pros.stiffness = ( | ||
| self.default_joint_stiffness[0].cpu().numpy().tolist() | ||
| ) | ||
| usd_drive_pros.damping = ( | ||
| self.default_joint_damping[0].cpu().numpy().tolist() | ||
| ) | ||
| usd_drive_pros.friction = ( | ||
| self.default_joint_friction[0].cpu().numpy().tolist() | ||
| ) | ||
| usd_drive_pros.max_effort = ( | ||
| self.default_joint_max_effort[0].cpu().numpy().tolist() | ||
| ) | ||
| usd_drive_pros.max_velocity = ( | ||
| self.default_joint_max_velocity[0].cpu().numpy().tolist() | ||
| ) | ||
|
|
There was a problem hiding this comment.
When use_usd_properties=True, this code writes per-DOF arrays (lists) back into JointDrivePropertiesCfg fields (stiffness, damping, etc.). Those fields are typed/handled as float | Dict[str, float] elsewhere (e.g., _set_default_joint_drive expects a number or a name->value mapping), so setting them to list can break later drive setup or user overrides. Consider storing USD-derived values as a Dict[joint_name, float] (or keep them on the Articulation instance) rather than mutating cfg.drive_pros to a list.
| # Write the USD properties back to cfg | |
| usd_drive_pros = self.cfg.drive_pros | |
| usd_drive_pros.stiffness = ( | |
| self.default_joint_stiffness[0].cpu().numpy().tolist() | |
| ) | |
| usd_drive_pros.damping = ( | |
| self.default_joint_damping[0].cpu().numpy().tolist() | |
| ) | |
| usd_drive_pros.friction = ( | |
| self.default_joint_friction[0].cpu().numpy().tolist() | |
| ) | |
| usd_drive_pros.max_effort = ( | |
| self.default_joint_max_effort[0].cpu().numpy().tolist() | |
| ) | |
| usd_drive_pros.max_velocity = ( | |
| self.default_joint_max_velocity[0].cpu().numpy().tolist() | |
| ) | |
| # NOTE: | |
| # We intentionally do not write these per-DOF USD properties back into | |
| # `self.cfg.drive_pros`, because `JointDrivePropertiesCfg` fields such | |
| # as `stiffness`, `damping`, etc. are expected to be either a scalar | |
| # float or a Dict[str, float], not a list. The tensors above should | |
| # be used directly wherever the full per-DOF information is required. |
| def export_usd(self): | ||
| self.sim.export_usd("test_export.usda") | ||
|
|
There was a problem hiding this comment.
export_usd() is not a pytest test (no test_ prefix) and it doesn't assert anything about the export result. As written, the new SimulationManager.export_usd() functionality isn't covered by CI. Consider adding a real test (e.g., def test_export_usd(...)) that exports to a temporary directory and asserts the file exists and is non-empty, and cleans it up afterward.
| def create_caffe(sim: SimulationManager) -> Robot: | ||
| """ | ||
| Create a caffe (container) articulated object in the simulation. | ||
|
|
||
| Args: | ||
| sim (SimulationManager): The simulation manager instance. | ||
|
|
||
| Returns: | ||
| Robot: The caffe object added to the simulation. | ||
| """ |
There was a problem hiding this comment.
create_caffe constructs an articulation via sim.add_articulation(...), but the return type annotation and docstring claim it returns a Robot. This is misleading for readers/tools and can cause type-checking issues; update the annotation/docstring to the correct type (likely Articulation).
| robot = create_robot(sim) | ||
| table = create_table(sim) | ||
| caffe = create_caffe(sim) | ||
| cup = create_cup(sim) | ||
|
|
||
| sim.export_usd("w1_coffee_scene.usda") | ||
|
|
||
| logger.log_info("Scene exported successfully.") | ||
|
|
||
| if not args.headless: | ||
| sim.open_window() | ||
| logger.log_info("Press Ctrl+C to exit.") | ||
| try: | ||
| while True: | ||
| sim.update(step=1) | ||
| except KeyboardInterrupt: | ||
| logger.log_info("\nExit") | ||
|
|
||
|
|
There was a problem hiding this comment.
The window loop catches KeyboardInterrupt but never calls sim.destroy(). Most tutorial scripts clean up in a finally block to release resources; add a finally: sim.destroy() (and possibly handle the headless/no-window path too) so the script exits cleanly.
| robot = create_robot(sim) | |
| table = create_table(sim) | |
| caffe = create_caffe(sim) | |
| cup = create_cup(sim) | |
| sim.export_usd("w1_coffee_scene.usda") | |
| logger.log_info("Scene exported successfully.") | |
| if not args.headless: | |
| sim.open_window() | |
| logger.log_info("Press Ctrl+C to exit.") | |
| try: | |
| while True: | |
| sim.update(step=1) | |
| except KeyboardInterrupt: | |
| logger.log_info("\nExit") | |
| try: | |
| robot = create_robot(sim) | |
| table = create_table(sim) | |
| caffe = create_caffe(sim) | |
| cup = create_cup(sim) | |
| sim.export_usd("w1_coffee_scene.usda") | |
| logger.log_info("Scene exported successfully.") | |
| if not args.headless: | |
| sim.open_window() | |
| logger.log_info("Press Ctrl+C to exit.") | |
| try: | |
| while True: | |
| sim.update(step=1) | |
| except KeyboardInterrupt: | |
| logger.log_info("\nExit") | |
| finally: | |
| sim.destroy() |
| else: | ||
| # Read current properties from USD-loaded entities and write back to cfg | ||
| # Use first entity as reference | ||
| first_entity: MeshObject = entities[0] | ||
|
|
There was a problem hiding this comment.
entities[0] is accessed when cfg.use_usd_properties is True, but load_mesh_objects_from_cfg() can return an empty list (e.g., USD import finds no rigid bodies and only logs an error). This will raise IndexError during object construction. Add a guard that validates entities is non-empty and raise/log a clear error before reading USD properties (or fall back to cfg defaults).
Description
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
black .command to format the code base.