Match prim path expressions as whole-path regular expressions - #6841
Conversation
66b4930 to
6019926
Compare
github-actions
Bot
added
asset
d60dd9e to
c44ab0d
Compare
find_matching_prims matched each '/'-separated token against prim names
at that depth, while find_first_matching_prim compiled the same argument
as one regex over the full path. Both take a parameter named
prim_path_regex and document the same contract, so an expression such as
'/World/Robot/.*link2' returned nothing from the first and a depth-2 prim
from the second.
Match the whole path in both, and have find_first_matching_prim delegate.
Whole-path matching does not imply traversing the whole stage: the
expression's longest literal prefix gives the traversal root, and when no
wildcard can span a '/', the separator count bounds the descent. On a
93k-prim stage a per-environment query visits about 1k prims.
Two changes follow, because '.*' had been carrying structure rather than
meaning what regex says it means.
make_clone_plan derived an asset's destination template by substituting
'.*' for '{}' in the configured prim path. str.replace is positionally
blind, so a second wildcard below the environment slot produced a
template with two slots that raised IndexError when formatted; the
environment root was hardcoded besides, so a non-default namespace
excluded every asset from the plan. Take the slot from the environment
template instead, which CloneCfg now carries directly: a template always
yields a regex, whereas recovering a template from a regex requires
guessing which part of the text is the wildcard.
With '.*' free to mean what regex says, the environment namespace spells
its slot '[^/]+' so it cannot match across a '/' and select a prim nested
deeper under an environment. path.match accepts a character class in the
clone slot for that, since the text '[^/]+' contains a '/' and so cannot
match the one-segment alternative.
A segment wildcard is written as the character class [^/], whose text
contains a '/' that is not a separator. Every caller that reached for
str.split("/") to take a path expression apart therefore cut a class in
half, yielding a truncated pattern that raised "unterminated character
set" at re.compile or a body name like "]+_FOOT". Route those callers
through split_path_expr, which splits on separators only.
The same conflation appeared where an expression names an index slot
rather than a wildcard. spawn_multi_asset and the clone decorator
substituted a literal ".*", so a slot spelled "[^/]*" silently survived
into the prim path. Normalize to glob first, which collapses every
spelling to the single '*' the index replaces.
Newton registered a tracked ray-cast target under one spelling of the
environment slot and looked it up under another, raising KeyError.
Spell it from the shared template on both sides.
Replace the hand-rolled regex-to-glob replace chains with
path_expr_to_glob so the engine boundary has one implementation, and
qualify the export so isaaclab.sim resolves it.
A segment wildcard has several spellings, and code that substitutes a concrete environment index into a path expression was matching exactly one of them. The visualizer camera view looked for "env_[^/]*" while the namespace is built as "env_[^/]+", so str.replace found nothing and the camera kept a wildcard where a concrete environment belonged, rendering a different scene than the golden. The OVRTX deformable bindings had the same shape against a literal ".*". Match the wildcard rather than a spelling of it. path_to_source reported its destination as a glob. Callers use it to build the path expression its name promises and then convert to glob at the engine boundary, so the star reached find_first_matching_prim as a quantifier and matched nothing -- "Failed to find articulation root prim at '/World/envs/env_*/Robot'". Extending the clone-slot match to accept a character class is what began routing callers down this branch, so report a path expression and let the boundary do the converting.
The ovphysx tests wrote their prim paths with a bare "*", which only resolved because the matcher used to rewrite a lone star into ".*". That rewrite is gone, since a star is a quantifier and the rewrite could not tell the two apart, so spell the wildcard the way the expression is now read. The ovphysx view "pattern=" arguments are fnmatch globs and keep their stars. The ovphysx frame transformer stripped the env prefix with a bare "[^/]+" alternative, which consumed the opening half of a character class and left a body name of "]*". Try the wildcard spellings first, matching the PhysX copy. Isaac Sim's XformPrim.resolve_paths applies one regex per path segment, so a segment wildcard has to reach it as ".*" -- "[^/]" holds a separator and gets split across two of its segments. Convert where the gripper view is built rather than rewriting the expression the IsaacLab matcher still needs.
The fabric particle sync substitutes the instance index into a deformable's visual mesh path, first in the env slot and then for any wildcard left over. The second pass still only recognised ".*", so a path spelling the wildcard as a character class kept it and resolved to no prim, leaving that instance unsynced.
The imu and pva sensor tests still handed their sensor prim paths a bare "*", so the sensors resolved nothing once the matcher stopped rewriting a lone star into ".*". Their spawn paths were already converted; bring the sensor paths with them, and correct the note that called the prim path an fnmatch glob -- the glob is the ovphysx binding underneath it, which IsaacLab derives. The ovphysx view tests keep their stars: those strings are passed to OvPhysxView as binding patterns, which really are fnmatch globs.
Spelling a prim path in full means spelling the wildcard that selects one
environment, and "[^/]+" is an implementation detail no configuration
should have to carry. The {ENV_REGEX_NS} macro already exists for this,
but only InteractiveScene expanded it, for the assets it collects. A
direct environment builds its own assets, so its configurations had to
write the namespace out.
Expand the macro where an asset or a sensor is constructed, so both kinds
of environment read it, and use it across the task configurations. In
AssetBase this has to run before the cfg is queued for replication: the
clone plan keys its rows by that cfg, and would otherwise record the
macro as a destination. The expansion is a plain replace rather than
str.format, because the rest of an expression may hold braces of its own
(a repetition count, say).
The paths left spelling the namespace are the ones no expansion reaches:
Newton coupler bodies, direct spawner calls, f-strings that would consume
the macro themselves, and docstrings showing what it expands to.
|
Too many files changed for review (174 files, 100 file limit). Bypass the limit by tagging |
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The whole-path regex unification and template-driven clone destinations address real inconsistencies, but four actionable issues remain: CloneCfg.clone_regex is removed without the required deprecation period, traversal bounding can prune valid plain-regex matches, a FrameView call passes an unexpanded namespace macro, and clone prototype naming is inconsistent for segment-safe leaf wildcards.
- Design and architecture: Using one whole-path regex contract for both prim finders and deriving clone destinations from
clone_templateare sound design choices. However,_bound_searchunder-approximates regex constructs that can consume/: a negated character class such as[^A]+can span path separators while the implementation still applies a fixed depth limit, causing valid matches to be skipped. - API: The new helpers and signatures are exported and documented consistently, but replacing the public
CloneCfg.clone_regexfield outright violates the repository requirement that public API removals receive a prior deprecation. Retain it as a deprecated compatibility shim for one release with migration guidance. - Implementation: Two changed paths remain inconsistent with the new expression handling.
check_terrain_importer.pypasses{ENV_REGEX_NS}/balldirectly toFrameView, which does not perform asset or sensor macro expansion and therefore forwards a non-absolute expression to the finder. In the clone decorator, copied destinations normalize segment-wildcard spellings throughpath_expr_to_glob, while the prototype path still only replaces.*; a[^/]*leaf therefore produces a different or invalid prototype name. Normalize and reuse the leaf for both paths.
Significant concerns. Posted 4 actionable findings inline.
Automated review; human maintainers own approval decisions.
| segments = masked.split("/")[1:] | ||
| literal = list(itertools.takewhile(lambda s: s and not _REGEX_METACHARACTERS & set(s), segments)) | ||
| # an unescaped '.' outside a class matches '/', so the expression can reach any depth | ||
| unbounded = re.search(r"(?<!\\)\.", masked) is not None |
There was a problem hiding this comment.
🟡 Warning · Implementation — Depth bound assumes only dot spans separators
find_matching_prims now documents plain-regex semantics, but _bound_search treats an expression as depth-bounded unless an unescaped . appears. A negated class that does not exclude / (e.g. /World/Robot/[^A]+) can match a deeper prim, yet traversal prunes at the separator count, silently dropping valid matches. Disable the depth bound whenever a class, group, or alternation may consume /.
find_matching_prims promises full-path Python regex semantics. Walk the complete stage and let re.fullmatch alone decide which prim paths match, without narrowing the root or pruning by inferred depth.
hujc7
left a comment
hujc7
left a comment
There was a problem hiding this comment.
Agent review: six comments, all on the matcher and its call sites; the clone-template and macro-expansion groups read sound and I have no comments on them.
Important
The Startup cost section no longer describes the code.
It says whole-path matching "does not walk the whole stage: the expression bounds the search. Its longest literal prefix is the traversal root..." -- but 312fe8cd removed _bound_search, and the docstring on find_matching_prims now says the opposite: "Every prim on the stage is tested; the expression does not imply a traversal root or depth limit."
That also leaves the benchmark as the only performance evidence for a change whose cost profile it no longer measures: 35.39 s -> 35.80 s on env_creation was taken with the bound in place, on the arm that avoided full-stage walks. Worth re-running isaaclab benchmark startup --task Isaac-Velocity-Rough-AnymalD --num_envs 4096 against the current head and updating both the prose and the table -- a full-stage walk per call at 4096 envs is exactly the case the original table was there to rule out.
| prefix_expr = "/".join(path_expr.split("/")[: prim_path.count("/") + 1]) | ||
| return re.match(f"^{_normalize_legacy_wildcard_pattern(prefix_expr)}$", prim_path) is not None | ||
| prefix_expr = "/".join(split_path_expr(path_expr)[: prim_path.count("/") + 1]) | ||
| return re.match(f"^{prefix_expr}$", prim_path) is not None |
There was a problem hiding this comment.
Agent found bug: fullmatch landed in find_matching_prims but not in its sibling.
find_matching_prims now compiles the bare expression and calls pattern.fullmatch(...), which anchors the complete expression. This line still builds f"^{prefix_expr}$" and calls re.match, so a top-level | splits it into ^A and B$ -- the first matches by prefix, the second can never match:
matches_path_expr_prefix("/World/envs/env_[^/]+/Robot/cart|pole", "/World/envs/env_0/Robot/cartXX")
# -> TrueThe two functions now read the same expression differently, which is the situation this PR set out to remove.
| return re.match(f"^{prefix_expr}$", prim_path) is not None | |
| return re.fullmatch(prefix_expr, prim_path) is not None |
Related, at the caller rather than here
envs/mdp/events.py:2566 and :2757 build "|".join(body_names) straight into a path expression without grouping. Under fullmatch that pattern no longer selects <body>/visuals for any of the names -- it selects the bare body prim for the first name only. Grouping the join as (?:{body_names_regex}) fixes it at the caller.
-
matches_path_expr_prefixusesfullmatch;events.pygroups its joined body names
There was a problem hiding this comment.
Addressed in a4be8bd: matches_path_expr_prefix now uses fullmatch, and both events callers group the body-name alternation. The visual-layout probes also use clone-aware source resolution instead of scanning every destination clone.
| if index < len(tokens) - 1: | ||
| all_prims = output_prims | ||
| output_prims = [] | ||
| for prim in Usd.PrimRange(stage.GetPseudoRoot(), Usd.TraverseInstanceProxies(Usd.PrimAllPrimsPredicate)): |
There was a problem hiding this comment.
Agent review: removing the bound was right for correctness, but it moves two things worth stating.
Traversal predicate. develop reached prims via GetAllChildren(), which stops at instance boundaries; this walks TraverseInstanceProxies(Usd.PrimAllPrimsPredicate).
develop |
this PR | |
|---|---|---|
find_matching_prims |
stops at instance boundaries | descends into instance proxies |
find_first_matching_prim |
stage.Traverse() -- active, defined, loaded, concrete |
all prims, including inactive / over-only / class |
Descending into instance proxies looks intended. The knock-on to find_first_matching_prim may not be: it can now return a deactivated or over-only prim where it previously returned None, changing the exists-check at sensors/camera/camera.py:171 and the schema read at sim/schemas/schemas_actuators.py:128. Worth a line in Changed, and worth deciding whether that delegation should keep the default predicate.
Cost. Every call is now a full-stage walk with a regex per prim, as the updated docstring says. That is the honest implementation; the concern is only that the evidence for it is stale -- see the comment on the description.
There was a problem hiding this comment.
Addressed in a4be8bd: the all-authored-prims policy is now explicit in the API docs and pinned by tests for inactive and undefined prims. Unscoped queries remain honest full-stage walks; ClonePlan-owned discovery now walks only its concrete source subtree. I also removed the stale bounded-implementation benchmark and claim from the PR description.
| for child in children: | ||
| if hasattr(child, "prim_path"): | ||
| child.prim_path = child.prim_path.format(ENV_REGEX_NS=self.cloner_cfg.clone_regex) | ||
| child.prim_path = child.prim_path.format(ENV_REGEX_NS=self._env_regex_ns) |
There was a problem hiding this comment.
Agent found bug: the macro now has two expansions, and the one the scene uses is not brace-safe.
expand_env_regex_ns uses str.replace and documents why -- "a plain replace, not str.format: the rest of the expression may hold braces of its own" (cloner/cloner_cfg.py:35). This line still uses str.format, so a config that hits the documented hazard fails depending on who builds it:
"{ENV_REGEX_NS}/Robot/link_[0-9]{2}"
AssetBase / SensorBase -> /World/envs/env_[^/]+/Robot/link_[0-9]{2}
InteractiveScene -> IndexError: Replacement index 2 out of range
A brace quantifier is a normal spelling in the regex dialect this PR promotes, so the divergence is reachable rather than theoretical. Routing this line through expand_env_regex_ns also removes the second place that knows how the macro expands.
-
InteractiveSceneexpands the macro throughexpand_env_regex_ns
There was a problem hiding this comment.
Addressed in a4be8bd: InteractiveScene routes every ENV_REGEX_NS expansion through expand_env_regex_ns, as does the multi-mesh ray caster. A regression test verifies that regex quantifiers such as {2} survive macro expansion.
| assert [prim.GetPath().pathString for prim in matches] == ["/World/Robot/foo", "/World/Robot/foo/bar"] | ||
|
|
||
|
|
||
| def test_find_matching_prims_has_no_inferred_traversal_bounds(): |
There was a problem hiding this comment.
Agent suggestion: this one asserts the implementation rather than the contract.
The two behavioural tests added alongside it are exactly right. This one parses the function's own source and asserts that GetPrimAtPath, PruneChildren and _bound_search do not appear in it -- so it fails on any future rewrite that keeps the contract, including a legitimate reintroduction of bounding that is provably sound. AGENTS.md asks for observable behaviour and public contracts rather than implementation details.
The property worth pinning is behavioural and needs no introspection: a match must be returned no matter where it sits. test_find_matching_prims_uses_unbounded_full_path_regex already does that for [^A]+; a case whose match lies outside the expression's literal prefix -- say find_matching_prims(r"/World/[^/]+/foo") with /World/A/foo and /World/B/foo present -- closes the same gap without reading the source.
Also still open: path_expr_to_glob, split_path_expr and expand_env_regex_ns have no test mention anywhere on the branch, and the first two are pure string functions that need no stage.
There was a problem hiding this comment.
Applied the behavioral suggestions in a4be8bd: literal-prefix-independent matching plus tests for path_expr_to_glob, split_path_expr, and expand_env_regex_ns. I retained the structural negative gate because regex-derived root or depth inference is a rejected architecture here, then extended the gate to cover the shared subtree matcher as well.
| _CHARACTER_CLASS = re.compile(r"\[\^?[^]]*\]") | ||
| """Matches a regex character class, whose text may hold a ``/`` that is not a path separator.""" | ||
|
|
||
| _SEGMENT_WILDCARD = re.compile(r"\[\^/\][*+]|\.\*") |
There was a problem hiding this comment.
Agent review: which segment-wildcard spellings are canonical? Two places answer differently.
_SEGMENT_WILDCARD recognises .*, [^/]* and [^/]+. The namespace and clone_template emit [^/]+; resolve_matching_prims_from_source, scene_data/deformable_discovery.py:329 and most converted task cfgs emit [^/]*, which also matches the empty segment. Picking one and using it everywhere would make the intended spelling obvious to the next author writing a cfg.
Related: path_expr_to_glob's docstring says every regex spelling of a segment wildcard maps onto *, but [^/] and [^/]{2} pass through into the glob verbatim. Either narrow the docstring or widen the pattern.
There was a problem hiding this comment.
Addressed in a4be8bd: framework-owned environment slots now consistently use [^/]+, while arbitrary user regex is left untouched. The path_expr_to_glob documentation is narrowed to its explicitly supported adapter spellings rather than claiming general regex-to-glob translation.
|
|
||
| sim.reset() | ||
| view = OvPhysxFrameView("/World/envs/env_.*/Cube/CameraMount", device=device) | ||
| view = OvPhysxFrameView("/World/envs/env_[^/]*/Cube/CameraMount", device=device) |
There was a problem hiding this comment.
Agent review: the conversion sweep left four call sites behind, including one in this file.
This line converted; :77 in the same file did not, and both go to find_matching_prims:
:77 FrameView("/World/envs/env_.*/WorldCamera")
:195 OvPhysxFrameView("/World/envs/env_[^/]*/Cube/CameraMount")
The other three all pre-date the branch point, so they are sweep misses rather than merge fallout:
isaaclab/test/sim/check_meshes.py:139 clone() root "/World/Origin.*/Object{idx:02d}"
isaaclab/test/cloner/test_replicate_session.py:55 prim_path="/World/envs/env_.*/Robot"
isaaclab_visualizers/test/test_newton_adapter.py:219 prim_path="/World/envs/env_.*/Camera"
Nothing fails today -- I traced each. The last two are SimpleNamespace fakes that never reach the matcher, and neither :77 nor check_meshes.py has a same-named prim nested deeper. Raising it because the safety argument for the whole change is that every pattern still selects what it selected before, and these four are the ones where that has to be argued from stage shape rather than read off the expression.
There was a problem hiding this comment.
Addressed all four sweep misses in a4be8bd, using the required non-empty single-segment spelling [^/]+ for environment and generated-origin slots.
# Description Fixes `IsaacContrib-Factory-Franka` startup after segment-safe prim-path expressions were introduced in #6841. The Factory collision analyzer assumed cloned environment expressions contained `.*`. It rewrote that spelling to `0` to find a source prim, then rebuilt an `env_.*` expression. The current `{ENV_REGEX_NS}` expansion is `/World/envs/env_[^/]+`, so the rewrite no longer produced a concrete USD path and startup failed with: ```text ValueError: Prim at path /World/envs/env_[^/]+/Robot is not valid. ``` This change resolves each collision body through `resolve_matching_prims_from_source`, the clone-plan-owned resolver. Its returned destination path expression is passed directly to point-cloud sampling, removing both wildcard-spelling rewrites and keeping ownership of clone-path resolution in the cloning subsystem. No new dependencies or test files are introduced. ## Type of change - Bug fix (non-breaking change which fixes an issue) ## Screenshots Not applicable; this is an environment-startup fix. ## Validation - Reproduced the exact failure on unmodified latest `develop` with the existing consolidated Factory smoke case. - Existing `IsaacContrib-Factory-Franka` smoke case: **1 passed in 39.33s**, including two environments and 20 random-action steps. - Full `uv run isaaclab -f` equivalent repository hook suite: passed. - `git diff --check upstream/develop...HEAD`: passed. ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the pre-commit checks with `isaaclab -f` - [x] Documentation changes are not required; the changelog fragment documents the fix - [x] My changes generate no new warnings - [x] The existing consolidated Factory smoke test proves the fix; no new test file is required - [x] I have added a changelog fragment under `source/isaaclab_tasks/changelog.d/` - [x] My name already exists in `CONTRIBUTORS.md`
3 participants
Description
find_matching_primsmatches each/-separated token against prim names at that depth.find_first_matching_primcompiles the same argument as one regex over the full path. Bothtake a parameter named
prim_path_regexand document the identical contract. So oneexpression on one stage gives two answers:
This makes both read the argument as a plain Python regular expression over the whole path,
and has
find_first_matching_primdelegate tofind_matching_prims.Two follow-on changes are needed, because
.*had been carrying structure rather thanmeaning what regex says.
The clone slot comes from the template, not from the text.
make_clone_planderived anasset's destination with
prim_path.replace(".*", "{}").str.replaceis positionally blind,so a second wildcard below the environment produces two slots:
CloneCfgnow carriesclone_template, and the plan splits the cfg path at the knownenvironment depth. A template always yields a regex; recovering a template from a regex means
guessing which part of the text is the wildcard.
The environment slot is segment-safe. With
.*free to mean what regex says, thenamespace spells its slot
[^/]+, so{ENV_REGEX_NS}/Robotselectsenv_0/Robotand not aRobotnested deeper.Configurations do not spell that out.
{ENV_REGEX_NS}previously resolved only for assets anInteractiveScenecollected, so a direct environment had to write the namespace by hand. Itis now expanded where an asset or a sensor is built, and used across the task configurations —
no configuration names
[^/]+itself.Search scope and startup cost
An unscoped
find_matching_prims(expr, stage)is deliberately an honest full-stage query: itapplies
re.fullmatchto every traversed prim and does not inspect the expression to infer aroot or depth limit. That keeps Python regex semantics intact, including expressions whose
classes, groups, or alternations can consume
/.Clone-aware discovery has a narrower candidate scope for an architectural reason rather than a
regex heuristic. It rebases the destination expression through the active
ClonePlan, thenwalks only the concrete source/prototype subtree supplied by that plan. It never scans every
cloned destination environment merely to rediscover identical source structure.
The previous startup table measured the now-removed regex-bounded implementation, so it was
removed rather than presented as evidence for the current head.
Breaking change
An expression whose trailing token meant "direct children only" now means "whole subtree", and
should spell that token
[^/]+. Across production prim-path literals: 659 hold no wildcard andcannot change; none end in a bare
.*; 6 hold a mid-pattern.*, and those change only if asame-named prim exists deeper under the environment.
CloneCfg.clone_regexis removed rather than deprecated — its value isclone_template.format("[^/]+"), so the two can no longer disagree. Worth a maintainer's callwhether that needs a shim.
Green tests are weak evidence for the classification above: the suite does not construct the
stage shapes on which the mid-pattern class changes meaning.
Not in this change
resolve_matching_prims_from_sourcestill has a legacy no-clone-plan branch that infers anenvironment boundary heuristically. Ad-hoc multi-instance test scenes depend on that inference,
so replacing it is a separate decision. ClonePlan-owned expressions do not use this branch.
Type of change