Conversation
|
There was a problem hiding this comment.
Isaac Lab Review Bot
The deletion callback fix correctly normalizes PhysX event payloads, invalidates matching collections through the established lifecycle, and guarantees callback cleanup. Before merge, it should preserve the previously accepted string-path input and satisfy repository metadata requirements for the new test file and user-visible package change.
- Design and architecture: Routing matching member and root deletions through
_invalidate_initialize_callbackis coherent and ensures the root view is invalidated. However, the override previously declared and handled a plain path string; the new normalization supports event and dictionary forms but silently ignores that existing form. Supporting all three forms would retain compatibility while fixing PhysX event handling. - API: Although
_on_prim_deletionis private, its changed annotation and behavior remove the prior string-path contract without the bug fix requiring that removal. Normalize strings directly as paths in addition to extractingprim_pathfrom event and dictionary payloads. - Implementation: The prefix matching and
try/finallycleanup are internally consistent, and the regression tests cover matching, nonmatching, root, and cleanup-failure paths. Add string-input coverage, apply the repository SPDX header template to the new test module, and add the required changelog fragment for the user-visibleisaaclab_physxfix.
Minor fixes needed. Posted 3 actionable findings inline.
Automated review; human maintainers own approval decisions.
Description
This fixes the rigid object collection deletion callback for PhysX deletion events.
PhysX delivers a deletion event whose path is stored in
event.payload["prim_path"], but the collection override treated the event object itself as a string. Matching a member deletion could therefore fail before cleanup, leaving the collection initialized and retaining its root view.The callback now:
Fixes #7799
Type of change
Tests
git diff --checkpython -m py_compileon the modified implementation and test files.pytestand the Isaac Sim runtime are not installed).