-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add check to avoid creating transforms for non-xformable prims #4348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Greptile SummaryAdded early validation check in Key Changes:
Critical Issue:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant create_prim
participant USD Stage
participant standardize_xform_ops
User->>create_prim: create_prim(path, type, translation, orientation, scale)
create_prim->>USD Stage: DefinePrim(path, type)
USD Stage-->>create_prim: prim
alt prim is valid
create_prim->>create_prim: Apply attributes, USD references, semantic labels
create_prim->>create_prim: Check if prim.IsA(UsdGeom.Xformable)
alt prim is NOT Xformable (Material, Shader)
create_prim->>create_prim: logger.debug("Not Xformable, skipping transforms")
create_prim-->>User: return prim (without transforms)
else prim is Xformable (Xform, Mesh, Scope, etc.)
create_prim->>create_prim: Convert position/orientation to local frame
create_prim->>standardize_xform_ops: standardize_xform_ops(prim, translation, orientation, scale)
standardize_xform_ops->>standardize_xform_ops: Check if prim.IsA(UsdGeom.Xformable)
alt prim is NOT Xformable
standardize_xform_ops->>standardize_xform_ops: logger.error("Not Xformable")
standardize_xform_ops-->>create_prim: return False
else prim is Xformable
standardize_xform_ops->>USD Stage: Set xformOp:translate/orient/scale
standardize_xform_ops-->>create_prim: return True
end
create_prim-->>User: return prim (with transforms)
end
else prim is invalid
create_prim-->>User: raise ValueError
end
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (4)
-
source/isaaclab/isaaclab/sim/utils/prims.py, line 169 (link)logic: Scope prims ARE Xformable in USD (UsdGeom.Scope inherits from UsdGeom.Xformable), so mentioning "scope prims" here is misleading
-
source/isaaclab/isaaclab/sim/utils/transforms.py, line 136 (link)logic: Scope prims ARE Xformable in USD (UsdGeom.Scope inherits from UsdGeom.Xformable), remove mention of scope prims
-
source/isaaclab/test/sim/test_utils_prims.py, line 252-257 (link)logic: The test incorrectly assumes Scope is not Xformable. In USD,
UsdGeom.Scopeinherits fromUsdGeom.Xformable, so Scope prims ARE Xformable and can have transforms. Remove Scope from this test and only test with truly non-Xformable types like Material and Shader -
source/isaaclab/test/sim/test_utils_prims.py, line 285-304 (link)logic: Remove the Scope test section entirely - Scope prims ARE Xformable in USD and can have transforms. Test will fail or give incorrect results. Consider adding a Shader prim test instead
3 files reviewed, 4 comments
Description
This MR ensures we don't try setting transforms for non-xformable prims (Scopes, Materials, Shaders). Previously, they were not being set but an error was being thrown.
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there