Skip to content

Newton RGB default fixes#5168

Open
nblauch wants to merge 2 commits intoisaac-sim:developfrom
nblauch:newton_rgb_fixes
Open

Newton RGB default fixes#5168
nblauch wants to merge 2 commits intoisaac-sim:developfrom
nblauch:newton_rgb_fixes

Conversation

@nblauch
Copy link
Copy Markdown

@nblauch nblauch commented Apr 3, 2026

Description

Newton Warp renderer visual parity fixes: ground plane color propagation and white background.

Ground plane color (from_files.py): spawn_ground_plane currently only sets diffuse_tint on the visual grid shader, which RTX reads. Non-RTX renderers like Newton extract color from USD primvars —
but the CollisionPlane prim has no color attribute, so Newton falls back to a random palette color. This fix additionally sets displayColor on the collision prim so any renderer can resolve the configured
color.

White background (newton_warp_renderer.py): Newton's default clear color is black (0x00000000), while RTX renders a white background. This passes a white ClearData to match RTX behavior, affecting
both the shaded color and albedo channels.

Type of change

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

Screenshots

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 bug Something isn't working isaac-lab Related to Isaac Lab team labels Apr 3, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 3, 2026

Greptile Summary

This PR adds Newton/non-RTX visual parity fixes: it propagates the configured ground-plane color to a displayColor USD primvar so renderers that ignore the RTX shader can resolve it, and sets a white ClearData in the Newton renderer to match RTX's white background default.

  • collision_prim is only assigned inside if cfg.physics_material is not None: but is referenced unconditionally at line 246 in the color-setting block — when physics_material=None this is a NameError at runtime.
  • The changelog was not updated for either affected package (isaaclab, isaaclab_newton), which is required by the contribution guidelines.

Confidence Score: 4/5

Mostly safe but has one P1 scoping bug in from_files.py that causes a NameError when physics_material is None.

The Newton renderer change is correct and safe. The ground-plane color fix has a real runtime defect: collision_prim is out of scope when cfg.physics_material is None, producing an unhandled NameError. While the default config keeps physics_material non-None, the code path is reachable and the fix is trivial.

source/isaaclab/isaaclab/sim/spawners/from_files/from_files.py — collision_prim scoping bug

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/sim/spawners/from_files/from_files.py Adds displayColor primvar to the collision plane for non-RTX renderers, but references collision_prim outside the scope where it is assigned, causing a NameError when cfg.physics_material is None.
source/isaaclab_newton/isaaclab_newton/renderers/newton_warp_renderer.py Passes a white ClearData (0xFFFFFFFF) for both shaded color and albedo channels to match RTX's white default background; straightforward and correct.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant spawn_ground_plane
    participant USD Stage
    participant Newton Renderer

    Caller->>spawn_ground_plane: spawn_ground_plane(prim_path, cfg)
    spawn_ground_plane->>USD Stage: create_prim (grid plane USD)
    alt cfg.physics_material is not None
        spawn_ground_plane->>USD Stage: get_first_matching_child_prim (Plane type)
        USD Stage-->>spawn_ground_plane: collision_prim
        spawn_ground_plane->>USD Stage: bind_physics_material(collision_prim)
    end
    alt cfg.color is not None
        spawn_ground_plane->>USD Stage: change_prim_property(diffuse_tint shader)
        Note over spawn_ground_plane: NEW: also set displayColor primvar
        spawn_ground_plane->>USD Stage: CreateDisplayColorAttr().Set(color) on collision_prim
    end
    spawn_ground_plane-->>Caller: prim

    Caller->>Newton Renderer: render(render_data)
    Note over Newton Renderer: NEW: ClearData(color=0xFFFFFFFF, albedo=0xFFFFFFFF)
    Newton Renderer->>Newton Renderer: newton_sensor.update(..., clear_data=white)
    Newton Renderer-->>Caller: rendered output (white background)
Loading

Reviews (1): Last reviewed commit: "Make default background color white like..." | Re-trigger Greptile

Comment on lines +246 to +249
if collision_prim is not None:
UsdGeom.Gprim(collision_prim).CreateDisplayColorAttr().Set(
[Gf.Vec3f(*cfg.color)]
)
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.

P1 collision_prim may be undefined when cfg.physics_material is None

collision_prim is assigned only inside the if cfg.physics_material is not None: block (lines 210–222). If a caller passes physics_material=None, that block is skipped entirely, so collision_prim is never bound. Reaching if collision_prim is not None: on line 246 then raises a NameError at runtime.

Suggested change
if collision_prim is not None:
UsdGeom.Gprim(collision_prim).CreateDisplayColorAttr().Set(
[Gf.Vec3f(*cfg.color)]
)
collision_prim = get_first_matching_child_prim(
prim_path,
predicate=lambda _prim: _prim.GetTypeName() == "Plane",
stage=stage,
)
if collision_prim is not None:
UsdGeom.Gprim(collision_prim).CreateDisplayColorAttr().Set(
[Gf.Vec3f(*cfg.color)]
)

Alternatively, initialise collision_prim = None before the if cfg.physics_material is not None: block so it is always defined when the color section runs.

Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot bot left a comment

Choose a reason for hiding this comment

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

Code Review — PR #5168: Newton RGB default fixes

Good targeted fix for Newton rendering parity. Two commits, two files, clear intent. One bug and a couple of suggestions below.

Overall: The approach is sound — propagating displayColor to the collision prim for non-RTX renderers and setting white clear color to match RTX defaults are both correct fixes. The scoping bug on collision_prim needs to be fixed before merge.

)
# Also set displayColor on the collision plane so non-RTX renderers
# (e.g. Newton) can pick up the configured color.
if collision_prim is not None:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: NameError when cfg.physics_material is None

collision_prim is only assigned inside the if cfg.physics_material is not None: block (~line 214). If a caller sets physics_material=None, the variable is never bound, and this line raises NameError — the if collision_prim is not None guard doesn't help because the name itself doesn't exist.

While the default GroundPlaneCfg sets physics_material=RigidBodyMaterialCfg() (non-None), this is a public API — callers can and do override defaults. The simplest fix: look up the collision prim independently here rather than relying on the physics-material block's side effect:

        # Also set displayColor on the collision plane so non-RTX renderers
        # (e.g. Newton) can pick up the configured color.
        color_target_prim = get_first_matching_child_prim(
            prim_path,
            predicate=lambda _prim: _prim.GetTypeName() == "Plane",
            stage=stage,
        )
        if color_target_prim is not None:
            UsdGeom.Gprim(color_target_prim).CreateDisplayColorAttr().Set(
                [Gf.Vec3f(*cfg.color)]
            )

This is slightly more robust than collision_prim = None at function top, because it also works if the physics-material block raises and is caught elsewhere.

shape_index_image=render_data.outputs.instance_segmentation_image,
clear_data=newton.sensors.SensorTiledCamera.ClearData(
clear_color=0xFFFFFFFF, clear_albedo=0xFFFFFFFF,
),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit: Hardcoded clear color should be a config parameter

Hardcoding 0xFFFFFFFF here makes it impossible for users to get a non-white background without subclassing the renderer. Consider adding a clear_color: int = 0xFFFFFFFF field to NewtonWarpRendererCfg and reading it from self.cfg here:

clear_data=newton.sensors.SensorTiledCamera.ClearData(
    clear_color=self.cfg.clear_color, clear_albedo=self.cfg.clear_color,
),

Not blocking — the RTX-matching default is correct — but this would be a cleaner API. Also note self.cfg isn't stored currently; you'd need to save it in __init__.

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

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant