Newton color inconsistency

[huidongc]

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

[greptile-apps]

Greptile Summary

This PR fixes Newton rendering color inconsistencies by adding a replace_newton_shape_colors utility in newton_model_utils.py that maps USD material colors (OmniPBR diffuse_color_constant x diffuse_tint or primvars:displayColor) to Newton's shape_color array via a Warp scatter kernel, called at model finalization in both the NewtonManager physics path and the PhysxSceneDataProvider visualization path.

• The test file test_newton_model_utils.py imports _ISAACLAB_REPLACE_NEWTON_SHAPE_COLORS_ENV and _omnipbr_linear_diffuse_from_material from newton_model_utils.py, but neither symbol exists in the module — this causes ImportError at collection time, making the entire test file non-functional.
• replace_newton_shape_colors returns None implicitly in all code paths, but every test assertion checks the integer return value (== 0, == 1, == 2), causing assertion failures even if the import issue were fixed.

Confidence Score: 4/5

Safe to merge once the two P1 test/implementation contract issues are resolved.

Two P1 defects affect the new test suite: missing module-level symbols cause ImportError at collection time, and the missing return value makes every count assertion fail. The core runtime color-replacement behavior is unaffected.

source/isaaclab/isaaclab/sim/utils/newton_model_utils.py and source/isaaclab/test/sim/test_newton_model_utils.py need reconciliation: add _ISAACLAB_REPLACE_NEWTON_SHAPE_COLORS_ENV constant, rename/alias _get_omnipbr_albedo to _omnipbr_linear_diffuse_from_material, and add return num_color_updates.

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/sim/utils/newton_model_utils.py	Core implementation of shape-color replacement; missing two symbols the test file tries to import and replace_newton_shape_colors has no return statement for the success path.
source/isaaclab/test/sim/test_newton_model_utils.py	New test suite that imports non-existent symbols and asserts on integer return values from a function that returns None — tests will fail with ImportError at collection time.
source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py	Calls replace_newton_shape_colors after model finalization; straightforward integration with no issues.
source/isaaclab_physx/isaaclab_physx/scene_data_providers/physx_scene_data_provider.py	Imports and calls replace_newton_shape_colors in both prebuilt-artifact and USD-fallback model build paths; previous duplicate constant removed.
source/isaaclab_newton/isaaclab_newton/renderers/newton_warp_renderer.py	Sets clear_color=0xFFEEEEEE for the background; comment 'Linear RGB 93% gray' is slightly misleading (value is sRGB).
source/isaaclab/isaaclab/cloner/cloner_utils.py	Sorting of prototype roots added for deterministic traversal order; no issues identified.
source/isaaclab_tasks/test/test_rendering_correctness.py	Updated golden images and test fixtures; golden images only written when absent (not on regression); no new issues.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[USD Stage] --> B{Path used?}
    B -->|Newton physics path| C[NewtonManager.start_simulation]
    B -->|PhysX + Newton renderer| D[PhysxSceneDataProvider._build_newton_model_from_usd]
    B -->|PhysX + prebuilt artifact| E[PhysxSceneDataProvider._try_use_prebuilt_newton_artifact]
    C --> F[builder.finalize model]
    D --> F
    E --> G[artifact.model]
    F --> H[replace_newton_shape_colors model, stage]
    G --> H
    H --> I{Env var disabled?}
    I -->|yes| J[return 0 no-op]
    I -->|no| K[Traverse shape_label list]
    K --> L{Has bound material?}
    L -->|OmniPBR| M[diffuse_color_constant x diffuse_tint linear RGB]
    L -->|Other material| N[Leave shape unchanged]
    L -->|No material| O{Has displayColor?}
    O -->|yes| P[Use displayColor as linear RGB]
    O -->|no| Q[Use 18% gray fallback]
    M --> R[_scatter_shape_color_rows_kernel linear to sRGB OETF on GPU]
    P --> R
    Q --> R
    R --> S[model.shape_color updated]
    S --> T[Newton renderer uses corrected colors]

Loading

_{Reviews (2): Last reviewed commit: "warning about the deprecation" | Re-trigger Greptile}

