Honor MJCF geom inertia selection#3596
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesMJCF inertia inference
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MJCF
participant parse_body
participant _process_body_geoms
participant temporary_ModelBuilder
participant target_body
MJCF->>parse_body: provide compiler and body definitions
parse_body->>_process_body_geoms: pass inertia inference policy
_process_body_geoms->>temporary_ModelBuilder: re-parse selected unloaded geoms
temporary_ModelBuilder-->>_process_body_geoms: return inferred mass and inertia
_process_body_geoms->>target_body: update body mass, inertia, and center of mass
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@newton/_src/utils/import_mjcf.py`:
- Around line 1604-1607: Reject bodies missing <inertial> in the
inertia-selection logic around has_inertial_definition and
infer_body_inertia_from_geoms by raising ValueError when inertia_from_geom is
"false", ignore_inertial_definitions is false, and no inertial definition
exists. Add coverage in newton/tests/test_import_mjcf.py at lines 2833-2852
using a false-mode body without <inertial> and assert import raises.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 147d2025-5d1d-45a9-bb50-f2f0fd0679f7
📒 Files selected for processing (3)
CHANGELOG.mdnewton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
CI failure analysis: every failing job reaches the same warning for explicit mass on the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
newton/tests/test_import_mjcf.py (1)
2854-2865: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a prefix-first name for the MJCF fixture.
Rename
missing_inertial_mjcftomjcf_missing_inertialso MJCF fixtures group predictably in autocomplete.Proposed fix
- missing_inertial_mjcf = """<?xml version="1.0" ?> + mjcf_missing_inertial = """<?xml version="1.0" ?> ... - newton.ModelBuilder().add_mjcf(missing_inertial_mjcf) + newton.ModelBuilder().add_mjcf(mjcf_missing_inertial)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/tests/test_import_mjcf.py` around lines 2854 - 2865, Rename the MJCF fixture variable from missing_inertial_mjcf to mjcf_missing_inertial and update its use in the ModelBuilder().add_mjcf call, leaving the test behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@newton/tests/test_import_mjcf.py`:
- Around line 2854-2865: Rename the MJCF fixture variable from
missing_inertial_mjcf to mjcf_missing_inertial and update its use in the
ModelBuilder().add_mjcf call, leaving the test behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0973062e-148e-489f-99bf-f97249fabf2d
📒 Files selected for processing (2)
newton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/utils/import_mjcf.py
|
Addressed the fixture naming nit in d719ab2. The change only renames the local test variable to use the MJCF prefix. Validation: the targeted |
|
To unblock CI while #3595 awaits review, this branch now includes #3595 as an explicit temporary dependency in 634f4a9. The mesh-mass fix was not reimplemented here. After #3595 merges, I will update this branch from Local validation on the combined branch:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
newton/tests/test_import_mjcf.py (1)
2892-2925: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert inertia and COM, not only mass, for
inertiafromgeommodes.This regression test can pass even if
inertiafromgeom="true"overrides mass correctly but leaves the authored inertia or COM in place. Use distinct authored and geom-derived values, then assertbody_inertiaandbody_comfor all three modes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/tests/test_import_mjcf.py` around lines 2892 - 2925, Extend test_compiler_inertiafromgeom_modes with distinct authored and geom-derived center-of-mass and inertia values, then define expected body_inertia and body_com for each inertiafromgeom mode. Assert builder.body_inertia and builder.body_com alongside body_mass for all modes, ensuring true uses geom-derived values while auto and false preserve the authored inertial data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@newton/tests/test_import_mjcf.py`:
- Around line 2892-2925: Extend test_compiler_inertiafromgeom_modes with
distinct authored and geom-derived center-of-mass and inertia values, then
define expected body_inertia and body_com for each inertiafromgeom mode. Assert
builder.body_inertia and builder.body_com alongside body_mass for all modes,
ensuring true uses geom-derived values while auto and false preserve the
authored inertial data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 06655444-b1c1-4692-af74-6200ccc87d34
📒 Files selected for processing (3)
CHANGELOG.mdnewton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.py
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- newton/_src/utils/import_mjcf.py
|
Addressed the inertia/COM coverage feedback in 23426d3. The test now gives the authored inertial and geom different COM and inertia values, then checks mass, COM, and the full inertia tensor for Validation:
|
Apply compiler inertiafromgeom and inertiagrouprange semantics. Compute inertia from visual geoms even when visual shapes are not retained by the importer.
Match MuJoCo when inertiafromgeom is false by rejecting bodies that omit the required inertial element.
23426d3 to
c3826ef
Compare
|
PR #3595 has merged, so I rebuilt this branch on current upstream Validation after rebuilding:
The explicit pre-commit version is only for local compatibility with Git 2.30; no project dependency changed. |
adenzler-nvidia
left a comment
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Implementation looks good — the semantics match MuJoCo's compiler docs (inertiafromgeom modes, inclusive inertiagrouprange, compile error on false + missing <inertial>), the group-range zeroing happens before every consumer including the mesh path, and the scratch-builder transfer math checks out. Just a few nitpicks inline, mostly about hardening the tests around the new machinery.
| msg="Visual geom with explicit mass should contribute non-zero inertia", | ||
| ) | ||
|
|
||
| def test_visual_geom_mass_without_parsing_visuals(self): |
There was a problem hiding this comment.
All three new tests put a single massive geom into an empty body, so new_com == p in _update_body_mass and the parallel-axis/rotation terms are identically zero — a sign or frame error in the scratch-builder transfer would pass every test here. Could you add a case with an off-origin, rotated visual plus a massive collider elsewhere (so the combined COM differs from both geom COMs), importing with parse_visuals=True and False and asserting body_mass, body_com, and the full body_inertia match between the two runs?
There was a problem hiding this comment.
Addressed in 710516f. test_geom_inertia_independent_of_visual_loading now combines an off-origin rotated visual box with a rotated collider at a different position. It verifies that the combined COM differs from both geom COMs and compares mass, COM, and the full inertia tensor between parse_visuals=True and parse_visuals=False.
| unloaded_geoms = [] | ||
| if parse_visuals_as_colliders: | ||
| loaded_geom_ids = {id(geom) for geom in visuals} | ||
| unloaded_geoms = [geom for geom in colliders if id(geom) not in loaded_geom_ids] |
There was a problem hiding this comment.
This branch is untested — nothing under newton/tests/ uses parse_visuals_as_colliders at all. It's the mirror image of the tested case (here the collision geoms go through the scratch builder), so it deserves the same mass/COM/inertia equality check against a parse_visuals=True reference import.
There was a problem hiding this comment.
Addressed in 710516f. The same regression now includes parse_visuals_as_colliders=True and compares its mass, COM, and full inertia tensor against the parse_visuals=True reference.
| <body name="test"> | ||
| <freejoint/> | ||
| <geom name="visual" class="visual" type="sphere" size="0.1" | ||
| contype="0" conaffinity="0" group="2" mass="0.012"/> |
There was a problem hiding this comment.
The unloaded-geom path is only exercised with explicit mass=, but the mechanism behind #3593 is equally the density path (a visual geom without mass picking up the default density inside the scratch builder). Worth a density-only variant asserting equal mass/inertia for parse_visuals=True vs False.
There was a problem hiding this comment.
Addressed in 710516f. The regression now runs both explicit-mass and density-only cases across all three visual-loading modes and compares mass, COM, and full inertia.
| return shapes | ||
|
|
||
| def accumulate_inertia_from_unloaded_geoms(defaults, body_name, link, geoms, incoming_xform=None, label_prefix=""): | ||
| """Accumulate selected geom inertia without retaining visual shapes.""" |
There was a problem hiding this comment.
The unloaded geoms aren't always visual: in the parse_visuals_as_colliders branch they are the displaced collision geoms. Maybe "Accumulate inertia from geoms not loaded as shapes, via a scratch builder."
There was a problem hiding this comment.
Addressed in 710516f. The docstring now reads: Accumulate inertia from geoms not loaded as shapes, via a scratch builder.
| ) | ||
|
|
||
| def test_compiler_inertiafromgeom_modes(self): | ||
| """Test compiler control over geom inference when inertial data exists.""" |
There was a problem hiding this comment.
The second half of the test asserts the missing-<inertial> error, exactly the case where inertial data does not exist. Maybe "Test inertiafromgeom modes with and without an element."
There was a problem hiding this comment.
Addressed in 710516f. The test docstring now describes inertiafromgeom modes with and without an inertial element.
| @@ -735,6 +755,11 @@ def parse_shapes( | |||
| # We'll add the explicit mass to the body separately | |||
There was a problem hiding this comment.
nit: this promise is now conditional — the group-range check below can null geom_mass_explicit three lines later. The block comment above already covers the intent, so these two lines could go.
There was a problem hiding this comment.
Addressed in 710516f. I removed the two stale comments; the existing block comment remains as the accurate description of the explicit-mass path.
| @@ -1943,7 +2038,7 @@ def parse_body( | |||
| ) | |||
|
|
|||
| m = builder.body_mass[link] | |||
There was a problem hiding this comment.
nit: this read is now dead — m is only used after being reassigned inside the branch below.
There was a problem hiding this comment.
Addressed in 710516f. I removed the dead builder.body_mass[link] read.
| ) | ||
| mass = inertia_builder.body_mass[inertia_link] | ||
| if mass > 0.0: | ||
| builder._update_body_mass( |
There was a problem hiding this comment.
nit: unlike the explicit-mass path in parse_shapes, this skips the body_lock_inertia check. It's redundant today (this helper only runs when inference is on, which also skips the <inertial> block that sets the lock), but a guard here would keep future call sites safe.
There was a problem hiding this comment.
Addressed in 710516f. The scratch-builder transfer now checks not builder.body_lock_inertia[link] before updating body mass properties.
| if inertia_from_geom == "false" and not ignore_inertial_definitions and not has_inertial_definition: | ||
| raise ValueError( | ||
| f"MJCF body '{body_name}' requires an <inertial> element when compiler inertiafromgeom=\"false\"." | ||
| ) |
There was a problem hiding this comment.
This unconditional guard rejects every body without an <inertial> element when inertiafromgeom="false". MuJoCo only validates mass and inertia for bodies that have joints; fixed bodies are valid with zero mass. I reproduced this for both a body fixed to the world and a fixed child of a moving body: MuJoCo 3.10 accepts them, while this PR raises ValueError.
There was a problem hiding this comment.
Confirmed with MuJoCo 3.10 and addressed in 710516f. The missing-inertial guard now applies only to bodies with an authored joint or freejoint. The regression covers a fixed body attached to the world and a fixed child under a moving body, while retaining the error for a moving body without <inertial>. The focused tests and all 235 MJCF importer tests pass locally.
3 participants
Description
Newton's MJCF importer did not follow MuJoCo's compiler rules for choosing body inertia.
This change adds support for:
inertiafromgeom="auto","true", and"false"inertiagrouprangeparse_visuals=FalseLoaded shapes still use the normal mass path. Visual geoms that are intentionally not loaded are evaluated in a temporary builder, so they affect body mass and inertia without appearing in the model.
Closes #3593.
Checklist
Test plan
uv run --extra dev -m newton.tests -k test_visual_geom_mass_without_parsing_visualsuv run --extra dev -m newton.tests -k test_compiler_inertiagrouprangeuv run --extra dev -m newton.tests -k test_compiler_inertiafromgeom_modesuv run --extra dev -m newton.tests -p test_import_mjcf.py(234 tests)uvx pre-commit run -aBug fix
Before this change,
parse_visuals=Falsecould change body mass, excluded geom groups still contributed inertia, andinertiafromgeom="true"did not override explicit inertial data. These cases now follow MuJoCo's compiler semantics.Summary by CodeRabbit
<compiler inertiafromgeom>modes and<compiler inertiagrouprange>filtering.massfor small mesh geoms without spurious warnings.<inertial>data is missing.