Skip to content

feat: import usd#127

Merged
MahooX merged 14 commits intomainfrom
feat-usd
Mar 3, 2026
Merged

feat: import usd#127
MahooX merged 14 commits intomainfrom
feat-usd

Conversation

@MahooX
Copy link
Collaborator

@MahooX MahooX commented Feb 10, 2026

Description

Support importing USD file
This pull request adds support for importing and configuring simulation objects from USD files, allowing users to choose whether to use physical properties defined in the USD file or override them with configuration values.
Only one env is supported now.

Documentation and Tutorial:

  • Added a new tutorial script import_usd.py demonstrating how to set up a simulation scene, import objects from USD files, and use the new configuration options.

Type of change

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds initial USD import support to the simulation stack, including an option to keep physics/drive properties authored in USD instead of overriding them from config, plus a tutorial demonstrating usage (single-env only).

Changes:

  • Add use_usd_properties to RigidObjectCfg and ArticulationCfg to control whether config overrides USD-authored properties.
  • Implement USD loading paths for mesh rigid objects and for articulations/robots.
  • Add a new tutorial script scripts/tutorials/sim/import_usd.py demonstrating USD import.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
scripts/tutorials/sim/import_usd.py New tutorial showcasing importing rigid objects/articulations from USD.
embodichain/lab/sim/utility/sim_utils.py Adds USD import handling for mesh rigid objects in load_mesh_objects_from_cfg.
embodichain/lab/sim/sim_manager.py Adds USD import handling for add_articulation and add_robot.
embodichain/lab/sim/objects/rigid_object.py Skips applying config physical attrs when use_usd_properties=True.
embodichain/lab/sim/objects/articulation.py Skips applying config articulation settings/drive defaults when use_usd_properties=True.
embodichain/lab/sim/cfg.py Introduces the new use_usd_properties configuration flags and docs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +224 to +237
# TODO: currently not supporting multiple arenas for USD
_env:dexsim.environment.Env = dexsim.default_world().get_env()
results = _env.import_from_usd_file(fpath,return_object=True)
print(f"import usd result: {results}")

rigidbodys_found=[]
for key,value in results.items():
if isinstance(value, dexsim.cuda.pybind.models.MeshObject):
rigidbodys_found.append(value)
if len(rigidbodys_found)==0:
logger.log_error(f"No rigid body found in USD file: {fpath}")
elif len(rigidbodys_found)>1:
logger.log_error(f"Multiple rigid bodies found in USD file: {fpath}.")
elif len(rigidbodys_found)==1:
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

USD import currently ignores the provided env_list and always imports into dexsim.default_world().get_env(), then returns a single object. If env_list contains multiple arenas/envs, SimulationManager.add_rigid_object() will receive an obj_list whose length doesn’t match the number of envs. Since the code explicitly says multiple arenas aren’t supported, add an explicit guard (e.g., raise via logger.log_error when len(env_list) != 1) and import into env_list[0] to avoid silently creating mismatched batches.

Suggested change
# TODO: currently not supporting multiple arenas for USD
_env:dexsim.environment.Env = dexsim.default_world().get_env()
results = _env.import_from_usd_file(fpath,return_object=True)
print(f"import usd result: {results}")
rigidbodys_found=[]
for key,value in results.items():
if isinstance(value, dexsim.cuda.pybind.models.MeshObject):
rigidbodys_found.append(value)
if len(rigidbodys_found)==0:
logger.log_error(f"No rigid body found in USD file: {fpath}")
elif len(rigidbodys_found)>1:
logger.log_error(f"Multiple rigid bodies found in USD file: {fpath}.")
elif len(rigidbodys_found)==1:
# TODO: currently not supporting multiple arenas for USD
if len(env_list) != 1:
logger.log_error(
f"USD import currently supports exactly one environment, "
f"but got {len(env_list)}."
)
_env: dexsim.environment.Env = env_list[0]
results = _env.import_from_usd_file(fpath, return_object=True)
print(f"import usd result: {results}")
rigidbodys_found = []
for key, value in results.items():
if isinstance(value, dexsim.cuda.pybind.models.MeshObject):
rigidbodys_found.append(value)
if len(rigidbodys_found) == 0:
logger.log_error(f"No rigid body found in USD file: {fpath}")
elif len(rigidbodys_found) > 1:
logger.log_error(f"Multiple rigid bodies found in USD file: {fpath}.")
elif len(rigidbodys_found) == 1:

