Unify primitive mesh refinement controls - #7357
Conversation
Greptile SummaryThe PR unifies primitive mesh surface subdivision around
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking issue identified. The common refinement path validates the documented lower bound, applies subdivision consistently to primitive meshes, preserves direct schema-call defaults, and forwards the new tetrahedralization factor only on the existing volume-deformable path. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
C[MeshCfg.edge_refinement] --> S[Primitive surface mesh]
S --> R[subdivide_to_size using bbox diagonal / refinement]
R --> D{Deformable?}
D -- No --> U[Write USD mesh]
D -- Surface --> P[Apply surface deformable properties]
D -- Volume --> T[Automatic tetrahedralization using 1 / refinement]
P --> U
T --> U
Reviews (1): Last reviewed commit: "Refine Franka soft lift cuboid mesh" | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The shared edge_refinement implementation is internally consistent, but the PR removes the public MeshRectangleCfg.resolution API without deprecation and does not document the changed primitive defaults as breaking changes with migration guidance in the changelog.
- Design and architecture: Centralizing primitive subdivision in
_spawn_mesh_geom_from_meshand forwarding the normalized target to volume tetrahedralization is coherent. However, the public rectangle configuration transition must retain a deprecation path rather than deletingresolutionimmediately. - API:
MeshCfg.edge_refinementis documented and validated, and direct callers ofdefine_deformable_body_propertiesretain the existing0.1default. Existing rectangle callers usingresolutionwill now fail during configuration construction, contrary to the repository rule requiring deprecation before public API removal. The changed surface-refinement and tetrahedralization defaults also need a major changelog entry with explicit migration guidance. - Implementation: The changed spawner path consistently applies subdivision across primitive meshes and forwards
1.0 / edge_refinementonly for volume deformables. The required fixes are compatibility handling forMeshRectangleCfg.resolutionand release-note coverage for the changed defaults; no separate implementation defect was established.
Minor fixes needed. Posted 2 actionable findings inline.
Automated review; human maintainers own approval decisions.
| Added | ||
| ^^^^^ | ||
|
|
||
| * Added ``MeshCfg.edge_refinement``, defaulting to ``4.0``, to control surface mesh resolution for all primitives and |
There was a problem hiding this comment.
🟡 Warning · Api — Default behavior changes filed as minor addition
This fragment records only the new field, but the change also alters existing behavior: cuboid refinement default moves 1.0 -> 4.0, all other primitives gain subdivision they never had, and primitive volume deformables now tetrahedralize at 0.25 instead of 0.1. Unmodified user configs silently produce different render and simulation meshes. Record these default changes in a major fragment with migration guidance (e.g. set edge_refinement=1.0).
| If None, then no physics material will be added. | ||
| """ | ||
|
|
||
| edge_refinement: float = 4.0 |
There was a problem hiding this comment.
🟡 Warning · Api — Public resolution field removed without deprecation
Introducing edge_refinement here deletes MeshRectangleCfg.resolution, so existing configurations passing resolution=(nx, ny) now fail at construction with a TypeError. Repository guidelines forbid removing a public API without a prior deprecation and migration path. Keep resolution for one release, warn when it is set, and map it onto edge_refinement (with defined precedence) before deleting it.
Rigid mesh primitives gained nothing from surface subdivision: subdivide_to_size splits triangles linearly, so it only inserts coplanar vertices and leaves the surface, and any collision approximation of it, unchanged. Measured on cuboid, cylinder, cone and capsule, the convex hull volume is identical to machine precision and every new vertex lies on the original surface to within 1e-17 m. The default factor of 4.0 was therefore costing a 10-12x vertex count on every rigid primitive for no benefit. Gate the subdivision on deformable_props so rigid primitive meshes stay identical to develop. The range validation stays unconditional.
Rebasing onto develop pushed _spawn_mesh_geom_from_mesh past the C901 complexity limit (31 > 30), since develop grew the collision-fragment handling in the same function. Move the edge-refinement validation and subdivision into a helper to bring it back under the cap.
87e1398 to
791fcc6
Compare
|
run-ci |
|
rendering test seems to have failed twice - do we need a golden image update for this? |
Ah yes that could very well be! I changed the default resolution so maybe there is some slight differences. Let me regenerate them! |
Replacing MeshRectangleCfg.resolution with edge_refinement keeps the cloth mesh geometrically identical, 81 vertices at the same positions, but flips 32 of the 64 quad diagonals: the removed grid generator alternated them, while subdivide_to_size inherits one direction from its seed triangles. That slightly changes the cloth's bending response, so motion_vectors is the only affected data type, since it is the only one captured after stepping the environment. It scored 3.32% against a 3.00% threshold, identically on both CI shards. Every static data type still matches, because the rest geometry is unchanged. The new image is the reference output from the CI run for this branch.
|
run-ci |
|
Backported to |
## Description - Add `MeshCfg.edge_refinement`, defaulting to `4.0`, to control surface subdivision for all primitive mesh spawners using a bounding-box-diagonal target. - Forward `1.0 / edge_refinement` to automatic tetrahedralization for volume deformables spawned through `MeshCfg` primitive spawners. Direct callers of `define_deformable_body_properties` retain its existing default factor of `0.1`. - Document that values near `1.0` should be avoided for volume deformables because they can make TetWild tetrahedralization significantly slower. - Replace `MeshRectangleCfg.resolution` with scalar `edge_refinement`. This removes independent per-axis resolution control, and subdivision counts can be discontinuous for non-power-of-two factors. - Migrate existing cloth callsites and examples, and set the Franka soft-lift cuboid refinement to `8.0`. This is a breaking change. Unconfigured primitive surfaces now use an edge-refinement factor of `4.0`, and unconfigured volume primitive spawners use a tetrahedralization factor of `0.25` instead of `0.1`. Rectangles now use the common recursive edge-length subdivision instead of the previous per-axis grid. ## Type of change - [x] Breaking change - [x] New feature ## Release backport - [x] <!-- backport-active-release --> Backport this pull request to the active release branch after it merges into `develop` ## Testing - [x] Ruff, format, compilation, and diff checks - [x] Mesh-spawner integration suite: 21 tests - [x] Newton surface-deformable initialization and freefall integration test - [x] PhysX deformables demo: completed setup and simulation - [x] Franka cloth environment: 20 steps each on Newton, PhysX, and OvPhysX - [x] Franka cloth camera environment: 5 PhysX/RTX steps with image observations - [x] Scripted Franka cloth lift: 50 steps each on Newton and PhysX - [x] Franka soft lift with refinement `8.0`: 10 steps each on Newton and Isaac Sim PhysX - [x] Seed-42 RSL-RL scratch smoke at 2,048 environments: 55 soft updates and 11 cloth updates with finite losses - [x] Seed-42 matched checkpoint continuation at 2,048 environments: 52 updates each on develop and the PR mesh; final-20 success was 67.1% versus 81.5% for soft and 85.3% versus 87.6% for cloth - [x] TetWild timing check for all five volume primitives at the default factor of `0.25` PhysX reports an invalid surface-deformable-view warning in the demo and cloth environment. The same warning is present on untouched `develop` with the same runtime and is not introduced by this change. ## Checklist - [x] Changelog fragments added - [x] Existing callsites migrated (cherry picked from commit 3fbbd15)
Description
MeshCfg.edge_refinement, defaulting to4.0, to control surface subdivision for all primitive mesh spawners using a bounding-box-diagonal target.1.0 / edge_refinementto automatic tetrahedralization for volume deformables spawned throughMeshCfgprimitive spawners. Direct callers ofdefine_deformable_body_propertiesretain its existing default factor of0.1.1.0should be avoided for volume deformables because they can make TetWild tetrahedralization significantly slower.MeshRectangleCfg.resolutionwith scalaredge_refinement. This removes independent per-axis resolution control, and subdivision counts can be discontinuous for non-power-of-two factors.8.0.This is a breaking change. Unconfigured primitive surfaces now use an edge-refinement factor of
4.0, and unconfigured volume primitive spawners use a tetrahedralization factor of0.25instead of0.1. Rectangles now use the common recursive edge-length subdivision instead of the previous per-axis grid.Type of change
Release backport
developTesting
8.0: 10 steps each on Newton and Isaac Sim PhysX0.25PhysX reports an invalid surface-deformable-view warning in the demo and cloth environment. The same warning is present on untouched
developwith the same runtime and is not introduced by this change.Checklist