[Fix] Resolve package:// meshes when merging fixed joints in URDF conversion - #7084
[Fix] Resolve package:// meshes when merging fixed joints in URDF conversion#7084hujc7 wants to merge 4 commits into
Conversation
`--install ... ,importers` names a token the install CLI does not define, so every kit-less image build logs "Unknown install token 'importers'. Skipping" and the selector reads as if it installs something. Nothing else references it.
Merging fixed joints rewrites the URDF into a scratch directory, and the importer then resolves `package://` URLs against that copy, where no meshes exist. Every mesh silently dropped, so conversion produced an articulation with no geometry. Deriving the package from the URDF's own `package.xml` keeps the URLs anchored to the source tree, so `ros_package_paths` is only needed for packages laid out unconventionally.
Greptile SummaryThe PR derives a ROS package mapping from a URDF’s nearest
Confidence Score: 4/5The unreadable-manifest failure should be fixed before merging because automatic discovery can now abort otherwise valid URDF conversions. The new package discovery catches XML syntax errors but not filesystem errors raised while opening a discovered manifest, and the regression test does not cover the actual fixed-joint geometry path. Files Needing Attention: source/isaaclab/isaaclab/sim/converters/urdf_converter.py; source/isaaclab/test/sim/test_urdf_converter.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
U[URDF asset path] --> D{Explicit ROS package paths?}
D -->|Yes| E[Use configured mappings]
D -->|No| F[Walk ancestors for package.xml]
F --> P[Parse package name]
P --> I[Build URDFImporterConfig]
E --> I
I --> M[Merge fixed joints]
M --> R[Resolve package:// meshes]
R --> O[Generated USD geometry]
Reviews (1): Last reviewed commit: "Derive the ROS package root from the URD..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try: | ||
| name = ElementTree.parse(manifest).getroot().findtext("name") | ||
| except ElementTree.ParseError: | ||
| logger.warning(f"UrdfConverter: could not parse '{manifest}' to resolve 'package://' URLs.") | ||
| return None |
There was a problem hiding this comment.
If an ancestor package.xml exists but cannot be opened, ElementTree.parse raises an OSError that bypasses this handler, causing conversion to abort even for an otherwise readable URDF.
| try: | |
| name = ElementTree.parse(manifest).getroot().findtext("name") | |
| except ElementTree.ParseError: | |
| logger.warning(f"UrdfConverter: could not parse '{manifest}' to resolve 'package://' URLs.") | |
| return None | |
| try: | |
| name = ElementTree.parse(manifest).getroot().findtext("name") | |
| except (ElementTree.ParseError, OSError): | |
| logger.warning(f"UrdfConverter: could not read or parse '{manifest}' to resolve 'package://' URLs.") | |
| return None |
There was a problem hiding this comment.
Confirmed and fixed in df007d0. Verified the failure mode directly rather than by inspection: with chmod 000 on the manifest, Path.is_file() still returns True and ElementTree.parse raises PermissionError. ElementTree.ParseError derives from SyntaxError, not OSError, so the old handler could never have caught it. The handler is now except (ElementTree.ParseError, OSError).
| 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 |
There was a problem hiding this comment.
This assertion tests _find_ros_package directly but never runs fixed-joint conversion with a package:// mesh and verifies generated geometry, so the reported geometry-loss regression can recur while the test still passes.
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!
There was a problem hiding this comment.
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 test_package_url_meshes_survive_fixed_joint_merge in 8b501c3: it builds a throwaway ROS package (a package.xml, an OBJ mesh, and a URDF whose only geometry is a package:// visual behind a fixed joint), converts with merge_fixed_joints=True, and asserts a mesh survives. Verified it fails with the wiring reverted and passes with it.
One detail worth recording for anyone extending these tests: the importer writes the geometry behind an instanceable reference, so the assertion traverses with Usd.TraverseInstanceProxies(). A plain Stage.Traverse() stops at the instance boundary and reports zero meshes even on a perfectly good conversion.
![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
The change derives a ROS package mapping from the nearest ancestor package.xml when no explicit ros_package_paths are configured, preserving package:// mesh resolution after fixed-joint merging. It also removes the obsolete importers selector from the kit-less image install command.
- Design and architecture: The package-discovery workaround is localized at the URDF importer boundary, uses only the standard library, and preserves explicit
ros_package_pathsas the authoritative configuration. Walking to the nearest ancestor manifest follows the conventional ROS package layout, with the residual tradeoff that URDFs nested beneath an unrelated package manifest will inherit that package. - API: No public fields or importer argument shapes change. The user-visible behavior of an empty
ros_package_pathslist now includes automatic package derivation, while explicitly configured mappings retain their previous behavior. The source-package changelog records this semantic change. - Implementation: The derived mapping is deterministic from
asset_path; absent or blank package names fall back to the prior empty mapping, while malformed XML logs a warning and falls back. The helper test covers package discovery and the no-package case. Existing cached conversions may still require forced regeneration or output removal, consistent with the converter's documented cache behavior.
No blocking issues. No inline issue met the actionable-evidence threshold; the assessment above records the review feedback.
Automated review; human maintainers own approval decisions.
`ElementTree.ParseError` derives from `SyntaxError`, so an ancestor `package.xml` that exists but cannot be opened raised an uncaught `OSError` and aborted an otherwise valid conversion.
The helper test asserted only that a package root is discoverable, so it still passed with the call-site wiring deleted. The new test converts a URDF whose only geometry is a `package://` visual behind a fixed joint and asserts a mesh survives, which fails without the wiring. The geometry is behind an instanceable reference, so it is reached through `Usd.TraverseInstanceProxies()`; a plain `Stage.Traverse()` stops at the instance boundary and sees no meshes at all.
2 participants
Description
Two independent changes to URDF conversion and the kit-less image.
1.
package://meshes are dropped when fixed joints are mergedMerging fixed joints rewrites the URDF into a scratch directory
(
/tmp/urdf_import_<robot>_*/), and the importer resolvespackage://<pkg>/...relative to that copy. No meshes live there, so every visual and collision mesh
resolves to a missing file. Conversion still succeeds: the resulting asset has the
full joint and rigid-body hierarchy but no geometry at all, and nothing renders.
This affects any ROS-style description converted with the default
UrdfConverterCfg(merge_fixed_joints=True), unless the user already knew to setros_package_pathsby hand.UrdfConverternow derives the mapping from the URDF's own location: it walks up tothe nearest
package.xml, reads its<name>, and passes the resulting{"name", "path"}entry to the importer. An explicitly configuredros_package_pathsstill takes precedence, so unconventional layouts are unaffected.Measured on
anymal_d_simple_description,merge_fixed_joints=True, no explicitros_package_paths:payloads/geometries.usdTracked against the Isaac Sim URDF importer as nvbug 6583703; this works around it
from the Isaac Lab side.
2. Kit-less image passes an install token that no longer exists
docker/Dockerfile.kitlessstill passesimporterstoisaaclab.sh --install. Thatextra was removed in #6935, when the standalone URDF/MJCF importers became base
dependencies. An unknown token is warned about and skipped (
install.py:1195), so theimage is correct today — this only drops the dead token and its stale comment, so
kit-less builds stop logging
Unknown install token 'importers'.Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched packageCONTRIBUTORS.mdor my name already exists there