Normalize imported and rendered color spaces#2411
Draft
eric-heiden wants to merge 4 commits intonewton-physics:mainfrom
Draft
Normalize imported and rendered color spaces#2411eric-heiden wants to merge 4 commits intonewton-physics:mainfrom
eric-heiden wants to merge 4 commits intonewton-physics:mainfrom
Conversation
Store shape colors in linear RGB so viewers and cameras can apply\noutput encoding explicitly instead of mixing color spaces.\n\nConvert USD-authored material and display colors to linear using USD\ncolor-space metadata, preserve texture color-space metadata, and add a\nSensorTiledCamera option to keep packed outputs linear instead of\nencoding them to sRGB.
Contributor
📝 WalkthroughWalkthroughModel shape colors and textures are now color-space aware: Model.shape_color is linear RGB, USD import honors authored color-space metadata for colors and textures, internal shading is evaluated in linear space, and packed sensor outputs may be encoded to display/sRGB or preserved as linear bytes via RenderConfig.encode_output_srgb. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant ModelB as ModelBuilder
participant USD as USD Importer
participant Model as Model
participant Render as Renderer (WARP)
participant Tex as TextureManager
participant Output as Output Encoder
participant Viewer as Viewer
App->>ModelB: add_shape(color=display/sRGB)
ModelB->>ModelB: srgb_to_linear(color)
ModelB->>Model: store shape_color (linear)
App->>USD: import_prim(material/texture)
USD->>USD: read sourceColorSpace / metadata
alt authored as sRGB
USD->>Model: convert and store linear color, set mesh.texture_color_space
else authored as RAW or auto
USD->>Model: store color/texture_color_space as-is
end
App->>Render: render(scene, config.encode_output_srgb)
Render->>Tex: load/sample texture
Tex->>Tex: if texture.color_space != RAW -> srgb_to_linear(sample)
Render->>Render: shade using linear RGB
alt config.encode_output_srgb == True
Render->>Output: linear_to_srgb(shaded) -> pack_rgba_to_uint32
else
Render->>Output: pack linear RGB bytes -> pack_rgba_to_uint32
end
Output->>Viewer: display/unpack according to encoding
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
newton/_src/usd/utils.py (1)
1935-1947:⚠️ Potential issue | 🟠 MajorExclude
texture_color_spacefrom the "did we resolve anything?" sentinels.After adding
"texture_color_space": "auto"to the properties dict, this branch can now treat an otherwise-empty material as populated, and the merge loop here can never replace"auto"with a resolved direct-input color space. That breaksdisplayColorfallback and can downgrade raw textures back to the default sRGB path.💡 Suggested fix
if source_shader is None: material_props = _extract_material_input_properties(material, target_prim) - if any(value is not None for value in material_props.values()): + if any(material_props.get(key) is not None for key in ("texture", "color", "metallic", "roughness")): return material_props return None @@ - for key in ("texture", "texture_color_space", "color", "metallic", "roughness"): + for key in ("texture", "color", "metallic", "roughness"): if properties.get(key) is None and material_props.get(key) is not None: properties[key] = material_props[key] + if ( + properties.get("texture") is not None + and material_props.get("texture") is not None + and properties.get("texture_color_space") == TEXTURE_COLOR_SPACE_AUTO + ): + properties["texture_color_space"] = material_props["texture_color_space"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/usd/utils.py` around lines 1935 - 1947, The code treats "texture_color_space":"auto" as a resolved value which prevents later fallbacks and merges; update the logic around _extract_material_input_properties and the merge loop so "texture_color_space" is excluded from the "did we resolve anything?" sentinel (i.e., when checking any(value is not None for value in material_props.values()) ignore the texture_color_space entry) and modify the merge condition for the keys tuple so that for "texture_color_space" you allow overwriting when properties.get("texture_color_space") == "auto" (treat "auto" as unresolved) — change the check from properties.get(key) is None to properties.get(key) is None or (key == "texture_color_space" and properties.get(key) == "auto") while still sourcing values from material_props.newton/_src/sensors/warp_raytrace/render_context.py (1)
701-725:⚠️ Potential issue | 🟡 MinorDon't leave procedural
TextureDataon the implicit color-space default.Lines 701-725 make file-backed textures explicit, but
newton/_src/sensors/warp_raytrace/utils.py, Lines 558-574 still creates the checkerboardTextureData()without settingcolor_space. With the new shading path, that helper no longer states whether its hard-coded0xFF808080/0xFFBFBFBFbytes are raw-linear or sRGB, so it can drift from the rest of the pipeline. Please set that field there as well, instead of relying on the implicit default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/sensors/warp_raytrace/render_context.py` around lines 701 - 725, The checkerboard TextureData created in the helper (the procedural/file-backed branch uses TextureData with color_space set) must also explicitly set its color_space; update the checkerboard TextureData construction in the helper in warp_raytrace/utils.py to assign data.color_space using texture_color_space_to_id(...) (e.g., the same default used elsewhere such as texture_color_space_to_id("auto") or the appropriate pipeline default) so the checkerboard's color space matches the rest of the shading path; reference the TextureData instance and the texture_color_space_to_id function when making the change.
🧹 Nitpick comments (4)
newton/_src/utils/import_usd.py (1)
527-528: Defershape_colorresolution until a supported geometry type is known.Line 528 now resolves material bindings for every uncached child, including
Xformand other nodes that never create a shape. Moving that lookup behind the supported-geometry branches avoids extra traversal work and cache entries on large USD hierarchies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/utils/import_usd.py` around lines 527 - 528, The code currently calls _get_material_props_cached(prim) for every uncached child (path_name) even for non-geometry prims; move the shape_color lookup so it only runs after you detect a supported geometry type for prim (e.g., inside the branches that handle Mesh/Sphere/Cube or after checking prim.GetTypeName()/UsdGeom predicates), leaving the existing path_name not in path_shape_map check in place but deferring shape_color resolution until inside the geometry-specific branches where a shape is actually created; use the same variables (path_name, prim, shape_color, _get_material_props_cached, path_shape_map) to locate and update the logic.newton/tests/test_sensor_tiled_camera.py (1)
12-12: Keep the test oracle independent of Newton’s own converter.Line 12 imports
linear_to_srgb_rgb(), and Lines 208-212 use it to derive the expected bytes. That makes the assertion less independent of the production color-space code. A tiny local reference conversion or fixed expected bytes would make this regression test stronger.Also applies to: 208-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_sensor_tiled_camera.py` at line 12, The test imports and uses Newton’s linear_to_srgb_rgb() converter to build the expected byte results, making the oracle dependent on production code; remove the import of linear_to_srgb_rgb from test_sensor_tiled_camera.py and replace the derived-expected logic (the code around where expected bytes are computed) with either a small, self-contained local conversion implementation (e.g., the sRGB gamma mapping applied to linear RGB values used in the test) or with fixed hard-coded expected byte values; update the assertion to use that local converter or fixed bytes and ensure you do not call linear_to_srgb_rgb() from the production module in the test.newton/_src/sensors/sensor_tiled_camera.py (1)
210-213: Clarify whereencode_output_srgbis configured in theupdate()docs.
update()does not take arender_configargument, sorender_config.encode_output_srgb=Falseis ambiguous here. Consider referencingself.render_config.encode_output_srgb(or setting it at construction viaconfig=SensorTiledCamera.RenderConfig(...)).✏️ Suggested doc wording tweak
- color_image: Output for packed RGBA color. The bytes are sRGB by - default, or linear when - ``render_config.encode_output_srgb=False``. None to skip. + color_image: Output for packed RGBA color. The bytes are sRGB by + default, or linear when + ``self.render_config.encode_output_srgb=False``. None to skip.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/sensors/sensor_tiled_camera.py` around lines 210 - 213, Docstring refers to render_config.encode_output_srgb ambiguously; update the docs to point to the instance config by referencing self.render_config.encode_output_srgb (or note that it can be set at construction via SensorTiledCamera.RenderConfig passed as config) and clarify that update() itself does not accept a render_config argument—update the parameter descriptions for color_image/depth_image accordingly.newton/tests/test_import_usd.py (1)
20-20: Keep these sRGB expectations independent from production code.Importing
srgb_to_linear_rgbfromnewton._src.utils.colormeans these assertions can still pass if the shared conversion logic regresses in the same way on both sides. Prefer fixed reference values, or at least a dedicated test-local reference implementation, for these expectations.Also applies to: 6537-6556, 6558-6585, 6587-6617, 6620-6651
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_import_usd.py` at line 20, The test imports the production function srgb_to_linear_rgb which makes the test pass even if both test and production code regress; remove the import of newton._src.utils.color.srgb_to_linear_rgb and replace usage with either hard-coded reference output values (fixed decimals) or a small test-local implementation of the sRGB→linear conversion inside newton/tests/test_import_usd.py, and update the assertions to compare against those fixed values; apply the same change to the other related test blocks referenced (around the ranges 6537-6556, 6558-6585, 6587-6617, 6620-6651) so all sRGB expectations are independent of production code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/sim/builder.py`:
- Around line 5216-5219: The param docstring for the color argument currently
lists fallbacks in the wrong order; update the wording wherever that paragraph
appears (the color param doc in builder functions) to reflect the actual
precedence: first use the explicit color argument, then fall back to src.color
(mesh/source color), then default_shape_color if set, and finally the per-shape
palette sequence (and note Mesh.color is consulted for mesh-backed shapes before
default_shape_color). Locate the docstring section mentioning "color: Optional
linear RGB..." and the similar paragraphs elsewhere and reorder/clarify the
fallback description to match the implemented precedence (color -> src.color ->
default_shape_color -> palette).
- Around line 197-200: The _normalize_shape_color helper must validate inputs
instead of blindly indexing: in _normalize_shape_color (and callers like any use
of default_shape_color), check that color is a sequence/iterable of exactly
three numeric components, raise a clear ValueError for malformed inputs (e.g.,
wrong length or non-numeric items), and then return the tuple of floats via
float(component) conversions; this ensures malformed values like (1.0, 0.0)
produce a descriptive ValueError rather than an IndexError.
In `@newton/_src/usd/utils.py`:
- Around line 1687-1689: The resolved texture color space stored in
properties["texture_color_space"] is not being propagated to the returned Mesh;
modify get_mesh() (and the equivalent branch around 1843-1847) so after
Mesh.create_from_usd(...) (or when populating mesh_out) you set
mesh_out._texture_color_space = properties.get("texture_color_space") (or the
resolved variable) before returning the mesh; ensure this assignment occurs in
both branches that call _find_texture_in_shader/source_shader so callers of
newton.usd.get_mesh() and Mesh.create_from_usd() receive the correct color space
instead of falling back to "auto".
In `@newton/_src/utils/color.py`:
- Line 1: The SPDX header in newton/_src/utils/color.py incorrectly uses 2025 as
the creation year; update the SPDX-FileCopyrightText line at the top of color.py
to use the file’s actual creation year (replace "2025" with the correct creation
year) so the file header follows the guideline of using the year the file was
first created.
---
Outside diff comments:
In `@newton/_src/sensors/warp_raytrace/render_context.py`:
- Around line 701-725: The checkerboard TextureData created in the helper (the
procedural/file-backed branch uses TextureData with color_space set) must also
explicitly set its color_space; update the checkerboard TextureData construction
in the helper in warp_raytrace/utils.py to assign data.color_space using
texture_color_space_to_id(...) (e.g., the same default used elsewhere such as
texture_color_space_to_id("auto") or the appropriate pipeline default) so the
checkerboard's color space matches the rest of the shading path; reference the
TextureData instance and the texture_color_space_to_id function when making the
change.
In `@newton/_src/usd/utils.py`:
- Around line 1935-1947: The code treats "texture_color_space":"auto" as a
resolved value which prevents later fallbacks and merges; update the logic
around _extract_material_input_properties and the merge loop so
"texture_color_space" is excluded from the "did we resolve anything?" sentinel
(i.e., when checking any(value is not None for value in material_props.values())
ignore the texture_color_space entry) and modify the merge condition for the
keys tuple so that for "texture_color_space" you allow overwriting when
properties.get("texture_color_space") == "auto" (treat "auto" as unresolved) —
change the check from properties.get(key) is None to properties.get(key) is None
or (key == "texture_color_space" and properties.get(key) == "auto") while still
sourcing values from material_props.
---
Nitpick comments:
In `@newton/_src/sensors/sensor_tiled_camera.py`:
- Around line 210-213: Docstring refers to render_config.encode_output_srgb
ambiguously; update the docs to point to the instance config by referencing
self.render_config.encode_output_srgb (or note that it can be set at
construction via SensorTiledCamera.RenderConfig passed as config) and clarify
that update() itself does not accept a render_config argument—update the
parameter descriptions for color_image/depth_image accordingly.
In `@newton/_src/utils/import_usd.py`:
- Around line 527-528: The code currently calls _get_material_props_cached(prim)
for every uncached child (path_name) even for non-geometry prims; move the
shape_color lookup so it only runs after you detect a supported geometry type
for prim (e.g., inside the branches that handle Mesh/Sphere/Cube or after
checking prim.GetTypeName()/UsdGeom predicates), leaving the existing path_name
not in path_shape_map check in place but deferring shape_color resolution until
inside the geometry-specific branches where a shape is actually created; use the
same variables (path_name, prim, shape_color, _get_material_props_cached,
path_shape_map) to locate and update the logic.
In `@newton/tests/test_import_usd.py`:
- Line 20: The test imports the production function srgb_to_linear_rgb which
makes the test pass even if both test and production code regress; remove the
import of newton._src.utils.color.srgb_to_linear_rgb and replace usage with
either hard-coded reference output values (fixed decimals) or a small test-local
implementation of the sRGB→linear conversion inside
newton/tests/test_import_usd.py, and update the assertions to compare against
those fixed values; apply the same change to the other related test blocks
referenced (around the ranges 6537-6556, 6558-6585, 6587-6617, 6620-6651) so all
sRGB expectations are independent of production code.
In `@newton/tests/test_sensor_tiled_camera.py`:
- Line 12: The test imports and uses Newton’s linear_to_srgb_rgb() converter to
build the expected byte results, making the oracle dependent on production code;
remove the import of linear_to_srgb_rgb from test_sensor_tiled_camera.py and
replace the derived-expected logic (the code around where expected bytes are
computed) with either a small, self-contained local conversion implementation
(e.g., the sRGB gamma mapping applied to linear RGB values used in the test) or
with fixed hard-coded expected byte values; update the assertion to use that
local converter or fixed bytes and ensure you do not call linear_to_srgb_rgb()
from the production module in the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c144981f-eee0-4f5a-9c43-c0bbf810432d
📒 Files selected for processing (23)
CHANGELOG.mdnewton/_src/geometry/types.pynewton/_src/sensors/sensor_tiled_camera.pynewton/_src/sensors/warp_raytrace/render.pynewton/_src/sensors/warp_raytrace/render_context.pynewton/_src/sensors/warp_raytrace/textures.pynewton/_src/sensors/warp_raytrace/tiling.pynewton/_src/sensors/warp_raytrace/types.pynewton/_src/sensors/warp_raytrace/utils.pynewton/_src/sim/builder.pynewton/_src/sim/model.pynewton/_src/usd/utils.pynewton/_src/utils/color.pynewton/_src/utils/import_usd.pynewton/_src/utils/mesh.pynewton/_src/viewer/gl/opengl.pynewton/_src/viewer/gl/shaders.pynewton/_src/viewer/viewer.pynewton/_src/viewer/viewer_rerun.pynewton/_src/viewer/viewer_viser.pynewton/tests/test_import_usd.pynewton/tests/test_sensor_tiled_camera.pynewton/tests/test_shape_colors.py
Comment on lines
+197
to
+200
| def _normalize_shape_color(color: Vec3 | None) -> tuple[float, float, float] | None: | ||
| if color is None: | ||
| return None | ||
| return (float(color[0]), float(color[1]), float(color[2])) |
Contributor
There was a problem hiding this comment.
Validate malformed color inputs here.
default_shape_color is new public state, but this helper blindly indexes three components. A bad value like (1.0, 0.0) currently fails later with an IndexError instead of a clear ValueError.
Suggested fix
`@staticmethod`
def _normalize_shape_color(color: Vec3 | None) -> tuple[float, float, float] | None:
if color is None:
return None
- return (float(color[0]), float(color[1]), float(color[2]))
+ try:
+ return (float(color[0]), float(color[1]), float(color[2]))
+ except (IndexError, TypeError, ValueError) as e:
+ raise ValueError("Shape colors must provide at least three numeric components.") from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/sim/builder.py` around lines 197 - 200, The
_normalize_shape_color helper must validate inputs instead of blindly indexing:
in _normalize_shape_color (and callers like any use of default_shape_color),
check that color is a sequence/iterable of exactly three numeric components,
raise a clear ValueError for malformed inputs (e.g., wrong length or non-numeric
items), and then return the tuple of floats via float(component) conversions;
this ensures malformed values like (1.0, 0.0) produce a descriptive ValueError
rather than an IndexError.
Comment on lines
+1687
to
+1689
| texture, texture_color_space = _find_texture_in_shader(source_shader, prim) | ||
| properties["texture"] = texture | ||
| properties["texture_color_space"] = texture_color_space |
Contributor
There was a problem hiding this comment.
Propagate the resolved texture color space onto the returned Mesh.
These branches now resolve texture_color_space, but get_mesh() still never copies that value into mesh_out._texture_color_space. Direct newton.usd.get_mesh() / Mesh.create_from_usd() callers will therefore fall back to auto even when the USD material explicitly resolved raw.
💡 Suggested fix
mesh_out = Mesh(
points,
faces.flatten(),
normals=normals,
uvs=uvs,
maxhullvert=maxhullvert,
color=material_props.get("color"),
texture=material_props.get("texture"),
metallic=material_props.get("metallic"),
roughness=material_props.get("roughness"),
)
+ if mesh_out.texture is not None:
+ mesh_out._texture_color_space = material_props.get("texture_color_space", TEXTURE_COLOR_SPACE_AUTO)
if return_uv_indices:
return mesh_out, uv_indicesAlso applies to: 1843-1847
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/usd/utils.py` around lines 1687 - 1689, The resolved texture
color space stored in properties["texture_color_space"] is not being propagated
to the returned Mesh; modify get_mesh() (and the equivalent branch around
1843-1847) so after Mesh.create_from_usd(...) (or when populating mesh_out) you
set mesh_out._texture_color_space = properties.get("texture_color_space") (or
the resolved variable) before returning the mesh; ensure this assignment occurs
in both branches that call _find_texture_in_shader/source_shader so callers of
newton.usd.get_mesh() and Mesh.create_from_usd() receive the correct color space
instead of falling back to "auto".
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
## Motivation Seven `add_shape_*` methods (`add_shape_ellipsoid`, `add_shape_box`, `add_shape_capsule`, `add_shape_cylinder`, `add_shape_cone`, `add_shape_heightfield`, `add_shape_gaussian`) used the abbreviated phrase "uses the default per-shape color" for their `color` parameter docstring. This is ambiguous — it could mean the palette sequence or the new `default_shape_color` attribute. Meanwhile `add_shape_plane` and `add_shape_sphere` already used the explicit wording: ``` uses :attr:`default_shape_color` when set, otherwise the per-shape palette color. ``` ## What changed - Replaced the abbreviated `"uses the default per-shape color"` in all seven methods with the explicit two-line form that matches `add_shape_plane` and `add_shape_sphere`. - Mesh-backed shapes (`add_shape_mesh`, `add_shape_convex_hull`) are left unchanged — they correctly document the `:attr:`~newton.Mesh.color`` fallback instead. ## Why this fix is correct This is a doc-only change. All nine non-mesh `add_shape_*` methods delegate to `add_shape()` which implements the `color → default_shape_color → palette` chain. The docstrings now consistently describe that chain. ## Verification - `uv run --extra dev -m newton.tests -k test_shape_colors` — all 9 tests pass - Confirmed zero remaining occurrences of `"uses the default per-shape color"` via grep
…ce property ## Motivation Three groups of review findings addressed: 1. **Copyright year** (#53173, #53185): `newton/_src/utils/color.py` is a brand-new file introduced in this PR (reviewed 2026-04-10) but had `Copyright (c) 2025`. Per AGENTS.md, SPDX lines use the year the file was first created. 2. **Missing public re-export** (#53175, #53183, #53271): The CHANGELOG tells users to "convert any display/sRGB values to linear first" when writing to `Model.shape_color`, but `srgb_to_linear_rgb` and `linear_to_srgb_rgb` only existed in `newton._src.utils.color` — an internal module users must not import per project conventions. Users had no public tool to perform the conversion. 3. **Leaky `_texture_color_space` abstraction** (#53182): The private `_texture_color_space` attribute was accessed from 4 external call sites via `getattr(shape, "_texture_color_space", "auto")`, duplicating the default in every caller with no type safety. ## Changes - **`newton/_src/utils/color.py`**: Update SPDX year to 2026. - **`newton/utils.py`**: Re-export `srgb_to_linear_rgb` and `linear_to_srgb_rgb` in the public `__all__`. - **`CHANGELOG.md`**: Append `with newton.utils.srgb_to_linear_rgb()` to migration note. - **`newton/_src/geometry/types.py`**: Add `texture_color_space` as a `@property` with getter/setter on `Mesh`. Update `copy()` to use the property. - **`newton/_src/sensors/warp_raytrace/render_context.py`**: Replace two `getattr(..., "_texture_color_space", "auto")` calls with `shape.texture_color_space`. - **`newton/_src/viewer/viewer.py`**: Replace `getattr(geo_src, "_texture_color_space", "auto")` with `geo_src.texture_color_space`. - **`newton/_src/utils/import_usd.py`**: Replace `mesh._texture_color_space = ...` with `mesh.texture_color_space = ...`. - **`newton/tests/test_import_usd.py`**: Replace `mesh._texture_color_space` with `mesh.texture_color_space` in two test assertions. ## Verification ```bash uv run --extra dev -m newton.tests -k test_shape_colors # 9 tests OK uv run --extra dev python -m unittest \ newton.tests.test_import_usd.TestImportSampleAssetsComposition.test_visible_collision_mesh_inherits_visual_material_properties \ newton.tests.test_import_usd.TestImportSampleAssetsComposition.test_visible_collision_mesh_preserves_texture_color_space_metadata \ newton.tests.test_import_usd.TestImportSampleAssetsComposition.test_preview_surface_srgb_base_color_converts_to_linear \ newton.tests.test_import_usd.TestImportSampleAssetsComposition.test_display_color_srgb_metadata_converts_to_linear \ newton.tests.test_import_usd.TestImportSampleAssetsComposition.test_omnipbr_diffuse_tint_multiplies_authored_base_color # 5 tests OK uv run python -c "from newton.utils import srgb_to_linear_rgb, linear_to_srgb_rgb; ..." # public import OK uv run python -c "from newton import Mesh; m = Mesh(...); assert m.texture_color_space == 'auto'; ..." # property OK uv run python docs/generate_api.py # newton_utils.rst now includes 20 symbols ```
…lor_space ## Motivation `_texture_color_space` was introduced in this PR as a private attribute on `Mesh`, but every consumer outside `types.py` accessed it through `getattr(shape, "_texture_color_space", "auto")` — a defensive pattern that signals the attribute should just be public. There is no validation, transformation, or lazy-computation reason to hide it behind a `@property`. ## What changed - **`newton/_src/geometry/types.py`**: Renamed `self._texture_color_space` → `self.texture_color_space` in `__init__`, `copy()`, and the `texture` setter. Added an inline docstring on the attribute in `__init__`. - **`newton/_src/sensors/warp_raytrace/render_context.py`**: Replaced two `getattr(shape, "_texture_color_space", "auto")` calls with direct `shape.texture_color_space` access. - **`newton/_src/utils/import_usd.py`**: Changed `mesh._texture_color_space =` to `mesh.texture_color_space =`. - **`newton/_src/viewer/viewer.py`**: Changed `getattr(geo_src, "_texture_color_space", "auto")` to `geo_src.texture_color_space`. - **`newton/tests/test_import_usd.py`**: Updated two assertions from `mesh._texture_color_space` to `mesh.texture_color_space`. ## Why this fix is correct The attribute is a simple string (`"auto"`, `"srgb"`, or `"raw"`) with no invariants beyond what the texture setter resets. A plain public attribute is the simplest correct API. The `texture` setter still resets it to `"auto"` when a new texture is assigned, preserving the existing semantics. ## Verification - `uv run --extra dev -m newton.tests -k test_shape_colors` — 9 tests pass - `uv run --extra dev -m newton.tests -k "test_import_usd.TestImportSampleAssetsComposition"` — 36 tests pass (includes `test_visible_collision_mesh_preserves_texture_color_space_metadata`) - `uv run --extra dev -m newton.tests -k test_visible_collision` — 3 tests pass - Confirmed zero remaining `_texture_color_space` attribute references via grep
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
newton/_src/geometry/types.py (1)
877-879: Normalizetexture_color_spacein the setter to prevent texture-cache fragmentation.Storing raw values allows equivalent metadata tokens to diverge (
None/"auto"/vendor aliases), which can duplicate texture entries downstream.♻️ Proposed fix
-from ..utils.texture import compute_texture_hash +from ..utils.color import normalize_texture_color_space +from ..utils.texture import compute_texture_hash @@ `@texture_color_space.setter` def texture_color_space(self, value: str): - self._texture_color_space = value + self._texture_color_space = normalize_texture_color_space(value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/types.py` around lines 877 - 879, The texture_color_space setter currently stores raw inputs which fragments the texture cache; update the texture_color_space setter to normalize inputs before assigning to self._texture_color_space: coerce None to a canonical token (e.g. "auto"), trim/normalize case, and map known vendor aliases or synonyms (e.g. "srgb", "sRGB", vendor-specific names) to a small set of canonical tokens; implement this normalization inside the texture_color_space property setter so callers use the normalized value consistently.newton/tests/test_import_usd.py (1)
6695-6723: Make the"auto"expectation explicit in the mock.This assertion is currently passing even if
resolve_material_properties_for_prim()regresses, because the mock never returns atexture_color_space. If this test is meant to pin the resolved default, include"texture_color_space": "auto"in the mocked payload instead of relying on downstream fallback.Suggested change
mock.patch( "newton._src.utils.import_usd.usd.resolve_material_properties_for_prim", return_value={ "color": None, "roughness": 0.35, "metallic": 0.75, "texture": "dummy.png", + "texture_color_space": "auto", }, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_import_usd.py` around lines 6695 - 6723, The mock for resolve_material_properties_for_prim used in the test should explicitly include the texture_color_space key to assert the resolved default; update the return_value in the mock patch for "newton._src.utils.import_usd.usd.resolve_material_properties_for_prim" to include "texture_color_space": "auto" so that the assertion comparing builder.shape_source[collision_shape].texture_color_space to "auto" is driven by the mocked payload (affecting the builder.add_usd flow and the shape source created by newton.ModelBuilder).newton/_src/utils/color.py (1)
26-80: Consider upgrading function docstrings to full Google style for user-facing helpers.These docstrings are clear, but adding
Args:/Returns:blocks would align with repository standards for parameterized APIs.As per coding guidelines, "Follow Google-style docstrings. Types in annotations, not docstrings. Use
Args:withname: descriptionformat."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/utils/color.py` around lines 26 - 80, Several public helper functions (srgb_to_linear_rgb, linear_to_srgb_rgb, linear_rgb_to_srgb_uint8, linear_image_to_srgb_uint8, normalize_texture_color_space) use short one-line docstrings; update each to Google-style docstrings that include an "Args:" section (parameter name: brief description, do NOT duplicate type info since types are in annotations) and a "Returns:" section describing the returned value/shape/units, and any raised exceptions (e.g., ValueError from linear_image_to_srgb_uint8). Keep descriptions concise and user-facing, follow existing wording but expand into the Args/Returns format to match repo guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/utils/color.py`:
- Around line 69-75: The code currently classifies "identity" as auto; remove
"identity" from the first conditional tuple (token in ("",
TEXTURE_COLOR_SPACE_AUTO, "unknown", "identity")) and add it to the raw group
tuple used in the second conditional (token in (TEXTURE_COLOR_SPACE_RAW, "data",
"lin_rec709_scene") or token.startswith("lin_")), so that "identity" is treated
as TEXTURE_COLOR_SPACE_RAW; keep the existing startswith checks for lin_ and
srgb_ unchanged.
In `@newton/tests/test_import_usd.py`:
- Line 20: Replace the private-module import with the public re-export: change
the import that currently pulls srgb_to_linear_rgb from newton._src.utils.color
to use the public API (import srgb_to_linear_rgb from newton.utils), so the test
imports the exported helper symbol srgb_to_linear_rgb and will fail if the
public re-export breaks.
---
Nitpick comments:
In `@newton/_src/geometry/types.py`:
- Around line 877-879: The texture_color_space setter currently stores raw
inputs which fragments the texture cache; update the texture_color_space setter
to normalize inputs before assigning to self._texture_color_space: coerce None
to a canonical token (e.g. "auto"), trim/normalize case, and map known vendor
aliases or synonyms (e.g. "srgb", "sRGB", vendor-specific names) to a small set
of canonical tokens; implement this normalization inside the texture_color_space
property setter so callers use the normalized value consistently.
In `@newton/_src/utils/color.py`:
- Around line 26-80: Several public helper functions (srgb_to_linear_rgb,
linear_to_srgb_rgb, linear_rgb_to_srgb_uint8, linear_image_to_srgb_uint8,
normalize_texture_color_space) use short one-line docstrings; update each to
Google-style docstrings that include an "Args:" section (parameter name: brief
description, do NOT duplicate type info since types are in annotations) and a
"Returns:" section describing the returned value/shape/units, and any raised
exceptions (e.g., ValueError from linear_image_to_srgb_uint8). Keep descriptions
concise and user-facing, follow existing wording but expand into the
Args/Returns format to match repo guidelines.
In `@newton/tests/test_import_usd.py`:
- Around line 6695-6723: The mock for resolve_material_properties_for_prim used
in the test should explicitly include the texture_color_space key to assert the
resolved default; update the return_value in the mock patch for
"newton._src.utils.import_usd.usd.resolve_material_properties_for_prim" to
include "texture_color_space": "auto" so that the assertion comparing
builder.shape_source[collision_shape].texture_color_space to "auto" is driven by
the mocked payload (affecting the builder.add_usd flow and the shape source
created by newton.ModelBuilder).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e7c058b6-1793-4655-9c58-9738de1498b1
📒 Files selected for processing (8)
CHANGELOG.mdnewton/_src/geometry/types.pynewton/_src/sensors/warp_raytrace/render_context.pynewton/_src/utils/color.pynewton/_src/utils/import_usd.pynewton/_src/viewer/viewer.pynewton/tests/test_import_usd.pynewton/utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- newton/_src/viewer/viewer.py
- CHANGELOG.md
Comment on lines
+69
to
+75
| if token in ("", TEXTURE_COLOR_SPACE_AUTO, "unknown", "identity"): | ||
| return TEXTURE_COLOR_SPACE_AUTO | ||
| if token in (TEXTURE_COLOR_SPACE_RAW, "data", "lin_rec709_scene") or token.startswith("lin_"): | ||
| return TEXTURE_COLOR_SPACE_RAW | ||
| if token in (TEXTURE_COLOR_SPACE_SRGB, "srgb_rec709_scene", "g22_rec709_scene") or token.startswith("srgb_"): | ||
| return TEXTURE_COLOR_SPACE_SRGB | ||
| return TEXTURE_COLOR_SPACE_AUTO |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In OpenUSD / UsdUVTexture color-space metadata conventions, what does the token "identity" mean? Should it be treated as raw/no-op (linear) rather than sRGB/default display?
💡 Result:
In OpenUSD color-space metadata conventions, the token "identity" means the color space is unknown. It is a backwards-compatibility designation equivalent to the modern "unknown" token. Like "raw", it indicates no color transformation should be applied to the data (treated as raw/no-op, linear). It should not be treated as sRGB or any display color space with gamma correction; instead, pass the values through unchanged, as linear data.
Citations:
- 1: https://openusd.org/dev/user_guides/color_user_guide.html
- 2: https://www.openusd.org/dev/user_guides/color_user_guide.html
- 3: https://graphics.pixar.com/usd/release/user_guides/color_user_guide.html
- 4: https://openusd.org/release/user_guides/color_user_guide.html
🏁 Script executed:
# Read the full function around lines 69-85 to see the complete logic
wc -l newton/_src/utils/color.pyRepository: newton-physics/newton
Length of output: 1893
🏁 Script executed:
# Get context around lines 69-85 to see the complete color space mapping logic
sed -n '50,100p' newton/_src/utils/color.pyRepository: newton-physics/newton
Length of output: 1893
Move "identity" to the raw color-space group per OpenUSD conventions.
Per OpenUSD color-space specifications, "identity" is a backwards-compatibility token meaning "no color transformation"—equivalent to "unknown" but correctly classified as linear (raw), not auto. The current code incorrectly groups it with auto-mapped tokens, which causes downstream sRGB conversion.
Required fix
- if token in ("", TEXTURE_COLOR_SPACE_AUTO, "unknown", "identity"):
+ if token in ("", TEXTURE_COLOR_SPACE_AUTO, "unknown"):
return TEXTURE_COLOR_SPACE_AUTO
- if token in (TEXTURE_COLOR_SPACE_RAW, "data", "lin_rec709_scene") or token.startswith("lin_"):
+ if token in ("identity", TEXTURE_COLOR_SPACE_RAW, "data", "lin_rec709_scene") or token.startswith("lin_"):
return TEXTURE_COLOR_SPACE_RAW📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if token in ("", TEXTURE_COLOR_SPACE_AUTO, "unknown", "identity"): | |
| return TEXTURE_COLOR_SPACE_AUTO | |
| if token in (TEXTURE_COLOR_SPACE_RAW, "data", "lin_rec709_scene") or token.startswith("lin_"): | |
| return TEXTURE_COLOR_SPACE_RAW | |
| if token in (TEXTURE_COLOR_SPACE_SRGB, "srgb_rec709_scene", "g22_rec709_scene") or token.startswith("srgb_"): | |
| return TEXTURE_COLOR_SPACE_SRGB | |
| return TEXTURE_COLOR_SPACE_AUTO | |
| if token in ("", TEXTURE_COLOR_SPACE_AUTO, "unknown"): | |
| return TEXTURE_COLOR_SPACE_AUTO | |
| if token in ("identity", TEXTURE_COLOR_SPACE_RAW, "data", "lin_rec709_scene") or token.startswith("lin_"): | |
| return TEXTURE_COLOR_SPACE_RAW | |
| if token in (TEXTURE_COLOR_SPACE_SRGB, "srgb_rec709_scene", "g22_rec709_scene") or token.startswith("srgb_"): | |
| return TEXTURE_COLOR_SPACE_SRGB | |
| return TEXTURE_COLOR_SPACE_AUTO |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/utils/color.py` around lines 69 - 75, The code currently
classifies "identity" as auto; remove "identity" from the first conditional
tuple (token in ("", TEXTURE_COLOR_SPACE_AUTO, "unknown", "identity")) and add
it to the raw group tuple used in the second conditional (token in
(TEXTURE_COLOR_SPACE_RAW, "data", "lin_rec709_scene") or
token.startswith("lin_")), so that "identity" is treated as
TEXTURE_COLOR_SPACE_RAW; keep the existing startswith checks for lin_ and srgb_
unchanged.
| from newton import BodyFlags, JointType | ||
| from newton._src.geometry.flags import ShapeFlags | ||
| from newton._src.geometry.utils import transform_points | ||
| from newton._src.utils.color import srgb_to_linear_rgb |
Contributor
There was a problem hiding this comment.
Import the public conversion helper here.
Line 20 reaches into newton._src, so these tests bypass the new newton.utils.srgb_to_linear_rgb re-export this PR is adding. A broken public export would still pass this file.
Suggested change
-from newton._src.utils.color import srgb_to_linear_rgb
+from newton.utils import srgb_to_linear_rgb🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/tests/test_import_usd.py` at line 20, Replace the private-module
import with the public re-export: change the import that currently pulls
srgb_to_linear_rgb from newton._src.utils.color to use the public API (import
srgb_to_linear_rgb from newton.utils), so the test imports the exported helper
symbol srgb_to_linear_rgb and will fail if the public re-export breaks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Keeping this PR as a draft until #2343 has been merged.
## Description
ModelBuilder/Modelshape colors in linear RGB and document that conventionSensorTiledCamera.RenderConfig.encode_output_srgbto control packed output encodingChecklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Bug fix
Steps to reproduce:
builder.shape_coloror render throughSensorTiledCamera.Minimal reproduction:
New feature / API change
Summary by CodeRabbit
New Features
Changed
Bug Fixes
Tests