Skip to content

Conversation

@ooctipus
Copy link
Collaborator

Description

Our pure-USD XFormPrim.set_world_poses() was writing world positions/orientations directly into local xformOp:translate/orient, instead of converting the desired world pose into the prim’s parent-local frame. For prims under a transformed parent/ancestor, this could bake the parent’s world translation into the child’s local translate, effectively applying the translation twice (and leading to misplaced collision geometry / missing contacts). This PR fixes set_world_poses() to compute local ops via world→local conversion using the parent transform, matching expected world-pose semantics.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@ooctipus ooctipus requested a review from Mayankm96 as a code owner December 21, 2025 10:58
@github-actions github-actions bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Dec 21, 2025
@ooctipus ooctipus requested review from kellyguo11 and removed request for Mayankm96 December 21, 2025 10:59
@ooctipus ooctipus changed the title Fix xform prim set_world_poses to respect its ancestral transform tree [Newton]Fixes xform prim set_world_poses to respect its ancestral transform tree Dec 21, 2025
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 21, 2025

Greptile Summary

Fixed critical bug in set_world_poses() where world positions/orientations were incorrectly written directly to local xform ops instead of converting through parent's transform. This caused prims under transformed parents to have parent translation applied twice, leading to misplaced collision geometry and missing contacts.

Key changes:

  • Added _world_to_local_tq() helper that converts world transforms to local transforms by computing parent_world_inverse * desired_world_transform
  • Modified set_world_poses() to use the new helper, ensuring world poses respect the ancestral transform tree
  • Refactored set_local_poses() to directly set local transforms instead of incorrectly delegating to set_world_poses()
  • Both methods now properly handle partial updates (position-only or orientation-only) by preserving existing values

Confidence Score: 4/5

  • This PR is safe to merge with low risk - it fixes a critical transform bug with clean implementation
  • Score of 4 reflects correct bug fix with proper matrix math, but has one critical logic issue that could cause incorrect transforms. The world-to-local conversion follows standard transform hierarchy math (parent_inverse * world = local), and the refactored set_local_poses() properly handles local transforms. However, the matrix composition order in _world_to_local_tq() needs verification
  • Pay attention to source/isaaclab/isaaclab/sim/prims/xform_prim.py line 241-243 for matrix composition order

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/sim/prims/xform_prim.py Fixed set_world_poses() to properly convert world to local transforms via parent inverse, and refactored set_local_poses() to directly set local transforms instead of delegating to world poses

Sequence Diagram

sequenceDiagram
    participant User
    participant XFormPrim
    participant USD_API
    participant Prim
    participant Parent

    User->>XFormPrim: set_world_poses(positions, orientations)
    XFormPrim->>Prim: ComputeLocalToWorldTransform()
    Prim-->>XFormPrim: current_world_transform
    
    alt positions is None
        XFormPrim->>XFormPrim: world_t = current_world.ExtractTranslation()
    else positions provided
        XFormPrim->>XFormPrim: world_t = Gf.Vec3d(positions[idx])
    end
    
    alt orientations is None
        XFormPrim->>XFormPrim: world_q = current_world.ExtractRotation().GetQuat()
    else orientations provided
        XFormPrim->>XFormPrim: world_q = Gf.Quatd(orientations[idx])
    end
    
    XFormPrim->>XFormPrim: _world_to_local_tq(prim, world_t, world_q)
    XFormPrim->>Prim: GetParent()
    Prim-->>XFormPrim: parent_prim
    
    alt parent exists and valid
        XFormPrim->>Parent: ComputeLocalToWorldTransform()
        Parent-->>XFormPrim: parent_w (parent world transform)
    else no parent
        XFormPrim->>XFormPrim: parent_w = identity matrix
    end
    
    XFormPrim->>XFormPrim: world_m = Matrix4d from (world_t, world_q)
    XFormPrim->>XFormPrim: local_m = parent_w.GetInverse() * world_m
    XFormPrim->>XFormPrim: Extract (local_t, local_q) from local_m
    XFormPrim-->>XFormPrim: return (local_t, local_q)
    
    XFormPrim->>USD_API: translate_op.Set(local_t)
    XFormPrim->>USD_API: orient_op.Set(local_q)
    USD_API-->>User: World pose set correctly
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional Comments (1)

  1. source/isaaclab/isaaclab/sim/prims/xform_prim.py, line 241-243 (link)

    logic: order of operations: SetRotate() must be called before SetTranslateOnly() for correct matrix composition

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant