Skip to content

Conversation

@ooctipus
Copy link
Collaborator

Description

This PR fixes the bug where the Newton._model has no access to num_envs

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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

@ooctipus ooctipus marked this pull request as ready for review November 25, 2025 22:51
@ooctipus ooctipus requested a review from Mayankm96 as a code owner November 25, 2025 22:51
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 25, 2025

Greptile Overview

Greptile Summary

This PR fixes a bug where the Newton model instance lacked access to num_envs, which is needed by the Newton viewer to display environment count in the UI.

Key Changes:

  • Added single line to set NewtonManager._model.num_envs = NewtonManager._num_envs after model finalization in start_simulation() method (newton_manager.py:140)

Analysis:
The fix properly exposes the environment count that was previously set by GridCloner to the Newton model object. The viewer component (NewtonViewerGL) displays this value in its Model Information UI panel. Without this assignment, model.num_envs would be undefined when accessed by the viewer.

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk - it's a simple one-line addition that resolves a missing attribute bug
  • The change is straightforward and correctly addresses the stated bug. The implementation follows the existing pattern (accessing _num_envs class variable) and adds the assignment at the appropriate point after model finalization. However, there's a minor concern: _num_envs could potentially be None if GridCloner hasn't set it before start_simulation() is called, which could result in the model having num_envs = None
  • No files require special attention - the change is minimal and localized

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/isaaclab/sim/_impl/newton_manager.py 4/5 Added assignment of num_envs to Newton model after finalization to expose environment count for visualization

Sequence Diagram

sequenceDiagram
    participant SC as SimulationContext
    participant GC as GridCloner
    participant NM as NewtonManager
    participant Model as Newton Model
    participant Viewer as NewtonViewerGL
    
    Note over GC,NM: Environment Setup Phase
    GC->>NM: Set _num_envs = num_clones
    GC->>NM: set_builder(builder)
    
    Note over SC,Model: Simulation Initialization
    SC->>NM: start_simulation()
    NM->>NM: finalize builder
    NM->>Model: builder.finalize(device)
    Model-->>NM: return model instance
    NM->>Model: model.num_envs = _num_envs
    Note right of Model: This line was added<br/>to expose num_envs
    NM->>NM: create states and control
    
    Note over NM,Viewer: Rendering Phase
    NM->>Viewer: render()
    Viewer->>Model: Access model.num_envs
    Viewer->>Viewer: Display environment count in UI
Loading

Copy link
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.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant