Support include below the root element - #549
Open
shubhxho wants to merge 1 commit into
Open
Conversation
MuJoCo allows <include/> wherever a child element is allowed and puts the contents of the named file in its place, but the parser only looked for it as a child of <mujoco>. Anything deeper was left in the tree and reached the schema, which reported it as a KeyError on the tag rather than as anything to do with includes. Expand those in place before parsing, recursively, so a file included from a subdirectory can include further files relative to itself. Top level includes still go through include_copy as whole models so they keep their own asset directories. Fixes google-deepmind#529. Signed-off-by: Shubh <shubh@shubhxho.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #529.
Problem
_parselooks for includes withxml_root.findall('include'), which only finds direct children of<mujoco>. MuJoCo allows<include/>wherever a child element is allowed. Anything deeper is left in the tree, reaches the schema, and comes back as an error about the tag rather than about includes:for
which
mujoco.MjModel.from_xml_pathaccepts. The motivating case in the issue is musculoskeletal models that group defaults per class:Fix
Expand non-root includes in place before parsing: read the named file, check its root is
<mujoco.*>, and put its children where the tag was. Recursive, so a file included from a subdirectory can include further files relative to itself.Top level includes are left alone. They keep going through
include_copyas whole parsed models, so their ownmodel_dirand asset handling are unchanged, and no existing model parses differently.After:
Testing
Three new assets and a test that the included content lands in the enclosing element rather than being merged at the root - a body included inside
<worldbody>hasworldbodyas its parent, and defaults included inside a class end up in that class. The three existing parameterised parse tests gain aWithNestedIncludecase.The four new tests fail without the change to
parser.py. I also checked the round trip compiles:mujoco.MjModel.from_xml_string(model.to_xml_string())on a model with an include nested two levels deep, in a subdirectory, that itself includes another file.Note
This picks the splice-in-place reading of nested includes, which is what MuJoCo does, rather than extending the whole-model
include_copypath downwards. Happy to change the approach if you would rather nested includes behave like the top level ones.