Skip to content

Let nested default classes accept the same children as the top level default - #543

Closed
adityasingh2400 wants to merge 1 commit into
google-deepmind:mainfrom
adityasingh2400:fix-nested-default-schema
Closed

Let nested default classes accept the same children as the top level default#543
adityasingh2400 wants to merge 1 commit into
google-deepmind:mainfrom
adityasingh2400:fix-nested-default-schema

Conversation

@adityasingh2400

Copy link
Copy Markdown

The nested <default> element in schema.xml carried a hand written copy of the top level <default> children, and that copy had drifted. Since the nested spec is self recursive, every default class below the top level used the stale copy, so PyMJCF rejected MJCF that MuJoCo itself accepts. <muscle> was missing entirely, which is the failure reported in #537, and <material> had lost metallic, roughness and its <layer> child. Also missing were group on eight actuator and tendon elements, actlimited and actrange on <general>, fromto on <site>, springlength on <tendon>, solreffriction on <pair>, projection on <camera>, and limited="auto" on <joint> and <tendon>.

Rather than re-copying the block, the nested element now inherits the enclosing element's children through a new inherit_children schema flag while keeping its own required class attribute. That deletes 316 lines of duplication and makes the two lists impossible to desynchronise as MuJoCo gains elements.

I confirmed with mujoco.MjSpec.from_string on MuJoCo 3.11.0 that all sixteen affected constructs are valid MJCF, and that all sixteen were rejected by PyMJCF before this change. The new tests in element_test.py cover the reported <muscle> case, the fourteen previously rejected attributes, and the invariant that the nested and top level specs stay in sync.

Fixes #537

…default

The nested <default> element in the PyMJCF schema carried a hand written
copy of the top level <default> children, and the copy had fallen behind.
Because the nested spec is self recursive, every default class below the
top level used the stale copy, so valid MJCF was rejected: <muscle> was
missing entirely, <material> had lost metallic, roughness and its <layer>
child, and group, actrange, fromto, springlength, solreffriction,
projection and limited="auto" were missing from various elements.

The nested element now inherits the enclosing element's children through a
new inherit_children schema flag while keeping its own required class
attribute, which removes 316 lines of duplication and makes the two lists
impossible to desynchronise as MuJoCo gains elements.

Fixes google-deepmind#537
@adityasingh2400

Copy link
Copy Markdown
Author

Closing this as superseded by 985d094, which replaced the hand-maintained schema.xml with the generated schema from MuJoCo's mjcf.schema.

I checked rather than assuming, since this PR changed schema.py and not only the schema data. On current main the nested <default> carries recursive="true" and enumerates its children inline, and schema.py already turns that attribute into a self-reference. Parsing the new schema gives 21 children for the top level <default> and 20 for the nested one, and the two sets are identical once default itself is set aside.

That is exactly the property this PR was adding an inherit_children mechanism to guarantee, so the mechanism is no longer needed. Thanks for the regeneration, it is a better fix than mine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nested default does not accept muscle children in PyMJCF

1 participant