Skip to content

[Newton] Refactors Simulation Manager and Context#4646

Merged
ooctipus merged 4 commits intoisaac-sim:dev/newtonfrom
ooctipus:newton/mannual_refactor_sim
Feb 20, 2026
Merged

[Newton] Refactors Simulation Manager and Context#4646
ooctipus merged 4 commits intoisaac-sim:dev/newtonfrom
ooctipus:newton/mannual_refactor_sim

Conversation

@ooctipus
Copy link
Copy Markdown
Collaborator

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (existing functionality will not work without user modification)
  • Documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Feb 19, 2026
@ooctipus ooctipus force-pushed the newton/mannual_refactor_sim branch from e05e458 to 920e51d Compare February 19, 2026 02:02
Comment thread source/isaaclab/isaaclab/envs/direct_rl_env.py Outdated
@@ -0,0 +1,170 @@
# Copyright (c) 2022-2026, The Isaac Lab Project Developers (https://github.com/isaac-sim/IsaacLab/blob/main/CONTRIBUTORS.md).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this from Piotr's changes?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I think so

def get_metadata(self) -> dict[str, Any]:
return dict(self._metadata)

def get_transforms(self) -> dict[str, Any] | None:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it help with the merge if we also implement this first?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got the newton scene data provider kind of working in dev/newton, but I thought I'd like to reserve the implementation detail to @matthewtrepte and i might be beneficial for him to directly work on develop and not worry about dev/newton. Maybe

Copy link
Copy Markdown
Collaborator

@AntoineRichard AntoineRichard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look great! Thanks @ooctipus! I would start moving some files about in the correct modules / package just to make sure we can make things work. We'd need to separate the PhysX / Newton Managers and move them to their respective modules. Same with the visualizers, scene data providers etc...

Comment thread source/isaaclab/isaaclab/managers/command_manager.py
Comment thread source/isaaclab/isaaclab/markers/visualization_markers.py
Comment thread source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py
@ooctipus ooctipus marked this pull request as ready for review February 20, 2026 01:56
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Feb 20, 2026

Greptile Summary

Refactored simulation architecture to extract physics backend logic from SimulationContext into dedicated PhysicsManager hierarchy, introduced SceneDataProvider abstraction for visualizers/renderers, and renamed Omniverse visualizer to Kit.

Key architectural changes:

  • Extracted PhysX and Newton physics logic from SimulationContext (1760→520 lines) into PhysxManager and NewtonManager that extend new abstract PhysicsManager base class
  • Introduced unified callback system via PhysicsEvent enum (MODEL_INIT, PHYSICS_READY, STOP) replacing backend-specific callbacks
  • Created SceneDataProvider interface allowing visualizers to access scene data (transforms, velocities, contacts) without tight coupling to physics backends
  • Moved Newton manager from sim/_impl/ to physics/ package for better organization
  • Renamed OVVisualizerKitVisualizer and omniversekit visualizer type
  • Updated 85 files including all task configs to use new import paths and API patterns

Impact:

  • Breaking change: visualizer type omniverse renamed to kit
  • Breaking change: import paths changed for Newton config (sim._impl.newton_manager_cfgphysics.newton_manager_cfg)
  • Assets now register callbacks via PhysicsManager.register_callback() instead of backend-specific methods

Confidence Score: 4/5

  • Large architectural refactoring with good separation of concerns; breaking changes are clearly scoped to config imports and visualizer naming
  • This is a well-structured refactoring that improves maintainability by separating physics backend concerns. The changes follow clear patterns (extract class, introduce interface), callbacks are properly migrated to unified system, and the scope is appropriate for the PR title. However, confidence is not higher due to: (1) large surface area with 85 files changed across multiple subsystems, (2) breaking changes to public API (visualizer names, import paths), (3) complex callback lifecycle management that could have edge cases, and (4) limited test coverage updates visible in the diff
  • Pay close attention to source/isaaclab/isaaclab/physics/physx_manager.py (new animation recording lifecycle), source/isaaclab/isaaclab/sim/simulation_context.py (singleton pattern and cleanup order), and task config files to ensure backward compatibility isn't broken unintentionally

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/sim/simulation_context.py Heavily refactored to delegate physics to PhysicsManager, simplifying from 1760+ lines to ~520 lines; visualization now uses scene data providers
source/isaaclab/isaaclab/physics/physics_manager.py New abstract base class introducing unified callback system for physics lifecycle events across backends
source/isaaclab/isaaclab/physics/physx_manager.py New PhysX manager extracting PhysX-specific logic from SimulationContext, including animation recording and fabric updates
source/isaaclab/isaaclab/physics/newton_manager.py Moved from sim/_impl/ to physics/, now extends PhysicsManager with unified callback system
source/isaaclab/isaaclab/sim/scene_data_providers/scene_data_provider.py New backend-agnostic interface for visualizers/renderers to access scene data (transforms, velocities, contacts)
source/isaaclab/isaaclab/sim/scene_data_providers/physx_scene_data_provider.py PhysX implementation of scene data provider, supports body poses via PhysX tensor views with XformPrimView fallback
source/isaaclab/isaaclab/visualizers/visualizer.py Refactored base class to use SceneDataProvider instead of direct simulation access
source/isaaclab/isaaclab/assets/asset_base.py Switched from NewtonManager.add_on_start_callback to PhysicsManager.register_callback with unified PhysicsEvent

Class Diagram

