Use the same order for the GEO_ types as in MuJoCo#507
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new function to convert MuJoCo-compatible geometry type integers into legacy collision detection orderings. Geometry type constants are replaced by a new Changes
Sequence Diagram(s)sequenceDiagram
participant Kernel as Collision Kernel
participant Types as Geometry Types
participant Converter as geo_new_to_old_map
Kernel->>Types: Get geometry type constant
Kernel->>Converter: Convert new geometry type to old ordering
Converter-->>Kernel: Return legacy geometry type integer
Kernel->>Kernel: Compare legacy geometry type integers
Kernel-->>Kernel: Proceed with collision logic
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 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)
✨ 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 comments)
Other keywords and placeholders
Documentation and Community
|
Signed-off-by: twidmer <twidmer@nvidia.com>
7faf21b to
0d36f4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/geometry/kernels.py(3 hunks)newton/geometry/types.py(1 hunks)
⏰ 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 (3)
newton/geometry/kernels.py (3)
769-794: LGTM! Conversion function correctly maps geometry types.The
geo_new_to_old_pythonfunction provides a clean bridge between the new MuJoCo-compatible geometry type ordering and the legacy collision detection system. The mapping is comprehensive and handles unsupported types (HFIELD, ELLIPSOID) appropriately by treating them as equivalent to GEO_NONE.
822-822: LGTM! Conversion function usage maintains deterministic collision pair ordering.The use of
geo_new_to_old_pythonin the shape pair comparison ensures that collision pairs maintain consistent ordering based on the legacy collision detection scheme, which is essential for deterministic collision processing.
922-922: LGTM! Consistent conversion function usage across collision kernels.The use of
geo_new_to_old_pythoninbroadphase_collision_pairsis consistent with its usage incount_contact_points, ensuring uniform collision pair ordering logic across the collision detection system.
Signed-off-by: twidmer <twidmer@nvidia.com>
42cbc26 to
8d4f161
Compare
nvlukasz
left a comment
There was a problem hiding this comment.
I love how this simplifies the imports (just import the type instead of all the constants). Good change, just a couple of questions/suggestions about the new_to_old conversion.
Signed-off-by: twidmer <twidmer@nvidia.com>
|
@nvlukasz can it be that github uses an older warp version that does not yet support IntEnum? I cannot really explain the github CI failures otherwise. The tests pass on my local machine. |
The versions of all dependencies we use in CI/CD are always from the Ideally the pull request that needs a newer dependency will update both the For uv, you would run Then add and commit the modified |
Signed-off-by: twidmer <twidmer@nvidia.com>
Signed-off-by: twidmer <twidmer@nvidia.com>
Signed-off-by: twidmer <twidmer@nvidia.com>
Signed-off-by: twidmer <twidmer@nvidia.com>
1cde574 to
5500c81
Compare
Signed-off-by: twidmer <twidmer@nvidia.com>
…ormance Signed-off-by: twidmer <twidmer@nvidia.com>
|
@nvtw I'm curious if the regression in |
@shi-eric We re-ran it a few times with a few different variations, but the results seem consistent overall. The failure might be due to this branch being one commit behind main, and that commit increased the perf of the benchmark that's failing. I just merged main into this branch, so we shall see. |
|
I started getting this error in not sure if this is related to this PR or not @nvlukasz @nvtw ? |
Make sure your Warp is updated to at least |
I am on (1.9.0.dev20250808) now but I still see |
Not saying this is what you're doing, but I get the: error when I uninstall the locked version of |
If this is in Kit, make sure you're using Warp extensions with version If you don't see the Warp init message on startup, you can print |
|
no this happens directly in Newton when running examples; However, it seems that upgrading to warp-lang 1.9.0.dev20250812 fixes this |
Also use IntEnum instead of named constants Signed-off-by: twidmer <twidmer@nvidia.com>
Also use IntEnum instead of named constants Signed-off-by: twidmer <twidmer@nvidia.com>
Description
Use the same order for the GEO_ types as in MuJoCo. Add a conversion method such that type ordering remains unchanged - otherwise big if elif trees must be adjusted which is a big hassle.
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this MR.
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
Refactor