Fix locked D6 root base classification - #3792
Conversation
Classify articulation roots from their effective DOF count so fully locked D6 roots report fixed-base semantics. Add focused selection coverage.
📝 WalkthroughWalkthroughThe fix changes how ChangesFixed-base DOF check
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Test as test_root_base_classification_uses_dof_count
participant View as ArticulationView.__init__
participant Model as model_joint_qd_start
Test->>View: construct with root joint (fixed/d6/free)
View->>Model: read qd_start at root joint boundary
Model-->>View: dof start indices
View->>View: compute root_joint_dof_count
View->>View: set is_fixed_base = (root_joint_dof_count == 0)
View-->>Test: is_fixed_base, is_floating_base
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@newton/_src/utils/selection.py`:
- Around line 757-759: The root_joint_dof_count is currently extracted only from
the first articulation (arti_0), but when multiple articulations are selected,
each root joint may have a different effective DOF count despite matching total
articulation DOFs and joint types. Since is_fixed_base is shared across all
selected articulations, this can cause incorrect classification. Update the code
to extract the root_joint_dof_count for each selected articulation and add a
validation check that all roots have identical DOF counts, incorporating this
check into the existing "Articulations are not identical" validation logic
alongside the existing joint type and total DOF comparisons. Also add a
regression test case that covers a mixed-root layout scenario where roots have
different DOF distributions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6240cae1-06f9-4343-b4b2-c2abe1d9aefc
📒 Files selected for processing (3)
CHANGELOG.mdnewton/_src/utils/selection.pynewton/tests/test_selection.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Description
Fixes #3727.
ArticulationViewclassified fixed bases solely from a root joint'sJointType, so a fully locked, zero-DOF D6 root reported neither fixed nor floating. This caused downstream consumers to reserve six nonexistent base DOFs and select incorrect Jacobian columns.Classify a root as fixed when its effective DOF count is zero. Keep floating-base classification restricted to
FREEandDISTANCEroots, preserving their existing coordinate-layout assumptions.Known limitation
ArticulationViewalready assumes selected articulations share one template, but its existing validation can allow heterogeneous root-DOF layouts and then expose metadata from the first articulation. This predates this PR; #3792 only corrects zero-DOF root classification for supported identical views. The validation gap is tracked separately in #3793.Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Bug fix
Steps to reproduce:
ArticulationViewfor it.is_fixed_baseandis_floating_baseare bothFalsewithout this change.Minimal reproduction:
Summary by CodeRabbit
Bug Fixes
Tests