%%{init: {'theme': 'neutral'}}%%
classDiagram
    class SimulationContext {
        +SimulationCfg cfg
        +PhysicsManager physics_manager
        +SceneDataProvider scene_data_provider
        +initialize_visualizers()
        +reset()
        +step()
        +forward()
    }
    
    class PhysicsManager {
        <<abstract>>
        +initialize(sim_context)
        +reset(soft)
        +step()
        +forward()
        +register_callback(callback, event, order)
        +deregister_callback(id)
    }
    
    class PhysxManager {
        +AnimationRecorder _anim_recorder
        +_warmup_and_create_views()
        +_configure_physics()
        +step()
    }
    
    class NewtonManager {
        +ModelBuilder _builder
        +Model _model
        +State _state_0
        +initialize_solver()
        +step()
    }
    
    class SceneDataProvider {
        <<interface>>
        +get_transforms()
        +get_velocities()
        +get_newton_model()
        +get_usd_stage()
        +get_metadata()
    }
    
    class PhysxSceneDataProvider {
        +_stage
        +_physics_sim_view
        +_newton_model
        +get_transforms()
        +get_velocities()
    }
    
    class NewtonSceneDataProvider {
        +_model
        +_state
        +get_transforms()
        +get_newton_model()
    }
    
    class Visualizer {
        <<abstract>>
        +SceneDataProvider scene_data_provider
        +initialize(provider)
        +step(dt, state)
        +close()
    }
    
    class KitVisualizer {
        +_viewport_window
        +step(dt, state)
    }
    
    class NewtonVisualizer {
        +_window
        +step(dt, state)
    }
    
    class RerunVisualizer {
        +_rec
        +step(dt, state)
    }
    
    SimulationContext --> PhysicsManager : uses
    SimulationContext --> SceneDataProvider : creates
    SimulationContext --> Visualizer : manages
    PhysicsManager <|-- PhysxManager : extends
    PhysicsManager <|-- NewtonManager : extends
    SceneDataProvider <|.. PhysxSceneDataProvider : implements
    SceneDataProvider <|.. NewtonSceneDataProvider : implements
    Visualizer <|-- KitVisualizer : extends
    Visualizer <|-- NewtonVisualizer : extends
    Visualizer <|-- RerunVisualizer : extends
    Visualizer --> SceneDataProvider : uses
Loading

Last reviewed commit: d291a11

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

85 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@ooctipus ooctipus force-pushed the newton/mannual_refactor_sim branch from d291a11 to a7281e7 Compare February 20, 2026 02:41
@ooctipus ooctipus force-pushed the newton/mannual_refactor_sim branch from a7281e7 to 3214457 Compare February 20, 2026 02:42
@ooctipus ooctipus changed the title [Newton] Refactor Simulation Manager and Context [Newton] Refactors Simulation Manager and Context Feb 20, 2026
@ooctipus ooctipus merged commit c0a9b83 into isaac-sim:dev/newton Feb 20, 2026
4 of 10 checks passed
AntoineRichard added a commit that referenced this pull request Apr 17, 2026
## Summary

Fix incorrect attribute name `contact_margin` → `gap` on Newton
`ShapeConfig` in `NewtonManager.create_builder()`.

Newton PR #1732 renamed `contact_margin` to `gap` (broad-phase AABB
expansion). The IsaacLab code was never updated, creating a dead
attribute that Python silently accepted. The intended 1 cm default shape
gap was never applied.

## Newton PR #1732 rename mapping

| Old field | New field | Semantics |
|-----------|-----------|-----------|
| `thickness` | `margin` | Shape surface extension (affects contact
forces) |
| `contact_margin` | **`gap`** | Broad-phase AABB expansion (detection
range only) |
| `rigid_contact_margin` | `rigid_gap` | Builder-level default gap
(already 0.1) |

## Timeline

| Date | Newton | IsaacLab | `contact_margin` valid? |
|------|--------|----------|------------------------|
| Feb 19 | pin: `51ce35e` (has `contact_margin`) | #4646 adds
`contact_margin = 0.01` | Yes |
| Feb 24 | **PR #1732 renames `contact_margin` → `gap`** | — | — |
| Mar 2 | pin updated to `v0.2.3` (after rename) | #4761 keeps
`contact_margin` | **No — broken** |
| Mar 9 | — | #4883 removes the line | Removed |
| Apr 13 | — | #4822 re-adds `contact_margin` | **No — still broken** |

## Ablation: gap vs margin

We conducted an ablation study to understand the impact. Note: `margin`
(shape surface
extension) is a different field from `gap` (broad-phase range). The
original code intended
to set `gap`, but setting `margin` also has a significant positive
effect on training for
rough terrain locomotion.

| Setting | `gap` | `margin` | Go1 Reward (300 iter) | Effect |
|---------|-------|----------|----------------------|--------|
| Dead field (baseline) | 0.1 (default) | 0.0 | ~1.0 | — |
| `gap=0.01` only | 0.01 | 0.0 | 0.66 | No training improvement |
| `margin=0.01` only | 0.1 (default) | 0.01 | 18.77 | Major improvement
|
| `gap=0.01` + `margin=0.01` | 0.01 | 0.01 | 16.54 | Similar to
margin-only |

**This PR fixes the correct field migration** (`contact_margin` →
`gap`). The `margin` setting
for rough terrain contact quality will be addressed separately in the
rough terrain env PR
(#5248) via a new `default_shape_margin` config field on `NewtonCfg`.

## Test plan

- [x] Verified `contact_margin` is not a field on `ShapeConfig` (Newton
1.1.0.dev0)
- [x] Verified `gap` is the correct replacement field per Newton PR
#1732
- [x] Confirmed by camevor (Newton developer)
- [x] Ablation study: gap alone doesn't change training, so existing
tests should pass

Co-authored-by: Antoine RICHARD <antoiner@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants