Support cloned mesh terrain spawning - #5407
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if destination_paths: | ||
| root_layer = stage.GetRootLayer() | ||
| with Sdf.ChangeBlock(): | ||
| for destination_path in destination_paths: | ||
| _ensure_prim_specs(root_layer, destination_path) | ||
| Sdf.CopySpec(root_layer, Sdf.Path(source_path), root_layer, Sdf.Path(destination_path)) |
There was a problem hiding this comment.
Sdf.CopySpec duplicates the raw SdfSpec data verbatim, including stored relationship targets. The visual and physics material bindings on mesh_prim are set to absolute paths ({source_path}/visualMaterial, {source_path}/physicsMaterial). After the copy, each destination_path/mesh prim still references the source material paths — the copied material specs under each destination_path/ are never actually bound and go unused. All terrain copies end up sharing the source prim's material by accident rather than by design. If a caller later tries to rebind individual materials, they will find the relationship pointing to the wrong (source) prim path.
| useEnvIds=(len(current_worlds) == num_envs - 1) and device != "cpu", | ||
| useFabricForReplication=use_fabric, |
There was a problem hiding this comment.
useEnvIds condition doesn't verify source is env_0
useEnvIds=True requires the source prim to live in environment 0 so PhysX's env-ID counter starts from zero and matches the expected index layout. The current guard only checks that the replication fan-out covers all num_envs - 1 other environments, but not which source env is doing the replicating. A source in env_2 that maps to every other env (e.g. current_worlds = [0, 1, 3]) also satisfies len == num_envs - 1 and would receive useEnvIds=True, potentially causing incorrect env-ID assignment inside PhysX.
A more precise check would also verify that the source prim's own env index is 0, e.g.:
useEnvIds=(
(len(current_worlds) == num_envs - 1)
and device != "cpu"
and int(self_id) == 0 # source is env_0
),| def spawn_from_mesh( | ||
| prim_path: str, | ||
| cfg: from_files_cfg.MeshFileCfg, | ||
| mesh: trimesh.Trimesh, | ||
| translation: tuple[float, float, float] | None = None, | ||
| orientation: tuple[float, float, float, float] | None = None, | ||
| **kwargs, | ||
| ) -> Usd.Prim: |
There was a problem hiding this comment.
spawn_from_mesh bypasses the @clone spawner contract
Every other spawner in this file (spawn_from_usd, spawn_from_urdf, spawn_from_mjcf) is decorated with @clone, which is the standard hook that drives regex-path expansion and honours SpawnerCfg.copy_from_source. spawn_from_mesh rolls its own path-resolution logic and skips the decorator entirely. As a result:
MeshFileCfg.copy_from_source(inherited fromSpawnerCfg) is silently ignored.- Any tooling or framework code that relies on the
@clonedecorator's side-effects won't apply to mesh spawning.
If the custom clone-via-Sdf.CopySpec approach is intentional (to avoid the Isaac Cloner dependency), it should be clearly documented on the function and the inherited copy_from_source field should either be overridden to False in MeshFileCfg or removed.
| root_parts = root_path.strip("/").split("/") | ||
| for prefix_len in range(len(root_parts), 0, -1): | ||
| prefix_path = "/" + "/".join(root_parts[:prefix_len]) | ||
| if not _is_regex_prim_path(prefix_path): | ||
| continue | ||
|
|
||
| source_parent_paths = find_matching_prim_paths(prefix_path) | ||
| if not source_parent_paths: | ||
| continue |
There was a problem hiding this comment.
The loop iterates from the longest prefix down to length 1, and continues whenever prefix_path has no regex characters. Because the non-regex prefix check at line 74 handles the all-plain case before the loop, the only time we reach the loop body is when at least one segment contains regex. However, as the loop shortens the prefix, it will eventually peel off the regex segments and be left with plain prefixes like /World/envs or /World — all of which are skipped. If find_matching_prim_paths returns an empty list for the first (longest, regex-containing) prefix, every subsequent iteration will continue and the function will always raise RuntimeError, even if a shorter regex prefix might have matched.
In practice this works for the standard env_.* usage because the full-length regex prefix is what matches stage prims, but the fallback iterations add no safety net.
f0f2f91 to
a0f659a
Compare
![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 MeshFileCfg and spawn_from_mesh for spawning meshes from file paths, in-memory trimesh.Trimesh objects, or raw triangle vertex/face data. It also refactors the terrain mesh importing to use this new spawner and fixes the @clone decorator to properly handle regex patterns in the middle of prim paths (e.g., /World/envs/env_.*/Terrain/Ground). Additionally, it restores PhysX env IDs for homogeneous GPU replication.
Architecture Impact
- Cross-module:
TerrainImporternow delegates toMeshFileCfginstead of the legacycreate_prim_from_meshhelper, consolidating mesh spawning logic. - Clone decorator: The
_resolve_clone_parent_pathsfunction changes how regex-containing paths are resolved, which affects all spawners using the@clonedecorator. This could impact existing code that relied on the previous behavior. - PhysX replication: Re-enabling
useEnvIds=Truefor homogeneous GPU rows may affect physics behavior in multi-env scenarios.
Implementation Verdict
Minor fixes needed
Test Coverage
Good coverage for the new MeshFileCfg spawner with tests for trimesh objects, triangle mesh data, file paths, regex cloning scenarios, and backward compatibility of create_prim_from_mesh. The PhysX replicate changes have a new test for env ID selection. Missing: edge case tests for normalize_vertex_colors with 0-255 range values and validate_triangle_mesh_data with malformed inputs.
CI Status
All tests pass except Build Latest Docs: failure which appears unrelated to this PR's changes (likely a docs infrastructure issue).
Findings
🟡 Warning: source/isaaclab/isaaclab/sim/spawners/from_files/from_files.py:64-67 — Potential silent failure when accessing trimesh visual colors
vertex_colors = mesh_source.mesh.visual.vertex_colorsIf the trimesh object has no visual data or uses a different visual type (e.g., TextureVisuals), accessing .visual.vertex_colors may return None or raise an AttributeError. Consider checking hasattr(mesh_source.mesh.visual, 'vertex_colors') or wrapping in a try-except to provide a clearer error message.
🟡 Warning: source/isaaclab/isaaclab/sim/utils/prims.py:56-68 — Regex pattern matching may miss valid prim path characters
The _VALID_PRIM_PATH_REGEX pattern r"^[a-zA-Z0-9/_]+$" doesn't account for hyphens (-) or dots (.) which are valid in USD prim names. This could cause legitimate prim paths containing these characters to be incorrectly treated as regex patterns.
🔵 Improvement: source/isaaclab/isaaclab/sim/spawners/from_files/from_files.py:282-286 — displayOpacity uses full RGBA channel but set expects float array
display_prim_attr.Set(vertex_colors[:, 3])The code correctly extracts the alpha channel, but the pattern of setting displayColor (RGB) and displayOpacity (A) separately is good. However, consider adding a comment clarifying that displayOpacity expects per-vertex floats, not RGBA tuples, for maintainability.
🔵 Improvement: source/isaaclab/isaaclab/utils/mesh.py:117-119 — Color normalization heuristic may misidentify float colors
if np.max(colors) > 1.0:
colors /= 255.0This heuristic assumes colors > 1.0 are in 0-255 range, but fractional colors like 1.5 (which are invalid) would be silently scaled. Consider either clamping after normalization or raising a warning for unexpected ranges.
🔵 Improvement: source/isaaclab/isaaclab/sim/spawners/from_files/from_files.py:221-231 — USD file extension check could be consolidated
The extension check uses a tuple _USD_FILE_EXTENSIONS which is good, but the os.path.splitext(mesh_path)[1].lower() pattern appears only here. Consider extracting a helper like is_usd_file(path) for clarity and potential reuse.
🟡 Warning: source/isaaclab/isaaclab/sim/utils/prims.py:719-722 — Intermediate prim creation may conflict with existing prims
for path_part in dest_path.strip("/").split("/"):
current_path = f"{current_path}/{path_part}"
Sdf.CreatePrimInLayer(rl, current_path)If an intermediate prim already exists with a different type, Sdf.CreatePrimInLayer will overwrite it. This is unlikely in normal usage but could cause subtle bugs if the user has pre-existing prims along the path with specific types. Consider checking stage.GetPrimAtPath(current_path).IsValid() before creating.
🔴 Critical: source/isaaclab/isaaclab/terrains/utils.py:78-93 — Return type change is a breaking API change
The create_prim_from_mesh function now returns the result of mesh_cfg.func() (a Usd.Prim) instead of implicitly returning None. While this is technically an improvement, any code that explicitly checked for None return or didn't expect a return value could be affected. The function previously returned nothing; now it returns a prim. This should be documented in the changelog as a behavior change.
🔵 Improvement: source/isaaclab_physx/isaaclab_physx/cloner/physx_replicate.py:105 — Magic number comparison for homogeneous detection
useEnvIds=(len(current_worlds) == num_envs - 1) and device != "cpu",The num_envs - 1 comparison assumes self-exclusion always removes exactly one environment. This is correct given the self-exclusion logic above, but a comment explaining this invariant would aid future maintainers. Consider: # num_envs - 1 because self is excluded for homogeneous rows.
a0f659a to
5a5a561
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Follow-up Review
The new commits add a test for test_physx_replicate_env_id_selection that verifies the useEnvIds flag behavior across different device/mapping scenarios - this addresses the previous concern about the magic number comparison by providing explicit test coverage for the homogeneous detection logic.
Previous findings remain unaddressed:
- The
_VALID_PRIM_PATH_REGEXstill doesn't account for hyphens/dots in prim names - The trimesh visual.vertex_colors access could still fail silently for non-ColorVisuals types
- The
create_prim_from_meshreturn type change is not documented as a breaking change in the changelog (the changelog says "Updated terrain mesh importing" but doesn't mention the return value change forcreate_prim_from_mesh)
No new issues introduced by the latest commit. The test coverage improvement for PhysX replication is good.
Implementation Verdict
Ship it (with minor documentation note: consider adding a note about create_prim_from_mesh now returning a prim to the changelog's "Changed" section)
Add MeshFileCfg and spawn_from_mesh so terrain and object code can spawn mesh file paths, trimesh objects, or generated triangle mesh data through one spawner. Keep collision opt-in while allowing callers to choose triangle, convex, or SDF mesh collision approximations. Update clone resolution to materialize concrete suffixes under regex matched parents for generated terrain meshes, and restore PhysX env IDs for full homogeneous GPU replication rows.
The conda CDN started returning 403 to lychee's user-agent some time after Apr 27 2026, blocking any PR that touches an .rst file. The URLs themselves resolve fine in browsers; only the bot fetches are rejected. Skip them from the link check until lychee or the CDN cooperates.
5a5a561 to
6c90366
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Follow-up Review
The new commit adds test_physx_replicate_env_id_selection which provides explicit test coverage for the useEnvIds flag behavior across CPU/GPU and partial/full mapping scenarios. This addresses the previous concern about the magic number comparison by demonstrating the intended behavior through tests.
Previous findings remain unaddressed but are minor:
_VALID_PRIM_PATH_REGEXhyphen/dot handling- Trimesh
visual.vertex_colorstype safety create_prim_from_meshreturn type change not documented
No new issues introduced by the latest commit.
Implementation Verdict
Ship it
|
Hi @ooctipus — thanks for putting this one up! 🙏 We're doing a cleanup pass over the Isaac Lab PR backlog, which had grown past 400 open pull requests, and we're closing out the ones that have gone quiet so the queue is reviewable again. Why this PR is being closed: Here is exactly what we found on this PR when we reviewed the backlog:
It was picked up by the sweep because it has been open for about 4 months. It was then put in the "close" bucket because the author has been silent for about 4 months — which is the signal we used to tell apart pull requests that are still being worked on from ones that have genuinely been set aside. We deliberately did not close pull requests that were approved and ready to land, or that were small and clearly still fixing a live bug — there were 27 of those, and we are merging them rather than closing them. No judgement on the change itself — this is purely backlog hygiene. If this is still wanted, please reopen it or re-submit against 🤖 This comment was drafted with AI assistance as part of a maintainer-led sweep of the Isaac Lab pull request backlog. A maintainer is behind this cleanup — but if this closure looks wrong, it may well be, so please push back and we'll take another look. |
2 participants
Description
Adds
MeshFileCfgandspawn_from_meshwith onemeshfield for mesh paths, in-memorytrimesh.Trimeshobjects, or triangle vertex and face data. Collision stays opt-in: terrain uses triangle-mesh collision explicitly, while object users can choose convex, SDF, or other mesh collision approximations with rigid-body and mass properties.Also updates generated terrain mesh spawning under regex environment roots and restores PhysX env IDs for full homogeneous GPU replication rows.
No linked issue.
Type of change
Screenshots
Not applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there