Refine source prim resolution for schema roots#6183
Conversation
Greptile SummaryThis PR extends
Confidence Score: 3/5The core utility function's predicate semantics changed from a filter to a descendant search, and one existing production caller (frame_transformer.py) still uses the old predicate path without being updated — it could now return extra, unintended rigid-body prims in scenes with nested physics APIs. The six new call sites in the PhysX/Newton assets are straightforward and logically equivalent to the old three-step pattern. However, frame_transformer.py calls resolve_matching_prims_from_source with predicate=has_rigid_body_api and was not updated. Its old behavior was to discard source prims that didn't carry RigidBodyAPI directly; the new behavior descends into children and can return additional prims that the frame transformer never expected, potentially tracking wrong or extra rigid bodies silently at runtime. source/isaaclab/isaaclab/sim/utils/queries.py — predicate semantics, keyword-only contract, and count-check ordering all need attention. source/isaaclab_physx/isaaclab_physx/sensors/frame_transformer/frame_transformer.py — existing predicate caller not updated. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["resolve_matching_prims_from_source(path_expr, predicate, expected_num_matches)"]
A --> B{Clone plan present?}
B -->|Yes| C[resolve_clone_plan_source → results]
B -->|No| D[find_first_matching_prim → instance-root walk → results]
C --> E{predicate is not None?}
D --> E
E -->|Yes - NEW| F["For each (source, dest) in results:<br/>get_all_matching_child_prims(source, predicate)<br/>→ (child, dest + child_suffix)"]
E -->|No| G[results unchanged]
F --> H{expected_num_matches set?}
G --> H
H -->|Yes, count mismatch| I[raise RuntimeError: expected N found M]
H -->|No or count OK| J{raise_if_no_matches and empty?}
J -->|Yes| K[raise RuntimeError: no prim found]
J -->|No| L[return results]
Reviews (1): Last reviewed commit: "Refine source prim resolution for schema..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if predicate is not None: | ||
| results = [(prim, dest) for prim, dest in results if predicate(prim)] | ||
| results = [ | ||
| (child, dest + child.GetPath().pathString[len(source.GetPath().pathString) :]) | ||
| for source, dest in results | ||
| for child in get_all_matching_child_prims(source.GetPath(), predicate) | ||
| ] |
There was a problem hiding this comment.
predicate callers
The predicate parameter's semantics have fundamentally changed: previously it filtered the resolved source prims themselves (if predicate(prim)), now it descends into each source prim and collects matching children. This is a silent breaking change for frame_transformer.py (line 191-193), which calls resolve_matching_prims_from_source(prim_path, predicate=has_rigid_body_api, raise_if_no_matches=False) expecting the old filter semantics. With the new code, if the resolved source prim itself has RigidBodyAPI and one of its descendants also does (e.g. a nested link), both are now returned as separate results — duplicating the tracked rigid body and corrupting the frame-transformer's view of which bodies to follow. Similarly, task_space_actions.py keeps its manual two-step pattern (no predicate + separate get_all_matching_child_prims), so it is unaffected, but frame_transformer.py is not updated and is now silently broken.
| if expected_num_matches is not None and len(results) != expected_num_matches: | ||
| raise RuntimeError(f"Expected {expected_num_matches} prims at '{path_expr}', found {len(results)}.") | ||
| if raise_if_no_matches and not results: | ||
| raise RuntimeError(f"No prim found at '{path_expr}'.") |
There was a problem hiding this comment.
expected_num_matches check overshadows raise_if_no_matches
When expected_num_matches >= 1 and the resolved result list is empty, the first RuntimeError fires with the message "Expected N prims …, found 0" before the raise_if_no_matches guard is ever reached. That guard is therefore unreachable for all call sites that supply a positive expected_num_matches (every new call site in this PR uses 1). This means the informative "No prim found at '...'" error is never emitted in those cases, and raise_if_no_matches=False cannot suppress the count-validation error when expected_num_matches is also set — the two options interact in a confusing way. Consider short-circuiting the count check when not results and not raise_if_no_matches.
| def resolve_matching_prims_from_source( | ||
| path_expr: str, | ||
| *, | ||
| predicate: Callable[[Usd.Prim], bool] | None = None, | ||
| expected_num_matches: int | None = None, | ||
| env_regex_ns: str = "/World/envs/env_.*", | ||
| raise_if_no_matches: bool = True, | ||
| ) -> list[tuple[Usd.Prim, str]]: |
There was a problem hiding this comment.
* (keyword-only separator) was removed from the original signature, silently making predicate, expected_num_matches, env_regex_ns, and raise_if_no_matches positional. The new call sites exploit this by passing positional args, but all other existing callers rely on keyword arguments (e.g. raise_if_no_matches=False). If any future caller accidentally passes a boolean as the second positional arg it would be silently treated as predicate. Restoring the * separator after expected_num_matches preserves the original public-API contract for env_regex_ns and raise_if_no_matches.
| def resolve_matching_prims_from_source( | |
| path_expr: str, | |
| *, | |
| predicate: Callable[[Usd.Prim], bool] | None = None, | |
| expected_num_matches: int | None = None, | |
| env_regex_ns: str = "/World/envs/env_.*", | |
| raise_if_no_matches: bool = True, | |
| ) -> list[tuple[Usd.Prim, str]]: | |
| def resolve_matching_prims_from_source( | |
| path_expr: str, | |
| predicate: Callable[[Usd.Prim], bool] | None = None, | |
| expected_num_matches: int | None = None, | |
| *, | |
| env_regex_ns: str = "/World/envs/env_.*", | |
| raise_if_no_matches: bool = True, | |
| ) -> list[tuple[Usd.Prim, str]]: |
ooctipus
force-pushed
the
ooctipus/path-expr-standardization
branch
2 times, most recently
from
a4e47c1 to
ff77914
Compare
ooctipus
requested review from
Mayankm96,
jtigue-bdai,
kellyguo11 and
pascal-roth
as code owners
ooctipus
force-pushed
the
ooctipus/path-expr-standardization
branch
from
ff77914 to
47d4a47
Compare
ooctipus
force-pushed
the
ooctipus/path-expr-standardization
branch
from
47d4a47 to
c39a0a8
Compare
| """Return whether ``prim_path`` matches ``path_expr`` up to ``prim_path`` depth. | ||
|
|
||
| This is useful for deletion callbacks: deleting ``/World/envs/env_0`` should invalidate | ||
| an asset configured at ``/World/envs/env_.*/Robot``, while deleting a sibling should not. |
There was a problem hiding this comment.
Should this be /World/envs/env_0/Robot? The above line is /World/envs/env_0.
3 participants
Summary
resolve_matching_prims_from_sourceso callers can resolve matching descendant schema roots from the source instance.Checks
uv run pre-commit run --files ...uv run python -m compileall ...Note: local changelog check against
origin/developis not representative in this worktree because the fork ref is stale and reports unrelated immutable fragments; PR CI should run againstisaac-sim/develop.