Copilot uses AI. Check for mistakes.
Comment on lines +585 to +588
if not cfg.use_usd_properties:
# Set articulation configuration in DexSim
set_dexsim_articulation_cfg(entities, self.cfg)

Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

Articulation.__init__ now skips set_dexsim_articulation_cfg(...) when cfg.use_usd_properties is True, regardless of whether the asset was actually loaded from USD. This contradicts ArticulationCfg.use_usd_properties docstring (“ignored for URDF files”) and can lead to URDF-loaded articulations missing expected config application. Consider forcing use_usd_properties=False for non-USD fpath (similar to what load_mesh_objects_from_cfg does for non-USD meshes) or gating this behavior on file extension.

Copilot uses AI. Check for mistakes.
is_usd = cfg.fpath.endswith((".usd", ".usda", ".usdc"))
if is_usd:
# TODO: currently not supporting multiple arenas for USD
env= self._env
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

The USD import path explicitly notes it doesn’t support multiple arenas, but it doesn’t enforce that constraint: it always imports into self._env even when self._arenas is populated. This can produce batch-size mismatches and downstream indexing errors. Add an explicit check (e.g., raise when len(self._arenas) > 0) so multi-env configurations fail fast with a clear error.

Suggested change
env= self._env
if len(self._arenas) > 0:
raise RuntimeError(
"USD articulations currently support only a single arena, "
f"but {len(self._arenas)} arenas are configured. "
"Please use a single environment or a non-USD asset format."
)
env = self._env

Copilot uses AI. Check for mistakes.
Comment on lines +550 to +554
"""Whether to use physical properties from USD file instead of config.

When True: Keep all physical properties (drive, physics attrs, etc.) from USD file.
When False (default): Override USD properties with config values (URDF behavior).
Only effective for USD files, ignored for URDF files.
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

RigidObjectCfg.use_usd_properties docstring mentions “URDF behavior” and “drive” properties, but rigid objects are mesh-based and don’t have URDF or joint drives in this codepath. Consider rewording to avoid confusion (e.g., “Only effective for USD mesh files; ignored for non-USD meshes” and remove “drive” wording).

Suggested change
"""Whether to use physical properties from USD file instead of config.
When True: Keep all physical properties (drive, physics attrs, etc.) from USD file.
When False (default): Override USD properties with config values (URDF behavior).
Only effective for USD files, ignored for URDF files.
"""Whether to use physical properties stored in the USD asset instead of config values.
When True: keep physics-related attributes (e.g., collision, mass, physical materials) defined in the USD file.
When False (default): override physics-related attributes from the USD file with values from this config.
Only effective when loading USD mesh assets; ignored for non-USD mesh formats.

Copilot uses AI. Check for mistakes.
Comment on lines +1117 to +1122
is_usd = cfg.fpath.endswith((".usd", ".usda", ".usdc"))
if is_usd:
# TODO: currently not supporting multiple arenas for USD
env= self._env
results = env.import_from_usd_file(cfg.fpath, return_object=True)
print("USD import results:", results)
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

Similar to add_articulation(), the USD import path in add_robot() doesn’t support multiple arenas but doesn’t enforce it: it always imports into self._env even if self._arenas is populated. Add an explicit guard (raise when len(self._arenas) > 0) to prevent creating robots with a batch size that doesn’t match the simulation’s number of envs.

Copilot uses AI. Check for mistakes.
from embodichain.lab.sim.cfg import RigidBodyAttributesCfg
from embodichain.lab.sim.shapes import CubeCfg, MeshCfg
from embodichain.lab.sim.objects import RigidObject, RigidObjectCfg,ArticulationCfg,Articulation
from dexsim.utility.path import get_resources_data_path
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

Import of 'get_resources_data_path' is not used.

Suggested change
from dexsim.utility.path import get_resources_data_path

Copilot uses AI. Check for mistakes.
@yuecideng yuecideng marked this pull request as draft February 10, 2026 05:38
for entity in entities:
entity.set_body_scale(*cfg.body_scale)
entity.set_physical_attr(cfg.attrs.attr())
if not cfg.use_usd_properties:
Copy link
Contributor

Choose a reason for hiding this comment

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

Also check whether the fpath is USD file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added check

set_dexsim_articulation_cfg(entities, self.cfg)

# Init joint drive parameters.
num_entities = len(entities)
Copy link
Contributor

Choose a reason for hiding this comment

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

These parts must be set and should not be placed inside the if condition. self._set_default_joint_drive() could be placed here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adjusted


is_usd = fpath.endswith((".usd", ".usda", ".usdc"))
if is_usd:
# TODO: currently not supporting multiple arenas for USD
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add log_error here to check the num_envs number

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added log_error

is_usd = fpath.endswith((".usd", ".usda", ".usdc"))
if is_usd:
# TODO: currently not supporting multiple arenas for USD
_env:dexsim.environment.Env = dexsim.default_world().get_env()
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use the first arena from env_list

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

usd-related API are now in class env. They will need to be moved to arena later.

# ----------------------------------------------------------------------------

"""
This script demonstrates how to create a simulation scene using SimulationManager.
Copy link
Contributor

Choose a reason for hiding this comment

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

Change description

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed description related to USD

)
)

usdpath="/home/xiemh/model/004_sugar_box/004_sugar_box_xmh.usda"
Copy link
Contributor

Choose a reason for hiding this comment

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

Add to huggingface when this PR is ready to merged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Uploaded assets to huggingface and deleted hardcode fpath

@@ -0,0 +1,164 @@
# ----------------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Also add docs for usd

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added docs

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 14 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +224 to +227
# TODO: currently not supporting multiple arenas for USD
_env: dexsim.environment.Env = dexsim.default_world().get_env()
results = _env.import_from_usd_file(fpath, return_object=True)
print(f"import usd result: {results}")
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

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

USD import bypasses the provided env_list by calling dexsim.default_world().get_env(), which can import into a different world/env than the SimulationManager passed into this function. Use env_list[0] (and explicitly error if len(env_list) != 1 since USD multi-arena isn’t supported yet) and avoid print() in library code (use the project logger or remove).

Suggested change
# TODO: currently not supporting multiple arenas for USD
_env: dexsim.environment.Env = dexsim.default_world().get_env()
results = _env.import_from_usd_file(fpath, return_object=True)
print(f"import usd result: {results}")
# USD import currently supports exactly one arena/env.
if len(env_list) != 1:
raise ValueError(
f"USD import currently supports exactly one arena/env, but got {len(env_list)}."
)
_env = env_list[0]
results = _env.import_from_usd_file(fpath, return_object=True)

Copilot uses AI. Check for mistakes.
obj_list.append(art)
is_usd = cfg.fpath.endswith((".usd", ".usda", ".usdc"))
if is_usd:
# TODO: currently not supporting multiple arenas for USD
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

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

USD import path currently ignores env_list/arenas and always imports into self._env, but the method still proceeds even when self._arenas is non-empty. Since batch entities assume one entity per env, this will silently create an articulation with a single instance in a multi-arena simulation. Add an explicit check (e.g., error if len(env_list) != 1) before importing USD.

Suggested change
# TODO: currently not supporting multiple arenas for USD
# TODO: currently not supporting multiple arenas for USD
if len(env_list) != 1:
logger.log_error(
"USD articulation import currently supports exactly one environment; "
f"got {len(env_list)}. This can happen when using multiple arenas."
)
return None

Copilot uses AI. Check for mistakes.
Comment on lines +214 to +216
if not cfg.use_usd_properties:
for entity in entities:
entity.set_body_scale(*cfg.body_scale)
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

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

With use_usd_properties=True, this skips both set_physical_attr and set_body_scale. The cfg field name/docstring suggests only physics properties should come from USD, but this change also makes cfg.body_scale ineffective for USD assets. Either document that scale will also come from USD, or apply set_body_scale regardless and only skip the physical-attribute override.

Suggested change
if not cfg.use_usd_properties:
for entity in entities:
entity.set_body_scale(*cfg.body_scale)
for entity in entities:
# Always apply configured body scale, even when using USD properties.
entity.set_body_scale(*cfg.body_scale)
# Only override physical attributes from cfg when not using USD properties.
if not cfg.use_usd_properties:

Copilot uses AI. Check for mistakes.
Comment on lines +214 to +217
if not cfg.use_usd_properties:
for entity in entities:
entity.set_body_scale(*cfg.body_scale)
entity.set_physical_attr(cfg.attrs.attr())
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

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

There are existing tests for rigid objects under tests/sim/objects/; this new use_usd_properties behavior isn’t covered. Add a unit test that verifies config-driven physical attributes are applied when use_usd_properties=False and are not overridden when True (can be done with a stub/mocked MeshObject).

Copilot uses AI. Check for mistakes.
Comment on lines +94 to +114
usdpath = "/home/xiemh/model/004_sugar_box/004_sugar_box_xmh.usda"
sugar_box: RigidObject = sim.add_rigid_object(
cfg=RigidObjectCfg(
uid="sugar_box",
shape=MeshCfg(fpath=usdpath),
body_type="dynamic",
init_pos=[0.2, 0.2, 1.0],
use_usd_properties=True,
)
)

# Add objects to the scene
h1: Articulation = sim.add_articulation(
cfg=ArticulationCfg(
uid="h1",
# fpath="/home/xiemh/model/Collected_ur10/ur10.usd",
fpath="/home/xiemh/model/Collected_h1/h1.usda",
build_pk_chain=False,
init_pos=[-0.2, -0.2, 1.0],
use_usd_properties=False,
)
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

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

The tutorial hard-codes absolute local USD paths (e.g., /home/...). This will break for anyone else and for CI/doc builds. Prefer accepting these as CLI args and/or building paths from packaged assets (you already import get_resources_data_path, but it’s unused).

Copilot uses AI. Check for mistakes.
Comment on lines +585 to +588
if not cfg.use_usd_properties:
# Set articulation configuration in DexSim
set_dexsim_articulation_cfg(entities, self.cfg)

Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

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

There are existing articulation/robot tests under tests/sim/objects/; the new use_usd_properties branch isn’t covered. Add a unit test to confirm that drive/physical overrides happen when False and are skipped when True (can be validated with a stub articulation entity that records setter calls).

Copilot uses AI. Check for mistakes.
@yuecideng yuecideng marked this pull request as ready for review February 28, 2026 08:16


if __name__ == "__main__":
main()
Copy link
Contributor

Choose a reason for hiding this comment

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

Also add two test cases for rigid object and articulation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added tests/sim/objects/test_usd.py

Copilot AI review requested due to automatic review settings March 2, 2026 04:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 586 to 590
is_usd_file = cfg.fpath.endswith((".usd", ".usda", ".usdc"))
use_cfg_properties = not (cfg.use_usd_properties and is_usd_file)

# Set articulation configuration in DexSim
set_dexsim_articulation_cfg(entities, self.cfg)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

When use_usd_properties=True for a USD articulation, this still calls set_dexsim_articulation_cfg(...), which sets body scale and physical attributes from the config and therefore overrides USD-authored physical properties. Gate those physical-attribute writes behind use_cfg_properties (or update the flag/docstring to reflect what is actually preserved). Also note build_pk_chain=True will later attempt to parse the USD path as URDF; consider auto-disabling pk-chain building for USD or raising a clear error.

Copilot uses AI. Check for mistakes.
# Create the simulation instance
sim = SimulationManager(sim_cfg)
# Open window when the scene has been set up
if not args.headless:
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this part after objects loading

Copilot AI review requested due to automatic review settings March 2, 2026 10:15
@MahooX MahooX changed the title draft: import usd feat: import usd Mar 2, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings March 2, 2026 10:43
xiemenghong and others added 7 commits March 2, 2026 18:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +57 to +66
sim_cfg = SimulationManagerCfg(
width=1920,
height=1080,
headless=True,
physics_dt=1.0 / 100.0, # Physics timestep (100 Hz)
sim_device=args.device,
enable_rt=True, # Enable ray tracing for better visuals
num_envs=1,
arena_space=3.0,
)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

SimulationManagerCfg is created with headless=True unconditionally, so the --headless CLI flag doesn’t affect window creation and the if not args.headless: sim.open_window() path is inconsistent with the config. Use headless=args.headless (and consider making enable_rt configurable as well) so the tutorial behaves as the CLI implies.

Copilot uses AI. Check for mistakes.
Comment on lines +44 to +45
sugar_box_path = get_data_path("SugarBox/sugar_box_usd/sugar_box.usda")
sugar_box: RigidObject = self.sim.add_rigid_object(
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

These tests resolve USD asset paths but don’t assert that the files exist. Adding os.path.isfile(...) assertions (as done in other sim object tests) will make dataset download/extraction failures much easier to diagnose.

Copilot uses AI. Check for mistakes.
Comment on lines +81 to +100
```python
from embodichain.data import get_data_path

# Import USD with properties from file
usd_cfg = RigidObjectCfg(
shape=MeshCfg(fpath=get_data_path("path/to/object.usd")),
body_type="dynamic",
use_usd_properties=True # Keep USD properties
)
obj = sim.add_rigid_object(cfg=usd_cfg)

# Or override USD properties with config
usd_cfg_override = RigidObjectCfg(
shape=MeshCfg(fpath=get_data_path("path/to/object.usd")),
body_type="dynamic",
use_usd_properties=False, # Use config instead
attrs=RigidBodyAttributesCfg(mass=2.0)
)
obj2 = sim.add_rigid_object(cfg=usd_cfg_override)
```
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

The USD import documentation snippet is missing required imports (RigidObjectCfg, MeshCfg, RigidBodyAttributesCfg) and relies on an existing sim variable, so it won’t run as written when copied. Include the necessary imports (or reference the earlier setup snippet) so the example is executable.

Copilot uses AI. Check for mistakes.
use_usd_properties=True,
init_pos=[0.0, 0.0, 0.1],
)
)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

test_import_rigid doesn’t assert anything about the imported object (it will pass as long as no exception is raised). Add at least one assertion (e.g., returned handle is not None, has expected uid, and/or that use_usd_properties affects whether config attrs override USD) to ensure the new behavior is actually validated.

Suggested change
)
)
# Basic validation that the rigid object was imported as expected.
assert sugar_box is not None
assert isinstance(sugar_box, RigidObject)
if hasattr(sugar_box, "uid"):
assert sugar_box.uid == "sugar_box"

Copilot uses AI. Check for mistakes.
# TODO: Currently add checking for num_envs when file is USD. After we support spawn via cloning, we can remove this.
if len(env_list) > 1:
logger.log_error(f"Currently not supporting multiple arenas for USD.")
env = self._env
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

Same as add_articulation: the USD import branch uses self._env instead of the single target arena (env_list[0]). In num_envs>=1 this can spawn the robot outside the arena grid and produces behavior inconsistent with URDF loading.

Suggested change
env = self._env
# Use the same arena selection logic as for URDF loading
env = env_list[0]

