-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[Fix] Resolve package:// meshes when merging fixed joints in URDF conversion #7084
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| Fixed | ||
| ^^^^^ | ||
|
|
||
| * Fixed URDF conversion producing an asset with no geometry when the URDF referenced its meshes | ||
| through ``package://`` URLs and fixed joints were merged. The ROS package is now derived from the | ||
| URDF's own location, so ``UrdfConverterCfg.ros_package_paths`` only has to be set for packages | ||
| laid out unconventionally. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -823,3 +823,73 @@ def test_physics_variant_raises_again_on_retry(tmp_path): | |
| for _ in range(2): | ||
| with pytest.raises(ValueError, match="no 'physx' physics variant"): | ||
| UrdfConverter(config) | ||
|
|
||
|
|
||
| def test_ros_package_derived_from_urdf_location(tmp_path): | ||
| """The ROS package holding a URDF is derived from its path, so ``package://`` URLs resolve. | ||
|
|
||
| Merging fixed joints relocates the URDF to a scratch directory, and the importer resolves | ||
| ``package://`` against that copy, so the mapping has to name the source package. | ||
| """ | ||
| from isaaclab.sim.converters.urdf_converter import _find_ros_package | ||
|
|
||
| package = tmp_path / "my_robot_description" | ||
| (package / "urdf").mkdir(parents=True) | ||
| (package / "package.xml").write_text("<package><name>my_robot_description</name></package>") | ||
| urdf = package / "urdf" / "robot.urdf" | ||
| urdf.write_text("<robot name='robot'/>") | ||
|
|
||
| assert _find_ros_package(str(urdf)) == {"name": "my_robot_description", "path": str(package)} | ||
| # a URDF outside any package has no mapping to derive | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This assertion tests Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, and this was a real gap — the helper test still passed with the call-site wiring deleted, which is exactly the failure mode you describe. Added One detail worth recording for anyone extending these tests: the importer writes the geometry behind an instanceable reference, so the assertion traverses with |
||
| loose = tmp_path / "loose.urdf" | ||
| loose.write_text("<robot name='robot'/>") | ||
| assert _find_ros_package(str(loose)) is None | ||
|
|
||
|
|
||
| @pytest.mark.isaacsim_ci | ||
| def test_package_url_meshes_survive_fixed_joint_merge(sim_config, tmp_path): | ||
| """A ``package://`` mesh survives fixed-joint merging without an explicit package mapping. | ||
|
|
||
| Merging rewrites the URDF into a scratch directory, so an unanchored ``package://`` URL | ||
| resolves against a copy holding no meshes and every mesh silently drops out. | ||
| """ | ||
| _, config = sim_config | ||
|
|
||
| package = tmp_path / "test_mesh_description" | ||
| (package / "meshes").mkdir(parents=True) | ||
| (package / "package.xml").write_text("<package><name>test_mesh_description</name></package>") | ||
| (package / "meshes" / "tetra.obj").write_text( | ||
| "v 0 0 0\nv 0.1 0 0\nv 0 0.1 0\nv 0 0 0.1\nf 1 3 2\nf 1 2 4\nf 1 4 3\nf 2 3 4\n" | ||
| ) | ||
| # the mesh is the only geometry, so any Mesh prim in the output proves the URL resolved | ||
| urdf = package / "robot.urdf" | ||
| urdf.write_text( | ||
| "<robot name='test_package_url'>" | ||
| "<link name='root_link'/>" | ||
| "<joint name='root_to_base' type='fixed'>" | ||
| "<parent link='root_link'/><child link='base_link'/></joint>" | ||
| "<link name='base_link'><visual><geometry>" | ||
| "<mesh filename='package://test_mesh_description/meshes/tetra.obj'/>" | ||
| "</geometry></visual><inertial><mass value='1'/>" | ||
| "<inertia ixx='1.0' ixy='0.0' ixz='0.0' iyy='1.0' iyz='0.0' izz='1.0'/></inertial></link>" | ||
| "<joint name='base_to_link1' type='continuous'>" | ||
| "<parent link='base_link'/><child link='link_1'/>" | ||
| "<axis xyz='0 0 1'/><origin xyz='0 0 0.2'/></joint>" | ||
| "<link name='link_1'><inertial><mass value='1'/>" | ||
| "<inertia ixx='1.0' ixy='0.0' ixz='0.0' iyy='1.0' iyz='0.0' izz='1.0'/></inertial></link>" | ||
| "</robot>" | ||
| ) | ||
|
|
||
| output_dir = os.path.join(str(tmp_path), "urdf_package_url") | ||
| os.makedirs(output_dir, exist_ok=True) | ||
| config.asset_path = str(urdf) | ||
| config.merge_fixed_joints = True | ||
| config.force_usd_conversion = True | ||
| config.usd_dir = output_dir | ||
|
|
||
| from pxr import Usd, UsdGeom | ||
|
|
||
| stage = Usd.Stage.Open(UrdfConverter(config).usd_path) | ||
| # the geometry is behind an instanceable reference, which a plain Traverse() does not descend into | ||
| meshes = [p for p in Usd.PrimRange.Stage(stage, Usd.TraverseInstanceProxies()) if p.IsA(UsdGeom.Mesh)] | ||
| assert len(meshes) > 0, "the 'package://' visual mesh was dropped by the merged conversion" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an ancestor
package.xmlexists but cannot be opened,ElementTree.parseraises anOSErrorthat bypasses this handler, causing conversion to abort even for an otherwise readable URDF.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed and fixed in df007d0. Verified the failure mode directly rather than by inspection: with
chmod 000on the manifest,Path.is_file()still returnsTrueandElementTree.parseraisesPermissionError.ElementTree.ParseErrorderives fromSyntaxError, notOSError, so the old handler could never have caught it. The handler is nowexcept (ElementTree.ParseError, OSError).