Fix inconsistent asset paths in mj_saveLastXML: canonicalize includes…
Fixes: #2941
Problem
Users observed inconsistent behavior of mj_saveLastXML with respect to asset file paths:
- loading an included model directly produced relative paths in saved XML,
- loading a top-level scene via a relative path resulted in doubled prefixes like
xmls/xmls/...and broken loading, - loading the top-level scene via an absolute path produced absolute paths in the saved XML.
Root cause
The problem was caused by two interacting issues:
- The parser (
IncludeXMLin xml.cpp) mixed different filename forms when checking and recording included files: it checked theincludedset against the rawfilenamebut then inserted a dir-prefixedfilename(i.e.dir + filename) intoincluded. This inconsistent canonicalization caused themjSpecto contain a mix of relative and dir-prefixed paths, which the writer later serialized incorrectly. - The writer emits asset filenames using
spec->modelfilediras the base. When saving to a new target file, the writer had no information about the output directory and thus could not deterministically produce paths relative to the output file — causing inconsistent absolute/relative outputs.
What this PR does
-
Parser canonicalization (
xml.cc):- Canonicalize include filenames early (
fullname = dir + filename) and usefullnameconsistently forincludedchecks and insertions and for subsequent processing. - This prevents having mixed forms in
mjSpecand avoids doubled prefixes when the writer later emits file paths.
- Canonicalize include filenames early (
-
Output-dir awareness at write time (
xml_api.cc):- Temporarily override
spec->modelfiledirwith the directory of the output file when calling the writer viaGetGlobalModel().ToXML(...). This is implemented in a backward-compatible way (temporary override + restore). - This lets the existing writer produce asset paths relative to the output file location, removing ambiguity and making saved XML deterministic for the common
mj_saveLastXMLcase.
- Temporarily override
Why this fixes #2941
- Parser canonicalization eliminates mixed path forms inside the
mjSpec, preventing double-prefix issues likexmls/xmls/.... - Temporarily setting
spec->modelfiledirto the output directory ensures the writer computes relative/absolute paths consistently w.r.t. the output location, so the saved XML can be loaded reliably.
Testing
Run the three cases reported in #2941 and verify saved XML loads in each case:
- Load the child XML directly:
-
model = mujoco.MjModel.from_xml_path("xmls/panda_2f85/panda_2f85.xml") -
mujoco.mj_saveLastXML("test.xml", model) -
mj_loadXML("test.xml")should succeed.
-
- Load scene.xml via relative path:
-
model = mujoco.MjModel.from_xml_path("xmls/scene.xml") -
mujoco.mj_saveLastXML("test.xml", model) -
mj_loadXML("test.xml")should succeed (noxmls/xmls/...errors).
-
- Load scene.xml via absolute path:
-
model = mujoco.MjModel.from_xml_path(os.path.abspath("xmls/scene.xml")) -
mujoco.mj_saveLastXML("test.xml", model) -
mj_loadXML("test.xml")should succeed.
-
Additional tests:
- VFS/custom resource provider cases.
-
mj_saveXMLandmj_saveXMLStringcallers (consider parity later).
Files changed
-
src/xml/xml.cc(parser include canonicalization) -
src/xml/xml_api.cc(temporary modelfiledir override for writer calls, helper DirnameOfPath)
@yuvaltassa please check this pr
i will fix it asap
Hi @Adityakk9031, thanks so much for opening this PR addressing the issue I posted.
Just wanted to check whether you’re still planning to finish it up. If you’re busy, I’d be happy to help or take over the remaining work to get it across the finish line -- whatever works best for you!
i wiil fix it today
@havess Check now
ok