[kellyguo11]

Review Summary

This PR fixes Newton rendering color inconsistency by:

1. Setting a default shape color (18% gray / 0.18 linear) for objects without materials
2. Setting a camera clear color (0xFFEEEEEE) to match the Isaac RTX renderer background
3. Removing the pytest.skip for Newton RGB tests now that colors are deterministic
4. Refactoring _validate_camera_outputs to collect all failures before reporting

The color fix is straightforward and well-motivated. All CI checks pass. A few items below.

Should-fix

Duplicated constant across packages — _DEFAULT_SHAPE_COLOR in physx_scene_data_provider.py duplicates DEFAULT_SHAPE_COLOR from isaaclab_newton.constants. Greptile flagged this too. At minimum add a comment noting the duplication is intentional to avoid cross-package dependency. Better: consider a shared constants module in isaaclab core since both PhysX and Newton providers need this value.

Suggestions

Document color space — 0.18 linear is indeed standard 18% gray, and 0xFFEEEEEE is ARGB with R=G=B=0xEE (~93.3% sRGB white). A brief note in constants.py specifying the color space (linear vs sRGB) would help future contributors, since Newton's ModelBuilder.default_shape_color and SensorTiledCamera.ClearData.clear_color may interpret these differently.

Test behavior change — The refactored _validate_camera_outputs now collects all failures and reports them at the end instead of failing on the first assertion. This is better for debugging (you see ALL broken data types in one run, and all goldens get created on first run), but it's a behavior change worth noting in the PR description.

Overall this is a clean, focused fix. 👍

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR fixes non-deterministic rendering colors in the Newton backend by addressing two root causes: (1) non-deterministic iteration order of a Python set in the cloner, and (2) a missing explicit clear color in the Newton warp renderer. It also refactors _validate_camera_outputs to collect all failures before reporting, and re-enables the previously skipped Newton RGB rendering test. The core fix is correct and well-targeted.

Design Assessment

Design is sound. The two-pronged fix correctly identifies the root causes of the inconsistency:

• Sorting the prototype roots in clone_from_template eliminates non-deterministic iteration order from Python's set, which caused different scene traversal orders across runs. This is the right place to fix it — the cloner is responsible for deterministic environment setup.
• Adding an explicit clear_color to the Newton renderer ensures the background is consistent. Without it, uninitialized buffer contents could bleed through.

Both changes are minimal and self-contained.

Findings

🟡 Warning: Hardcoded clear color should be configurable — source/isaaclab_newton/isaaclab_newton/renderers/newton_warp_renderer.py:224

The clear color 0xFFEEEEEE (light gray, ARGB) is hardcoded directly in the render() method. Other renderer properties (textures, shadows, ambient lighting, backface culling, max distance) are all exposed through NewtonWarpRendererCfg. Hardcoding this value means users cannot customize the background color without modifying source code.

Consider adding a clear_color: int = 0xFFEEEEEE field to NewtonWarpRendererCfg and referencing self.cfg.clear_color here. This follows the existing pattern for renderer configuration and maintains API consistency.

That said, this is a reasonable default for the bug fix — making it configurable could be a follow-up.

🟡 Warning: Test behavior change — fail-all vs. fail-fast — source/isaaclab_tasks/test/test_rendering_correctness.py:528–594

The refactored _validate_camera_outputs now collects all failures into failed_data_types and reports them in a single pytest.fail() at the end. Previously, the function would assert (fail-fast) on the first failing data type.

This is a reasonable improvement for developer experience (seeing all failures at once), but note the behavioral change: if an early data type produces an all-zero image (max_val <= 0), the function now continues and still validates subsequent data types. Previously, the assert would halt immediately. In most cases this is fine, but if the all-zero output indicates a fundamental rendering failure (e.g. broken scene), subsequent comparisons may produce misleading diff results.

Consider adding a brief comment explaining the intentional shift from fail-fast to fail-all, so future maintainers understand the design choice.

🔵 Suggestion: Pre-commit formatting fix needed — source/isaaclab/isaaclab/cloner/cloner_utils.py:50–52

