Rename custom warp sim USD attributes#589
Conversation
Signed-off-by: camevor <camevor@nvidia.com>
Signed-off-by: camevor <camevor@nvidia.com>
📝 WalkthroughWalkthroughMigrates USD physics attribute namespace from warp to newton across importer logic and USD assets. Updates read/write keys for scene, bodies, joints, articulations, and shape contact parameters. No public API signature changes; control flow remains the same aside from reading different attribute names. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Signed-off-by: camevor <camevor@nvidia.com>
eric-heiden
left a comment
There was a problem hiding this comment.
Thanks! @apradhana, @gregklar for viz.
Signed-off-by: camevor <camevor@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 5
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)
1011-1027: Shape contact params read only new names; add warp fallbacks (snake and camel) to avoid behavior changes.Otherwise legacy assets won’t carry over contact stiffness/damping/friction/adherence/thickness.
- shape_params = { - "body": body_id, - "xform": shape_xform, - "cfg": ModelBuilder.ShapeConfig( - ke=parse_float_with_fallback(prim_and_scene, "newton:contact_ke", builder.default_shape_cfg.ke), - kd=parse_float_with_fallback(prim_and_scene, "newton:contact_kd", builder.default_shape_cfg.kd), - kf=parse_float_with_fallback(prim_and_scene, "newton:contact_kf", builder.default_shape_cfg.kf), - ka=parse_float_with_fallback(prim_and_scene, "newton:contact_ka", builder.default_shape_cfg.ka), - thickness=parse_float_with_fallback( - prim_and_scene, "newton:contact_thickness", builder.default_shape_cfg.thickness - ), - mu=material.dynamicFriction, - restitution=material.restitution, - density=body_density.get(body_path, default_shape_density), - collision_group=collision_group, - ), - "key": path, - } + # Read newton:* first, then fall back to warp:* (snake and camel) + _ke = parse_float_with_fallback(prim_and_scene, "newton:contact_ke", None) + if _ke is None: + _ke = parse_float_with_fallback(prim_and_scene, "warp:contact_ke", None) + if _ke is None: + _ke = parse_float_with_fallback(prim_and_scene, "warp:contactKe", builder.default_shape_cfg.ke) + + _kd = parse_float_with_fallback(prim_and_scene, "newton:contact_kd", None) + if _kd is None: + _kd = parse_float_with_fallback(prim_and_scene, "warp:contact_kd", None) + if _kd is None: + _kd = parse_float_with_fallback(prim_and_scene, "warp:contactKd", builder.default_shape_cfg.kd) + + _kf = parse_float_with_fallback(prim_and_scene, "newton:contact_kf", None) + if _kf is None: + _kf = parse_float_with_fallback(prim_and_scene, "warp:contact_kf", None) + if _kf is None: + _kf = parse_float_with_fallback(prim_and_scene, "warp:contactKf", builder.default_shape_cfg.kf) + + _ka = parse_float_with_fallback(prim_and_scene, "newton:contact_ka", None) + if _ka is None: + _ka = parse_float_with_fallback(prim_and_scene, "warp:contact_ka", None) + if _ka is None: + _ka = parse_float_with_fallback(prim_and_scene, "warp:contactKa", builder.default_shape_cfg.ka) + + _th = parse_float_with_fallback(prim_and_scene, "newton:contact_thickness", None) + if _th is None: + _th = parse_float_with_fallback(prim_and_scene, "warp:contact_thickness", None) + if _th is None: + _th = parse_float_with_fallback(prim_and_scene, "warp:contactThickness", builder.default_shape_cfg.thickness) + + shape_params = { + "body": body_id, + "xform": shape_xform, + "cfg": ModelBuilder.ShapeConfig( + ke=_ke, + kd=_kd, + kf=_kf, + ka=_ka, + thickness=_th, + mu=material.dynamicFriction, + restitution=material.restitution, + density=body_density.get(body_path, default_shape_density), + collision_group=collision_group, + ), + "key": path, + }
1-1298: Remove legacywarpSim:armatureandphysics:newton:…attributes from USD assetsThe grep scan shows that test and example USD assets still contain stale names that are no longer read by
import_usd:
- newton/tests/assets/ant.usda
• Line 132:custom uint physics:newton:articulation_index = 0
• Lines 131, 519, 565, 610, 656, 701, 747, 792, 838:float warpSim:armature = 0.01- newton/examples/assets/ant.usda (same occurrences at the same line numbers)
Please remove all instances of:
warpSim:armaturephysics:newton:articulation_index- Any other legacy
warp:orphysics:warp:metadatafrom your USD asset files (e.g. under
tests/assetsandexamples/assets) to complete the migration and avoid confusing fallbacks inimport_usd.
🧹 Nitpick comments (5)
newton/_src/utils/import_usd.py (3)
67-67: Docstring grammar: “with as” → “as”.Tighten phrasing.
- joint_drive_gains_scaling (float): The default scaling of the PD control gains (stiffness and damping), if not set in the PhysicsScene with as "newton:joint_drive_gains_scaling". + joint_drive_gains_scaling (float): The default scaling of the PD control gains (stiffness and damping), if not set on the PhysicsScene as "newton:joint_drive_gains_scaling".
72-72: Docstring grammar: “with as” → “as”.Same issue as above.
- collapse_fixed_joints (bool): If True, fixed joints are removed and the respective bodies are merged. Only considered if not set on the PhysicsScene with as "newton:collapse_fixed_joints". + collapse_fixed_joints (bool): If True, fixed joints are removed and the respective bodies are merged. Only considered if not set on the PhysicsScene as "newton:collapse_fixed_joints".
109-114: Optional: centralize legacy→new name mapping to de-duplicate fallbacks.Current changes repeat string constants and fallback logic. A small utility (e.g., parse_float_any/prioritized list) would reduce errors and ease future migrations.
I can push a compact helper like:
def parse_float_any(prims, names: list[str], default: float | None) -> float | None: for n in names: v = parse_float_with_fallback(prims, n, None) if v is not None: return v return defaultThen use parse_float_any((prim, physics_scene_prim), ["newton:armature","warpSim:armature","warp:armature"], builder.default_body_armature), etc.
Also applies to: 128-141
newton/examples/assets/ant.usda (1)
131-131: Update armature attributes to “newton:armature” (or preserve importer fallbacks)The USD importer now reads body armature from
newton:armature; this file still defineswarpSim:armatureon multiple prims, which will be ignored unless fallback support remains in place. Please choose one of the following:
- Update all occurrences of
warpSim:armaturetonewton:armatureinnewton/examples/assets/ant.usda, or- Ensure the importer continues to fall back to
warpSim:armature(recommended short-term to avoid breakage).Locations in newton/examples/assets/ant.usda:
- Line 132:
float warpSim:armature = 0.01- Line 519:
float warpSim:armature = 0.01- Line 565:
float warpSim:armature = 0.01- Line 610:
float warpSim:armature = 0.01- Line 656:
float warpSim:armature = 0.01- Line 701:
float warpSim:armature = 0.01- Line 747:
float warpSim:armature = 0.01- Line 792:
float warpSim:armature = 0.01- Line 838:
float warpSim:armature = 0.01Example patch snippet:
- float warpSim:armature = 0.01 + float newton:armature = 0.01Let me know if you’d like me to generate a full patch.
newton/tests/assets/ant_multi.usda (1)
104-106: Confirm intent to leave physics:engine = "warp".Line 104 still sets physics:engine = "warp". If the engine token is expected to remain "warp" while only custom attribute namespaces move to physics:newton, this is fine—just confirming that this is intentional and documented in docs/migration.rst. If the long-term intent is to set engine = "newton", consider tracking that separately to avoid confusion.
📜 Review details
Configuration used: .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 (6)
newton/_src/utils/import_usd.py(7 hunks)newton/examples/assets/ant.usda(1 hunks)newton/tests/assets/ant.usda(1 hunks)newton/tests/assets/ant_multi.usda(64 hunks)newton/tests/assets/cube_cylinder.usda(1 hunks)newton/tests/assets/pendulum_revolute_vs_d6.usda(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: shi-eric
PR: newton-physics/newton#521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
PR: newton-physics/newton#519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
newton/tests/assets/cube_cylinder.usda
📚 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/utils/import_usd.py
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
PR: newton-physics/newton#521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
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 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
🧬 Code Graph Analysis (1)
newton/_src/utils/import_usd.py (1)
newton/_src/sim/builder.py (1)
collapse_fixed_joints(1687-1984)
⏰ 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 (5)
newton/_src/utils/import_usd.py (1)
853-856: Good: writes articulation index under physics:newton:articulation_index.The snake_case rename and namespace migration are correct, and the attribute is properly marked custom.
newton/tests/assets/cube_cylinder.usda (1)
50-50: LGTM: scene gate migrated to newton:collapse_fixed_joints.Matches importer reads added in import_usd.parse_usd.
newton/tests/assets/pendulum_revolute_vs_d6.usda (1)
23-23: LGTM: articulation index migrated to physics:newton:articulation_index (snake_case).Consistent with writer side in import_usd.py.
newton/tests/assets/ant_multi.usda (1)
105-105: LGTM: Renamed to physics:newton:articulation_index with correct typing and sequential values.The migration from physics:warp:articulationIndex → physics:newton:articulation_index is consistently applied across env_0..env_63. Type remains a custom uint and indices 0..63 match env suffixes. Looks good and aligns with the PR’s objective.
Also applies to: 313-313, 438-438, 563-563, 689-689, 814-814, 939-939, 1063-1063, 1188-1188, 1313-1313, 1439-1439, 1563-1563, 1688-1688, 1813-1813, 1938-1938, 2063-2063, 2189-2189, 2313-2313, 2439-2439, 2564-2564, 2689-2689, 2813-2813, 2939-2939, 3064-3064, 3189-3189, 3313-3313, 3439-3439, 3564-3564, 3689-3689, 3813-3813, 3938-3938, 4063-4063, 4189-4189, 4313-4313, 4439-4439, 4563-4563, 4689-4689, 4814-4814, 4939-4939, 5064-5064, 5189-5189, 5314-5314, 5439-5439, 5564-5564, 5689-5689, 5814-5814, 5939-5939, 6064-6064, 6189-6189, 6314-6314, 6439-6439, 6564-6564, 6689-6689, 6814-6814, 6939-6939, 7064-7064, 7189-7189, 7314-7314, 7439-7439, 7564-7564, 7689-7689, 7814-7814, 7939-7939, 8063-8063
newton/tests/assets/ant.usda (1)
131-131: Confirm newton attribute usage and fallback
- No occurrences of the deprecated
physics:warp:articulationIndexremain in the repo.- The importer only creates and reads the new
physics:newton:articulation_indexattribute—there is currently no fallback to the legacy key. If you need to maintain backward compatibility with older USD assets that still usephysics:warp:articulationIndex, please add a fallback code path or document that legacy support has been intentionally dropped.- “warp”-related tokens (e.g.
warpSceneAPI,physics:engine = "warp",warpSim:armature) remain in bothtests/assetsandexamples/assets. This appears intentional for testing/examples, but consider updating or documenting why these residual tokens persist (or removing the “-warp” suffix fromomni_layer.authoring_layer) to avoid confusion.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…physics#589) Signed-off-by: camevor <camevor@nvidia.com>
…physics#589) Signed-off-by: camevor <camevor@nvidia.com>
Description
Renames the custom USD attributes' prefix from
warptonewton, and uses snake case everywhere, e.g.physics:warp:articulationIndex→physics:newton:articulation_index.Newton Migration Guide
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
Refactor
Tests
Chores
Documentation