Add dedicated gravity for global world -1 - #3724
Conversation
The VBD particle forward step indexes gravity[0], so world-local particles in every environment use world 0's gravity. Pass particle_world to the kernel and select the corresponding gravity, preserving world 0 as the fallback for global particles. Add a regression test with distinct gravity values across two worlds. Fixes newton-physics#3692
Select gravity from shape_world for each world-frame site that is not attached to a body. Preserve world 0 as the fallback for global sites.
Append the builder default to Model.gravity so world -1 has an independent gravity vector and entity world indices can be used directly. Keep local-only runtime gravity arrays compatible, and cover global particles, bodies, sensors, coupling, and inverse dynamics.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds dedicated gravity for global world ChangesGlobal gravity and world-index semantics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ModelBuilder
participant Model
participant Solver
participant SensorIMU
ModelBuilder->>Model: finalize local and global gravity entries
Model->>Solver: provide gravity and entity world mappings
Solver->>Solver: apply gravity[world_idx]
Model->>SensorIMU: provide gravity and shape_world
SensorIMU->>SensorIMU: apply gravity[shape_world[site_idx]]
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
🧹 Nitpick comments (1)
newton/_src/sim/model.py (1)
1970-1975: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the gravity units in
set_gravity.Add
[m/s²]to thegravityargument description, consistent withModel.gravity, so callers know the expected SI units.As per coding guidelines, public API docstrings under
newton/**/*.pymust include SI units for physical quantities.
[details][/details]Proposed documentation fix
- gravity: Gravity vector with shape ``(3,)``, a local-world array with shape + gravity: Gravity vector [m/s²] with shape ``(3,)``, a local-world array with shape🤖 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/sim/model.py` around lines 1970 - 1975, Update the gravity argument description in set_gravity to state that gravity is measured in SI units of [m/s²], matching the Model.gravity documentation while preserving the existing shape and behavior descriptions.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/_src/sim/model.py`:
- Around line 1970-1975: Update the gravity argument description in set_gravity
to state that gravity is measured in SI units of [m/s²], matching the
Model.gravity documentation while preserving the existing shape and behavior
descriptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 68de413c-69e5-4b9a-a6e2-1e7a0377522d
📒 Files selected for processing (28)
CHANGELOG.mddocs/concepts/worlds.rstnewton/_src/sensors/sensor_imu.pynewton/_src/sim/builder.pynewton/_src/sim/inverse_dynamics.pynewton/_src/sim/model.pynewton/_src/solvers/coupled/interface.pynewton/_src/solvers/featherstone/kernels.pynewton/_src/solvers/implicit_mpm/implicit_mpm_solver_kernels.pynewton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/solver.pynewton/_src/solvers/style3d/kernels.pynewton/_src/solvers/vbd/particle_vbd_kernels.pynewton/_src/solvers/vbd/rigid_vbd_kernels.pynewton/_src/solvers/vbd/solver_vbd.pynewton/_src/solvers/xpbd/kernels.pynewton/_src/viewer/viewer_gui.pynewton/examples/cloth/example_cloth_franka.pynewton/examples/contacts/example_contacts_rj45_plug.pynewton/examples/sensors/example_sensor_imu.pynewton/examples/softbody/example_softbody_franka.pynewton/tests/test_cloth.pynewton/tests/test_coupled_solver.pynewton/tests/test_implicit_mpm.pynewton/tests/test_inverse_dynamics.pynewton/tests/test_runtime_gravity.pynewton/tests/test_sensor_imu.pynewton/tests/test_solver_style3d.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
eric-heiden
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/_src/solvers/coupled/solver_coupled_proxy.py (1)
116-120: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCorrect fix for global-slot selection.
The rewritten
_reset_world_selectedcorrectly derives the global slot asworld_mask.shape[0] - 1and returns it forworld == -1, fixing the prior bug where global selection silently returnedFalsefor masks longer than 1 element. Verified boundary cases (world_count == 0, world >= 0 in-range/out-of-range) are all handled correctly via short-circuit evaluation.A one-line comment noting that
global_world_indexis the reserved trailing slot for world-1would help future readers, since the indexing convention isn't otherwise self-evident from this snippet alone.🤖 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/solvers/coupled/solver_coupled_proxy.py` around lines 116 - 120, Add a concise comment in _reset_world_selected explaining that global_world_index is the reserved trailing slot corresponding to world -1. Leave the existing selection logic 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.
Inline comments:
In `@newton/tests/test_coupled_solver.py`:
- Around line 2444-2478: Add a local proxy mapping alongside the existing
global-only mapping in the coupled reset test, initialize its history arrays
with distinct sentinel values, and assert those values remain unchanged after
reset with only the final global-mask entry selected. Keep the existing global
mapping assertions to verify only targeted global entities are cleared.
---
Nitpick comments:
In `@newton/_src/solvers/coupled/solver_coupled_proxy.py`:
- Around line 116-120: Add a concise comment in _reset_world_selected explaining
that global_world_index is the reserved trailing slot corresponding to world -1.
Leave the existing selection logic unchanged.
🪄 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 Plus
Run ID: cc743d55-52ff-4ebc-ae4d-dd5359df7402
📒 Files selected for processing (16)
CHANGELOG.mddocs/concepts/worlds.rstnewton/_src/sim/model.pynewton/_src/solvers/coupled/solver_coupled.pynewton/_src/solvers/coupled/solver_coupled_proxy.pynewton/_src/solvers/kamino/solver_kamino.pynewton/_src/solvers/kamino/tests/test_solvers_dvi.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/solvers/solver.pynewton/_src/solvers/vbd/rigid_vbd_kernels.pynewton/_src/solvers/vbd/solver_vbd.pynewton/tests/test_coupled_solver.pynewton/tests/test_custom_solver.pynewton/tests/test_mujoco_reset.pynewton/tests/test_solver_kamino_dvi.pynewton/tests/test_solver_vbd.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/sim/model.py
aefe5b0 to
04e0d20
Compare
eric-heiden
changed the title
vreutskyy
left a comment
vreutskyy
left a comment
There was a problem hiding this comment.
Three findings from local reproductions where the dedicated global gravity behavior does not carry through existing update paths or solver backends.
| full_shape = (self.world_count + 1, 3) | ||
| if gravity_np.shape == local_shape: | ||
| current = self.gravity.numpy() | ||
| current[: self.world_count] = gravity_np |
There was a problem hiding this comment.
[P1] Legacy gravity updates no longer affect global-only entities
ModelBuilder.finalize() reports world_count == 1 even when all entities belong to world -1. Consequently, set_gravity(..., world=0) and (world_count, 3) updates now modify gravity[0], while those entities read gravity[-1]. In a SemiImplicit reproduction, both calls succeeded but the particle continued accelerating at -9.81. The new Model.gravity shape also makes existing direct assignments with (world_count, 3) arrays incompatible.
There was a problem hiding this comment.
Fixed. Implicit single-world models keep the legacy one-entry Model.gravity, so world 0 updates, local arrays, and direct assignments still affect global-only entities.
| if self.world_gravity: | ||
| # Use per-world gravity from world_gravity list | ||
| gravity_vecs = self.world_gravity | ||
| gravity_vecs = [*self.world_gravity, global_gravity] |
There was a problem hiding this comment.
[P1] Kamino maps global entities to world 0
ModelKamino.from_newton() rewrites every -1 world index to 0 whenever world_count == 1, including models containing both global and world-0 entities. With global gravity -2 and world-0 gravity -5, constructing Kamino changed body_world from [-1, 0] to [0, 0]; one step produced Z velocities [-0.5, -0.5] instead of [-0.2, -0.5].
There was a problem hiding this comment.
Fixed. Kamino now uses a conversion-only world mapping and leaves model.body_world unchanged. Global bodies keep world -1 and use global gravity.
|
|
||
| g = wp.vec3(0.0, 0.0, 0.0) | ||
| if world >= 0 and world < gravity.shape[0]: | ||
| if world >= -1 and world < gravity.shape[0]: |
There was a problem hiding this comment.
[P1] MuJoCo uses world-0 gravity for global bodies
MuJoCo continues to initialize its gravity from model.gravity[0] (solver_mujoco.py:4998). This coupling kernel also reads body_world only when more than one MuJoCo world exists. With global gravity -2 and world-0 gravity -5, both bodies received Z velocity -0.5 after one step.
There was a problem hiding this comment.
Fixed. MuJoCo now corrects gravity per body when global and world-0 bodies share one MuJoCo world. The coupling hook also reports each body's gravity.
Preserve legacy gravity storage for implicit single-world models and avoid mutating Newton world assignments during Kamino conversion. Apply per-body gravity corrections for mixed global/local MuJoCo models and cover the update paths across solvers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/_src/solvers/mujoco/kernels.py (1)
241-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a brief comment explaining the
mujoco_worldselection logic.The branch that keeps
mujoco_world = 0unless the model uses separate MuJoCo worlds is not obvious.apply_mjc_body_f_kernel, right below, documents an equivalent invariant in its docstring. Add a short comment here that explains whybody_gravcomp.shape[0] > 1selects the body's own world, and why it stays0otherwise (single shared MuJoCo world, where global and local bodies coexist).As per coding guidelines: "Keep comments brief and explain why non-obvious code exists rather than restating what the code does."
🤖 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/solvers/mujoco/kernels.py` around lines 241 - 266, Add a brief rationale comment immediately above the mujoco_world selection branch in eval_mujoco_coupling_gravity_acceleration_kernel: when body_gravcomp.shape[0] > 1, use gravity_world for separate MuJoCo worlds; otherwise retain world 0 because global and local bodies share one MuJoCo world. Keep the comment concise and explain the invariant rather than restating the assignments.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.
Inline comments:
In `@newton/_src/solvers/kamino/_src/core/model.py`:
- Around line 741-749: Update the model parameter type hints on convert_joints
and convert_geometries in conversions.py from Model to Model | ModelView,
matching convert_rigid_bodies and the ModelView value passed by this call site.
Preserve their existing behavior and imports, adding the ModelView type
reference if needed.
---
Nitpick comments:
In `@newton/_src/solvers/mujoco/kernels.py`:
- Around line 241-266: Add a brief rationale comment immediately above the
mujoco_world selection branch in
eval_mujoco_coupling_gravity_acceleration_kernel: when body_gravcomp.shape[0] >
1, use gravity_world for separate MuJoCo worlds; otherwise retain world 0
because global and local bodies share one MuJoCo world. Keep the comment concise
and explain the invariant rather than restating the assignments.
🪄 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 Plus
Run ID: 40e6b54c-0f78-4a8c-8d2a-f9c1d036bfaf
📒 Files selected for processing (9)
CHANGELOG.mddocs/concepts/worlds.rstnewton/_src/sim/builder.pynewton/_src/sim/model.pynewton/_src/solvers/kamino/_src/core/conversions.pynewton/_src/solvers/kamino/_src/core/model.pynewton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_runtime_gravity.py
🚧 Files skipped from review as they are similar to previous changes (3)
- newton/_src/sim/model.py
- CHANGELOG.md
- newton/_src/sim/builder.py
Allow joint and geometry conversion helpers to accept ModelView, matching their call sites and the rigid body conversion helper.
3 participants
Description
Closes #3723.
Closes #3692.
Closes #3715.
Give
Model.gravitya dedicated final entry for global world-1, so global entities no longer inherit world 0 gravity. Entity-indexed gravity reads now use their world indices directly across the core solvers, VBD, Style3D, implicit MPM, MuJoCo coupling, sensors, and inverse dynamics.This fully incorporates the VBD fix from #3695 and the IMU fix from #3716 as separate commits, preserving Junseo Oh's (@pietis) authorship:
particle_world, fixing [BUG] SolverVBD particles ignore per-world gravity #3692 for particles in local worlds with different gravity values. Global particles select the dedicated final gravity entry for world-1.shape_world, fixing [BUG] SensorIMU world-frame sites ignore per-world gravity #3715 for world-frame sites in local worlds. Body-attached sites continue to select throughbody_world, and global sites and bodies use world-1's dedicated gravity.Runtime gravity updates remain backward compatible with arrays containing only local-world values, while
world=-1and full arrays can update the global value explicitly.Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Bug fix
Steps to reproduce:
-1with gravity distinct from world 0.Minimal reproduction:
New feature / API change
Summary by CodeRabbit
New Features
Bug Fixes
Documentation