Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a per-articulation topological joint evaluation order to Model (computed in the builder) and threads it through FK/IK kernels (including batched paths) so joint-indexed accesses use the topological order instead of declaration order. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as Builder
participant Model as Model
participant Host as Host
participant FKKernel as FK Kernel
Builder->>Builder: build per-articulation edge lists\nrun topological_sort per articulation
Builder->>Model: set m.joint_eval_order (wp.array int32)
Host->>Model: read model.joint_eval_order
Host->>FKKernel: launch eval_articulation_fk(..., joint_eval_order)
FKKernel->>FKKernel: for i in 0..num_joints-1\n joint_idx = joint_eval_order[i]\n read joint props via joint_idx\n compute transforms/velocities
FKKernel->>Host: write body transforms/velocities
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
bobboli
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
bobboli
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
Add a per-articulation topological evaluation order so kinematic kernels no longer depend on storage order within an articulation. This keeps public joint indices stable while making eval_fk and the batched FK path robust to out-of-order joints. Add regressions covering FK equivalence and IK round-tripping for an articulation whose joints are stored out of parent-before-child order.
bobboli
force-pushed
the
bobboli/fix-joint-eval-order
branch
from
c768b2f to
50f943c
Compare
bobboli
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
bobboli
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/sim/articulation.py`:
- Around line 207-211: The lines that compute X_wpj at the top (X_wpj = X_pj; if
parent >= 0: X_wp = body_q[parent]; X_wpj = X_wp * X_wpj) are dead — X_wpj is
overwritten later and never used in the joint-type dispatch — so remove that
initial computation; delete the X_wpj initialization and the conditional block
referencing parent and body_q to avoid the duplicated/unused transform and rely
on the later correct computation (referencing X_wpj, X_pj, parent, body_q).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3d2d3cb7-cb63-4d83-8381-02c7f996159b
📒 Files selected for processing (5)
newton/_src/sim/articulation.pynewton/_src/sim/builder.pynewton/_src/sim/ik/ik_common.pynewton/_src/sim/model.pynewton/tests/test_model.py
Drop the duplicated X_wpj setup at the top of eval_single_articulation_fk. It is overwritten later before use, so removing it keeps the FK path clear and addresses the review comment without changing behavior.
bobboli
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
bobboli
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/sim/articulation.py (1)
202-255:⚠️ Potential issue | 🟡 MinorRename local
typeto avoid shadowing Python builtin.
typeshadows the builtintype()(Ruff A001). Rename tojoint_type_idand update all 6 comparisons within the method.♻️ Proposed fix
- type = joint_type[joint_index] + joint_type_id = joint_type[joint_index] @@ - if type == JointType.PRISMATIC: + if joint_type_id == JointType.PRISMATIC: @@ - if type == JointType.REVOLUTE: + if joint_type_id == JointType.REVOLUTE: @@ - if type == JointType.BALL: + if joint_type_id == JointType.BALL: @@ - if type == JointType.FREE or type == JointType.DISTANCE: + if joint_type_id == JointType.FREE or joint_type_id == JointType.DISTANCE: @@ - if type == JointType.D6: + if joint_type_id == JointType.D6:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/sim/articulation.py` around lines 202 - 255, The local variable named type shadows the Python builtin; rename it to joint_type_id (assigned from joint_type[joint_index]) and update every place that currently checks type == JointType.* (PRISMATIC, REVOLUTE, BALL, FREE, DISTANCE, D6) to use joint_type_id instead; ensure the assignment and all six comparisons in the function that reference joint_type[joint_index], X_j/v_j construction blocks, and any subsequent uses are updated to the new name (e.g., replace occurrences in the blocks handling JointType.PRISMATIC, REVOLUTE, BALL, FREE/DISTANCE, and D6).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@newton/_src/sim/articulation.py`:
- Around line 202-255: The local variable named type shadows the Python builtin;
rename it to joint_type_id (assigned from joint_type[joint_index]) and update
every place that currently checks type == JointType.* (PRISMATIC, REVOLUTE,
BALL, FREE, DISTANCE, D6) to use joint_type_id instead; ensure the assignment
and all six comparisons in the function that reference joint_type[joint_index],
X_j/v_j construction blocks, and any subsequent uses are updated to the new name
(e.g., replace occurrences in the blocks handling JointType.PRISMATIC, REVOLUTE,
BALL, FREE/DISTANCE, and D6).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9471269c-d9bd-496f-aa6e-781c2ed8b666
📒 Files selected for processing (1)
newton/_src/sim/articulation.py
Rename the local type variable in eval_single_articulation_fk to joint_type_id so it no longer shadows the Python builtin. This is a small readability cleanup from review and does not change behavior.
bobboli
requested a deployment
to
external-pr-approval
GitHub Actions
Waiting
bobboli
requested a deployment
to
external-pr-approval
GitHub Actions
Waiting
Member
|
Thanks @bobboli, this is a good contribution. We need to figure out how the individual solvers should treat the joint ordering, Featherstone and MuJoCo have their own kinematics functions that are not updated here. Also Kamino may need special treatment, so we will get back to this PR soon. |
Member
|
Backlog for now, @eric-heiden will follow up. Thanks for the contribution @bobboli! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix
eval_fkfor articulations whose joints are stored out of topological order.This PR introduces a per-articulation
joint_eval_orderarray that preservesstable public joint indices while letting FK evaluate joints parent-before-child
internally. It also adds regressions covering FK equivalence and FK->IK
round-tripping for an out-of-order articulation.
Closes #910.
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Summary by CodeRabbit
New Features
Tests