Fix lazy conversion loading a stale MJCF or URDF conversion

source/isaaclab/changelog.d/fix-converter-lazy-cache.fixed.rst

@@ -0,0 +1,3 @@
+* Fixed lazy MJCF and URDF conversion returning an older USD after the importer wrote a changed asset to a numbered
+  folder. The cache now records the generated file and reuses only the most recent matching conversion. Old
+  hash-only cache records cause one conversion because they do not identify the generated file.

source/isaaclab/isaaclab/sim/converters/asset_converter_base.py

@@ -29,11 +29,16 @@ class AssetConverterBase(abc.ABC):
     :meth:`_convert_asset` method to provide the actual conversion.
 
     The file conversion is lazy if the output directory (:obj:`AssetConverterBaseCfg.usd_dir`) is provided.
-    In the lazy conversion, the USD file is re-generated only if:
+    The most recent conversion in that directory is reused unless:
 
     * The asset file is modified.
     * The configuration parameters are modified.
-    * The USD file does not exist.
+    * The requested USD file name changes.
+    * The generated USD file does not exist.
+
+    The importer may generate a numbered file instead of the requested one. The cache records the generated path
+    so a later conversion cannot return an older file at the requested path. Old hash-only records are cache misses
+    because they do not identify the generated file.
 
     To override this behavior to force conversion, the flag :obj:`AssetConverterBaseCfg.force_usd_conversion`
     can be set to True.
@@ -45,9 +50,9 @@ class AssetConverterBase(abc.ABC):
     triggered conversions try to use the same directory for reading/writing the generated files.
 
     .. note::
-        Changes to the parameters :obj:`AssetConverterBaseCfg.asset_path`, :obj:`AssetConverterBaseCfg.usd_dir`, and
-        :obj:`AssetConverterBaseCfg.usd_file_name` are not considered as modifications in the configuration instance
-        that trigger the USD file re-generation.
+        The configuration hash excludes :obj:`AssetConverterBaseCfg.asset_path`,
+        :obj:`AssetConverterBaseCfg.usd_dir`, and :obj:`AssetConverterBaseCfg.usd_file_name`. The asset contents are
+        hashed, and the requested USD file name is checked separately.
 
     """
 
@@ -87,27 +92,40 @@ def __init__(self, cfg: AssetConverterBaseCfg):
         self._usd_file_name = usd_file_name
 
         os.makedirs(self.usd_dir, exist_ok=True)
-        self._usd_file_exists = os.path.isfile(self.usd_path)
-        # the recorded hash tells whether the cached USD was generated from this asset and config
-        self._dest_hash_path = os.path.join(self.usd_dir, ".asset_hash")
-        self._asset_hash = self._config_to_hash(cfg)
+        record_path = os.path.join(self.usd_dir, ".asset_hash")
+        asset_hash = self._config_to_hash(cfg)
+        requested = pathlib.PurePath(self._usd_file_name).as_posix()
         try:
-            with open(self._dest_hash_path) as f:
-                self._is_same_asset = f.readline() == self._asset_hash
-        except FileNotFoundError:
-            self._is_same_asset = False
-
-        # convert the asset to USD if the hash is different or USD file does not exist
-        if cfg.force_usd_conversion or not self._usd_file_exists or not self._is_same_asset:
+            with open(record_path, encoding="utf-8") as f:
+                record = json.load(f)
+        except (FileNotFoundError, ValueError):
+            record = {}
+        if not isinstance(record, dict):
+            record = {}
+        generated = record.get("generated")
+
+        if (
+            not cfg.force_usd_conversion
+            and record.get("hash") == asset_hash
+            and record.get("requested") == requested
+            and isinstance(generated, str)
+            and not pathlib.PurePath(generated).is_absolute()
+            and ".." not in pathlib.PurePath(generated).parts
+            and os.path.isfile(os.path.join(self.usd_dir, generated))
+        ):
+            self._usd_file_name = generated
+        else:
             # convert the asset to USD
             self._convert_asset(cfg)
             # importers put the physics payloads behind a "Physics" variant set and disagree on
             # which variant to select, so settle it here
             self._select_physics_variant(cfg.physics_variant)
-            # record the hash only now: writing it earlier would let a conversion that raised
+            # record the generated path only now: writing it earlier would let a conversion that raised
             # still count as cached, so an identical retry would skip it and return the asset
-            with open(self._dest_hash_path, "w") as f:
-                f.write(self._asset_hash)
+            generated = pathlib.PurePath(self._usd_file_name).as_posix()
+            record = {"hash": asset_hash, "requested": requested, "generated": generated}
+            with open(record_path, "w", encoding="utf-8") as f:
+                json.dump(record, f)
             # dump the configuration next to the asset, stamped with the converter that produced it
             config_path = os.path.join(self.usd_dir, "config.yaml")
             dump_yaml(config_path, to_dict(cfg))

source/isaaclab/isaaclab/sim/converters/asset_converter_base_cfg.py

@@ -53,7 +53,8 @@ class PhysicsVariant(StrEnum):
     force_usd_conversion: bool = False
     """Force the conversion of the asset file to usd. Defaults to False.
 
-    If True, then the USD file is always generated. It will overwrite the existing USD file if it exists.
+    If True, the USD file is always generated. Some importers write a new numbered output instead of overwriting
+    an existing one.
     """
 
     make_instanceable: bool = True

source/isaaclab/test/sim/test_mjcf_converter.py

@@ -105,11 +105,10 @@ def test_no_change(test_setup_teardown):
 
 
 def test_config_change(test_setup_teardown):
-    """Call conversion twice but change the config in the second call. This should generate a new USD file."""
+    """Reuse the importer's numbered output when the changed configuration is requested again."""
     sim, mjcf_config = test_setup_teardown
 
     mjcf_converter = MjcfConverter(mjcf_config)
-    time_usd_file_created = os.stat(mjcf_converter.usd_path).st_mtime_ns
 
     # change the config
     new_config = mjcf_config
@@ -120,7 +119,10 @@ def test_config_change(test_setup_teardown):
     new_mjcf_converter = MjcfConverter(new_config)
     new_time_usd_file_created = os.stat(new_mjcf_converter.usd_path).st_mtime_ns
 
-    assert time_usd_file_created != new_time_usd_file_created
+    assert new_mjcf_converter.usd_path != mjcf_converter.usd_path
+    lazy_mjcf_converter = MjcfConverter(new_config)
+    assert lazy_mjcf_converter.usd_path == new_mjcf_converter.usd_path
+    assert os.stat(lazy_mjcf_converter.usd_path).st_mtime_ns == new_time_usd_file_created
 
 
 def test_create_prim_from_usd(test_setup_teardown):

source/isaaclab/test/sim/test_urdf_converter.py

@@ -22,8 +22,10 @@
             allow_module_level=True,
         )
 
+import json
 import math
 import os
+import shutil
 import warnings
 from types import SimpleNamespace
 
@@ -97,15 +99,14 @@ def test_no_change(sim_config):
 
 @pytest.mark.isaacsim_ci
 def test_config_change(sim_config, tmp_path):
-    """Call conversion twice but change the config in the second call. This should generate a new USD file."""
+    """Reuse the importer's numbered output, and reject old records that cannot identify that output."""
 
     sim, config = sim_config
     output_dir = os.path.join(str(tmp_path), "urdf_config_change")
     os.makedirs(output_dir, exist_ok=True)
 
     config.usd_dir = output_dir
     urdf_converter = UrdfConverter(config)
-    time_usd_file_created = os.stat(urdf_converter.usd_path).st_mtime_ns
 
     # change the config
     new_config = config
@@ -116,7 +117,19 @@ def test_config_change(sim_config, tmp_path):
     new_urdf_converter = UrdfConverter(new_config)
     new_time_usd_file_created = os.stat(new_urdf_converter.usd_path).st_mtime_ns
 
-    assert time_usd_file_created != new_time_usd_file_created
+    assert new_urdf_converter.usd_path != urdf_converter.usd_path
+    lazy_urdf_converter = UrdfConverter(new_config)
+    assert lazy_urdf_converter.usd_path == new_urdf_converter.usd_path
+    assert os.stat(lazy_urdf_converter.usd_path).st_mtime_ns == new_time_usd_file_created
+
+    # A hash-only record cannot identify B's missing numbered output while A still exists.
+    record_path = os.path.join(output_dir, ".asset_hash")
+    with open(record_path, encoding="utf-8") as f:
+        old_hash = json.load(f)["hash"]
+    shutil.rmtree(os.path.dirname(new_urdf_converter.usd_path))
+    with open(record_path, "w", encoding="utf-8") as f:
+        f.write(old_hash)
+    assert UrdfConverter(new_config).usd_path != urdf_converter.usd_path
 
 
 @pytest.mark.isaacsim_ci