Adds Collision Group feature to Interactive Scene#5165
Adds Collision Group feature to Interactive Scene#5165michaellin6 wants to merge 3 commits intoisaac-sim:developfrom
Conversation
visual_materials, outdated API signature.
michaellin6
requested review from
Mayankm96,
jtigue-bdai and
kellyguo11
as code owners
github-actions
bot
added
documentation
|
run_franka_collision_groups.py |
Greptile SummaryThis PR introduces Key observations:
Confidence Score: 3/5Merging is risky without addressing the silent filter_collisions suppression and the str.replace path substitution bug. The core collision-group logic (matrix, USD prim creation, inter-env isolation) looks sound and is well-tested. However, two P1 issues reduce confidence: (1) the default filter_collisions=True is silently ignored with no logging, which will silently break user expectations, and (2) the str.replace-based env-path rewriting is fragile and can produce corrupt prim paths for assets whose path contains the env prefix more than once. The docstring/set issues are minor but add to the overall noise. source/isaaclab/isaaclab/scene/interactive_scene.py — _apply_collision_groups path substitution and the filter_collisions warning gap; source/isaaclab/isaaclab/scene/interactive_scene_cfg.py — docstring wording. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[InteractiveScene.__init__] --> B[_add_entities_from_cfg\npopulates _global_prim_paths]
B --> C[clone_environments]
C --> D{physx backend?}
D -- No --> E[Done]
D -- Yes --> F{collision_groups set?}
F -- Yes --> G[_apply_collision_groups]
F -- No --> H{filter_collisions=True?}
H -- Yes --> I[filter_collisions\n_global_prim_paths]
H -- No --> E
G --> G1[Step 1: Collect entity prim paths\nfrom cfg.__dict__]
G1 --> G2[Step 2: Validate assets and\ncollides_with references]
G2 --> G3[Step 3: Build collision matrix\nmutual-agreement logic]
G3 --> G4[Step 4: Resolve env_0 prim paths\nper group]
G4 --> G5[Step 5: Set InvertCollisionGroupFilterAttr=True\non PhysxSceneAPI]
G5 --> G6[Step 6: Create USD prims under\n/World/collisions via Sdf.ChangeBlock]
G6 --> G7[Per-env PhysicsCollisionGroup\nwith filteredGroups allowlist]
G7 --> G8{has global paths?}
G8 -- Yes --> G9[Create global_group prim\nwire all env groups to it]
G8 -- No --> E
G9 --> E
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if self.cfg.collision_groups: | ||
| self._apply_collision_groups() | ||
| elif self.cfg.filter_collisions: | ||
| self.filter_collisions(self._global_prim_paths) |
There was a problem hiding this comment.
filter_collisions=True silently ignored with no warning
When collision_groups is set, the default value of filter_collisions=True is silently swallowed by the elif. Since users almost never explicitly set filter_collisions=False, every user who enables collision_groups will hit this branch without knowing it. If filter_collisions is True and collision_groups is also set, a warning should be emitted so the behaviour is transparent:
if self.cfg.collision_groups:
if self.cfg.filter_collisions:
logger.warning(
"Both 'collision_groups' and 'filter_collisions=True' are set. "
"'filter_collisions' is ignored when 'collision_groups' is configured — "
"inter-environment isolation is handled by _apply_collision_groups instead."
)
self._apply_collision_groups()
elif self.cfg.filter_collisions:
self.filter_collisions(self._global_prim_paths)Without this warning, users who rely on the default filter_collisions=True will get no indication that that flag has no effect.
| within the same environment can collide. This is orthogonal to :attr:`filter_collisions`, | ||
| which controls *inter*-environment collision isolation. |
There was a problem hiding this comment.
The phrase "orthogonal to filter_collisions" implies the two can be used independently and simultaneously, but the code in interactive_scene.py is an if/elif — when collision_groups is set, filter_collisions is silently ignored. The tutorial documentation actually says the opposite: collision_groups replaces filter_collisions. This is a contradiction that will confuse users.
Consider revising to:
| within the same environment can collide. This is orthogonal to :attr:`filter_collisions`, | |
| which controls *inter*-environment collision isolation. | |
| When set, this attribute **replaces** :attr:`filter_collisions`: inter-environment collision | |
| isolation is handled internally by the collision group logic, so :attr:`filter_collisions` | |
| is ignored even if it is ``True`` (the default). |
![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
Summary
This PR adds intra-environment collision filtering via named collision groups. The implementation correctly creates USD PhysicsCollisionGroup prims with allowlist semantics using InvertCollisionGroupFilter. However, there's a critical issue with unassigned assets breaking inter-env isolation, plus documentation inconsistencies that will confuse users.
Design Assessment
Problem solved: Allow fine-grained collision filtering between assets within the same environment (e.g., a phantom sensor that shouldn't collide with anything).
Is this the right design? The approach of creating per-env collision groups with allowlist semantics is sound and follows PhysX best practices. The mutual-agreement collision matrix is a good design choice. However:
-
Alternative considered: Single "default" group for unassigned assets — The current design leaves unassigned assets outside all collision groups, which breaks inter-env isolation (critical bug). A safer design would auto-create a "default" group containing all unassigned assets, ensuring they still get inter-env isolation.
-
Alternative considered: Keep
filter_collisionsfor unassigned assets — Could runfilter_collisionsfor assets not in any collision group, but this would create conflicting group semantics.
Does it follow the framework's ownership model? Yes — collision group setup belongs in InteractiveScene during scene construction, alongside the existing filter_collisions logic.
Verdict: Acceptable short-term, but needs a fix for unassigned assets. The current implementation silently breaks inter-env isolation when any asset is not assigned to a collision group. This is a footgun that will cause hard-to-debug physics issues.
Architecture Impact
InteractiveSceneCfggainscollision_groupsfield — additive, non-breakingInteractiveScene.__init__now branches between_apply_collision_groups()andfilter_collisions()— changes collision setup path but maintains existing behavior whencollision_groups=None- Scene module exports
CollisionGroupCfg— additive change to public API
No cross-module callers affected since this is a new feature.
Implementation Verdict
Significant concerns — One critical issue (unassigned assets), one changelog error, and documentation inconsistencies.
Test Coverage
✅ Tests included and well-structured:
test_collision_groups_prim_creation— verifies USD prim structuretest_collision_groups_mutual_agreement— verifies allowlist logictest_collision_groups_collides_with_none— verifies None vs [] semanticstest_collision_groups_invalid_asset_name— error handlingtest_collision_groups_invalid_group_reference— error handlingtest_collision_groups_none_preserves_existing_behavior— backward compat
🟡 Missing test: No test for unassigned assets scenario — should verify that assets not in any group still get inter-env isolation (or that a warning/error is raised).
CI Status
pre-commit: ✅ passedBuild Base Docker Image: ❌ failed (likely infra, not PR-related)- Other tests: skipped (blocked by Docker build)
Findings
🔴 Critical: interactive_scene.py — Unassigned assets break inter-env isolation
When collision_groups is set, filter_collisions is skipped entirely. Assets not assigned to any collision group are not added to any PhysicsCollisionGroup prim. With InvertCollisionGroupFilter=True, these unassigned prims collide with everything including assets in other environments — silently breaking inter-env isolation.
Fix options:
- Auto-create a "default" group containing all unassigned assets
- Raise an error if any asset with
prim_pathcontainingenv_regex_nsis not in a group - Emit a warning documenting the limitation
🟡 Warning: CHANGELOG.rst:17 — Documents fix not in this PR
The changelog mentions fixing CreateShaderPrimFromSdrCommand in visual_materials.py, but this file is not changed in the PR. Remove this entry or include the actual fix.
🟡 Warning: interactive_scene_cfg.py:171 — Misleading "orthogonal" claim
Docstring says collision_groups is "orthogonal to filter_collisions" but they are actually mutually exclusive — when collision_groups is set, filter_collisions is ignored.
🔵 Improvement: interactive_scene_cfg.py:47 — Clarify self-collision behavior
Docstring says collides_with=[] means "collides with nothing (fully isolated)" but the code always enables self-collision (name_a == name_b check). Should say "collides only with itself" or "isolated from other groups".
| # finer-grained intra-env restrictions). | ||
| if self.cfg.collision_groups: | ||
| self._apply_collision_groups() | ||
| elif self.cfg.filter_collisions: |
There was a problem hiding this comment.
🔴 Critical: Unassigned assets break inter-env isolation
When collision_groups is set, filter_collisions is skipped. But assets NOT assigned to any collision group won't be in any PhysicsCollisionGroup prim. With InvertCollisionGroupFilter=True, these unassigned prims will collide with everything — including assets in other environments.
Example: Scene with robot, table, sensor where only robot and table are in collision groups. The unassigned sensor collides across all envs.
Suggested fix: After Step 2 validation, check for unassigned assets and either:
- Auto-create a "default" group for them
- Raise
ValueErrorlisting unassigned assets - At minimum, log a warning
| elif self.cfg.filter_collisions: | |
| if self.cfg.collision_groups: | |
| self._apply_collision_groups() | |
| # TODO: Warn or handle assets not assigned to any collision group | |
| elif self.cfg.filter_collisions: |
There was a problem hiding this comment.
Agreeing with the bot here. At the very least a warning should be thrown.
| Fixed | ||
| ^^^^^ | ||
|
|
||
| * Fixed ``CreateShaderPrimFromSdrCommand`` call in :func:`~isaaclab.sim.spawners.materials.visual_materials.spawn_preview_surface` |
There was a problem hiding this comment.
🟡 Warning: This fix is not in this PR
The visual_materials.py file is not modified in this PR. Either:
- Remove this changelog entry, or
- Include the actual fix in this PR
| * Fixed ``CreateShaderPrimFromSdrCommand`` call in :func:`~isaaclab.sim.spawners.materials.visual_materials.spawn_preview_surface` |
There was a problem hiding this comment.
The bot is correct!
| belong to each group and which groups are allowed to collide with each other. | ||
|
|
||
| When set, the scene creates USD ``PhysicsCollisionGroup`` prims that control which assets | ||
| within the same environment can collide. This is orthogonal to :attr:`filter_collisions`, |
There was a problem hiding this comment.
🟡 Misleading: collision_groups and filter_collisions are mutually exclusive, not orthogonal
When collision_groups is set, filter_collisions is completely ignored (see interactive_scene.py lines 216-218). This docstring will confuse users into thinking both can be used together.
| within the same environment can collide. This is orthogonal to :attr:`filter_collisions`, | |
| within the same environment can collide. When set, this **replaces** :attr:`filter_collisions` | |
| for collision filtering — the scene handles both inter-environment isolation and intra-environment | |
| filtering through the collision groups mechanism. |
| """List of collision group names that this group is allowed to collide with. | ||
|
|
||
| * ``None`` (default): This group collides with all other defined collision groups. | ||
| * ``[]`` (empty list): This group collides with nothing (fully isolated). |
There was a problem hiding this comment.
🔵 Improvement: Clarify that self-collision is always enabled
The phrase "collides with nothing (fully isolated)" is misleading — assets within the same group always collide with each other due to the name_a == name_b check in _apply_collision_groups.
| * ``[]`` (empty list): This group collides with nothing (fully isolated). | |
| * ``[]`` (empty list): This group is isolated from all other groups (only collides with itself). |
| within the same environment can collide. This is orthogonal to :attr:`filter_collisions`, | ||
| which controls *inter*-environment collision isolation. | ||
|
|
||
| Assets not assigned to any collision group follow default physics behavior and collide |
There was a problem hiding this comment.
🔴 Critical: This statement is incorrect
With InvertCollisionGroupFilter=True, assets not in any collision group will collide with everything — not just "everything in their environment". They bypass inter-env isolation entirely.
| Assets not assigned to any collision group follow default physics behavior and collide | |
| .. warning:: | |
| Assets not assigned to any collision group are not protected by inter-environment | |
| collision filtering. They may collide with assets in other environments. Assign all | |
| physics-enabled assets to at least one collision group when using this feature. |
AntoineRichard
left a comment
AntoineRichard
left a comment
There was a problem hiding this comment.
Both Newton and PhysX can work with collision groups, I don't think we should have a PhysX only option at this level. Or we should have errors thrown when using newton with this. But ideally, we should be able to make something that works regardless of the engine.
Plus shouldn't collision group be parsed by newton @camevor already?
| Fixed | ||
| ^^^^^ | ||
|
|
||
| * Fixed ``CreateShaderPrimFromSdrCommand`` call in :func:`~isaaclab.sim.spawners.materials.visual_materials.spawn_preview_surface` |
There was a problem hiding this comment.
The bot is correct!
| # finer-grained intra-env restrictions). | ||
| if self.cfg.collision_groups: | ||
| self._apply_collision_groups() | ||
| elif self.cfg.filter_collisions: |
There was a problem hiding this comment.
Agreeing with the bot here. At the very least a warning should be thrown.
| def _apply_collision_groups(self): | ||
| """Create USD PhysicsCollisionGroup prims for unified collision filtering. | ||
|
|
||
| This method replaces the cloner's ``filter_collisions`` when :attr:`InteractiveSceneCfg.collision_groups` | ||
| is configured. It creates a single, unified set of collision groups that handles both: | ||
|
|
||
| - **Inter-environment isolation**: each env's groups only reference same-env groups in | ||
| ``filteredGroups``, so prims in different environments never collide. | ||
| - **Intra-environment filtering**: within each environment, only groups listed in each | ||
| other's ``filteredGroups`` can collide (allowlist semantics). | ||
| - **Global prim collisions**: a dedicated global group (for ground plane, etc.) is created | ||
| and wired so all env groups can collide with it. | ||
|
|
||
| Raises: | ||
| ValueError: If an asset name in a collision group does not exist in the scene config. | ||
| ValueError: If a group name in ``collides_with`` does not exist in ``collision_groups``. | ||
| """ | ||
| collision_groups_cfg = self.cfg.collision_groups | ||
| if not collision_groups_cfg: | ||
| return | ||
|
|
||
| # -- Step 1: Collect all scene entity names and their prim paths | ||
| entity_prim_paths: dict[str, str] = {} | ||
| for asset_name, asset_cfg in self.cfg.__dict__.items(): | ||
| if asset_name in InteractiveSceneCfg.__dataclass_fields__ or asset_cfg is None: | ||
| continue | ||
| if hasattr(asset_cfg, "prim_path"): | ||
| entity_prim_paths[asset_name] = asset_cfg.prim_path | ||
|
|
||
| # -- Step 2: Validate config | ||
| group_names = list(collision_groups_cfg.keys()) | ||
| for group_name, group_cfg in collision_groups_cfg.items(): | ||
| # validate asset references | ||
| for asset_name in group_cfg.assets: | ||
| if asset_name not in entity_prim_paths: | ||
| available = list(entity_prim_paths.keys()) | ||
| raise ValueError( | ||
| f"Collision group '{group_name}' references asset '{asset_name}' which does not" | ||
| f" exist in the scene config. Available entities: {available}" | ||
| ) | ||
| # validate collides_with references | ||
| if group_cfg.collides_with is not None: | ||
| for ref_group in group_cfg.collides_with: | ||
| if ref_group not in collision_groups_cfg: | ||
| raise ValueError( | ||
| f"Collision group '{group_name}' lists '{ref_group}' in collides_with," | ||
| f" but no such group is defined. Available groups: {group_names}" | ||
| ) | ||
|
|
||
| # -- Step 3: Build collision matrix | ||
| # Two groups collide only when BOTH sides agree. A pair (A, B) collides iff: | ||
| # - A lists B (or A uses None = "all") AND B lists A (or B uses None = "all") | ||
| # This means collides_with=[] is always respected — no other group can force | ||
| # collisions onto an isolated group. | ||
| group_collides: dict[str, set[str]] = {} | ||
| for group_name in group_names: | ||
| group_collides[group_name] = set() | ||
|
|
||
| for i, name_a in enumerate(group_names): | ||
| cfg_a = collision_groups_cfg[name_a] | ||
| for name_b in group_names[i:]: | ||
| cfg_b = collision_groups_cfg[name_b] | ||
| # check if A accepts B | ||
| a_accepts_b = cfg_a.collides_with is None or name_b in cfg_a.collides_with or name_a == name_b | ||
| # check if B accepts A | ||
| b_accepts_a = cfg_b.collides_with is None or name_a in cfg_b.collides_with or name_a == name_b | ||
| if a_accepts_b and b_accepts_a: | ||
| group_collides[name_a].add(name_b) | ||
| group_collides[name_b].add(name_a) | ||
|
|
||
| # -- Step 4: Resolve prim paths per group for env_0 | ||
| group_env0_paths: dict[str, list[str]] = {name: [] for name in group_names} | ||
| for group_name, group_cfg in collision_groups_cfg.items(): | ||
| for asset_name in group_cfg.assets: | ||
| prim_path = entity_prim_paths[asset_name] | ||
| # resolve regex to env_0 path | ||
| env0_path = prim_path.replace(self.env_regex_ns, self.env_prim_paths[0]) | ||
| group_env0_paths[group_name].append(env0_path) | ||
|
|
||
| # -- Step 5: Ensure InvertCollisionGroupFilterAttr is set | ||
| physx_scene = PhysxSchema.PhysxSceneAPI(self.stage.GetPrimAtPath(self.physics_scene_path)) | ||
| invert_attr = physx_scene.GetInvertCollisionGroupFilterAttr() | ||
| if not invert_attr or not invert_attr.Get(): | ||
| physx_scene.CreateInvertCollisionGroupFilterAttr().Set(True) | ||
|
|
||
| # -- Step 6: Create USD collision group prims | ||
| collision_root = "/World/collisions" | ||
| has_global_paths = len(self._global_prim_paths) > 0 | ||
| global_group_path = f"{collision_root}/global_group" | ||
|
|
||
| # create the scope prim in the root layer | ||
| with Usd.EditContext(self.stage, Usd.EditTarget(self.stage.GetRootLayer())): | ||
| UsdGeom.Scope.Define(self.stage, collision_root) | ||
|
|
||
| with Sdf.ChangeBlock(): | ||
| root_spec = self.stage.GetRootLayer().GetPrimAtPath(collision_root) | ||
|
|
||
| # -- create global group for ground plane and other global prims | ||
| if has_global_paths: | ||
| global_group = Sdf.PrimSpec(root_spec, "global_group", Sdf.SpecifierDef, "PhysicsCollisionGroup") | ||
| global_group.SetInfo(Usd.Tokens.apiSchemas, Sdf.TokenListOp.Create({"CollectionAPI:colliders"})) | ||
|
|
||
| expansion_rule = Sdf.AttributeSpec( | ||
| global_group, | ||
| "collection:colliders:expansionRule", | ||
| Sdf.ValueTypeNames.Token, | ||
| Sdf.VariabilityUniform, | ||
| ) | ||
| expansion_rule.default = "expandPrims" | ||
|
|
||
| global_includes = Sdf.RelationshipSpec(global_group, "collection:colliders:includes", False) | ||
| for gpath in self._global_prim_paths: | ||
| global_includes.targetPathList.Append(gpath) | ||
|
|
||
| # filteredGroups for global group — will be populated below with all env groups | ||
| global_filtered = Sdf.RelationshipSpec(global_group, "physics:filteredGroups", False) | ||
| # global group collides with itself (e.g. multiple global prims can collide) | ||
| global_filtered.targetPathList.Append(global_group_path) | ||
|
|
||
| # -- create per-env collision groups | ||
| for env_idx, env_prim_path in enumerate(self.env_prim_paths): | ||
| for group_name in group_names: | ||
| prim_name = f"env{env_idx}_{group_name}" | ||
|
|
||
| # create PhysicsCollisionGroup prim | ||
| collision_group = Sdf.PrimSpec(root_spec, prim_name, Sdf.SpecifierDef, "PhysicsCollisionGroup") | ||
| collision_group.SetInfo(Usd.Tokens.apiSchemas, Sdf.TokenListOp.Create({"CollectionAPI:colliders"})) | ||
|
|
||
| # expansion rule | ||
| expansion_rule = Sdf.AttributeSpec( | ||
| collision_group, | ||
| "collection:colliders:expansionRule", | ||
| Sdf.ValueTypeNames.Token, | ||
| Sdf.VariabilityUniform, | ||
| ) | ||
| expansion_rule.default = "expandPrims" | ||
|
|
||
| # includes relationship — asset prim paths for this env | ||
| includes_rel = Sdf.RelationshipSpec(collision_group, "collection:colliders:includes", False) | ||
| for env0_asset_path in group_env0_paths[group_name]: | ||
| # replace env_0 path with this env's path | ||
| env_asset_path = env0_asset_path.replace(self.env_prim_paths[0], env_prim_path) | ||
| includes_rel.targetPathList.Append(env_asset_path) | ||
|
|
||
| # filteredGroups — same-env groups only (provides inter-env isolation) | ||
| filtered_groups = Sdf.RelationshipSpec(collision_group, "physics:filteredGroups", False) | ||
| for collide_group_name in group_collides[group_name]: | ||
| collide_prim_path = f"{collision_root}/env{env_idx}_{collide_group_name}" | ||
| filtered_groups.targetPathList.Append(collide_prim_path) | ||
|
|
||
| # allow collision with global group (ground plane, etc.) | ||
| if has_global_paths: | ||
| filtered_groups.targetPathList.Append(global_group_path) | ||
| # also let global group collide with this env group | ||
| global_filtered.targetPathList.Append(f"{collision_root}/{prim_name}") | ||
|
|
||
| logger.info( | ||
| f"Created collision groups at '{collision_root}'" | ||
| f" with {len(group_names)} groups across {len(self.env_prim_paths)} environments" | ||
| f" (global paths: {len(self._global_prim_paths)})." | ||
| ) |
There was a problem hiding this comment.
I don't think this belongs here. Could we add it to the IsaacLab utilities?
Yes, Newton parses both Given the collision-group based env separation mechanism used for PhysX, making use of filter pairs for more granular collision filtering would give a backend-agnostic mechanism with fine-grained intra-env filtering. |
|
Marking this as draft for now, please move it back to "ready" when the filtering is backend agnostic. |
|
@kellyguo11 @rdsa-nv looking at this it seems that this may be a bit tricky to implement and we may want to provide some guidance on how to implement this to make sure it can be easily expanded upon in the future. Can you evaluate the importance of this feature so that we can put it on the roadmap? |
3 participants
Description
Currently, there is only the option to add collision filter inter-envs, but no easy way to specify it intra-env. This PR adds such feature.
Fixes # (issue)
Type of change
Screenshots
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there