Improve USD parsing, MJC conversion for Allegro hand#730
Conversation
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
📝 WalkthroughWalkthroughAdds a per-environment joint-transform kernel and joint update flow, introduces managed MuJoCo viewer APIs/state, refactors USD import to per‑articulation parsing and joint ordering, adds an Allegro-hand example and tests/assets, enforces float32 replicate offsets, extends solver notify flags with ALL, and hardens selection empty-slice handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Example/Test
participant Solver as SolverMuJoCo
participant Kernel as update_joint_transforms_kernel
participant MJC as MuJoCo Model
App->>Solver: notify_model_changed(SolverNotifyFlags.JOINT_PROPERTIES)
Solver->>Kernel: launch(joint_X_p, joint_X_c, joint_mjc_dof_start, ...)
Kernel-->>Solver: joint_pos, joint_axis, body_pos, body_quat
Solver->>MJC: write jnt_pos/jnt_axis/body_pos/body_quat into mjw_model
App->>Solver: step() / render_mujoco_viewer(...)
sequenceDiagram
autonumber
participant USD as USD Scene
participant Import as parse_usd
participant Builder as ModelBuilder
participant Coll as Collision Filterer
USD-->>Import: enumerate prims
Import->>Import: split ignore paths (regex vs non-regex) and filter prims
Import->>Builder: add_articulation(articulation_path)
alt articulation has no joints
Import->>Builder: add single body + free joint
else
Import->>Import: optional topological sort of joints
Import->>Builder: add bodies in sorted order
Import->>Builder: add joints
end
Import->>Coll: generate shape pairs via itertools.combinations when self-collisions disabled
Coll-->>Builder: apply shape_collision_filter_pairs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (4)
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.
Actionable comments posted: 2
🧹 Nitpick comments (4)
newton/_src/utils/import_usd.py (3)
782-786: Good addition: cache ignored rigid-body paths; consider precompiling regexesCaching ignored body paths is useful. For larger stages, precompile patterns once (e.g., at argument processing) to reduce repeated compilation and speed up matching across the many checks.
Outside-changes sketch:
# right after ignore_paths defaulting if ignore_paths is None: ignore_paths = [] compiled_ignores = [re.compile(p) for p in ignore_paths] # then replace: any(re.match(p, path) for p in ignore_paths) # with: any(rx.match(path) for rx in compiled_ignores)
929-937: Order of operations: avoid dict lookup before ignore checksYou dereference joint_descriptions[joint_key] before evaluating ignore filters. Reorder to avoid potential KeyError and wasted work when the joint path is ignored.
Apply this diff:
- joint_key = str(p) - joint_desc = joint_descriptions[joint_key] - if any(re.match(p, joint_key) for p in ignore_paths): + joint_key = str(p) + if any(re.match(p, joint_key) for p in ignore_paths): continue + joint_desc = joint_descriptions[joint_key] if str(joint_desc.body0) in ignored_body_paths: continue if str(joint_desc.body1) in ignored_body_paths: continue joint_names.append(joint_key)
942-950: Single-body articulations: add guard for empty art_bodiesIf no body was inserted (unexpected but possible in malformed files), art_bodies[0] will raise. Add a sanity guard to skip with a warning.
Suggested change:
- single_body_articulation = True + single_body_articulation = TrueAnd later when using it:
- # just a single rigid body articulation - child_body_id = art_bodies[0] + # just a single rigid body articulation + if not art_bodies: + warnings.warn(f"No bodies added for single-body articulation {path}. Skipping free joint.", stacklevel=2) + continue + child_body_id = art_bodies[0]newton/_src/solvers/mujoco/solver_mujoco.py (1)
1944-2019: Simplified add_geoms signature — LGTMRemoving the unused incoming_xform parameter clarifies intent; using per-shape tf is fine. Consider copying size before mutating zeros to avoid mutating model arrays in-place.
Outside-changes sketch:
size = shape_size[shape].copy() ... size[size == 0.0] = nonzero
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
newton/_src/solvers/mujoco/solver_mujoco.py(4 hunks)newton/_src/utils/import_usd.py(8 hunks)newton/tests/test_mujoco_solver.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
PR: newton-physics/newton#519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
PR: newton-physics/newton#519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
🧬 Code graph analysis (2)
newton/_src/utils/import_usd.py (1)
newton/_src/sim/builder.py (1)
add_joint_free(1605-1656)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
newton/_src/solvers/flags.py (1)
SolverNotifyFlags(22-43)
⏰ 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). (4)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (9)
newton/_src/utils/import_usd.py (6)
833-835: Ignore rigid bodies by path — LGTMSkipping rigid bodies early and tracking in ignored_body_paths will prevent downstream mass/shape updates and is the right call.
877-878: Skip entire articulation by path — LGTMEarly-out for ignored articulation roots avoids unnecessary work and keeps maps consistent.
890-891: Skip articulation members that were ignored as bodies — LGTMThis avoids dangling references and joint inconsistencies.
995-1011: Root joint incoming_xform path — LGTMConditionally applying articulation_xform only to the root joint when appropriate is consistent.
1048-1049: Skip shapes by path — LGTMThe path-based filter here keeps visual/physics loading consistent with body/joint filters.
1254-1255: Guard against invalid body_id — LGTMSkipping MassAPI overrides for missing bodies avoids -1 indexing and aligns with earlier ignore behavior.
newton/_src/solvers/mujoco/solver_mujoco.py (2)
2343-2347: Notify all relevant flags on conversion — LGTMIncluding SHAPE_PROPERTIES unconditionally ensures properties are synchronized before put_data.
2361-2366: mjw_data initialization call formatting — LGTMNo behavioral change; parameters are correct.
newton/tests/test_mujoco_solver.py (1)
455-455: Relaxed eigenvalue tolerance to places=4 — LGTMReasonable given numerical differences from eigen decomposition and sorting; keeps test robust.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
1407-1414: Honor JOINT_PROPERTIES in notify_model_changed.Currently ignored; joint frame edits won’t trigger updates if callers send the semantically correct flag.
Apply this diff:
def notify_model_changed(self, flags: int): if flags & SolverNotifyFlags.BODY_INERTIAL_PROPERTIES: self.update_model_inertial_properties() + if flags & SolverNotifyFlags.JOINT_PROPERTIES: + self.update_joint_properties() if flags & SolverNotifyFlags.JOINT_DOF_PROPERTIES: self.update_joint_properties() if flags & SolverNotifyFlags.SHAPE_PROPERTIES: self.update_geom_properties()
2080-2141: Indexing bug with filtered joints (selected_joints) → wrong array indices.
jiindexes the filtered list, but full-size arrays are indexed withji. Use the absolute joint index.Apply this focused diff:
- for ji in joint_order: - parent, child = joints_simple[ji] + for ji in joint_order: + parent, child = joints_simple[ji] + abs_j = selected_joints[ji] @@ - tf = wp.transform(*joint_parent_xform[ji]) - tf = tf * wp.transform_inverse(wp.transform(*joint_child_xform[ji])) + tf = wp.transform(*joint_parent_xform[abs_j]) + tf = tf * wp.transform_inverse(wp.transform(*joint_child_xform[abs_j])) @@ - joint_pos = wp.vec3(*joint_child_xform[ji, :3]) - joint_rot = wp.quat(*joint_child_xform[ji, 3:]) + joint_pos = wp.vec3(*joint_child_xform[abs_j, :3]) + joint_rot = wp.quat(*joint_child_xform[abs_j, 3:]) @@ - j_type = joint_type[ji] - qd_start = joint_qd_start[ji] + j_type = joint_type[abs_j] + qd_start = joint_qd_start[abs_j] @@ - joint_mjc_dof_start[ji] = len(spec.joints) + joint_mjc_dof_start[abs_j] = len(spec.joints) @@ - lin_axis_count, ang_axis_count = joint_dof_dim[ji] + lin_axis_count, ang_axis_count = joint_dof_dim[abs_j]Also applies to: 2132-2141, 2142-2274
♻️ Duplicate comments (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
1111-1151: Warp kernel: invalid truthiness checks on arrays; branchless updates or scalar flags required.Inside @wp.kernel,
if shape_mu:/if shape_size:etc. are always truthy and unsafe. Write unconditionally or guard with scalar booleans.Apply this diff (branchless, safe):
- if shape_collision_radius: - # update bounding radius - geom_rbound[worldid, geom_idx] = shape_collision_radius[shape_idx] + # update bounding radius + geom_rbound[worldid, geom_idx] = shape_collision_radius[shape_idx] - if shape_mu: - # update friction (slide, torsion, roll) - mu = shape_mu[shape_idx] - geom_friction[worldid, geom_idx] = wp.vec3f(mu, torsional_friction * mu, rolling_friction * mu) + # update friction (slide, torsion, roll) + mu = shape_mu[shape_idx] + geom_friction[worldid, geom_idx] = wp.vec3f(mu, torsional_friction * mu, rolling_friction * mu) - if shape_ke and shape_kd: - # update solref (stiffness, damping as time constants) + # update solref (stiffness, damping as time constants) # MuJoCo uses time constants, Newton uses direct stiffness/damping # convert using heuristic: time_const = sqrt(mass/stiffness) ke = shape_ke[shape_idx] kd = shape_kd[shape_idx] if ke > 0.0: # use provided time constant for stiffness time_const_stiff = contact_stiffness_time_const if kd > 0.0: time_const_damp = kd / (2.0 * wp.sqrt(ke)) else: time_const_damp = 1.0 else: time_const_stiff = contact_stiffness_time_const time_const_damp = 1.0 geom_solref[worldid, geom_idx] = wp.vec2f(time_const_stiff, time_const_damp) - if shape_size: - # update size - geom_size[worldid, geom_idx] = shape_size[shape_idx] + # update size + geom_size[worldid, geom_idx] = shape_size[shape_idx] - if shape_transform: - # update position and orientation - tf = shape_transform[shape_idx] - incoming_xform = shape_incoming_xform[shape_idx] - tf = incoming_xform * tf - pos = tf.p - quat = tf.q - geom_pos[worldid, geom_idx] = pos - geom_quat[worldid, geom_idx] = wp.quatf(quat.w, quat.x, quat.y, quat.z) + # update position and orientation + tf = shape_transform[shape_idx] + incoming_xform = shape_incoming_xform[shape_idx] + tf = incoming_xform * tf + pos = tf.p + quat = tf.q + geom_pos[worldid, geom_idx] = pos + geom_quat[worldid, geom_idx] = wp.quatf(quat.w, quat.x, quat.y, quat.z)
🧹 Nitpick comments (4)
newton/_src/sim/builder.py (1)
594-597: Nit: avoid intermediate float64 allocations in-loop.Initialize zeros as float32 to skip downcast.
Apply this diff:
- offset = np.zeros(3) + offset = np.zeros(3, dtype=np.float32) @@ - offset = np.zeros(3) + offset = np.zeros(3, dtype=np.float32)Also applies to: 603-607
newton/examples/robot/example_robot_allegro_hand.py (2)
47-55: Remove hard-coded joint counts in kernel.Derive per-env joint index and driven-DOF count via kernel args to keep the example model-agnostic.
Apply this diff:
-@wp.kernel -def move_hand( +@wp.kernel +def move_hand( joint_qd_start: wp.array(dtype=wp.int32), sim_time: wp.array(dtype=wp.float32), sim_dt: float, + joints_per_env: int, + num_finger_dofs: int, # outputs joint_target: wp.array(dtype=wp.float32), joint_parent_xform: wp.array(dtype=wp.transform), ): env_id = wp.tid() - root_joint_id = env_id * 22 + root_joint_id = env_id * joints_per_env t = sim_time[env_id] @@ - for i in range(20): + for i in range(num_finger_dofs): joint_target[root_dof_start + i] = wp.sin(t + float(i) * 0.1) * 0.2 + 0.5And update the launch:
- wp.launch( + wp.launch( move_hand, dim=self.num_envs, - inputs=[self.model.joint_qd_start, self.env_time, self.sim_dt], + inputs=[self.model.joint_qd_start, self.env_time, self.sim_dt, + self.model.joint_count // self.model.num_envs, 20], outputs=[self.control.joint_target, self.model.joint_X_p], )
158-160: Use correct notify flag (and include both).You update joint targets and joint parent transforms; notify both to future-proof semantics.
Apply this diff:
- self.solver.notify_model_changed(SolverNotifyFlags.JOINT_DOF_PROPERTIES) + self.solver.notify_model_changed( + SolverNotifyFlags.JOINT_PROPERTIES | SolverNotifyFlags.JOINT_DOF_PROPERTIES + )newton/_src/solvers/mujoco/solver_mujoco.py (1)
991-1007: Simplify mapping arrays to per-env size.
joint_mjc_dof_startis sized [joint_count] but read withjoint_in_env. Consider storing it as [joints_per_env] to make intent and bounds explicit.Also applies to: 1021-1029, 1038-1044
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
newton/_src/sim/builder.py(2 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(13 hunks)newton/_src/utils/import_usd.py(9 hunks)newton/examples/robot/example_robot_allegro_hand.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/utils/import_usd.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
PR: newton-physics/newton#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/solvers/mujoco/solver_mujoco.py
🧬 Code graph analysis (2)
newton/examples/robot/example_robot_allegro_hand.py (4)
newton/_src/solvers/flags.py (1)
SolverNotifyFlags(22-43)newton/_src/sim/builder.py (4)
ModelBuilder(67-4459)add_usd(710-808)replicate(618-640)add_ground_plane(2497-2517)newton/_src/sim/joints.py (1)
JointMode(81-95)newton/_src/solvers/mujoco/solver_mujoco.py (1)
SolverMuJoCo(1153-2664)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
newton/_src/sim/joints.py (1)
JointType(20-44)newton/_src/solvers/flags.py (1)
SolverNotifyFlags(22-43)
⏰ 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). (4)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (1)
newton/_src/sim/builder.py (1)
581-581: Float32 offsets: good change.Aligns with device arrays and reduces casts.
Also applies to: 608-611
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
| # Shows how to set up a simulation of a Allegro Hand articulation | ||
| # from a Mujoco file using newton.ModelBuilder.add_mjcf(). | ||
| # | ||
| # Command: python -m newton.examples robot_allegro_hand --num-envs 16 |
There was a problem hiding this comment.
I will update this comment and add this to the tests and README.md.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
adenzler-nvidia
left a comment
There was a problem hiding this comment.
generally looking good! Thanks a lot. Looking forward to the tests :-)
AntoineRichard
left a comment
There was a problem hiding this comment.
It works nicely in IL. The one weird thing is collisions, but that's outside the scope of this PR.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
…fixes Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
newton/tests/test_mujoco_solver.py (1)
448-448: Lower tolerance for inertia eigvals — prefer rtol/atol to reduce flakiness.Dropping to
places=4is reasonable but still brittle across platforms. Considernp.testing.assert_allclose(..., rtol=5e-4, atol=1e-5)instead.Apply:
- places=4, + rtol=5e-4, atol=1e-5,
🧹 Nitpick comments (1)
newton/tests/test_mujoco_solver.py (1)
1427-1431: Convert wp types to arrays in asserts to avoid implicit conversion issues.Use component arrays for
tf.pandtf.qto keep NumPy happy.Apply:
- np.testing.assert_allclose(solver.mjw_model.body_pos.numpy()[0, mjc_body], tf.p, atol=1e-6) + np.testing.assert_allclose( + solver.mjw_model.body_pos.numpy()[0, mjc_body], + [tf.p.x, tf.p.y, tf.p.z], + atol=1e-6, + ) np.testing.assert_allclose( solver.mjw_model.body_quat.numpy()[0, mjc_body], [tf.q.w, tf.q.x, tf.q.y, tf.q.z], atol=1e-6 ) @@ - np.testing.assert_allclose(solver.mjw_model.body_pos.numpy()[0, mjc_body], tf.p, atol=1e-6) + np.testing.assert_allclose( + solver.mjw_model.body_pos.numpy()[0, mjc_body], + [tf.p.x, tf.p.y, tf.p.z], + atol=1e-6, + ) np.testing.assert_allclose( solver.mjw_model.body_quat.numpy()[0, mjc_body], [tf.q.w, tf.q.x, tf.q.y, tf.q.z], atol=1e-6 ) @@ - np.testing.assert_allclose( - solver.mjw_model.body_pos.numpy()[0, mjc_body], - tf.p, - atol=1e-6, - ) + np.testing.assert_allclose( + solver.mjw_model.body_pos.numpy()[0, mjc_body], + [tf.p.x, tf.p.y, tf.p.z], + atol=1e-6, + ) np.testing.assert_allclose( solver.mjw_model.body_quat.numpy()[0, mjc_body], [tf.q.w, tf.q.x, tf.q.y, tf.q.z], atol=1e-6, )Also applies to: 1462-1466, 1493-1502
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (82)
docs/api/_generated/newton.Axis.rstis excluded by!**/_generated/**docs/api/_generated/newton.AxisType.rstis excluded by!**/_generated/**docs/api/_generated/newton.CollisionPipeline.rstis excluded by!**/_generated/**docs/api/_generated/newton.Contacts.rstis excluded by!**/_generated/**docs/api/_generated/newton.Control.rstis excluded by!**/_generated/**docs/api/_generated/newton.EqType.rstis excluded by!**/_generated/**docs/api/_generated/newton.GeoType.rstis excluded by!**/_generated/**docs/api/_generated/newton.JointMode.rstis excluded by!**/_generated/**docs/api/_generated/newton.JointType.rstis excluded by!**/_generated/**docs/api/_generated/newton.Mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.Model.rstis excluded by!**/_generated/**docs/api/_generated/newton.ModelBuilder.rstis excluded by!**/_generated/**docs/api/_generated/newton.ParticleFlags.rstis excluded by!**/_generated/**docs/api/_generated/newton.SDF.rstis excluded by!**/_generated/**docs/api/_generated/newton.ShapeFlags.rstis excluded by!**/_generated/**docs/api/_generated/newton.State.rstis excluded by!**/_generated/**docs/api/_generated/newton.Style3DModel.rstis excluded by!**/_generated/**docs/api/_generated/newton.Style3DModelBuilder.rstis excluded by!**/_generated/**docs/api/_generated/newton.count_rigid_contact_points.rstis excluded by!**/_generated/**docs/api/_generated/newton.eval_fk.rstis excluded by!**/_generated/**docs/api/_generated/newton.eval_ik.rstis excluded by!**/_generated/**docs/api/_generated/newton.geometry.BroadPhaseAllPairs.rstis excluded by!**/_generated/**docs/api/_generated/newton.geometry.BroadPhaseExplicit.rstis excluded by!**/_generated/**docs/api/_generated/newton.geometry.BroadPhaseSAP.rstis excluded by!**/_generated/**docs/api/_generated/newton.geometry.compute_shape_inertia.rstis excluded by!**/_generated/**docs/api/_generated/newton.geometry.remesh_mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.geometry.transform_inertia.rstis excluded by!**/_generated/**docs/api/_generated/newton.get_joint_dof_count.rstis excluded by!**/_generated/**docs/api/_generated/newton.ik.IKJacobianMode.rstis excluded by!**/_generated/**docs/api/_generated/newton.ik.IKJointLimitObjective.rstis excluded by!**/_generated/**docs/api/_generated/newton.ik.IKObjective.rstis excluded by!**/_generated/**docs/api/_generated/newton.ik.IKPositionObjective.rstis excluded by!**/_generated/**docs/api/_generated/newton.ik.IKRotationObjective.rstis excluded by!**/_generated/**docs/api/_generated/newton.ik.IKSolver.rstis excluded by!**/_generated/**docs/api/_generated/newton.selection.ArticulationView.rstis excluded by!**/_generated/**docs/api/_generated/newton.sensors.ContactSensor.rstis excluded by!**/_generated/**docs/api/_generated/newton.sensors.populate_contacts.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverBase.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverFeatherstone.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverImplicitMPM.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverMuJoCo.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverNotifyFlags.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverSemiImplicit.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverStyle3D.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverVBD.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverXPBD.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.RecorderBasic.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.RecorderModelAndState.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.boltzmann.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.color_graph.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.create_box_mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.create_capsule_mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.create_cone_mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.create_cylinder_mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.create_plane_mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.create_sphere_mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.download_asset.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.leaky_max.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.leaky_min.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.plot_graph.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.quat_between_axes.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.quat_decompose.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.quat_from_euler.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.quat_to_euler.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.quat_to_rpy.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.quat_twist.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.quat_twist_angle.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.smooth_max.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.smooth_min.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.transform_twist.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.transform_wrench.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.vec_abs.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.vec_leaky_max.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.vec_leaky_min.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.vec_max.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.vec_min.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.velocity_at_point.rstis excluded by!**/_generated/**docs/api/_generated/newton.viewer.RecorderImGuiManager.rstis excluded by!**/_generated/**docs/api/_generated/newton.viewer.ViewerGL.rstis excluded by!**/_generated/**docs/api/_generated/newton.viewer.ViewerNull.rstis excluded by!**/_generated/**docs/api/_generated/newton.viewer.ViewerRerun.rstis excluded by!**/_generated/**docs/api/_generated/newton.viewer.ViewerUSD.rstis excluded by!**/_generated/**
📒 Files selected for processing (6)
docs/api/newton_geometry.rst(1 hunks)docs/api/newton_sensors.rst(1 hunks)docs/api/newton_utils.rst(1 hunks)docs/api/newton_viewer.rst(1 hunks)newton/sensors.py(2 hunks)newton/tests/test_mujoco_solver.py(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/api/newton_sensors.rst
- docs/api/newton_geometry.rst
🧰 Additional context used
🧬 Code graph analysis (2)
newton/sensors.py (1)
newton/_src/sensors/contact_sensor.py (1)
MatchKind(30-40)
newton/tests/test_mujoco_solver.py (3)
newton/_src/sim/builder.py (3)
ModelBuilder(67-4459)add_body(1179-1241)add_joint_revolute(1362-1444)newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(1143-2729)notify_model_changed(1393-1401)newton/_src/solvers/flags.py (1)
SolverNotifyFlags(22-46)
⏰ 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). (4)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (7)
newton/sensors.py (2)
19-19: Re-export MatchKind in public API.Change is correct and aligns with docs. No behavioral impact.
30-30: Update all with MatchKind.Good; keeps public surface consistent with the import above.
newton/tests/test_mujoco_solver.py (3)
16-16: Add time import for skipped debug test.Fine; harmless since the test is skipped.
1386-1502: Solid end-to-end test for joint frame updates.Covers child-frame updates, parent-frame updates, and body pose recomposition. Nice.
1408-1413: Don't assume the first joint is at [0,0] — derive the mjc joint index from a mapping.Test currently indexes joints with a hard-coded index; a repo search returned no exposed joint-index mapping. Verify whether the solver exposes a mapping (e.g., to_mjc_*_index or similar); if it does, use that mapping in the test. If not, add a stable mapping API or resolve the joint index by name before indexing.
docs/api/newton_viewer.rst (1)
13-13: Confirm ViewerFile exists and is exported from newton.viewerVerification script returned no results — confirm class newton.viewer.ViewerFile is defined in newton/viewer.py and that newton/viewer.py exports it (e.g., include 'ViewerFile' in all or otherwise make it importable for Sphinx autodoc). Suggested checks: rg -nP 'class\s+ViewerFile\b' newton -S; rg -nP "all\s*=.*\bViewerFile\b" newton -S || true
docs/api/newton_utils.rst (1)
40-40: Expose run_benchmark in docs — confirmedDefined in newton/_src/utils/benchmark.py (has a docstring) and re-exported as newton.utils.run_benchmark in newton/utils.py via
from ._src.utils.benchmark import run_benchmark(also added to all). Sphinx can import newton.utils.run_benchmark — approving the docs change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
newton/sensors.py (1)
28-33: all update: LGTM; tiny ordering nit.Consider grouping classes first then functions (or alphabetical) for readability.
docs/api/newton_sensors.rst (1)
13-14: Docs entries added: LGTM.Optional: place MatchKind under an “Enums” rubric or add a one‑liner in its docstring so autosummary shows context.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (82)
docs/api/_generated/newton.Axis.rstis excluded by!**/_generated/**docs/api/_generated/newton.AxisType.rstis excluded by!**/_generated/**docs/api/_generated/newton.CollisionPipeline.rstis excluded by!**/_generated/**docs/api/_generated/newton.Contacts.rstis excluded by!**/_generated/**docs/api/_generated/newton.Control.rstis excluded by!**/_generated/**docs/api/_generated/newton.EqType.rstis excluded by!**/_generated/**docs/api/_generated/newton.GeoType.rstis excluded by!**/_generated/**docs/api/_generated/newton.JointMode.rstis excluded by!**/_generated/**docs/api/_generated/newton.JointType.rstis excluded by!**/_generated/**docs/api/_generated/newton.Mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.Model.rstis excluded by!**/_generated/**docs/api/_generated/newton.ModelBuilder.rstis excluded by!**/_generated/**docs/api/_generated/newton.ParticleFlags.rstis excluded by!**/_generated/**docs/api/_generated/newton.SDF.rstis excluded by!**/_generated/**docs/api/_generated/newton.ShapeFlags.rstis excluded by!**/_generated/**docs/api/_generated/newton.State.rstis excluded by!**/_generated/**docs/api/_generated/newton.Style3DModel.rstis excluded by!**/_generated/**docs/api/_generated/newton.Style3DModelBuilder.rstis excluded by!**/_generated/**docs/api/_generated/newton.count_rigid_contact_points.rstis excluded by!**/_generated/**docs/api/_generated/newton.eval_fk.rstis excluded by!**/_generated/**docs/api/_generated/newton.eval_ik.rstis excluded by!**/_generated/**docs/api/_generated/newton.geometry.BroadPhaseAllPairs.rstis excluded by!**/_generated/**docs/api/_generated/newton.geometry.BroadPhaseExplicit.rstis excluded by!**/_generated/**docs/api/_generated/newton.geometry.BroadPhaseSAP.rstis excluded by!**/_generated/**docs/api/_generated/newton.geometry.compute_shape_inertia.rstis excluded by!**/_generated/**docs/api/_generated/newton.geometry.remesh_mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.geometry.transform_inertia.rstis excluded by!**/_generated/**docs/api/_generated/newton.get_joint_dof_count.rstis excluded by!**/_generated/**docs/api/_generated/newton.ik.IKJacobianMode.rstis excluded by!**/_generated/**docs/api/_generated/newton.ik.IKJointLimitObjective.rstis excluded by!**/_generated/**docs/api/_generated/newton.ik.IKObjective.rstis excluded by!**/_generated/**docs/api/_generated/newton.ik.IKPositionObjective.rstis excluded by!**/_generated/**docs/api/_generated/newton.ik.IKRotationObjective.rstis excluded by!**/_generated/**docs/api/_generated/newton.ik.IKSolver.rstis excluded by!**/_generated/**docs/api/_generated/newton.selection.ArticulationView.rstis excluded by!**/_generated/**docs/api/_generated/newton.sensors.ContactSensor.rstis excluded by!**/_generated/**docs/api/_generated/newton.sensors.populate_contacts.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverBase.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverFeatherstone.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverImplicitMPM.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverMuJoCo.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverNotifyFlags.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverSemiImplicit.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverStyle3D.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverVBD.rstis excluded by!**/_generated/**docs/api/_generated/newton.solvers.SolverXPBD.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.RecorderBasic.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.RecorderModelAndState.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.boltzmann.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.color_graph.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.create_box_mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.create_capsule_mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.create_cone_mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.create_cylinder_mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.create_plane_mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.create_sphere_mesh.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.download_asset.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.leaky_max.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.leaky_min.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.plot_graph.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.quat_between_axes.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.quat_decompose.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.quat_from_euler.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.quat_to_euler.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.quat_to_rpy.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.quat_twist.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.quat_twist_angle.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.smooth_max.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.smooth_min.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.transform_twist.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.transform_wrench.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.vec_abs.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.vec_leaky_max.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.vec_leaky_min.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.vec_max.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.vec_min.rstis excluded by!**/_generated/**docs/api/_generated/newton.utils.velocity_at_point.rstis excluded by!**/_generated/**docs/api/_generated/newton.viewer.RecorderImGuiManager.rstis excluded by!**/_generated/**docs/api/_generated/newton.viewer.ViewerGL.rstis excluded by!**/_generated/**docs/api/_generated/newton.viewer.ViewerNull.rstis excluded by!**/_generated/**docs/api/_generated/newton.viewer.ViewerRerun.rstis excluded by!**/_generated/**docs/api/_generated/newton.viewer.ViewerUSD.rstis excluded by!**/_generated/**
📒 Files selected for processing (6)
docs/api/newton_geometry.rst(1 hunks)docs/api/newton_sensors.rst(1 hunks)docs/api/newton_utils.rst(1 hunks)docs/api/newton_viewer.rst(1 hunks)newton/sensors.py(2 hunks)newton/tests/test_mujoco_solver.py(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/api/newton_geometry.rst
- docs/api/newton_utils.rst
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/tests/test_mujoco_solver.py
🧰 Additional context used
🧬 Code graph analysis (1)
newton/sensors.py (1)
newton/_src/sensors/contact_sensor.py (1)
MatchKind(30-40)
⏰ 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 Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (1)
newton/sensors.py (1)
17-21: Expose MatchKind publicly: LGTM.Re-exporting MatchKind from contact_sensor aligns API and docs; no issues spotted.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
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_usd.py (1)
1279-1300: Fix mat33 construction for inverse inertia
wp.mat33(*inertia)passes three row arrays, which is likely invalid. Use the same constructor form as forbody_inertia.Apply:
- if inertia.any(): - builder.body_inv_inertia[body_id] = wp.inverse(wp.mat33(*inertia)) + if inertia.any(): + builder.body_inv_inertia[body_id] = wp.inverse(wp.mat33(inertia))
♻️ Duplicate comments (1)
newton/_src/utils/import_usd.py (1)
988-1020: Write free-joint state to the correct joint and typeYou assign a wp.transform directly into
builder.joint_q[-7:]. This targets the tail globally (not necessarily the new joint) and writes the wrong type.Fix by capturing the returned id and slicing with
joint_q_start; convert to 7 floats:- builder.add_joint_free(child=child_body_id) - builder.joint_q[-7:] = articulation_xform + jid = builder.add_joint_free(child=child_body_id) + q_start = builder.joint_q_start[jid] + # set [px,py,pz,qx,qy,qz,qw] + builder.joint_q[q_start : q_start + 7] = list(articulation_xform)If the intent is to preserve the child body’s pose and apply an extra parent transform, consider composing:
list(wp.mul(articulation_xform, builder.body_q[child_body_id])).
🧹 Nitpick comments (9)
newton/tests/test_selection.py (2)
29-41: Good coverage of empty‑DOF selection; add one assert to lock in root transform semantics (optional).Since excluding the FREE joint relies on fallback root transform logic, consider asserting identity transform to catch regressions.
Apply this diff (adds a lightweight check):
@@ - selection = ArticulationView(model, pattern="my_articulation", exclude_joint_types=[newton.JointType.FREE]) + selection = ArticulationView(model, pattern="my_articulation", exclude_joint_types=[newton.JointType.FREE]) self.assertEqual(selection.count, 1) self.assertEqual(selection.get_root_transforms(model).shape, (1,)) self.assertEqual(selection.get_dof_positions(model).shape, (1, 0)) self.assertEqual(selection.get_dof_velocities(model).shape, (1, 0)) self.assertEqual(selection.get_dof_forces(control).shape, (1, 0)) + # Optional: root should be identity given default builder state + t = selection.get_root_transforms(model)[0] + self.assertEqual(tuple(round(c, 6) for c in t.p), (0.0, 0.0, 0.0)) + self.assertEqual(tuple(round(c, 6) for c in t.q), (0.0, 0.0, 0.0, 1.0))If desired, I can also add a setter no‑op check on zero‑DOF selections.
29-41: Consider adding a companion test for empty link selections.This complements the empty‑DOF case and guards body‑frequency paths.
Apply this diff to add a small test:
@@ def test_empty_selection(self): ... self.assertEqual(selection.get_dof_forces(control).shape, (1, 0)) + + def test_empty_links_selection(self): + builder = newton.ModelBuilder() + builder.add_articulation("my_articulation") + body = builder.add_body() + builder.add_joint_free(child=body) + model = builder.finalize() + # exclude the only link by index; DOFs remain, links become empty + selection = ArticulationView(model, pattern="my_articulation", exclude_links=[body]) + self.assertEqual(selection.count, 1) + self.assertEqual(selection.get_link_transforms(model).shape, (1, 0))newton/tests/assets/four_link_chain_articulation.usda (1)
76-91: Consider avoidinginfliterals in authored USDSome USD stacks don’t round-trip
infreliably. Prefer omitting the attribute (unbounded) or a large finite default (and enforce in the importer).Option:
- float drive:angular:physics:maxForce = inf + # omit to denote unbounded, or pick a large finite sentinel in tests + # float drive:angular:physics:maxForce = 1e30newton/tests/test_import_usd.py (1)
252-267: Stabilize assertions on autogenerated free-joint keysRelying on exact names like "joint_1" can be brittle if Builder default naming changes. Prefer pattern checks (e.g., startswith("joint_")) or asserting non-USD-path keys.
Suggested tweak:
- expected_joint_keys=["joint_1", "joint_2", "joint_3", "joint_4"], + expected_joint_keys=None, # don't assert exact namesAnd inside test_filtering:
- self.assertEqual(builder.joint_key, expected_joint_keys, ...) + if expected_joint_keys is not None: + self.assertEqual(builder.joint_key, expected_joint_keys, ...) + else: + self.assertTrue(all(not k.startswith("/") for k in builder.joint_key), msg)newton/_src/utils/import_usd.py (5)
276-278: Make excludePaths detection robust (treat only absolute USD paths as literals)Using
" .* " not in pathis fragile. Limit excludePaths to absolute USD paths (literal) and keep everything else for regex matching at runtime.Apply:
- non_regex_ignore_paths = [path for path in ignore_paths if ".*" not in path] - ret_dict = UsdPhysics.LoadUsdPhysicsFromRange(stage, [root_path], excludePaths=non_regex_ignore_paths) + literal_ignores = [p for p in ignore_paths if isinstance(p, str) and p.startswith("/")] + ret_dict = UsdPhysics.LoadUsdPhysicsFromRange(stage, [root_path], excludePaths=literal_ignores)
785-789: Precompile ignore regex for repeated useYou call
re.matchin multiple hot paths. Precompile once to cut overhead.Add near where
ignore_pathsis finalized:+ ignore_patterns = [re.compile(p) for p in ignore_paths]Then replace
any(re.match(p, path) for p in ignore_paths)withany(rx.match(path) for rx in ignore_patterns).
939-953: Disconnected graph risk after joint filteringComment notes the risk; currently we silently proceed. Consider detecting disconnected subgraphs and either splitting into multiple articulations or raising with a clear error.
Minimal guard:
+ # after filtering, ensure graph connectivity over remaining bodies + # fallback: continue and let downstream tests catch, or raise if strict
1048-1058: Track non-articulated bodies in filters mapBodies added outside Articulation handling don’t enter
articulation_bodiesorarticulation_has_self_collision. To keep downstream logic uniform, add them with a default self-collision policy.Example:
- builder.add_articulation(key) + builder.add_articulation(key) + articulation_bodies[builder.articulation_count - 1] = [body_id] + articulation_has_self_collision[builder.articulation_count - 1] = enable_self_collisions
850-854: Units: density conversion comment indicates missing MPU scalingYou multiply density by KGPU but leave MPU^3 commented out. If you intend SI density in engine space, consider applying
/(linear_unit**3)or clarify expectations.Option:
- density = d * mass_unit # / (linear_unit**3) + density = d * mass_unit / (linear_unit**3)Validate against existing assets before changing behavior.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/utils/import_usd.py(13 hunks)newton/tests/assets/four_link_chain_articulation.usda(1 hunks)newton/tests/test_import_usd.py(2 hunks)newton/tests/test_selection.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
PR: newton-physics/newton#519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
PR: newton-physics/newton#519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
PR: newton-physics/newton#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_usd.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
PR: newton-physics/newton#587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/utils/import_usd.py
🧬 Code graph analysis (3)
newton/tests/test_selection.py (3)
newton/_src/utils/selection.py (5)
ArticulationView(125-830)get_root_transforms(627-647)get_dof_positions(727-737)get_dof_velocities(750-760)get_dof_forces(773-783)newton/_src/sim/builder.py (2)
add_articulation(642-648)add_joint_free(1605-1656)newton/_src/sim/joints.py (1)
JointType(20-44)
newton/tests/test_import_usd.py (1)
newton/_src/sim/builder.py (4)
ModelBuilder(67-4459)add_usd(710-808)joint_count(522-526)articulation_count(571-575)
newton/_src/utils/import_usd.py (2)
newton/_src/sim/builder.py (3)
add_articulation(642-648)add_body(1179-1241)add_joint_free(1605-1656)newton/_src/utils/import_mjcf.py (1)
parse_body(447-775)
🪛 Ruff (0.12.2)
newton/_src/utils/import_usd.py
941-941: Shebang should contain python, pytest, or uv run
(EXE003)
941-941: Shebang is present but file is not executable
(EXE001)
941-941: Shebang should be at the beginning of the file
(EXE005)
942-942: Shebang should contain python, pytest, or uv run
(EXE003)
942-942: Shebang is present but file is not executable
(EXE001)
942-942: Shebang should be at the beginning of the file
(EXE005)
943-943: Shebang should contain python, pytest, or uv run
(EXE003)
943-943: Shebang is present but file is not executable
(EXE001)
943-943: Shebang should be at the beginning of the file
(EXE005)
1016-1016: Shebang should contain python, pytest, or uv run
(EXE003)
1016-1016: Shebang is present but file is not executable
(EXE001)
1016-1016: Shebang should be at the beginning of the file
(EXE005)
⏰ 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). (4)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (15)
newton/tests/test_selection.py (4)
1-15: License header looks good.SPDX identifiers and Apache-2.0 header are correct.
16-20: Imports are minimal and appropriate.Directly importing
ArticulationViewkeeps the tests focused.
44-45: Main guard is fine.Harmless for unittest discovery and allows direct invocation.
23-27: Assert the exception message and use a context manager.Verified ArticulationView raises KeyError("No matching articulations"); pin the message with assertRaisesRegex.
- self.assertRaises(KeyError, ArticulationView, model, pattern="no_match") + with self.assertRaisesRegex(KeyError, r"No matching articulations"): + ArticulationView(model, pattern="no_match")newton/tests/assets/four_link_chain_articulation.usda (1)
47-54: LGTM: root fixed joint authored via body0-onlyAuthoring a FixedJoint with only body0 is consistent with importer behavior (child to world). Good fixture for filtering tests.
newton/tests/test_import_usd.py (2)
183-225: Nice, comprehensive filtering helperGood coverage across articulation count, types, body and joint keys for both ordering modes.
536-550: Good additions validating single-body articulation plumbingAsserting body/shape keys, FREE joint parent/child, and articulation key ensures importer semantics for free bodies remain correct.
newton/_src/utils/import_usd.py (8)
315-317: LGTM: skip visual shapes by ignore regexEarly-return here prevents unnecessary traversal and keeps maps clean.
835-838: LGTM: cache ignored body pathsAvoids repeated regex evaluations and keeps body scanning fast.
956-975: LGTM: single-body articulations handlingCreating one articulation per body and applying the articulation transform to body_q is clear and matches tests.
1039-1044: LGTM: per‑articulation self‑collision flagReading
physxArticulation:enabledSelfCollisionsfrom the articulation prim matches the PR goal.
1267-1271: LGTM: itertools.combinations for self‑collision filteringCleaner and reduces accidental duplicates versus nested loops.
215-223: Map USD axes onceThe
usd_axis_to_axismapping is correct and keeps joint axis translation explicit.
571-581: Drive gains scaling: consistent unit handlingConverting angular terms by DegreesToRadian and scaling gains meets the solver expectations.
988-1020: Action: address remaining tail-slice assignment to joint_q.Found one exact match: newton/_src/utils/import_usd.py:1019 —
builder.joint_q[-7:] = articulation_xform. No other occurrences ofjoint_q[-7:] =were found; other slice assignments (e.g.[:3],[3:7],[7:],[-3:], etc.) exist — confirm those are intentional or update to explicit indexed assignments.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
newton/_src/utils/import_usd.py (2)
436-445: Use int32 for mesh face indices to avoid precision loss
GetFaceVertexIndicesAttr()returns integral indices. Casting to float32 risks rounding for large meshes before converting back to int later. Read as int32 upfront.- points = np.array(mesh.GetPointsAttr().Get(), dtype=np.float32) - indices = np.array(mesh.GetFaceVertexIndicesAttr().Get(), dtype=np.float32) + points = np.array(mesh.GetPointsAttr().Get(), dtype=np.float32) + indices = np.array(mesh.GetFaceVertexIndicesAttr().Get(), dtype=np.int32)
1182-1185: Same here: face indices should be int32Mirror the fix in the shape parsing path.
- points = np.array(mesh.GetPointsAttr().Get(), dtype=np.float32) - indices = np.array(mesh.GetFaceVertexIndicesAttr().Get(), dtype=np.float32) + points = np.array(mesh.GetPointsAttr().Get(), dtype=np.float32) + indices = np.array(mesh.GetFaceVertexIndicesAttr().Get(), dtype=np.int32)
♻️ Duplicate comments (1)
newton/_src/utils/import_usd.py (1)
1010-1020: Bug: writing articulation transform into the wrong joint slice and wrong typeYou add a free joint but then assign a wp.transform to
builder.joint_q[-7:], which targets the tail of the global array (not this joint) and has the wrong type. Capture the returned joint_id and write 7 floats into that joint’s slice.- builder.add_joint_free(child=child_body_id) - builder.joint_q[-7:] = articulation_xform + joint_id = builder.add_joint_free(child=child_body_id) + q_start = builder.joint_q_start[joint_id] + # set [px,py,pz,qx,qy,qz,qw] from articulation_xform + builder.joint_q[q_start : q_start + 7] = list(articulation_xform)
🧹 Nitpick comments (5)
newton/_src/utils/import_usd.py (3)
276-278: Heuristic for “non‑regex” ignore paths is too weakChecking for ".*" isn’t sufficient. Treat entries with no regex metacharacters as exact paths; others remain regex. This avoids misclassification.
- non_regex_ignore_paths = [path for path in ignore_paths if ".*" not in path] + _regex_meta = re.compile(r"[.^$*+?{}\[\]|()]") + non_regex_ignore_paths = [p for p in ignore_paths if not _regex_meta.search(p)]
315-317: Pre‑compile ignore regex for repeated useYou match these patterns in multiple hot paths. Compile once to cut repeated compilations and speed up large USDs.
941-945: Guard against disconnected articulation graphs after filteringFiltering joints/bodies by ignore_paths can break chains. Detect and raise a clear error (or skip the articulation) when the joint graph becomes disconnected.
newton/_src/solvers/mujoco/solver_mujoco.py (2)
2692-2693: Remove unused noqa directives
PLC0415isn’t enabled here; the noqa is flagged. Importing inside the method is fine; just drop the comments.- import mujoco # noqa: PLC0415 - import mujoco.viewer # noqa: PLC0415 + import mujoco + import mujoco.viewer
2720-2725: Don’t swallow all exceptions on viewer closeNarrow the exception or at least log a warning; silent failures complicate debugging.
- try: - self._viewer.__exit__(None, None, None) - except Exception: - pass # Ignore errors during cleanup + try: + self._viewer.__exit__(None, None, None) + except Exception as e: + warnings.warn(f"Failed to close MuJoCo viewer cleanly: {e}", stacklevel=2) finally: self._viewer = None
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/_src/solvers/mujoco/solver_mujoco.py(19 hunks)newton/_src/utils/import_usd.py(13 hunks)newton/tests/test_examples.py(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-27T19:05:44.697Z
Learnt from: Milad-Rakhsha-NV
PR: newton-physics/newton#535
File: newton/tests/test_examples.py:320-414
Timestamp: 2025-08-27T19:05:44.697Z
Learning: In newton/examples/__init__.py, the robot policy example is registered with the key "robot_policy" (not "robot.example_robot_policy"), so tests should reference it as name="robot_policy".
Applied to files:
newton/tests/test_examples.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
PR: newton-physics/newton#579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, Example.use_mujoco is a high-level attribute that controls whether to use MuJoCo solver vs other solvers (like XPBD), while SolverMuJoCo.use_mujoco_cpu controls the backend within MuJoCo (CPU vs Warp). These are separate concepts serving different purposes - the PR rename only applies to the solver parameter, not the Example class attributes.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
PR: newton-physics/newton#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/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
PR: newton-physics/newton#519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
PR: newton-physics/newton#519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
PR: newton-physics/newton#587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/utils/import_usd.py
🧬 Code graph analysis (2)
newton/_src/solvers/mujoco/solver_mujoco.py (3)
newton/_src/sim/joints.py (1)
JointType(20-44)newton/_src/sim/builder.py (3)
flags(176-182)flags(185-190)joint_count(522-526)newton/_src/solvers/flags.py (1)
SolverNotifyFlags(22-46)
newton/_src/utils/import_usd.py (2)
newton/_src/sim/builder.py (3)
add_articulation(642-648)add_body(1179-1241)add_joint_free(1605-1656)newton/_src/utils/import_mjcf.py (1)
parse_body(447-775)
🪛 Ruff (0.12.2)
newton/_src/solvers/mujoco/solver_mujoco.py
2692-2692: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2693-2693: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2722-2723: try-except-pass detected, consider logging the exception
(S110)
2722-2722: Do not catch blind exception: Exception
(BLE001)
newton/_src/utils/import_usd.py
941-941: Shebang should contain python, pytest, or uv run
(EXE003)
941-941: Shebang is present but file is not executable
(EXE001)
941-941: Shebang should be at the beginning of the file
(EXE005)
942-942: Shebang should contain python, pytest, or uv run
(EXE003)
942-942: Shebang is present but file is not executable
(EXE001)
942-942: Shebang should be at the beginning of the file
(EXE005)
943-943: Shebang should contain python, pytest, or uv run
(EXE003)
943-943: Shebang is present but file is not executable
(EXE001)
943-943: Shebang should be at the beginning of the file
(EXE005)
1016-1016: Shebang should contain python, pytest, or uv run
(EXE003)
1016-1016: Shebang is present but file is not executable
(EXE001)
1016-1016: Shebang should be at the beginning of the file
(EXE005)
🔇 Additional comments (5)
newton/tests/test_examples.py (2)
332-335: UR10 test: USD gating + CPU override look good; confirm CUDA runtime.500 frames with
use_viewer=Truecan be slow on some CI GPUs. If runtime becomes an issue, consider addingtest_options_cuda={"num_frames": 200}(or similar). Otherwise, please confirm current CI timing is acceptable.
336-343: Viewer enabled in test but examples don't accept--viewer— fix required.newton/tests/test_examples.py (lines 336–343) calls add_example_test(... use_viewer=True ...).
newton/examples/robot/example_robot_allegro_hand.py and newton/examples/robot/example_robot_ur10.py were found but do not define a--viewerCLI arg.
Add--viewerparsing to the example scripts or remove/disableuse_viewer=Truein the test to avoid failures.⛔ Skipped due to learnings
Learnt from: dylanturpin PR: newton-physics/newton#634 File: newton/tests/test_examples.py:329-333 Timestamp: 2025-08-25T21:41:45.795Z Learning: Newton examples use a centralized CLI argument parsing system in newton/examples/__init__.py. The create_parser() function defines common arguments like --device, --viewer, --output-path, and --num-frames, while init(parser) creates viewers based on parsed arguments. Individual example scripts don't need to define these flags themselves - they inherit them from the centralized system via the example_map and runpy execution.newton/_src/utils/import_usd.py (2)
1267-1271: Good: efficient per‑articulation self‑collision filteringUsing
itertools.combinationsto filter pairs across bodies is clean and efficient.
849-854: Unit scaling of density: confirm expected conventionsYou set
density = d * mass_unitwithout accounting for stage length units. Ifphysics:densityis in kg/m^3 regardless of MPU, it’s fine; if authored per stage units, it should scale with1/linear_unit**3. Please confirm with USD conventions or your authoring pipeline.newton/_src/solvers/mujoco/solver_mujoco.py (1)
1396-1400: Correct separation of JOINT_PROPERTIES vs JOINT_DOF_PROPERTIESUsing distinct update paths for transforms vs DOF properties matches the new flags API and reduces unnecessary work.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
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/solvers/mujoco/solver_mujoco.py (1)
2085-2091: Use global joint index for all per‑joint arrays; compute joint→child rotation explicitly.Within the same loop you index arrays (X_p, X_c, qd_start, key, type) by
ji. These are global arrays; useselected_joints[ji]. Also rotate axes with joint→child, not child→joint.- tf = wp.transform(*joint_parent_xform[ji]) - tf = tf * wp.transform_inverse(wp.transform(*joint_child_xform[ji])) - joint_pos = wp.vec3(*joint_child_xform[ji, :3]) - joint_rot = wp.quat(*joint_child_xform[ji, 3:]) + g = selected_joints[ji] + tf = wp.transform(*joint_parent_xform[g]) * wp.transform_inverse(wp.transform(*joint_child_xform[g])) + joint_pos = wp.vec3(*joint_child_xform[g, :3]) + # joint_rot_child = child->joint; need joint->child + joint_rot = wp.quat_inverse(wp.quat(*joint_child_xform[g, 3:])) @@ - j_type = joint_type[ji] - qd_start = joint_qd_start[ji] - name = model.joint_key[ji] + j_type = joint_type[g] + qd_start = joint_qd_start[g] + name = model.joint_key[g] @@ - axis = wp.quat_rotate(joint_rot, wp.vec3(*joint_axis[ai])) + axis = wp.quat_rotate(joint_rot, wp.vec3(*joint_axis[ai])) @@ - axis = wp.quat_rotate(joint_rot, wp.vec3(*joint_axis[ai])) + axis = wp.quat_rotate(joint_rot, wp.vec3(*joint_axis[ai]))Note: After defining
joint_rot = joint→child, using forwardquat_rotatematches the established convention.Also applies to: 2114-2117, 2136-2169, 2202-2266
♻️ Duplicate comments (2)
newton/tests/assets/four_link_chain_articulation.usda (1)
52-53: LGTM: unit quaternions.
localRot0/1are normalized now; previous non-unit issue is resolved.newton/_src/solvers/mujoco/solver_mujoco.py (1)
2072-2075: Populatejoint_mjc_dof_startby global joint IDs (current code writes using local ordering).
jiindexes intojoint_order/selected_joints, not the global Newton joint id. This misaligns DOF starts and breaks per‑env mapping.- joint_mjc_dof_start = np.full(model.joint_count, -1, dtype=np.int32) + joint_mjc_dof_start = np.full(model.joint_count, -1, dtype=np.int32) @@ - joint_mjc_dof_start[ji] = len(spec.joints) + global_j = selected_joints[ji] + joint_mjc_dof_start[global_j] = len(spec.joints)Also applies to: 2124-2124
🧹 Nitpick comments (4)
newton/tests/assets/four_link_chain_articulation.usda (1)
47-54: Specify the second body for the fixed joint (to avoid importer ambiguity).
PhysicsFixedJoint "Joint0"binds onlyphysics:body0. Many USD pipelines expect both bodies. Addphysics:body1 = </Articulation>(or the appropriate parent) to make the root anchoring explicit.def PhysicsFixedJoint "Joint0" { rel physics:body0 = </Articulation/Body0> + rel physics:body1 = </Articulation> point3f physics:localPos0 = (0, 0, 0) point3f physics:localPos1 = (0, 0, 0) quatf physics:localRot0 = (1, 0, 0, 0) quatf physics:localRot1 = (1, 0, 0, 0) }newton/_src/solvers/mujoco/solver_mujoco.py (3)
1991-2039: Make debug plane RGBA opt‑in.Hard‑coding ground plane color affects all consumers loading the compiled MJCF. Gate behind a debug flag or viewer path.
- # make ground plane blue in the MuJoCo viewer (only used for debugging) - geom_params["rgba"] = [0.0, 0.3, 0.6, 1.0] + if os.environ.get("NEWTON_DEBUG_MJ_PLANE_RGBA") == "1": + geom_params["rgba"] = [0.0, 0.3, 0.6, 1.0]
2696-2698: Remove unusednoqadirectives.Ruff flags
# noqa: PLC0415as unused; imports inside function are acceptable without silencing.- import mujoco # noqa: PLC0415 - import mujoco.viewer # noqa: PLC0415 + import mujoco + import mujoco.viewer
2722-2730: Don’t swallow all exceptions on viewer close; log at debug level.Catching bare
Exceptionandpasshides real issues. Narrow or log.- if hasattr(self, "_viewer") and self._viewer is not None: - try: - self._viewer.__exit__(None, None, None) - except Exception: - pass # Ignore errors during cleanup - finally: - self._viewer = None + if hasattr(self, "_viewer") and self._viewer is not None: + try: + self._viewer.__exit__(None, None, None) + except RuntimeError as e: + warnings.warn(f"MuJoCo viewer close failed: {e}", stacklevel=2) + finally: + self._viewer = None
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/_src/solvers/mujoco/solver_mujoco.py(19 hunks)newton/tests/assets/four_link_chain_articulation.usda(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
PR: newton-physics/newton#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/tests/assets/four_link_chain_articulation.usdanewton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
PR: newton-physics/newton#579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, Example.use_mujoco is a high-level attribute that controls whether to use MuJoCo solver vs other solvers (like XPBD), while SolverMuJoCo.use_mujoco_cpu controls the backend within MuJoCo (CPU vs Warp). These are separate concepts serving different purposes - the PR rename only applies to the solver parameter, not the Example class attributes.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
🧬 Code graph analysis (1)
newton/_src/solvers/mujoco/solver_mujoco.py (3)
newton/_src/sim/joints.py (1)
JointType(20-44)newton/_src/sim/builder.py (3)
flags(176-182)flags(185-190)joint_count(522-526)newton/_src/solvers/flags.py (1)
SolverNotifyFlags(22-46)
🪛 Ruff (0.12.2)
newton/_src/solvers/mujoco/solver_mujoco.py
2696-2696: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2697-2697: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2726-2727: try-except-pass detected, consider logging the exception
(S110)
2726-2726: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (4)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (4)
newton/tests/assets/four_link_chain_articulation.usda (1)
76-91: Confirm USD ‘inf’ parsing across toolchains.
drive:angular:physics:maxForce = infand break limits useinf. Some parsers reject literalinf. If portability matters, consider large finite defaults or verify importer behavior.newton/_src/solvers/mujoco/solver_mujoco.py (3)
1264-1266: LGTM: first‑env sentinel mapping forjoint_mjc_dof_start.Doc clarifies first‑env semantics and default −1; aligns with kernel guard.
2360-2362: LGTM: propagatejoint_mjc_dof_startand trigger ALL updates post‑compile.This ensures kernels see the mapping and model fields are populated.
Also applies to: 2401-2419
2528-2530: Early returns for empty model parts are good.Avoids needless launches and edge‑case errors.
Also applies to: 2560-2563, 2638-2641
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Description
This PR introduces the ability for SolverMuJoCo to receive updated joint parent and child transforms which was necessary to handle fixed-base articulations with offset, such as the Allegro hand. Previously, if the hand had an offset (which Newton applies through the joint parent transform on the hand's fixed root joint), e.g. from
builder.replicate(), MjWarp wasn't aware of it so the cube fell through the hand for all other envs except the first one (which didn't have any offset).Changes
update_joint_propertiestoupdate_joint_dof_propertiessince it was only handlingSolverNotifyFlags.JOINT_DOF_PROPERTIESupdate_joint_properties()that updates the joint pos, axis and body pos, quat attributes in the MuJoCo Warp Model given the parent and child joint transforms from the Newton ModelSolverMuJoCo.render_mujoco_viewer()that allows users to run the MuJoCo viewer as a debugging toolignore_pathsregex handling in USD importer to properly filter out bodies, joints, shapes, articulationsArticulationViewwith an empty selection of joints/bodies/shapesNewton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
New Features
Improvements
Documentation
Tests