Stop the Newton cloner from approximating mesh colliders - #6892
Conversation
Newton replication ran a blanket convex-hull pass over every mesh collider, so a USD-authored physics:approximation was discarded the moment the asset was cloned. Assembly geometry -- threads, chamfered holes, anything concave that matters -- cannot survive that, and the only escape hatch was a scene-wide simplify_meshes flag that turned approximation off for every collider at once. Approximation belongs on the asset, not in the cloner. Drop the pass and the flag: the cloner now imports colliders as authored, and a config that wants a convex hull asks for it per collider through the new CollisionBaseCfg.mesh_collision_property, which reaches file-spawned USD assets that expose no approximation knob otherwise.
|
#6891 (Factory contact-rich assembly task on Newton) is stacked on this PR and needs it merged first. |
Greptile SummaryThis PR delegates mesh-collider approximation to Newton’s USD importer, adds per-collider approximation configuration, and removes the scene-wide
Confidence Score: 4/5The PR should not merge until heterogeneous per-world source mappings are validated or normalized before SolverMuJoCo initialization. The change removes the only visible enforcement of the solver’s homogeneous-world shape-sequence requirement while allowing differently approximated source builders to flow into the combined model. Files Needing Attention: source/isaaclab_newton/isaaclab_newton/cloner/newton_clone_utils.py and source/isaaclab_newton/test/cloner/test_collision_approximation.py Important Files Changed
Reviews (1): Last reviewed commit: "Stop the Newton cloner from approximatin..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| USD parse time and memory that only pays off when the shapes are rendered | ||
| or ray cast. | ||
| """ | ||
| authored = _authored_collision_approximations(stage) | ||
| builders = { | ||
| source: _build_source_builder( | ||
| stage, source, create_builder, schema_resolvers, ignore_paths, simplify_meshes, authored, load_visual_shapes | ||
| ) | ||
| return { |
There was a problem hiding this comment.
When a replication mapping uses per-world alternatives whose authored approximations produce different shape sequences, these builders now flow unchanged into the combined model, violating SolverMuJoCo's homogeneous-world requirement and causing solver initialization or simulation failure.
![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 cloner now delegates collision approximation to Newton’s USD importer and adds per-collider authoring through CollisionBaseCfg.mesh_collision_property. The implementation is covered by targeted approximation tests, but the public simplify_meshes configuration and keywords are removed without the required deprecation cycle.
- Design and architecture: Moving collision approximation ownership from the cloner to authored asset properties and the USD importer is a coherent separation of responsibilities. The reviewed patch provides a per-collider configuration path rather than retaining scene-wide remeshing behavior.
- API: The new
CollisionBaseCfg.mesh_collision_propertyfield exposes the existing mesh-collision dispatch path. However, removingNewtonCfg.simplify_meshesand the corresponding keywords fromNewtonReplicateContext,newton_physics_replicate, andbuild_source_buildersimmediately breaks existing configurations and callers, contrary to the repository requirement that public APIs be deprecated in a prior release. These interfaces should remain temporarily as deprecated, accepted-but-ignored shims with migration guidance. - Implementation: The importer path consistently uses
add_usd(skip_mesh_approximation=False), while existing visible-collider restoration and shape-color replacement remain in place. Tests cover authored approximation modes, unauthored meshes, mixed stages, differing source shape types, and SDF colliders. The remaining required implementation work is compatibility handling for the removedsimplify_meshesinputs.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| self.device = device | ||
| self.up_axis = up_axis | ||
| if simplify_meshes is None or load_visual_shapes is None: | ||
| if load_visual_shapes is None: |
There was a problem hiding this comment.
🟡 Warning · Api — Public simplify_meshes removed without deprecation
NewtonCfg.simplify_meshes and the simplify_meshes keyword on NewtonReplicateContext.__init__, newton_physics_replicate, and build_source_builders are deleted in one release, so existing configs and callers now fail with unknown-field or TypeError errors. Repository rules require deprecating public symbols in a prior release. Keep the field and keyword for one release as accepted-but-ignored shims that emit a deprecation warning pointing at mesh_collision_property.
Dropping the cloner's blanket approximation pass leaves every collider at its USD-authored approximation, and USD defaults that to none. A sweep of all 132 registered tasks, comparing each collider's resolved Newton shape before and after, found three tasks whose colliders changed: the lift multi-asset object and the reorient dex cube, both spawned as tessellated mesh primitives with no approximation authored. Author convexHull on their shared collision props, which the multi-asset spawner propagates to every variant, restoring the shapes those tasks resolved before.
The MPM particle-pour demo passed simplify_meshes=False to keep its thin colliders as exact triangle meshes. That is now the cloner's only behavior, so the argument is gone and the demo failed to construct its sim cfg at all. test_valid_properties_cfg asserts every field on a properties cfg is set, which the new optional mesh_collision_property breaks: None is its meaningful value, meaning "leave the USD-authored approximation alone". Exclude it there, and from the prim-attribute validator that would otherwise look for a physxCollision attribute named after a nested cfg.
this seems incorrect in the rendering-correctness test
|
The Lift object preset has two variants: a multi-asset spawner of Mesh* primitives, and a plain CuboidCfg that spawns an analytic UsdGeom.Cube. Only the former is a mesh, so only the former was ever hulled by the cloner -- authoring a mesh-collision approximation on the cube claims a property the prim cannot have. The collider sweep confirms it was doing nothing: the tasks the hull is meant to protect resolve identical shapes without it.
9f4260e to
1413c0d
Compare
#6896 is also failing, this doesn't look like this pr's issue |
2 participants
Description
Newton replication ran a blanket convex-hull pass over every mesh collider, so a USD-authored
physics:approximationwas discarded the moment the asset was cloned. Assembly geometry — threads, chamfered holes, anything concave that matters — cannot survive that, and the only escape hatch wasNewtonCfg.simplify_meshes, a scene-wide flag that turned approximation off for every collider at once.Approximation belongs on the asset, not in the cloner. This PR drops the pass and the flag: the cloner imports colliders as authored (
newton.ModelBuilder.add_usd(skip_mesh_approximation=...)), and a config that wants a convex hull asks for it per collider instead of getting one imposed.Changes
isaaclab_newtonnewton_clone_utils.py— the cloner no longer remeshes anything. Removes thephysics:approximation→ remeshing-method table and the two passes built on it.replicate.py,newton_manager_cfg.py,visualization_builder.py—simplify_meshesplumbing removed.test/cloner/test_collision_approximation.py— covers each authored mode (convexDecomposition,boundingSphere,boundingCube,meshSimplification,none) mapping to its shape type, an unauthored mesh staying a trimesh, mixed stages, and sources that resolve to different shape types being left alone.isaaclabCollisionBaseCfg.mesh_collision_property— the replacement mechanism. A file-spawned USD asset exposes no approximation knob throughcollision_props, so without this there is no way to author an approximation from a spawner config once the cloner stops doing it. The dispatch intomodify_mesh_collision_propertiesalready existed inschemas.pybehind agetattr; this declares the field that feeds it.isaaclab_taskslift_env_cfg.py,reorient_manager_env_cfg.py— authorconvexHullon the two assets that were relying on the implicit hulling (see the audit below).Collider audit: every task, before vs after
Rather than reason about which assets were affected, every registered task was launched at
num_envs=1on both revisions and each colliding shape's resolved NewtonGeoTypewas dumped and diffed.develop)okon both sides)Uncovered: 35 physx-only tasks, 8 with no
sim.physics, 15 camera/visuomotor variants whose non-render siblings were probed, 5 pre-existing Newton limitations (surface grippers, reversed gripper joints), 1 missing optional tetrahedralization dependency.Three tasks changed, all
CONVEX_MESH → BOXon the same prim:Isaac-Lift-KukaAllegro,Isaac-Reorient-Franka,Isaac-Reorient-KukaAllegro—/World/envs/env_0/Object/geometry/meshBoth root causes are tessellated mesh primitives that authored no approximation:
lift_env_cfg.ObjectCfgis aMultiAssetSpawnerCfgofMeshCuboidCfg/MeshSphereCfg/MeshCapsuleCfg/MeshConeCfgvariants sharing onecollision_props, and the Reorient object is the dex cube. Authoringmesh_collision_property=MeshCollisionPropertiesCfg(mesh_approximation_name="convexHull")on the shared props — which the multi-asset spawner propagates to every variant — restores all three to byte-identical collider dumps.Four other tasks (
IsaacContrib-Stack-Cube-SO101-{v0,IK-Abs-v0,Joint-Teleop-v0},IsaacContrib-Stack-Cube-Bin-Franka-IK-Rel-Mimic) showed shape-count deltas, but a same-revision control run reproduced the same spread with no code change at all — SO101 gave 2305 vs 2287 convex meshes on two runs ofdevelop, the bin 72 vs 70. Their convex decomposition is nondeterministic run to run and the observed deltas sit inside that. A control re-run ofIsaac-Reorient-Frankawas byte-identical, confirming the control discriminates.One measurement caveat: at
num_envs=1a multi-asset spawner instantiates one of its variants, so the sweep witnessed one shape per Lift task rather than all sixteen. The fix is on the shared props that propagate to every variant.Type of change
Note on the breaking change
NewtonCfg.simplify_meshesis removed outright rather than deprecated first, which departs from the deprecation rule inAGENTS.md. The field only ever selected between "approximate everything" and "approximate nothing"; a deprecated shim that still forced convex hulls would keep the geometry-destroying path alive for another release, and any scene that wants convex hulls can now express that per collider. Happy to add a shim instead if reviewers prefer.Out-of-tree scenes that relied on the implicit hulling and author no approximation will now load raw trimeshes. USD defaults
physics:approximationtonone, so the fix for those assets is the same one applied to Lift and Reorient here: author the approximation they actually want.Screenshots
n/a
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched packageCONTRIBUTORS.mdor my name already exists there