Updates URDF/MJCF importer to use the latest Isaac Sim importer#5394
Conversation
stevfeng
requested review from
Mayankm96,
jtigue-bdai and
kellyguo11
as code owners
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR refactors the URDF and MJCF converters to delegate the full conversion pipeline to Isaac Sim's native importers (URDFImporter/URDFImporterConfig and MJCFImporter/MJCFImporterConfig). The PR removes ~750 lines of duplicated IsaacLab-specific post-processing code (fix-base insertion, joint drives, link density, ArticulationRootAPI relocation) and replaces it with a thin translation layer. The urdf_utils.py module is reduced to a re-export shim forwarding to the canonical Isaac Sim implementation.
Architecture Impact
High impact, well-contained. The changes affect the asset conversion pipeline which runs offline before simulation. The public API (UrdfConverterCfg, MjcfConverterCfg, merge_fixed_joints) is preserved. However:
- Downstream callers using
UrdfConverterCfg.collision_from_visualswill see a type annotation change (was missing, nowbool) - Tests validate the new pipeline produces equivalent USD output
- No runtime simulation impact — converters only affect USD generation
Implementation Verdict
Minor fixes needed. The refactor is architecturally sound, but there are a few issues with config field types, missing type annotations, and a potential test fragility.
Test Coverage
Good coverage. The test file has been updated and covers:
- Config change detection for lazy conversion
- Drive type/gains application (scalar and per-joint dict)
fix_basecreates FixedJointmerge_fixed_jointsXML-level and full pipeline- Deprecated
NaturalFrequencyGainsCfgwarning collision_from_visuals,self_collision,link_density
Missing: No test for ros_package_paths resolution with package:// URIs. This is a new config field without coverage.
CI Status
No CI checks available yet.
Findings
🔴 Critical: urdf_converter_cfg.py:174 — ros_package_paths type mismatch with Isaac Sim API
The config declares ros_package_paths: list[dict[str, str]] = [] and the docstring says "Each entry is a dictionary with keys name and path". However, in urdf_converter.py:96, it's passed as:
ros_package_paths=list(cfg.ros_package_paths),The URDFImporterConfig.ros_package_paths in Isaac Sim may expect a different structure (likely list[tuple[str, str]] or a flat dict). Without verifying the Isaac Sim API contract, this could cause silent failures or exceptions when users actually try to resolve package:// URIs. The test suite does not exercise this path.
🟡 Warning: mjcf_converter_cfg.py:91-92 — override_gain_prm and override_bias_prm should validate length
The docstrings specify these are "10 floats" but there's no validation. If a user passes a list of wrong length, the error will surface deep in the Isaac Sim importer with a confusing message. Consider adding a validate_config hook:
def validate_config(self):
if self.override_gain_prm is not None and len(self.override_gain_prm) != 10:
raise ValueError("override_gain_prm must have exactly 10 elements")🟡 Warning: urdf_converter.py:88-89 — collision_type literal values may not match Isaac Sim enum
The UrdfConverterCfg.collision_type accepts "Bounding Sphere" and "Bounding Cube" (with spaces), and this string is passed directly to URDFImporterConfig(collision_type=...). If the Isaac Sim importer expects different casing (e.g., "BoundingSphere") or an enum, this will fail. The test test_collider_type_convex_decomposition uses collider_type (which doesn't exist in the config — should be collision_type), so the test isn't actually validating this.
🔴 Critical: test_urdf_converter.py:622 — Test uses non-existent config field collider_type
config.collider_type = "convex_decomposition" # WRONG: field is `collision_type`This line sets a non-existent attribute. The test passes because the default collision_type="Convex Hull" is used. The test name and docstring claim to verify "Convex Decomposition" but it's actually testing "Convex Hull". Additionally, the value should be "Convex Decomposition" (with capitals and space) per the Literal type.
🟡 Warning: urdf_converter_cfg.py:171 — merge_mesh added but not forwarded to importer
In urdf_converter.py:_convert_asset, the config field merge_mesh is correctly forwarded. However, looking at the diff, this was an existing field that's now properly wired up. Good.
🔵 Improvement: urdf_utils.py — Missing fallback for Isaac Sim versions without the import
The module unconditionally imports from isaacsim.asset.importer.urdf.impl.urdf_utils. If a user has an older Isaac Sim version where this module doesn't exist, they'll get an ImportError at module load time. Consider a try/except with a clear error message:
try:
from isaacsim.asset.importer.urdf.impl.urdf_utils import merge_fixed_joints
except ImportError as e:
raise ImportError(
"merge_fixed_joints requires Isaac Sim 4.5+ with the updated URDF importer"
) from e🔵 Improvement: urdf_converter.py:153 — Deprecation warning for NaturalFrequencyGainsCfg should use warnings.warn consistently
The method uses warnings.warn with stacklevel=2, which is correct for showing the user's call site. However, the warning is only triggered inside _convert_asset → _warn_unsupported_features, meaning stacklevel=2 points to _convert_asset, not the user's UrdfConverter(cfg) call. Should be stacklevel=4 to trace through _warn_unsupported_features → _convert_asset → AssetConverterBase.__init__ → user code.
🔵 Improvement: CHANGELOG.rst:4 — Date is in the future (2026-04-24)
The changelog date appears to be a placeholder. Should be updated to the actual release date.
Greptile SummaryThis PR refactors the URDF and MJCF converters to delegate the full conversion pipeline to the latest Isaac Sim importers (
Confidence Score: 4/5Mostly safe to merge, but the per-joint JointDriveCfg dict handling is a silent regression for existing users of that feature. The refactor is well-structured and the MJCF changes are clean. The one P1 finding — per-joint dict values for drive_type/target_type/stiffness/damping being forwarded to scalar-expecting importer fields without a warning — is a real regression for users of that documented API. The fix is low-effort. source/isaaclab/isaaclab/sim/converters/urdf_converter.py — specifically _unpack_joint_drive and _warn_unsupported_features. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant UrdfConverter
participant _unpack_joint_drive
participant URDFImporterConfig
participant URDFImporter
User->>UrdfConverter: UrdfConverterCfg(fix_base, joint_drive, ...)
UrdfConverter->>_unpack_joint_drive: JointDriveCfg
_unpack_joint_drive-->>UrdfConverter: (drive_type, target_type, stiffness, damping)
UrdfConverter->>URDFImporterConfig: urdf_path, usd_path, merge_fixed_joints, collision_*, fix_base, link_density, joint_drive_type, override_joint_stiffness, run_asset_transformer, debug_mode
URDFImporterConfig-->>UrdfConverter: config
UrdfConverter->>URDFImporter: import_urdf()
URDFImporter-->>UrdfConverter: robot_name/robot_name.usda
Reviews (1): Last reviewed commit: "update urdf mjcf importer to use the lat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @staticmethod | ||
| def _apply_fix_base(stage): | ||
| """Add a fixed joint from the world to the root link of the robot. | ||
|
|
||
| Args: | ||
| stage: The USD stage to modify. | ||
| """ | ||
| from pxr import UsdPhysics | ||
|
|
||
| default_prim = stage.GetDefaultPrim() | ||
| if not default_prim or not default_prim.IsValid(): | ||
| carb.log_warn("UrdfConverter: Cannot apply fix_base - no default prim found.") | ||
| return | ||
|
|
||
| # find the root link: first child with `RigidBodyAPI` under the prim hierarchy | ||
| root_link = None | ||
| for prim in stage.Traverse(): | ||
| if prim.HasAPI(UsdPhysics.RigidBodyAPI): | ||
| root_link = prim | ||
| break | ||
|
|
||
| if root_link is None: | ||
| carb.log_warn("UrdfConverter: Cannot apply fix_base - no rigid body link found.") | ||
| return | ||
|
|
||
| # create a fixed joint connecting the world to the root link | ||
| default_prim_path = default_prim.GetPath() | ||
| joint_path = default_prim_path.AppendChild("fix_base_joint") | ||
|
|
||
| fixed_joint = UsdPhysics.FixedJoint.Define(stage, joint_path) | ||
| # `body0` left empty => connected to the world frame | ||
| fixed_joint.CreateBody1Rel().SetTargets([root_link.GetPath()]) | ||
|
|
||
| @staticmethod | ||
| def _fix_articulation_root_for_fixed_base(usd_path: str): | ||
| """Move ArticulationRootAPI from the root rigid body to its parent prim. | ||
|
|
||
| After the asset transformer, ArticulationRootAPI ends up on the root rigid body. | ||
| When combined with a FixedJoint on that same body (``fix_base_joint``), PhysX treats | ||
| the articulation as a floating-base + external constraint (maximal coordinate tree) | ||
| rather than a proper fixed-base reduced-coordinate articulation. | ||
|
|
||
| Moving ArticulationRootAPI to the parent of the root rigid body (a non-rigid Xform / | ||
| Scope ancestor) resolves this, matching the pattern used by ``schemas.py``'s | ||
| ``fix_root_link``. | ||
|
|
||
| Changes are authored as **local opinions in the root layer** of the stage, which are | ||
| stronger than the variant-payload-sublayer opinions written by the asset transformer. | ||
| This means the root layer's ``delete apiSchemas`` overrides the ``prepend apiSchemas`` | ||
| in the deeper sublayers without modifying those files. | ||
|
|
||
| Args: | ||
| usd_path: Absolute path to the final ``.usda`` file produced by the asset transformer. | ||
| """ | ||
| from pxr import Usd, UsdPhysics | ||
|
|
||
| stage = Usd.Stage.Open(usd_path) | ||
| if not stage: | ||
| carb.log_warn( | ||
| f"UrdfConverter: Cannot open final stage at '{usd_path}'" | ||
| " for fix_base ArticulationRootAPI post-processing." | ||
| ) | ||
| return | ||
|
|
||
| # Find the root rigid body that incorrectly has ArticulationRootAPI applied. | ||
| root_body_prim = None | ||
| for prim in stage.Traverse(): | ||
| if prim.HasAPI(UsdPhysics.ArticulationRootAPI) and prim.HasAPI(UsdPhysics.RigidBodyAPI): | ||
| root_body_prim = prim | ||
| break | ||
|
|
||
| if root_body_prim is None: | ||
| # ArticulationRootAPI is already on a non-rigid ancestor (correct) or not present. | ||
| return | ||
|
|
||
| parent_prim = root_body_prim.GetParent() | ||
| if not parent_prim or not parent_prim.IsValid(): | ||
| carb.log_warn("UrdfConverter: Root rigid body has no valid parent prim — skipping ArticulationRootAPI fix.") | ||
| return | ||
|
|
||
| # Collect all articulation-related schema names applied to the root rigid body. | ||
| articulation_api_names = [ | ||
| name | ||
| for name in root_body_prim.GetAppliedSchemas() | ||
| if "ArticulationRoot" in name or name == "PhysxArticulationAPI" | ||
| ] | ||
|
|
||
| # --- Apply ArticulationRootAPI schemas to the parent prim --- | ||
| # (edit target is the root layer by default; writes local opinions) | ||
| UsdPhysics.ArticulationRootAPI.Apply(parent_prim) | ||
| already_on_parent = set(parent_prim.GetAppliedSchemas()) | ||
| for name in articulation_api_names: | ||
| if name != "PhysicsArticulationRootAPI" and name not in already_on_parent: | ||
| parent_prim.AddAppliedSchema(name) | ||
|
|
||
| # --- Copy USD articulation attributes to the parent prim --- | ||
| usd_art_api = UsdPhysics.ArticulationRootAPI(root_body_prim) | ||
| for attr_name in usd_art_api.GetSchemaAttributeNames(): | ||
| attr = root_body_prim.GetAttribute(attr_name) | ||
| val = attr.Get() if attr else None | ||
| if val is not None: | ||
| parent_attr = parent_prim.GetAttribute(attr_name) | ||
| if not parent_attr: | ||
| parent_attr = parent_prim.CreateAttribute(attr_name, attr.GetTypeName()) | ||
| parent_attr.Set(val) | ||
|
|
||
| # --- Copy physxArticulation:* attributes to the parent prim --- | ||
| for attr in root_body_prim.GetAttributes(): | ||
| aname = attr.GetName() | ||
| if aname.startswith("physxArticulation:"): | ||
| val = attr.Get() | ||
| if val is not None: | ||
| parent_attr = parent_prim.GetAttribute(aname) | ||
| if not parent_attr: | ||
| parent_attr = parent_prim.CreateAttribute(aname, attr.GetTypeName()) | ||
| parent_attr.Set(val) | ||
|
|
||
| # --- Remove ArticulationRootAPI schemas from the root rigid body --- | ||
| # Writing "delete" list-ops in the root layer overrides "prepend" in sublayers. | ||
| root_body_prim.RemoveAppliedSchema("PhysxArticulationAPI") | ||
| root_body_prim.RemoveAPI(UsdPhysics.ArticulationRootAPI) | ||
| for name in articulation_api_names: | ||
| if name not in ("PhysicsArticulationRootAPI", "PhysxArticulationAPI"): | ||
| root_body_prim.RemoveAppliedSchema(name) | ||
|
|
||
| # Save only the root layer (sublayers produced by the asset transformer are untouched). | ||
| stage.GetRootLayer().Save() | ||
|
|
||
| @staticmethod | ||
| def _apply_link_density(stage, density: float): | ||
| """Set default density on rigid body links that have no explicit mass. | ||
|
|
||
| Args: | ||
| stage: The USD stage to modify. | ||
| density: The density value in kg/m^3. | ||
| """ | ||
| from pxr import UsdPhysics | ||
|
|
||
| for prim in stage.Traverse(): | ||
| if not prim.HasAPI(UsdPhysics.MassAPI): | ||
| continue | ||
| mass_api = UsdPhysics.MassAPI(prim) | ||
| # only set density if mass is not explicitly specified (0.0 means auto-compute) | ||
| mass_attr = mass_api.GetMassAttr() | ||
| if mass_attr and mass_attr.HasValue() and mass_attr.Get() > 0.0: | ||
| continue | ||
| density_attr = mass_api.GetDensityAttr() | ||
| if not density_attr: | ||
| density_attr = mass_api.CreateDensityAttr() | ||
| density_attr.Set(density) | ||
|
|
||
| def _apply_joint_drives(self, stage, cfg: UrdfConverterCfg): | ||
| """Set joint drive properties (type, target, gains) on USD joints. | ||
| def _unpack_joint_drive(joint_drive: UrdfConverterCfg.JointDriveCfg | None) -> tuple: | ||
| """Translate an IsaacLab :class:`UrdfConverterCfg.JointDriveCfg` into flat importer fields. | ||
|
|
||
| Args: | ||
| stage: The USD stage to modify. | ||
| cfg: The URDF converter configuration containing joint drive settings. | ||
| """ | ||
| from pxr import UsdPhysics | ||
|
|
||
| # collect all joints with their metadata | ||
| joints: dict[str, tuple] = {} | ||
| for prim in stage.Traverse(): | ||
| if not (prim.IsA(UsdPhysics.RevoluteJoint) or prim.IsA(UsdPhysics.PrismaticJoint)): | ||
| continue | ||
| joint_name = prim.GetName() | ||
| is_revolute = prim.IsA(UsdPhysics.RevoluteJoint) | ||
| instance_name = "angular" if is_revolute else "linear" | ||
| joints[joint_name] = (prim, is_revolute, instance_name) | ||
|
|
||
| if not joints: | ||
| return | ||
|
|
||
| drive_cfg = cfg.joint_drive | ||
|
|
||
| # apply drive type (force / acceleration) | ||
| self._set_drive_type_on_joints(joints, drive_cfg) | ||
| # apply target type (none / position / velocity) | ||
| self._set_target_type_on_joints(joints, drive_cfg) | ||
| # apply gains (stiffness / damping) | ||
| self._set_drive_gains_on_joints(joints, drive_cfg) | ||
|
|
||
| # ------------------------------------------------------------------ | ||
| # Joint drive helpers | ||
| # ------------------------------------------------------------------ | ||
|
|
||
| @staticmethod | ||
| def _set_drive_type_on_joints(joints: dict, drive_cfg: UrdfConverterCfg.JointDriveCfg): | ||
| """Set the drive type (force or acceleration) on joint prims. | ||
| joint_drive: The nested IsaacLab joint-drive configuration, or ``None``. | ||
|
|
||
| Args: | ||
| joints: Mapping of joint name → (prim, is_revolute, instance_name). | ||
| drive_cfg: The joint drive configuration. | ||
| """ | ||
| from pxr import UsdPhysics | ||
|
|
||
| def _apply(prim, instance_name: str, drive_type: str): | ||
| drive = UsdPhysics.DriveAPI.Get(prim, instance_name) | ||
| type_attr = drive.GetTypeAttr() | ||
| if not type_attr: | ||
| type_attr = drive.CreateTypeAttr() | ||
| type_attr.Set(drive_type) | ||
|
|
||
| if isinstance(drive_cfg.drive_type, str): | ||
| for _name, (prim, _is_rev, inst) in joints.items(): | ||
| _apply(prim, inst, drive_cfg.drive_type) | ||
| elif isinstance(drive_cfg.drive_type, dict): | ||
| for pattern, drive_type in drive_cfg.drive_type.items(): | ||
| matches = [n for n in joints if re.search(pattern, n)] | ||
| if not matches: | ||
| raise ValueError( | ||
| f"Joint name pattern '{pattern}' in drive_type config matched no joints." | ||
| f" Available joints: {list(joints.keys())}" | ||
| ) | ||
| for name in matches: | ||
| prim, _, inst = joints[name] | ||
| _apply(prim, inst, drive_type) | ||
|
|
||
| @staticmethod | ||
| def _set_target_type_on_joints(joints: dict, drive_cfg: UrdfConverterCfg.JointDriveCfg): | ||
| """Set the target type (none, position, velocity) on joint prims. | ||
|
|
||
| For ``"none"``, both stiffness and damping are zeroed out. | ||
|
|
||
| Args: | ||
| joints: Mapping of joint name → (prim, is_revolute, instance_name). | ||
| drive_cfg: The joint drive configuration. | ||
| """ | ||
| from pxr import UsdPhysics | ||
|
|
||
| def _apply(prim, instance_name: str, target_type: str): | ||
| drive = UsdPhysics.DriveAPI.Get(prim, instance_name) | ||
| if target_type == "none": | ||
| drive.GetStiffnessAttr().Set(0.0) | ||
| drive.GetDampingAttr().Set(0.0) | ||
|
|
||
| if isinstance(drive_cfg.target_type, str): | ||
| for _name, (prim, _is_rev, inst) in joints.items(): | ||
| _apply(prim, inst, drive_cfg.target_type) | ||
| elif isinstance(drive_cfg.target_type, dict): | ||
| for pattern, target_type in drive_cfg.target_type.items(): | ||
| matches = [n for n in joints if re.search(pattern, n)] | ||
| if not matches: | ||
| raise ValueError( | ||
| f"Joint name pattern '{pattern}' in target_type config matched no joints." | ||
| f" Available joints: {list(joints.keys())}" | ||
| ) | ||
| for name in matches: | ||
| prim, _, inst = joints[name] | ||
| _apply(prim, inst, target_type) | ||
|
|
||
| @staticmethod | ||
| def _set_drive_gains_on_joints(joints: dict, drive_cfg: UrdfConverterCfg.JointDriveCfg): | ||
| """Set stiffness and damping on joint drive APIs. | ||
|
|
||
| For revolute joints the user-facing values (Nm/rad) are converted to the USD | ||
| convention (Nm/deg) by multiplying by ``pi / 180``. | ||
|
|
||
| Args: | ||
| joints: Mapping of joint name → (prim, is_revolute, instance_name). | ||
| drive_cfg: The joint drive configuration. | ||
| Returns: | ||
| Tuple ``(drive_type, target_type, stiffness, damping)`` suitable for | ||
| :class:`~isaacsim.asset.importer.urdf.URDFImporterConfig`. Entries are ``None`` when | ||
| the user did not request an override. | ||
| """ | ||
| from pxr import UsdPhysics | ||
|
|
||
| gains = drive_cfg.gains | ||
| if not isinstance(gains, UrdfConverterCfg.JointDriveCfg.PDGainsCfg): | ||
| return | ||
|
|
||
| def _set_stiffness(prim, instance_name: str, is_revolute: bool, value: float): | ||
| drive = UsdPhysics.DriveAPI.Get(prim, instance_name) | ||
| usd_value = value * math.pi / 180.0 if is_revolute else value | ||
| stiffness_attr = drive.GetStiffnessAttr() | ||
| if not stiffness_attr: | ||
| stiffness_attr = drive.CreateStiffnessAttr() | ||
| stiffness_attr.Set(usd_value) | ||
|
|
||
| def _set_damping(prim, instance_name: str, is_revolute: bool, value: float): | ||
| drive = UsdPhysics.DriveAPI.Get(prim, instance_name) | ||
| usd_value = value * math.pi / 180.0 if is_revolute else value | ||
| damping_attr = drive.GetDampingAttr() | ||
| if not damping_attr: | ||
| damping_attr = drive.CreateDampingAttr() | ||
| damping_attr.Set(usd_value) | ||
|
|
||
| # --- stiffness --- | ||
| if isinstance(gains.stiffness, (float, int)): | ||
| for _name, (prim, is_rev, inst) in joints.items(): | ||
| _set_stiffness(prim, inst, is_rev, gains.stiffness) | ||
| elif isinstance(gains.stiffness, dict): | ||
| for pattern, stiffness in gains.stiffness.items(): | ||
| matches = [n for n in joints if re.search(pattern, n)] | ||
| if not matches: | ||
| raise ValueError( | ||
| f"Joint name pattern '{pattern}' in stiffness config matched no joints." | ||
| f" Available joints: {list(joints.keys())}" | ||
| ) | ||
| for name in matches: | ||
| prim, is_rev, inst = joints[name] | ||
| _set_stiffness(prim, inst, is_rev, stiffness) | ||
|
|
||
| # --- damping --- | ||
| if gains.damping is None: | ||
| return | ||
| if isinstance(gains.damping, (float, int)): | ||
| for _name, (prim, is_rev, inst) in joints.items(): | ||
| _set_damping(prim, inst, is_rev, gains.damping) | ||
| elif isinstance(gains.damping, dict): | ||
| for pattern, damping in gains.damping.items(): | ||
| matches = [n for n in joints if re.search(pattern, n)] | ||
| if not matches: | ||
| raise ValueError( | ||
| f"Joint name pattern '{pattern}' in damping config matched no joints." | ||
| f" Available joints: {list(joints.keys())}" | ||
| ) | ||
| for name in matches: | ||
| prim, is_rev, inst = joints[name] | ||
| _set_damping(prim, inst, is_rev, damping) | ||
| if joint_drive is None: | ||
| return None, None, None, None | ||
|
|
||
| gains = joint_drive.gains | ||
| if isinstance(gains, UrdfConverterCfg.JointDriveCfg.PDGainsCfg): | ||
| stiffness = gains.stiffness | ||
| damping = gains.damping | ||
| else: | ||
| # `NaturalFrequencyGainsCfg` is deprecated; leave gains unchanged. | ||
| stiffness = None | ||
| damping = None | ||
|
|
||
| return joint_drive.drive_type, joint_drive.target_type, stiffness, damping |
There was a problem hiding this comment.
JointDriveCfg.drive_type, target_type, gains.stiffness, and gains.damping all accept dict[str, ...] for per-joint regex-pattern overrides. The old code explicitly handled this with re.search pattern matching. _unpack_joint_drive now extracts these values verbatim and forwards them to URDFImporterConfig(joint_drive_type=..., joint_target_type=..., override_joint_stiffness=..., override_joint_damping=...), which almost certainly expects scalar/string values — not dicts. Any user who relied on per-joint configs will either hit a runtime type error or have their settings silently ignored, with no warning emitted.
_warn_unsupported_features should detect dict-valued drive_type, target_type, stiffness, and damping and emit a deprecation warning, the same way it does for NaturalFrequencyGainsCfg.
| the canonical implementation to preserve the public import path | ||
| ``isaaclab.sim.converters.urdf_utils``. | ||
| """ | ||
|
|
There was a problem hiding this comment.
The entire urdf_utils.py module is now a single import from isaacsim.asset.importer.urdf.impl.urdf_utils. If that internal path changes or the isaacsim package is not installed (e.g., when the module is imported in a documentation/test environment without Isaac Sim), any import of isaaclab.sim.converters.urdf_utils will raise an ImportError at module load time rather than at conversion time. Consider wrapping the import in a try/except and raising a clearer error only when merge_fixed_joints is actually called, similar to how other Isaac Sim-specific imports are deferred inside methods across the codebase.
| run_asset_transformer: bool = True | ||
| """Whether to run the asset transformer profile after conversion. Defaults to True. | ||
|
|
||
| When enabled, the importer restructures the intermediate USD into a layered, | ||
| payload-based package. Disable for a single flat USD output. | ||
| """ | ||
|
|
||
| run_multi_physics_conversion: bool = True | ||
| """Whether to run the MJCF-to-PhysX physics conversion pass. Defaults to True.""" |
There was a problem hiding this comment.
run_asset_transformer and run_multi_physics_conversion default to True — worth documenting the behavioral implications
Defaulting them to True preserves existing behaviour, which is correct. However, if a user accidentally sets run_multi_physics_conversion=False, the resulting USD will lack PhysX joint attributes and the articulation will be unusable in simulation. Consider adding a .. warning:: block to both docstrings noting that disabling these flags produces a non-simulated asset intended only for inspection/debugging.
| # --------------------------------------------------------------- | ||
| # Physics / articulation options | ||
| # --------------------------------------------------------------- |
There was a problem hiding this comment.
NIT: We can remove these dunders here as they are redundant and not done at other places in the code.
| # --------------------------------------------------------------- | ||
| # Geometry / mesh options | ||
| # --------------------------------------------------------------- |
There was a problem hiding this comment.
NIT: We can remove these dunders here as they are redundant and not done at other places in the code.
| # Actuator gain overrides | ||
| # --------------------------------------------------------------- | ||
|
|
||
| override_gain_type: str | None = None |
There was a problem hiding this comment.
Why is this called override here and not apply? I suppose a one-to-one naming makes more sense unless justified.
There was a problem hiding this comment.
I'm calling it override because it will apply this overrides the imported gains settings on the mujoco actuators, not just the empty ones. Would you prefer if i use "apply" instead?
| """ | ||
|
|
||
| override_gain_prm: list[float] | None = None | ||
| """MuJoCo actuator gain parameter array (10 floats) override. Defaults to ``None``. |
There was a problem hiding this comment.
Could you explain here what the 10 floats correspond to?
There was a problem hiding this comment.
yup, I gave a more general explanation F = gain * control + bias, the param array doesn't need to be 10 elements I think
| """ | ||
|
|
||
| override_bias_prm: list[float] | None = None | ||
| """MuJoCo actuator bias parameter array (10 floats) override. Defaults to ``None``. |
There was a problem hiding this comment.
Here too. Could you explain what do the floats mean?
There was a problem hiding this comment.
yup, I gave a more general explanation F = gain * control + bias, the param array doesn't need to be 10 elements I think
| # --------------------------------------------------------------- | ||
|
|
||
| run_asset_transformer: bool = True | ||
| """Whether to run the asset transformer profile after conversion. Defaults to True. |
There was a problem hiding this comment.
This too needs more information. We should avoid docstrings that just describe what the name says. Ideally, they should add more information on what the attribute/function is doing internally, so that the developer is more informed.
There was a problem hiding this comment.
I agree, I listed out the file structure that the asset transformer generates for extra clarity
| payload-based package. Disable for a single flat USD output. | ||
| """ | ||
|
|
||
| run_multi_physics_conversion: bool = True |
There was a problem hiding this comment.
What does this mean? Is it only for PhysX or also for Newton?
There was a problem hiding this comment.
added explanation: yes, so this will convert mujoco: attributes to physx: attributes. I can imagine this will slowly fade away as newton bridges the gap between mujoco and physx.
| If None, the root link will be set by PhysX. | ||
|
|
||
| .. deprecated:: | ||
| This option is no longer supported by the URDF importer 3.0. A warning is logged if set. |
There was a problem hiding this comment.
An explanation on why is needed. Otherwise it stays unclear.
There was a problem hiding this comment.
I am not sure about the purpose of this config, so I kept the arg there purely for backwards compatibility
| to :class:`~isaacsim.asset.importer.urdf.URDFImporterConfig`. | ||
| """ | ||
|
|
||
| robot_type: str = "Default" |
There was a problem hiding this comment.
What does this mean? Could you explain what other types are there? Otherwise, it is unclear.
| """ | ||
|
|
||
| run_asset_transformer: bool = True | ||
| """Whether to run the asset transformer profile after conversion. Defaults to True. |
There was a problem hiding this comment.
Same docstring note here as the mjcf importer
There was a problem hiding this comment.
I agree, I listed out the file structure that the asset transformer generates for extra clarity
| payload-based package. Disable for a single flat USD output. | ||
| """ | ||
|
|
||
| run_multi_physics_conversion: bool = True |
There was a problem hiding this comment.
Same docstring note here as the mjcf importer
There was a problem hiding this comment.
agree, explained further. this will convert urdf: tags to newton, mujoco, and physx
| R: A 3x3 rotation matrix. | ||
| Historically, IsaacLab shipped its own copy of ``merge_fixed_joints`` for the URDF | ||
| pipeline. That logic has moved to the Isaac Sim URDF importer at | ||
| :mod:`isaacsim.asset.importer.urdf.impl.urdf_utils`, so this module now simply re-exports |
There was a problem hiding this comment.
Does this mean one needs to have Isaac Sim installed? If so, then this isn't ideal IMO.
There was a problem hiding this comment.
Since this module is still in develop, I suggest just removing it altogether and directly call isaacsim import where used in the codebase. Following that please update the changelog to say this module is deleted and check versioning accordingly.
There was a problem hiding this comment.
hopefully Isaac Sim is not a hard requirement, we are looking into shipping the importer as a separate isaac sim independent pip package. and yes sounds good, I will update the doc string
Mayankm96
changed the title
There was a problem hiding this comment.
Code Review Summary
This PR represents a significant architectural improvement by delegating URDF/MJCF conversion logic to the Isaac Sim importer, reducing code duplication and maintenance burden. The refactoring is well-executed overall.
✅ Strengths
-
Excellent Code Simplification: Removing ~400 lines of duplicated conversion logic from
urdf_converter.pyin favor of the upstream Isaac Sim implementation is the right architectural decision. -
Good Backwards Compatibility: The
urdf_utils.pyre-export shim preserves the public import path formerge_fixed_joints, preventing breaking changes for downstream users. -
Comprehensive Test Coverage: The 202 new lines of MJCF tests cover the new configuration options (self-collision, collision types, actuator gains, etc.).
-
Clear Documentation: The changelog entries thoroughly document all added/changed configuration fields.
🔍 Findings
1. Potential Issue: Dictionary-based joint drive gains not supported
File: urdf_converter.py (lines 145-171)
Severity: Medium
The _unpack_joint_drive() method only extracts scalar stiffness and damping values. The previous implementation supported regex-based per-joint dictionaries (dict[str, float]) for fine-grained control. If the Isaac Sim importer does not support per-joint overrides via override_joint_stiffness/override_joint_damping, this represents a feature regression.
Recommendation: Verify whether the Isaac Sim importer accepts dictionary-based gains, or document this as a breaking change for users who relied on per-joint gain specifications.
2. Missing validation for JointDriveCfg dictionary types
File: urdf_converter.py (line 159-163)
if isinstance(gains, UrdfConverterCfg.JointDriveCfg.PDGainsCfg):
stiffness = gains.stiffness
damping = gains.dampingIf gains.stiffness or gains.damping is a dict (for per-joint values), passing it directly to the importer may cause unexpected behavior. Consider adding type validation or conversion.
3. Type annotation missing for ros_package_paths
File: urdf_converter_cfg.py (line 174-178)
ros_package_paths: list[dict[str, str]] = []The docstring states each entry should have keys name and path, but there is no TypedDict or validation to enforce this structure. Consider using a @dataclass or TypedDict for better type safety.
📋 Previous Concerns Status
- ⏳ Per-joint dictionary gains not supported — not addressed (original inline comment still stands)
- ⏳ Re-export shim fallback — not addressed (original inline comment still stands)
- ⏳ Documentation for disabled flags — not addressed (original inline comment still stands)
🆕 Update (085e3f8): OVPhysX RigidObject Implementation
This is a substantial update adding a complete OVPhysX backend implementation for RigidObject, mirroring the PhysX API. Key additions:
New Files
rigid_object.py(1173 lines): Full OVPhysX-backed RigidObject implementation with pose/velocity writers, mass/COM/inertia setters, and wrench composersrigid_object_data.py(1198 lines): Data container with lazy timestamped buffers and ProxyArray wrapperstest_rigid_object.py(1134 lines): Comprehensive test suite covering initialization, state setting, external forces, and material properties (some xfail due to missing RIGID_BODY_MATERIAL TensorType)
Architecture Observations
✅ Well-designed:
- Device-lock mechanism at process level (
_locked_device) correctly handles ovphysx<=0.3.7's C++ constraint - Lazy binding creation via
_get_binding()avoids unnecessary allocations - CPU staging buffers for CPU-only bindings (mass/coms/inertia) handle device mismatch properly
- Clear documentation of HACK notes for Carbonite dual-instance workarounds
-
Process-global PhysX instance reuse (
ovphysx_manager.pylines 90-97, 158-178)
The PhysX instance is now intentionally kept alive across SimulationContext instances. The_release_physx()method only callsphysx.reset()rather than dropping the reference. This is documented well but users may be surprised if they expect fresh state between contexts. -
_configure_physx_scene_prim()device branching (lines 330-425)
GPU-specific attributes are only applied whendevice == "gpu". Verify this doesn't cause issues when switching device modes within the same process (though the device lock should prevent this). -
_read_binding_into()assertion vs exception (rigid_object_data.pyline 875)
Usesassertfor shape checking which is stripped in-Omode. Considerraise ValueErrorfor production safety.
Minor Issues
-
Changelog entries removed prematurely (lines removed from
changelog.d/)
The feature entries for heterogeneous Dexsuite and Newton extra were removed but the version bump is to 0.6.4. Verify these changes were already released in 0.6.3. -
Test device parametrization (
test_rigid_object.py)
Tests parametrize over["cuda:0", "cpu"]but the autouse fixture_ovphysx_skip_other_devicemay skip half the tests based on session lock. This is documented but could be confusing for CI.
Overall Assessment
The new OVPhysX RigidObject implementation is well-architected and follows the established patterns from the Articulation implementation. The device-lock mechanism and Carbonite workarounds are properly documented. The main previous concerns about URDF converter remain unaddressed but the incremental changes are solid.
Recommendation: Merge after addressing or acknowledging the per-joint dictionary gains concern from the original review.
Update (a8aa860): Reviewed 13 new commits (mostly merge commits from develop). Key upstream changes include:
- Backend-agnostic task-space accessors for IK/OSC (#5400)
- RSL-RL config updates for
rsl_rl >= 5.0(#5551) - New Scene Data Provider API (#5128)
- Newton dependency simplification using
newton[sim](#5566) - Shadow-Hand-Over MAPPO Newton backend (#5437)
The upstream merges introduce no new issues related to the URDF/MJCF importer changes in this PR. Previous concerns about per-joint dictionary gains and documentation remain unaddressed but are not blocking.
✅ No new issues found in the incremental changes.
Update (a3a3d0a): Reviewed incremental changes.
Changes Summary
- Documentation: Extensive improvements to URDF/MJCF importer docs (configuration parameter organization, notes about auto-deduplication)
- LEAPP Install Fix: Changed
pip install leapp→./isaaclab.sh -p -m pip install leappfor proper Isaac Lab environment installation - Changelog Consolidation: Released entries from
changelog.d/merged into main CHANGELOGs; version bumps to isaaclab 5.2.0/5.3.0 - Test Fix: Added
sim._disable_app_control_on_stop_handle = Truein MJCF converter test to prevent timeout
Previous Concerns Status
-
urdf_utils.pyre-export shim — ✅ Resolved (removed): The file was completely deleted rather than being a shim. Import path is now directly fromisaacsim.asset.importer.urdf.impl.urdf_utils. This is a clean break from the old API. Tests updated to import from the new location. -
Per-joint dictionary gains — ⏳ Still unaddressed: The inline comment about dict-valued
drive_type/target_type/gains being silently passed to scalar-expecting importer fields remains relevant. -
Documentation for disabled flags — ⏳ Still unaddressed: The inline comment about adding warnings for
run_asset_transformer=False/run_multi_physics_conversion=Falseremains relevant.
✅ No new issues found in the incremental changes. The documentation improvements are excellent.
Update (f1addb0): Reviewed incremental changes.
Changes Summary
- Changelog housekeeping: Added changelog fragment file
stevfeng-fix-converter-usd-path.minor.rstcontaining the properly formatted changelog entries - Main CHANGELOG.rst cleanup: Removed pre-generated 5.3.0 entries from the main changelog (will be auto-generated from fragments during release)
This follows the correct scriv/towncrier changelog fragment pattern - entries are maintained in changelog.d/ and compiled into the main CHANGELOG during releases.
✅ No new issues. This is correct changelog management - no code changes, just documentation housekeeping.
Update (1a665c5): Reviewed incremental changes since f1addb0.
Changes Summary
- Documentation RST fixes: Corrected section heading underlines in
import_new_asset.rst(changed from"""to~~~for proper heading hierarchy) - CHANGELOG.rst formatting: Fixed orphan paragraph indentation, added proper bullet prefix and blank lines between versions
- Changelog validation tooling: Added orphan paragraph detection to
tools/changelog/cli.pyto catch malformed fragments at CI time - Test fixtures: Added test case for the new orphan paragraph validation
Previous Concerns Status
- Per-joint dictionary gains — ⏳ Still unaddressed (not modified in this update)
- Documentation for disabled flags — ⏳ Still unaddressed (not modified in this update)
✅ No new issues found. These are purely documentation and CI tooling improvements with no impact on the URDF/MJCF converter functionality.
Update (178634f): Reviewed incremental changes since 1a665c5.
Changes Summary
This update contains a collection of bug fixes and improvements unrelated to the URDF/MJCF importer changes:
- AppLauncher CUDA fix: Deferred
torch.cuda.set_device()call until after SimulationApp starts to prevent OpenBLAS/NumPy fork crashes during Kit startup - Math utility edge case fixes:
create_rotation_matrix_from_view()now returns valid orthonormal frames when look-at direction is parallel to up axis (previously returned singular matrix);quat_from_matrix()now returns NaN for non-rotation inputs (singular, reflection, scale-error matrices) - Camera pose fixes:
Camera.set_world_poses_from_view()andRayCasterCamera.set_world_poses_from_view()now handle degenerate cases (eye==target) with warnings instead of silently applying garbage poses - PVA sensor debug visualizer fixes: Both newton and physx backends now skip bodies with zero acceleration when drawing direction arrows (previously drew undefined directions)
- LEAPP export script refactoring: Restructured as importable library with deferred runtime dependencies for testability
- CI test improvements: Added LEAPP package debug output, unset
HUB__ARGS__DETECT_ONLYflag - Import path fixes: Several files changed from
from isaaclab.utils import configclassto direct import - Test cleanup: Removed unnecessary Isaac Sim launcher imports from pure-Python tests
- Version bumps: isaaclab 5.2.1, isaaclab_newton 0.9.1, isaaclab_physx 0.7.1
Previous Concerns Status
- Per-joint dictionary gains — ⏳ Still unaddressed (not modified in this update)
- Documentation for disabled flags — ⏳ Still unaddressed (not modified in this update)
✅ No new issues found. These are solid bug fixes with comprehensive test coverage for the math edge cases.
stevfeng
force-pushed
the
dev/stevfeng/update_importers
branch
from
c638330 to
0ba9b59
Compare
stevfeng
force-pushed
the
dev/stevfeng/update_importers
branch
3 times, most recently
from
64447a7 to
17e01a9
Compare
github-actions
Bot
added
the
documentation
stevfeng
force-pushed
the
dev/stevfeng/update_importers
branch
from
a8aa860 to
a3a3d0a
Compare
stevfeng
force-pushed
the
dev/stevfeng/update_importers
branch
from
127f48d to
d7d45ba
Compare
4 participants
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.
In Isaac Sim, we have added more capabilities to handle joint presets, fixed joints, and other properties to the importers, so we can simplify the isaac lab importer workflow.
Fixes # (issue)
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there