CI shows the pre-commit check failed. The formatter wants the sorted(...) call on a single line:

prototype_roots = sorted({"/".join(str(prototype.GetPath()).split("/")[:-1]) for prototype in prototypes})

instead of the current multi-line version. This needs to be fixed before merge.

🔵 Suggestion: PR description is empty — The PR body still contains the default template without a filled-in description. For a bug fix that changes cloner behavior and test infrastructure, a brief explanation of the root cause and fix strategy would help reviewers and future archaeology. The commit message ("fix the source of inconsistent scene traversal in newton") is good but the PR description should expand on it.

Test Coverage

• Bug fix: The existing test_dexsuite_kuka_allegro_lift with Newton RGB rendering is the regression test — it was previously skipped with a pytest.skip("Newton Warp produces inconsistent RGB colors run-to-run") and this PR re-enables it. If the underlying non-determinism fix is correct, this test will now pass consistently. This is the right approach.
• Golden images: Updated to match the new deterministic output with the explicit clear color. Correct.
• Cloner sort fix: No dedicated unit test for the sorting change. Arguably, the rendering correctness test indirectly covers this (since inconsistent cloner order was the root cause), but a direct test asserting prototype_roots is sorted would make the invariant explicit. This is a nice-to-have, not critical.

CI Status

• ❌ pre-commit — Failed (formatting: sorted() call line length in cloner_utils.py)
• ⏳ Build Latest Docs, license-check, Docker builds — Pending
• ✅ Check for Broken Links, labeler, Detect Changes, Detect Doc Build Type — Passed

Verdict

COMMENT

The core fix is correct and well-targeted — sorting the prototype roots and setting an explicit clear color are both necessary to ensure deterministic Newton rendering. The test refactoring is a reasonable improvement. Two items need attention before merge: (1) fix the pre-commit formatting failure, and (2) fill in the PR description. The hardcoded clear color is worth discussing as a follow-up improvement but doesn't block this PR.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot — Follow-up

New commits (77f8b28→01955bc4): added newton_model_utils.py with USD-to-Newton color alignment, integrated it into Newton manager and PhysX scene data provider, and added a test file. The test file has import-level mismatches that will prevent it from running.

[isaaclab-review-bot]

🔍 Full Review Summary

This PR addresses Newton rendering color inconsistency through a comprehensive fix. The changes are well-structured and the implementation is solid.

✅ What's Good

1. Root cause fix - Sorting prototype_roots in cloner_utils.py addresses the nondeterministic ordering at the source

2. Well-documented workaround - The newton_model_utils.py module has excellent documentation:

   • Clear "DO NOT USE" warning
   • Comprehensive docstrings explaining the color resolution logic
   • Environment variable escape hatch (ISAACLAB_REPLACE_NEWTON_SHAPE_COLORS)
   • FutureWarning for deprecation path

3. Correct sRGB implementation - The linear-to-sRGB transfer function is mathematically correct

4. Comprehensive test coverage - Tests cover:

   • sRGB conversion accuracy
   • OmniPBR color extraction
   • Fallback to displayColor and gray
   • Material inheritance (strongerThanDescendants)
   • Guide shape exclusion
   • Env var disabling
   • Deprecation warning

5. Test refactoring - Collecting all failures before reporting is better for debugging

⚠️ Should-fix

1. Missing return value - The replace_newton_shape_colors function doesn't return num_color_updates, but tests assert on return values. Either add return num_color_updates or update tests.

📝 Suggestions (non-blocking)

1. Typo - maxmize → maximize in _canonical_prim_lookup_key docstring

2. Color space documentation - Add inline comments specifying linear vs sRGB for constants

3. Assert vs explicit error - Consider using explicit ValueError instead of assert for invalid prim check

4. Clear color documentation - Document the ARGB format for 0xFFEEEEEE

Overall

This is a well-executed fix for a tricky rendering consistency issue. The temporary nature is clearly communicated, and the escape hatch allows users to disable if needed. Once the return value issue is addressed, this should be ready to merge.