MJCF import: handle <frame> tags#1390
Merged
eric-heiden merged 18 commits intoJan 21, 2026
Merged
Conversation
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds recursive MJCF support: composes frame-derived transforms, propagates accumulated incoming_xform through bodies/geoms/sites, and updates joint placement to respect composed frame/body rotations. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as MJCF Parser
participant Body as parse_body()
participant Frames as process_frames()
participant GetXform as get_frame_xform()
participant Children as Child Parsers (Bodies/Geoms/Sites)
Parser->>Body: parse_body(..., incoming_xform=world_xform)
Body->>Body: compose local transform with incoming_xform
Body->>Frames: process_frames(frames, parent_body, defaults, childclass, world_xform)
loop for each frame
Frames->>GetXform: get_frame_xform(frame, incoming_xform)
GetXform-->>Frames: frame_transform
Frames->>Children: parse children with incoming_xform=frame_transform
Children->>Body: nested parse_body(..., incoming_xform=frame_transform)
Children-->>Frames: child elements parsed
end
Frames-->>Body: frames processed
Body-->>Parser: body parsed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@newton/_src/utils/import_mjcf.py`:
- Around line 570-635: In process_frames, the frame's merged defaults
(_defaults) and resolved childclass (_childclass) must be propagated to geoms,
sites, and nested frames; change calls to parse_shapes and _parse_sites_impl to
pass _defaults instead of defaults, and extend frame_stack entries to carry and
pass along current defaults and childclass (initialize the stack entries with
the incoming defaults/childclass), then when popping use those carried values to
compute/merge childclass for children and to pass into nested frame entries so
parse_body, parse_shapes, _parse_sites_impl and nested frames all receive the
correct _defaults and _childclass.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.py
🧰 Additional context used
📓 Path-based instructions (4)
newton/_src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
The
newton/_src/directory is internal library implementation only and must not be imported by user code (examples, documentation, or external users)
Files:
newton/_src/utils/import_mjcf.py
newton/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Any user-facing class/function/object added under
_srcmust be exposed via the public Newton API through re-exports
Files:
newton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use prefix-first naming for classes:ActuatorPD,ActuatorPID(notPDActuator,PIDActuator)
Use prefix-first naming for methods:add_shape_sphere()(notadd_sphere_shape())
Method names must usesnake_case
Prefer nested classes when self-contained: if a helper type or enum is only meaningful inside one parent class and doesn't need a public identity, define it as a nested class instead of a top-level class/module
Follow PEP 8 for Python code
Use Google-style docstrings with clear, concise explanations of what functions do, their parameters, and return values
Files:
newton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.py
**/*
📄 CodeRabbit inference engine (AGENTS.md)
CLI arguments must use
kebab-case(e.g.,--use-cuda-graph, not--use_cuda_graph)
Files:
newton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.py
🧠 Learnings (3)
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.587Z
Learning: In Newton's MuJoCo solver, when transforming joint axes from Newton's internal frame to MuJoCo's expected frame, use wp.quat_rotate(joint_rot, axis) not wp.quat_rotate_inv(joint_rot, axis). The joint_rot represents rotation from joint-local to body frame, so forward rotation is correct.
Applied to files:
newton/_src/utils/import_mjcf.py
📚 Learning: 2026-01-13T03:11:40.556Z
Learnt from: jumyungc
Repo: newton-physics/newton PR: 1333
File: newton/_src/sim/builder.py:0-0
Timestamp: 2026-01-13T03:11:40.556Z
Learning: Ensure all VBD damping semantics follow Rayleigh-style damping and are unitless. This convention, demonstrated in SolverVBD, should be applied consistently across all VBD-related constraints in the codebase. In newton/_src/sim/builder.py, validate that any damping terms use the same normalization and are treated as dimensionless; add comments and unit tests to enforce consistency across VBD components.
Applied to files:
newton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.py
📚 Learning: 2026-01-13T07:40:13.635Z
Learnt from: CR
Repo: newton-physics/newton PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-13T07:40:13.635Z
Learning: Applies to newton/examples/**/*.py : Examples must implement a `test_final()` method that verifies the simulation state is valid after the example has run
Applied to files:
newton/tests/test_import_mjcf.py
🧬 Code graph analysis (1)
newton/tests/test_import_mjcf.py (1)
newton/_src/sim/builder.py (2)
ModelBuilder(71-6844)add_mjcf(1534-1628)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Tests / Stop self-hosted EC2 runner
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (6)
newton/_src/utils/import_mjcf.py (5)
564-569: Encapsulated frame transform composition looks good.
636-688: Incoming transform composition and joint-relative handling look consistent.
1053-1063: Good: body-level frame traversal uses body-relative coordinates.
1264-1265: Root bodies now receive the import xform as expected.
1288-1298: Worldbody frame handling is wired in cleanly.newton/tests/test_import_mjcf.py (1)
1789-2001: Solid coverage for frame transform composition scenarios.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Member
|
LGTM, just check if the CodeRabbit comment makes sense? |
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Member
Author
|
That was a good catch from CodeRabbit, improved the PR and added tests for the childclass use-case. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/utils/import_mjcf.py (1)
1052-1069: Frames inside bodies ignore the body’schildclass.
process_frames(...)is called with the parent’schildclass, so if a body defineschildclass(especially alongside a distinctclass), frames under that body won’t inherit the intended defaults.🛠️ Proposed fix
- process_frames( - body.findall("frame"), - parent_body=link, - defaults=defaults, - childclass=childclass, - world_xform=world_xform, - body_relative_xform=wp.transform_identity(), # Geoms/sites need body-relative coords - ) + _frame_childclass = body.get("childclass") or childclass + if _frame_childclass is None: + _frame_defaults = defaults + else: + _frame_defaults = merge_attrib(defaults, class_defaults.get(_frame_childclass, {})) + + process_frames( + body.findall("frame"), + parent_body=link, + defaults=_frame_defaults, + childclass=_frame_childclass, + world_xform=world_xform, + body_relative_xform=wp.transform_identity(), # Geoms/sites need body-relative coords + )
🤖 Fix all issues with AI agents
In `@newton/_src/utils/import_mjcf.py`:
- Around line 683-689: When composing joint anchors for bodies with a
non-identity frame (the parent >= 0 branch using builder.body_q and
relative_xform), rotate the local joint offset before adding the position
instead of directly adding joint_pos; specifically, replace direct addition of
joint_pos to body_pos_for_joints with the joint_pos rotated by
body_ori_for_joints (or equivalently compose the anchor using the body frame
transform like the world case). Apply the same change in the
freejoint/base_joint composition paths where joint anchors are built
(references: parent, builder.body_q, relative_xform, body_pos_for_joints,
body_ori_for_joints, joint_pos, and the freejoint/base_joint anchor composition
sites) so anchors are computed by composing the transformed joint point with the
body transform rather than simple vector addition.
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@newton/_src/utils/import_mjcf.py`:
- Around line 617-629: process_frames currently forwards frame-contained geoms
directly to parse_shapes, bypassing the body-level visual/collider routing
implemented in parse_body (visual_classes, collider_classes, parse_visuals,
parse_visuals_as_colliders, contype/conaffinity). Update process_frames to route
child_geoms through the same selection logic as parse_body: either call the
extracted helper that partitions geoms into visual vs collider groups (and
respects parse_visuals/parse_visuals_as_colliders, contype/conaffinity) or
replicate that partitioning before invoking parse_shapes; ensure you pass the
same incoming_xform/composed_body_rel and use builder.body_key[parent_body] for
body_name so semantics match geoms placed directly in the body.
- Around line 683-693: The base-joint path incorrectly ignores incoming_xform
when parent == -1: in the block that sets
body_pos_for_joints/body_ori_for_joints (used later to build _xform) replace the
"World parent: use raw body position" branch so it derives these values from
world_xform (the transformed frame/import root) instead of body_pos/body_ori; in
other words, when parent < 0 set body_pos_for_joints = world_xform.p and
body_ori_for_joints = world_xform.q so base joints use the same world_xform as
other joint handling (see builder.body_q, world_xform, and the code that builds
_xform).
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@newton/_src/utils/import_mjcf.py`:
- Around line 1099-1106: process_frames is being called with the parent's
childclass so frames inside a <body> don't inherit that body's own childclass;
compute the effective childclass from the current body element (e.g.
frame_childclass = body.get("childclass", childclass) or similar) and pass that
to process_frames (childclass=frame_childclass) so frames inherit the body's
declared childclass when present.
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
eric-heiden
approved these changes
Jan 21, 2026
jnskkmhr
pushed a commit
to jnskkmhr/newton
that referenced
this pull request
Jan 22, 2026
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
eric-heiden
pushed a commit
to eric-heiden/newton
that referenced
this pull request
Jan 28, 2026
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
This was referenced Jan 29, 2026
This was referenced Feb 24, 2026
mmacklin
pushed a commit
to mmacklin/newton
that referenced
this pull request
Apr 7, 2026
Signed-off-by: Alain Denzler <adenzler@nvidia.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
2 participants
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.
This is needed for Apptronik Appollo. Visually verified on the apollo as well which is missing fingertips without this.
Before your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.