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: |
| 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` |
| 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. |
1 participant
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