Decouple physics-engine event terms into per-backend modules#5915
Decouple physics-engine event terms into per-backend modules#5915ooctipus wants to merge 1 commit into
Conversation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Follow-up Review for ce76b255 (split events)
This commit completes the refactoring by splitting physics event terms into per-backend modules. The architecture is now clean and extensible.
✅ What Works Well
-
Clean backend resolution via
_resolve_backend_event()- The dynamic import pattern with substring matching for manager names handles all backend variants gracefully (Newton, OVPhysX, PhysX) -
Proper re-export structure - Each backend owns its implementations:
isaaclab_physx.envs.mdp.physics_events- Full PhysX implementationsisaaclab_newton.envs.mdp.physics_events- Newton-specific implementationsisaaclab_ovphysx.envs.mdp.physics_events- Reuses PhysX + no-op material randomization
-
Helper functions properly exposed -
randomize_prop_by_op()andvalidate_scale_range()are now public inphysics_events.pyfor backend implementations to import -
Changelog entries are complete - All packages (isaaclab, isaaclab_experimental, isaaclab_newton, isaaclab_ovphysx, isaaclab_physx) have proper changelog fragments
-
Type hints preserved - The
__init__.pyicorrectly imports from botheventsandphysics_eventsmodules
🔍 Items Requiring Attention
-
OVPhysX module missing
__all__export consistencyisaaclab_ovphysx.envs.mdp.physics_eventsdefines__all__but imports fromisaaclab_physx- The re-exported classes like
RandomizeActuatorGainsare imported but the module structure should verify they are actually accessible viafrom isaaclab_ovphysx.envs.mdp import RandomizeActuatorGains
-
randomize_rigid_body_scaleduplication- The function exists in both
physics_events.py(core module with deprecation warning) ANDisaaclab_physx.envs.mdp.physics_events.RandomizeRigidBodyScale - The Newton backend raises
NotImplementedErrorfor this term - Consider removing the PhysX class implementation since the core function already handles USD authoring (backend-agnostic)
- The function exists in both
-
Module import path in
_resolve_backend_event()module = importlib.import_module(f"isaaclab_{backend}.envs.mdp") impl_cls = getattr(module, impl_name)
- This assumes the class is exposed at the
envs.mdplevel, but the implementations are inenvs.mdp.physics_events - Verify that
isaaclab_<backend>/envs/mdp/__init__.pyre-exports these classes
- This assumes the class is exposed at the
-
Test import path updates
- The test files correctly updated the import from
isaaclab.envs.mdp.eventstoisaaclab.envs.mdp.physics_events - ✅ This is correct since the public API is now via
physics_events
- The test files correctly updated the import from
📝 Minor Suggestions
-
Docstring for
_resolve_backend_event()- Consider adding a note about the import resolution order (ovphysx before physx due to substring matching) -
Changelog version consistency -
isaaclab_experimentalbumped to0.1.0which is a minor version bump. Verify this aligns with the semantic versioning policy for the other packages
Overall: The refactoring is well-structured and follows the backend abstraction pattern used elsewhere in IsaacLab. The main concern is verifying the import paths actually work at runtime - the _resolve_backend_event() function imports from isaaclab_{backend}.envs.mdp but the implementations are in the physics_events submodule. Please confirm the __init__.py files in each backend package re-export these classes.
Greptile SummaryThis PR decouples physics-engine-specific event terms from the generic
Confidence Score: 3/5Two real behavioral defects need fixing: gravity randomization on PhysX silently falls back to uniform regardless of configuration, and the Newton collider-offset implementation permanently overwrites stored defaults on each call. The PhysX gravity implementation hardcodes distribution='uniform' inside randomize_prop_by_op, discarding any configured distribution. Newton's RandomizeRigidBodyColliderOffsets mutates self.default_margin in-place, making repeated resets over disjoint env_ids subsets accumulate drift. source/isaaclab_physx/isaaclab_physx/envs/mdp/physics_events.py (gravity distribution) and source/isaaclab_newton/isaaclab_newton/envs/mdp/physics_events.py (collider default margin mutation) Important Files Changed
Reviews (1): Last reviewed commit: "Decouple physics-engine event terms into..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| gravity = randomize_prop_by_op( | ||
| gravity, | ||
| (self._dist_param_0.cpu(), self._dist_param_1.cpu()), | ||
| None, | ||
| slice(None), | ||
| operation=operation, | ||
| distribution="uniform", | ||
| ) | ||
| gravity = gravity[0].tolist() | ||
| self._physics_sim_view.set_gravity(self._carb.Float3(*gravity)) |
There was a problem hiding this comment.
randomize_prop_by_op is called with a hardcoded distribution="uniform", so any value configured in cfg.params["distribution"] (e.g. "log_uniform" or "gaussian") is silently discarded. Newton's RandomizePhysicsSceneGravity.__init__ correctly caches the configured distribution function in self._dist_fn, but the PhysX implementation never stores it. A user who sets distribution="gaussian" on PhysX will silently get uniform sampling with no error or warning.
| ) | ||
| self.default_margin[env_ids] = margin[env_ids] | ||
| margin_view[env_ids] = margin[env_ids] |
There was a problem hiding this comment.
default_margin mutated across calls, diverging from original defaults
self.default_margin[env_ids] = margin[env_ids] permanently overwrites the stored defaults. On the next reset for a different env_ids subset, those environments' default margin is the previously randomized value rather than the original scene default. The PhysX counterpart always clones from self.default_rest_offsets (never mutated). Because the operation is currently hardcoded to "abs" the mutation does not affect output values today, but would silently break if "add" or "scale" support is added.
| OVPhysX reuses the PhysX implementations for most event terms (see the ``ovphysx -> physx`` | ||
| fallback in :class:`~isaaclab.envs.mdp.physics_events._EventTermFactory`); only terms that need a | ||
| different OVPhysX behavior are defined here. |
There was a problem hiding this comment.
_EventTermFactory, a class that does not exist in the codebase. The runtime routing is the module-level function _resolve_backend_event in isaaclab.envs.mdp.physics_events.
| OVPhysX reuses the PhysX implementations for most event terms (see the ``ovphysx -> physx`` | |
| fallback in :class:`~isaaclab.envs.mdp.physics_events._EventTermFactory`); only terms that need a | |
| different OVPhysX behavior are defined here. | |
| OVPhysX reuses the PhysX implementations for most event terms (see the ``ovphysx -> physx`` | |
| fallback in :func:`~isaaclab.envs.mdp.physics_events._resolve_backend_event`); only terms that need a | |
| different OVPhysX behavior are defined here. |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| warnings.warn( | ||
| "'randomize_rigid_body_scale' is deprecated and only supported on the PhysX backend. Prefer" | ||
| " multi-asset spawning (see :class:`~isaaclab.sim.MultiAssetSpawnerCfg`) with per-scale USD" | ||
| " variants instead.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| # the USD scale authoring is backend-specific; resolve the active backend's implementation | ||
| _resolve_backend_event("RandomizeRigidBodyScale")(env, env_ids, scale_range, asset_cfg, relative_child_path) |
There was a problem hiding this comment.
On Newton, randomize_rigid_body_scale first emits a DeprecationWarning saying the function is "only supported on the PhysX backend", then immediately raises NotImplementedError. Users on Newton see the deprecation first, implying it could work if they switch backends, before hitting an uncaught error. The Newton path should skip the PhysX-flavored deprecation warning and raise NotImplementedError directly.
ooctipus
force-pushed
the
zhengyuz/decouple-physics-events
branch
4 times, most recently
from
527813b to
1f45c56
Compare
ooctipus
force-pushed
the
zhengyuz/decouple-physics-events
branch
from
1f45c56 to
ce76b25
Compare
1 participant
Summary
isaaclab.envs.mdp.eventsinto a newisaaclab.envs.mdp.physics_eventsmodule:randomize_rigid_body_material,randomize_rigid_body_scale,randomize_rigid_body_mass,randomize_rigid_body_inertia,randomize_rigid_body_com,randomize_rigid_body_collider_offsets,randomize_physics_scene_gravity,randomize_joint_parameters,randomize_fixed_tendon_parameters,randomize_actuator_gains(plus the sharedrandomize_prop_by_op/validate_scale_rangehelpers).events.pynow only holds backend-agnostic terms (resets, force/velocity, visual material/color).isaaclab_<backend>.envs.mdp.physics_eventsfor PhysX, Newton, and OVPhysX. The core public terms resolve the active backend's implementation at runtime via a small private resolver, soisaaclab.envs.mdp.events/physics_eventsno longer importpxror any backend.randomize_rigid_body_scale: it remains PhysX-only (Newton raisesNotImplementedError) and emits aDeprecationWarningrecommending multi-asset spawning (MultiAssetSpawnerCfg).envs/mdppackages follow the lazy__init__.py+.pyistub convention.Test plan
./isaaclab.sh -p -m pytest source/isaaclab_physx/test/assets/test_newton_actuators_physx.py./isaaclab.sh -p -m pytest source/isaaclab_newton/test/assets/test_newton_actuators_newton.pyrandomize_rigid_body_material/randomize_joint_parameterson both the PhysX and Newton backends and confirm parity with the previous behavior.randomize_rigid_body_scaleraises on Newton and warns (deprecated) on PhysX.