Decouple physics-engine event terms into per-backend modules#5915
Decouple physics-engine event terms into per-backend modules#5915ooctipus wants to merge 3 commits 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.
Code Review: Decouple physics-engine event terms into per-backend modules
Thanks for the clean architecture to decouple physics-specific event terms into per-backend modules, @ooctipus. The runtime resolver pattern via _resolve_backend_event is a solid approach for keeping the core package backend-agnostic. Below are findings from reviewing the diff:
🔴 High Severity
1. _resolve_backend_event uses startswith which is less resilient than the original substring matching
File: source/isaaclab/isaaclab/envs/mdp/physics_events.py (lines ~56-63)
The resolver uses manager_name.startswith("newton"), startswith("ovphysx"), startswith("physx") to detect the backend. In the original events.py, detection used substring matching ("newton" in manager_name, "physx" in manager_name) and explicit equality for OVPhysX.
Current Newton subclass managers (e.g. NewtonMJWarpManager → newtonmjwarpmanager) start with "newton", so this works. But any future PhysX variant whose class name doesn't start with "physx" (e.g. FastPhysXManager → fastphysxmanager) would fail with ValueError. The original substring approach was more resilient.
Suggested fix:
if "newton" in manager_name:
backend = "newton"
elif "ovphysx" in manager_name:
backend = "physx" if ovphysx_uses_physx else "ovphysx"
elif "physx" in manager_name:
backend = "physx"
else:
raise ValueError(f"Unknown physics manager: {manager_name!r}")Note: ordering matters — "ovphysx" contains "physx", so test "ovphysx" before "physx".
2. PhysX RandomizePhysicsSceneGravity hard-codes distribution="uniform", silently ignoring the user's distribution parameter
File: source/isaaclab_physx/isaaclab_physx/envs/mdp/physics_events.py (~line 247)
gravity = randomize_prop_by_op(
gravity,
(self._dist_param_0.cpu(), self._dist_param_1.cpu()),
None,
slice(None),
operation=operation,
distribution="uniform", # ← hard-coded, ignores user's `distribution` param
)The distribution parameter is accepted in __call__ but never propagated. The Newton implementation correctly caches self._dist_fn at init and uses it. This is a silent correctness bug: users requesting "log_uniform" or "gaussian" gravity randomization on PhysX will get uniform sampling without any warning.
Suggested fix: Cache the distribution name/function at init and use it in the call, or at minimum pass the distribution parameter through.
🟡 Medium Severity
3. Renamed helper functions may break downstream code importing the private API
File: source/isaaclab/isaaclab/envs/mdp/physics_events.py
The helpers _randomize_prop_by_op → randomize_prop_by_op and _validate_scale_range → validate_scale_range have been renamed (underscore prefix removed) for their new role as shared utilities. Any code that imported these from isaaclab.envs.mdp.events will break.
While these were private APIs (underscore-prefixed), downstream packages sometimes import them for custom event terms.
Consider adding backward-compatible aliases in events.py:
from .physics_events import randomize_prop_by_op as _randomize_prop_by_op # noqa: F401
from .physics_events import validate_scale_range as _validate_scale_range # noqa: F4014. PhysX RandomizePhysicsSceneGravity.__init__ does not validate the distribution parameter
File: source/isaaclab_physx/isaaclab_physx/envs/mdp/physics_events.py (~line 216)
The Newton implementation validates distribution at init and raises NotImplementedError for unknown values. The PhysX implementation skips this entirely. If finding #2 is fixed, add validation here as well.
5. randomize_rigid_body_scale instantiates a new backend object on every call
File: source/isaaclab/isaaclab/envs/mdp/physics_events.py (~line 118)
_resolve_backend_event("RandomizeRigidBodyScale")(env, env_ids, scale_range, asset_cfg, relative_child_path)Unlike other terms (which cache self._impl at init), this function creates a new RandomizeRigidBodyScale instance per call via importlib.import_module + getattr. Since the term is deprecated and should only run at USD-event time (once), this is acceptable but noted.
🟢 Low Severity / Nits
6. Top-level imports of carb and pxr in PhysX backend module
File: source/isaaclab_physx/isaaclab_physx/envs/mdp/physics_events.py (lines 13-14)
import carb
from pxr import Gf, Sdf, UsdGeom, VtOnly RandomizeRigidBodyScale uses pxr and RandomizePhysicsSceneGravity uses carb. Consider lazy imports inside the classes to reduce module load time when only a subset of terms is used.
7. Missing __all__ in core physics_events.py
The new module exports public symbols but doesn't define __all__. Adding it would make from .physics_events import * behavior explicit.
8. CI status: pre-commit and Check changelog fragments are currently failing
These are expected (PR description notes changelog fragments aren't included yet), but pre-commit failure may indicate formatting issues that should be resolved before merge.
✅ What looks good
- Clean separation of concerns: Backend-specific code now lives in the appropriate extension packages (
isaaclab_physx,isaaclab_newton,isaaclab_ovphysx). - Runtime resolver pattern:
_resolve_backend_eventis a simple, understandable mechanism for late-binding backend implementations. - OVPhysX strategy: Reusing PhysX implementations by default (via
ovphysx_uses_physx=True) with explicit no-op for material randomization is pragmatic. - Deprecation handling:
randomize_rigid_body_scaleproperly emitsDeprecationWarningand the Newton backend raisesNotImplementedError. - Import hygiene: The core module no longer imports
pxr,carb, orwarp— making it truly backend-agnostic. - Test updates: Both test files correctly update their imports to the new module path.
Summary
| # | Severity | Issue |
|---|---|---|
| 1 | 🔴 High | startswith in resolver less resilient than original substring matching |
| 2 | 🔴 High | PhysX gravity randomization silently ignores user-specified distribution |
| 3 | 🟡 Medium | Renamed private helpers break downstream private-API consumers |
| 4 | 🟡 Medium | PhysX gravity missing distribution validation |
| 5 | 🟡 Medium | Scale randomization instantiates backend on every call |
| 6 | 🟢 Low | Eager heavy imports in PhysX backend module |
| 7 | 🟢 Low | Missing __all__ in core physics_events.py |
| 8 | 🟢 Low | CI pre-commit check failing |
Overall this is a well-structured refactor with clean separation of concerns. The main actionable items are #1 (resolver resilience) and #2 (distribution parameter silently ignored). The architecture is sound and scales well for future backends.
Update (37b83f6): Changelog fragments added for all affected packages and minor whitespace cleanup in physics_events.py (removed extra blank lines). ✅ Addresses concern #8 (CI changelog check). Remaining concerns #1–#7 still apply to unchanged code.
Update (3942c86): Reviewed incremental changes since 37b83f6.
New feature: resample_interval_on_reset added to EventTermCfg — allows interval-mode event terms to preserve their per-environment timer across resets. Implementation looks correct in both the standard and experimental event managers, with a solid test covering both paths. ✅
Refactoring: isaaclab_experimental/envs/mdp/__init__.py now uses lazy_export() instead of explicit star-imports — cleaner and more maintainable. ✅
Previous concerns status:
- #2 (PhysX gravity distribution hardcoded) — still present at line 274 of PhysX
physics_events.py - #1 (resolver startswith) — still present (unchanged code)
- #3–#7 — unchanged
No new issues found in this increment.
Update (527813b): Lint-only change — reordered imports in isaaclab_newton/envs/mdp/physics_events.py to follow Python import conventions (third-party imports before local imports, TYPE_CHECKING block reorganized). ✅ No functional changes, no new concerns.
Previous concerns status: #1–#7 unchanged.
Update (1f45c56): Major increment — includes backend resolver fix + cartpole task consolidation.
✅ Fixed:
- #1 (resolver
startswith) — Now uses substring matching ("in") with correct ordering (ovphysxtested beforephysx). Exactly matches the recommendation. - #4 (PhysX gravity missing distribution validation) — Validation is now present at
__init__time.
Still present (unchanged code):
- #2 — PhysX
RandomizePhysicsSceneGravitystill hardcodesdistribution="uniform"at line ~283 - #3, #5, #6, #7 — Unchanged
New changes (no issues): Cartpole task restructuring (merged direct_cartpole + manager_cartpole → cartpole, dropped -v0 suffixes, removed deprecated alias registrations). All test imports updated consistently. isaaclab_experimental switched to lazy_export(). Clean refactor, no new concerns.
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
3 times, most recently
from
9a97962 to
527813b
Compare
ooctipus
force-pushed
the
zhengyuz/decouple-physics-events
branch
from
527813b to
1f45c56
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.