Conversation
Greptile SummaryThis PR adds Newton/non-RTX visual parity fixes: it propagates the configured ground-plane color to a
Confidence Score: 4/5Mostly 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: source/isaaclab/isaaclab/sim/spawners/from_files/from_files.py — collision_prim scoping bug Important Files Changed
Sequence DiagramsequenceDiagram
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)
Reviews (1): Last reviewed commit: "Make default background color white like..." | Re-trigger Greptile |
| if collision_prim is not None: | ||
| UsdGeom.Gprim(collision_prim).CreateDisplayColorAttr().Set( | ||
| [Gf.Vec3f(*cfg.color)] | ||
| ) |
There was a problem hiding this comment.
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.
| 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.
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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, | ||
| ), |
There was a problem hiding this comment.
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__.
Description
Newton Warp renderer visual parity fixes: ground plane color propagation and white background.
Ground plane color (
from_files.py):spawn_ground_planecurrently only setsdiffuse_tinton 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
displayColoron the collision prim so any renderer can resolve the configuredcolor.
White background (
newton_warp_renderer.py): Newton's default clear color is black (0x00000000), while RTX renders a white background. This passes a whiteClearDatato match RTX behavior, affectingboth the shaded color and albedo channels.
Type of change
Screenshots
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there