diff --git a/source/isaaclab/changelog.d/fix-converter-lazy-cache.fixed.rst b/source/isaaclab/changelog.d/fix-converter-lazy-cache.fixed.rst new file mode 100644 index 000000000000..3139615d720a --- /dev/null +++ b/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. diff --git a/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py b/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py index 753601334829..0e4623e73905 100644 --- a/source/isaaclab/isaaclab/sim/converters/asset_converter_base.py +++ b/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)) diff --git a/source/isaaclab/isaaclab/sim/converters/asset_converter_base_cfg.py b/source/isaaclab/isaaclab/sim/converters/asset_converter_base_cfg.py index 2c63ba05bc15..96d47fa93924 100644 --- a/source/isaaclab/isaaclab/sim/converters/asset_converter_base_cfg.py +++ b/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 diff --git a/source/isaaclab/test/sim/test_mjcf_converter.py b/source/isaaclab/test/sim/test_mjcf_converter.py index 7a0d05435d41..74673dca41a3 100644 --- a/source/isaaclab/test/sim/test_mjcf_converter.py +++ b/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): diff --git a/source/isaaclab/test/sim/test_urdf_converter.py b/source/isaaclab/test/sim/test_urdf_converter.py index 1a49377c1f87..6c02c0c3f42c 100644 --- a/source/isaaclab/test/sim/test_urdf_converter.py +++ b/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,7 +99,7 @@ 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") @@ -105,7 +107,6 @@ def test_config_change(sim_config, tmp_path): 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