Fixes MJCF importer extension and updates stale importer docs#5889
Conversation
Greptile SummaryThis PR fixes a missing
Confidence Score: 5/5The change is a two-line guard in MjcfConverter.init that mirrors the identical, already-deployed pattern in UrdfConverter; documentation updates replace a removed CLI flag with the current one. The fix is a direct copy of the UrdfConverter extension-enable pattern, which already works in production. The documentation changes update stale --headless references to the current --viz kit flag, confirmed present in AppLauncher. No new logic paths, no schema changes, and no side effects beyond enabling the extension when it is not already enabled. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant MjcfConverter
participant ExtManager as omni.kit.app ExtensionManager
participant AssetConverterBase
participant MJCFImporter as isaacsim.asset.importer.mjcf
Caller->>MjcfConverter: __init__(cfg)
MjcfConverter->>ExtManager: is_extension_enabled("isaacsim.asset.importer.mjcf")
alt extension not enabled
MjcfConverter->>ExtManager: set_extension_enabled_immediate(...)
end
MjcfConverter->>AssetConverterBase: super().__init__(cfg)
AssetConverterBase->>MjcfConverter: _convert_asset(cfg)
MjcfConverter->>MJCFImporter: import MJCFImporter, MJCFImporterConfig
MjcfConverter->>MJCFImporter: MJCFImporter(import_config).import_mjcf()
MJCFImporter-->>MjcfConverter: generated_usd_path
MjcfConverter-->>Caller: converter ready
Reviews (1): Last reviewed commit: "Fixes MJCF importer extension and update..." | Re-trigger Greptile |
There was a problem hiding this comment.
Review Summary
Clean, well-scoped bug fix that aligns the MJCF converter with the existing URDF converter pattern. The documentation updates correctly reflect the new --viz kit CLI argument. A few minor observations below.
Findings
1. No error handling for extension enablement (Low severity — pre-existing pattern)
set_extension_enabled_immediate() can fail silently if the isaacsim.asset.importer.mjcf extension is not installed or cannot be loaded. The URDF converter has the same gap, but since this is a bug fix PR adding the missing call, it would be valuable to at least log a warning or raise an informative error if enablement fails:
manager = omni.kit.app.get_app().get_extension_manager()
if not manager.is_extension_enabled("isaacsim.asset.importer.mjcf"):
manager.set_extension_enabled_immediate("isaacsim.asset.importer.mjcf", True)
if not manager.is_extension_enabled("isaacsim.asset.importer.mjcf"):
raise RuntimeError(
"Failed to enable 'isaacsim.asset.importer.mjcf' extension. "
"Ensure it is installed in the current Isaac Sim environment."
)This would surface a clear error at init time rather than a cryptic ImportError later when _convert_asset tries to import from the extension.
2. Documentation: headless behavior wording could be clearer (Nit)
The updated text says:
"To run the script headless and exit after the conversion is complete, omit
--viz kit."
This is accurate, but "omit --viz kit" could confuse users who might think they need --headless. If there is still a --headless flag (or if the default is truly no-viz), a brief clarification like "the script runs headless by default when --viz is not specified" would be helpful.
3. No regression test for the fix (Informational)
The PR checklist notes that tests were not added. Given this is a runtime-environment-dependent fix (requires the Isaac Sim extension system), a unit test may not be practical. However, if there is an existing integration test for MjcfConverter, it would now implicitly validate this path. Worth confirming that existing CI covers MJCF conversion end-to-end.
Overall Assessment
The fix is correct and follows established patterns (mirrors UrdfConverter.__init__()). The extension enablement placement before super().__init__() is necessary since the base class constructor can trigger _convert_asset(). Documentation and changelog updates are well done. Ship it. 🚀
Update (commit 69a4de7): New commit adds a proper regression test (test_converter_enables_importer_extension) that validates the MJCF importer extension gets enabled by the converter — this directly addresses finding #3. The test design is clean: it uses a helper to get the extension path without enabling it, then verifies the converter itself triggers enablement. Findings #1 (error handling suggestion) and #2 (doc nit) remain as-is — both are low priority and the original inline comments still stand. No new issues introduced. ✅
| # enable the MJCF importer extension | ||
| manager = omni.kit.app.get_app().get_extension_manager() | ||
| if not manager.is_extension_enabled("isaacsim.asset.importer.mjcf"): | ||
| manager.set_extension_enabled_immediate("isaacsim.asset.importer.mjcf", True) |
There was a problem hiding this comment.
Suggestion (low priority): Consider adding a post-check after set_extension_enabled_immediate() to raise an informative error if the extension fails to load. This mirrors a gap in the URDF converter too, but since you're touching this code anyway, it would improve debuggability:
if not manager.is_extension_enabled("isaacsim.asset.importer.mjcf"):
manager.set_extension_enabled_immediate("isaacsim.asset.importer.mjcf", True)
if not manager.is_extension_enabled("isaacsim.asset.importer.mjcf"):
raise RuntimeError(
"Failed to enable 'isaacsim.asset.importer.mjcf' extension. "
"Ensure it is installed in the current Isaac Sim environment."
)…sim#5889) # Description Adds missing isaac sim extension enable call for MJCF importer Updates stale importer docs to use new visualizer arguments ## Type of change <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) - Documentation update ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [ ] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task -->
…nvs, shutdown error, mjcf fix (#5888) # Description Cherry-pick PRs from develop: - #5866 - #5879 - #5882 - #5884 - #5889 --------- Signed-off-by: Yize Wang <yizew@nvidia.com> Signed-off-by: Kelly Guo <kellyg@nvidia.com> Co-authored-by: YizeWang <37894497+YizeWang@users.noreply.github.com> Co-authored-by: Yize Wang <yizew@nvidia.com>
Description
Adds missing isaac sim extension enable call for MJCF importer
Updates stale importer docs to use new visualizer arguments
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there