Copilot uses AI. Check for mistakes.
Comment on lines +70 to +88
```python
from embodichain.data import get_data_path

# Import USD with properties from file
usd_art_cfg = ArticulationCfg(
fpath=get_data_path("path/to/robot.usd"),
init_pos=(0, 0, 0.5),
use_usd_properties=True # Keep USD drive/physics properties
)
usd_robot = sim.add_articulation(cfg=usd_art_cfg)

# Or override USD properties with config (URDF behavior)
usd_art_cfg_override = ArticulationCfg(
fpath=get_data_path("path/to/robot.usd"),
init_pos=(0, 0, 0.5),
use_usd_properties=False, # Use config instead
drive_props=JointDrivePropertiesCfg(stiffness=5000, damping=500)
)
robot = sim.add_articulation(cfg=usd_art_cfg_override)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

This USD import example uses JointDrivePropertiesCfg but doesn’t import it, and it also assumes ArticulationCfg/sim are already in scope. Add the missing imports (and/or include a minimal complete snippet) so the documentation example works when copied.

Copilot uses AI. Check for mistakes.
| Parameter | Type | Default | Description |
| :--- | :--- | :--- | :--- |
| `fpath` | `str` | `None` | Path to the asset file (URDF/MJCF). |
| `fpath` | `str` | `None` | Path to the asset file (URDF/MJCF/USD). |
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove MJCF as we are not supported.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed

NUM_ARENAS = 1


class BaseUsdTest:
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add test for property check under use_usd_properties equal to True and False

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

checked usd attributes in unit test

Copilot AI review requested due to automatic review settings March 3, 2026 08:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +131 to +142
if target_joint_name in joint_names:
joint_idx = joint_names.index(target_joint_name)
# check for the first instance
assert torch.isclose(
stiffness[0, joint_idx], torch.tensor(10000000.0, dtype=torch.float32)
)
assert torch.isclose(
damping[0, joint_idx], torch.tensor(0.0, dtype=torch.float32)
)
assert torch.isclose(
max_force[0, joint_idx], torch.tensor(200.0, dtype=torch.float32)
)
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

target_joint_name is checked with an if and the test silently does nothing if the joint name isn't present. Since this test is meant to validate USD-imported drive properties, it should fail loudly (or pytest.skip with a clear message) when the expected joint is missing from the asset.

Suggested change
if target_joint_name in joint_names:
joint_idx = joint_names.index(target_joint_name)
# check for the first instance
assert torch.isclose(
stiffness[0, joint_idx], torch.tensor(10000000.0, dtype=torch.float32)
)
assert torch.isclose(
damping[0, joint_idx], torch.tensor(0.0, dtype=torch.float32)
)
assert torch.isclose(
max_force[0, joint_idx], torch.tensor(200.0, dtype=torch.float32)
)
assert (
target_joint_name in joint_names
), f"Expected joint '{target_joint_name}' not found in USD asset. Available joints: {joint_names}"
joint_idx = joint_names.index(target_joint_name)
# check for the first instance
assert torch.isclose(
stiffness[0, joint_idx], torch.tensor(10000000.0, dtype=torch.float32)
)
assert torch.isclose(
damping[0, joint_idx], torch.tensor(0.0, dtype=torch.float32)
)
assert torch.isclose(
max_force[0, joint_idx], torch.tensor(200.0, dtype=torch.float32)
)

Copilot uses AI. Check for mistakes.
Comment on lines +64 to +68
assert pytest.approx(body0.get_mass()) == default_attr.mass
assert pytest.approx(body0.get_linear_damping()) == default_attr.linear_damping
assert (
pytest.approx(body0.get_angular_damping()) == default_attr.angular_damping
)
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The pytest.approx(...) assertions are written in a reversed/ambiguous way (e.g., pytest.approx(body0.get_mass()) == default_attr.mass). The recommended/clear pattern is assert body0.get_mass() == pytest.approx(default_attr.mass) (and similarly for damping). This also avoids tolerance being computed relative to the actual value rather than the expected value.

Suggested change
assert pytest.approx(body0.get_mass()) == default_attr.mass
assert pytest.approx(body0.get_linear_damping()) == default_attr.linear_damping
assert (
pytest.approx(body0.get_angular_damping()) == default_attr.angular_damping
)
assert body0.get_mass() == pytest.approx(default_attr.mass)
assert body0.get_linear_damping() == pytest.approx(default_attr.linear_damping)
assert body0.get_angular_damping() == pytest.approx(default_attr.angular_damping)

Copilot uses AI. Check for mistakes.
)
body0 = sugar_box._entities[0].get_physical_body()
print(sugar_box._entities[0].get_physical_attr())
assert pytest.approx(body0.get_mass(), 0.001) == 0.514
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

This assertion uses pytest.approx(body0.get_mass(), 0.001) == 0.514, which (1) reverses the usual actual/expected ordering and (2) passes 0.001 as a positional rel tolerance rather than an explicit abs. Prefer assert body0.get_mass() == pytest.approx(0.514, abs=0.001) for clarity and correct tolerance semantics.

Suggested change
assert pytest.approx(body0.get_mass(), 0.001) == 0.514
assert body0.get_mass() == pytest.approx(0.514, abs=0.001)

Copilot uses AI. Check for mistakes.
fpath=get_data_path("path/to/robot.usd"),
init_pos=(0, 0, 0.5),
use_usd_properties=False, # Use config instead
drive_props=JointDrivePropertiesCfg(stiffness=5000, damping=500)
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The USD import doc example uses drive_props=..., but the actual config field is drive_pros (see ArticulationCfg.drive_pros in embodichain/lab/sim/cfg.py). As written, the example will raise a TypeError for an unexpected keyword argument. Update the docs to use the correct parameter name.

Suggested change
drive_props=JointDrivePropertiesCfg(stiffness=5000, damping=500)
drive_pros=JointDrivePropertiesCfg(stiffness=5000, damping=500)

Copilot uses AI. Check for mistakes.
local_env_ids = self._all_indices if env_ids is None else env_ids
num_instances = len(local_env_ids)
self.set_attrs(self.cfg.attrs, env_ids=local_env_ids)
# self.set_attrs(self.cfg.attrs, env_ids=local_env_ids)
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

RigidObject.reset() no longer restores physical attributes because the call to set_attrs(self.cfg.attrs, ...) was commented out. This changes reset semantics for all rigid objects (including non-USD) and can lead to state drift across episodes. If the intent is only to preserve USD-loaded properties, gate this behavior on cfg.use_usd_properties/USD file type rather than disabling it unconditionally.

Suggested change
# self.set_attrs(self.cfg.attrs, env_ids=local_env_ids)
# Reset physical attributes unless we explicitly want to preserve USD-authored properties.
if not getattr(self.cfg, "use_usd_properties", False):
self.set_attrs(self.cfg.attrs, env_ids=local_env_ids)

Copilot uses AI. Check for mistakes.
Comment on lines +50 to +52
default_attr = RigidBodyAttributesCfg()
sugar_box_path = get_data_path("SugarBox/sugar_box_usd/sugar_box.usda")
sugar_box: RigidObject = self.sim.add_rigid_object(
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

These tests use get_data_path(...) but (unlike other sim tests) do not assert that the resolved USD files actually exist. Adding os.path.isfile(...) checks for sugar_box_path and h1_path would make dataset download/extraction failures much easier to diagnose.

Copilot uses AI. Check for mistakes.
Comment on lines +98 to +100
expectied_damping = default_drive.damping
assert torch.allclose(
damping, torch.tensor(expectied_damping, dtype=torch.float32)
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

Typo in variable name: expectied_damping should be expected_damping for readability and to avoid propagating misspellings into future edits.

Suggested change
expectied_damping = default_drive.damping
assert torch.allclose(
damping, torch.tensor(expectied_damping, dtype=torch.float32)
expected_damping = default_drive.damping
assert torch.allclose(
damping, torch.tensor(expected_damping, dtype=torch.float32)

Copilot uses AI. Check for mistakes.
@MahooX MahooX merged commit 8a444fa into main Mar 3, 2026
17 of 19 checks passed
@MahooX MahooX deleted the feat-usd branch March 3, 2026 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants