Fix MJCF primitive shell inertia - #3738
Conversation
Interpret shell geom density as mass per area and compute primitive\nsurface mass properties with MuJoCo-compatible formulas. Handle both\ndensity-authored and explicit-mass geoms without changing collision geometry.
📝 WalkthroughWalkthroughChangesMJCF shell inertia support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MJCF
participant Importer
participant NewtonBody
participant MuJoCoTest
MJCF->>Importer: Parse primitive geom with shellinertia
Importer->>Importer: Compute surface area and shell inertia
Importer->>NewtonBody: Apply shell mass and inertia
MuJoCoTest->>NewtonBody: Compare mass and diagonal inertia
MuJoCoTest->>MuJoCo: Compare native mass properties
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
CHANGELOG.md (1)
98-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the issue reference.
Neighboring
Fixedentries cite the linked issue, for example lines 122, 136, and 139. This fix resolves issue#3737.♻️ Proposed entry
-- Import MJCF primitive geom `shellinertia` mass properties. +- Import MJCF primitive geom `shellinertia` mass properties. (`#3737`)🤖 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 `@CHANGELOG.md` at line 98, Update the MJCF primitive geom “shellinertia” changelog entry to include the issue reference `#3737`, matching the format used by neighboring Fixed entries.Source: Path instructions
newton/_src/utils/import_mjcf.py (1)
1308-1318: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWarn when
shellinertia="true"is ignored.Line 878 restricts shell handling to five primitive types. A mesh geom with
shellinertia="true"is common in MJCF assets. For such geoms the importer keeps the solid volumetric result and reports nothing, so the mass and inertia silently differ from native MuJoCo.Add a
verbosewarning for the ignored case so users can detect the gap.♻️ Proposed warning for unsupported shell types
primitive_shell_inertia = geom_attrib.get("shellinertia", "false").lower() == "true" and geom_type in { "sphere", "capsule", "cylinder", "ellipsoid", "box", } + if not primitive_shell_inertia and geom_attrib.get("shellinertia", "false").lower() == "true" and verbose: + print( + f"Warning: shellinertia is not supported for geom type '{geom_type}' " + f"({geom_name}); using solid mass properties" + )🤖 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/_src/utils/import_mjcf.py` around lines 1308 - 1318, In the MJCF geom import logic near the primitive shell handling, add a verbose warning when shellinertia is requested for an unsupported geom type such as mesh, while preserving the existing solid volumetric mass and inertia behavior. Reuse the importer’s existing verbose warning mechanism and ensure supported types continue through the current _compute_mjcf_primitive_shell_inertia path unchanged.
🤖 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 `@CHANGELOG.md`:
- Line 98: Update the MJCF primitive geom “shellinertia” changelog entry to
include the issue reference `#3737`, matching the format used by neighboring Fixed
entries.
In `@newton/_src/utils/import_mjcf.py`:
- Around line 1308-1318: In the MJCF geom import logic near the primitive shell
handling, add a verbose warning when shellinertia is requested for an
unsupported geom type such as mesh, while preserving the existing solid
volumetric mass and inertia behavior. Reuse the importer’s existing verbose
warning mechanism and ensure supported types continue through the current
_compute_mjcf_primitive_shell_inertia path unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6246d9ca-6e6f-44c3-82ef-c657f98caba4
📒 Files selected for processing (3)
CHANGELOG.mdnewton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.py
Description
Import
shellinertia="true"for sphere, capsule, cylinder, ellipsoid, and box geoms. The importer now treats density as mass per surface area and accumulates primitive shell mass properties using MuJoCo-compatible formulas, while preserving the original collision geometry.Closes #3737
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Bug fix
Steps to reproduce:
shellinertia="true".Minimal reproduction:
See #3737.
Summary by CodeRabbit
New Features
shellinertia="true".Bug Fixes
Tests