Refactor all color properties to linear space #2656
Merged
Conversation
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
## Walkthrough
This update removes all color space conversions between sRGB and linear space across lighting, material, shader uniform, particle, and post-processing modules. Color values are now assigned and stored directly without transformation. Documentation is updated to clarify the color space for the bloom threshold property. The default background solid color is changed to a darker gray, and a light intensity value is adjusted in multiple test and example cases.
## Changes
| Files | Change Summary |
|----------------------------------------------------------------------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------|
| packages/core/src/lighting/DirectLight.ts<br>packages/core/src/lighting/PointLight.ts<br>packages/core/src/lighting/SpotLight.ts | Removed sRGB-to-linear color conversion when assigning light colors; now assigns raw RGB values directly. |
| packages/core/src/particle/modules/ParticleGradient.ts<br>packages/core/src/particle/ParticleGenerator.ts | Eliminated sRGB-to-linear conversion for gradient color keys and particle start colors; raw color channels are stored/used directly.|
| packages/core/src/postProcess/PostProcessUberPass.ts | Removed sRGB-to-linear conversion for the bloom threshold; threshold value is now used directly. |
| packages/core/src/postProcess/effects/BloomEffect.ts | Updated documentation to state the threshold is expressed in linear space instead of gamma-space; default threshold changed. |
| packages/core/src/shader/ShaderUniform.ts | Removed sRGB-to-linear conversion when uploading uniform color values; raw color components are passed to WebGL uniforms. |
| packages/loader/src/gltf/extensions/KHR_materials_pbrSpecularGlossiness.ts<br>packages/loader/src/gltf/extensions/KHR_materials_sheen.ts<br>packages/loader/src/gltf/extensions/KHR_materials_volume.ts<br>packages/loader/src/gltf/parser/GLTFMaterialParser.ts | Removed linear-to-sRGB color conversions when assigning material color properties; now uses raw color factors directly. |
| packages/core/src/RenderPipeline/BasicRenderPipeline.ts | Refactored background color clearing to remove linear space conversion and use premultiplied color directly. |
| packages/core/src/Background.ts | Changed default private solid color to a darker gray with precise float values. |
| packages/core/src/lighting/AmbientLight.ts | Changed initial diffuse solid color to a darker RGB value. |
| packages/core/src/lighting/Light.ts | Removed intensity property and related method `_getLightIntensityColor()` from Light class. |
| packages/core/src/lighting/LightManager.ts | Changed intensity-based brightness calculation to use only color brightness for sunlight selection. |
| packages/loader/src/gltf/extensions/KHR_lights_punctual.ts | Removed separate intensity property; embedded intensity into light color RGB values directly. |
| e2e/case/* (multiple animator, primitive, camera, gltf, material, and other test cases) | Replaced DirectLight intensity assignments with color assignments using specific gray color values (~0.214 or ~0.3185). |
| examples/* (multiple example files) | Adjusted DirectLight intensity assignments similarly to test cases, replaced by explicit color assignments. |
| e2e/case/particleRenderer-dream.ts<br>e2e/case/particleRenderer-force.ts | Updated particle and background colors to more precise floating-point values instead of approximate fractions. |
| e2e/case/particleRenderer-shape-mesh.ts<br>e2e/case/particleRenderer-textureSheetAnimation.ts | Adjusted background and ambient light colors to darker or more precise float values; updated particle start color ranges. |
| e2e/case/camera-fxaa.ts<br>e2e/case/multi-camera-no-clear.ts<br>e2e/case/multi-scene-clear.ts<br>e2e/case/multi-scene-no-clear.ts<br>e2e/case/postProcess-HDR-bloom-ACES.ts<br>e2e/case/postProcess-HDR-bloom-neutral.ts<br>e2e/case/postProcess-LDR-bloom-neutral.ts<br>e2e/case/postProcess-customPass.ts | Adjusted bloom effect threshold values to new precise floating-point values; no other logic changes. |
| tests/src/core/Light.test.ts | Removed tests related to Light intensity property; adjusted assertions to reflect removal of intensity. |
| tests/src/core/postProcess/PostProcess.test.ts | Updated expected default bloom threshold value from 0.9 to 0.8 in test assertion. |
| tests/src/loader/GLTFLoader.test.ts | Removed SpotLight intensity assertion; adjusted expected SpotLight color to less intense red. |
| packages/core/src/Scene.ts | Changed initial fog color to a darker gray value. |
| packages/core/src/sky/SkyProceduralMaterial.ts | Changed default skyTint and groundTint colors to darker tones. |
| packages/ui/src/component/interactive/transition/ColorTransition.ts | Updated default hover, pressed, and disabled colors to more precise decimal values. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant GLTFLoader
participant Material
participant ShaderUniform
participant Renderer
GLTFLoader->>Material: Assign color factors (no color space conversion)
Material->>ShaderUniform: Pass raw color values
ShaderUniform->>Renderer: Upload raw color values to GPUsequenceDiagram
participant Light
participant ShaderUniform
participant Renderer
Light->>ShaderUniform: Assign light color (no sRGB-to-linear conversion)
ShaderUniform->>Renderer: Upload raw color to GPU
Possibly related PRs
Suggested reviewers
Poem
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.5 #2656 +/- ##
===========================================
+ Coverage 68.68% 69.03% +0.35%
===========================================
Files 971 974 +3
Lines 102088 102699 +611
Branches 8781 8909 +128
===========================================
+ Hits 70119 70899 +780
+ Misses 31703 31532 -171
- Partials 266 268 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
226-232: Streamlined premultiplication for background clear colorThe code now properly guards the clear operation with an explicit null check and eliminates the unnecessary color space conversion. Premultiplication is done directly without creating a temporary color object.
Consider updating the comment on line 103 which mentions converting from sRGB to linear color space, as this conversion is no longer performed after this refactoring.
-// Because internal render target is linear color space, so we should convert srgb background color to linear color space +// Background solid color is premultiplied before clearing the render target
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts(1 hunks)packages/core/src/particle/ParticleGenerator.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/particle/ParticleGenerator.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
- GitHub Check: build (22.x, windows-latest)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
e2e/case/camera-fxaa.ts (1)
20-20: Missing semicolon at end of statement.
ESLint (prettier/prettier) flags a missing semicolon after the assignment tobloomEffect.threshold.value.- bloomEffect.threshold.value = 0.7874122893956174 + bloomEffect.threshold.value = 0.7874122893956174;🧰 Tools
🪛 ESLint
[error] 20-20: Insert
;(prettier/prettier)
examples/compressed-texture.ts (1)
36-36: Avoid magic number; derive linear intensity from sRGB value.
The literal0.21404114048223255is the approximate linear‐space equivalent of a 0.5 sRGB intensity (likelyMath.pow(0.5, 2.2)). For clarity and maintainability, consider computing it at runtime or defining a named constant (e.g.,const DEFAULT_LINEAR_INTENSITY = Math.pow(BASE_INTENSITY_SRGB, GAMMA);).Normalize indentation to 2 spaces.
Prettier flags aprettier/prettiererror here because this line is indented with 4 spaces instead of 2. Please adjust to match the file's indentation style.🧰 Tools
🪛 ESLint
[error] 36-36: Replace
····with··(prettier/prettier)
examples/pbr-base.ts (1)
33-33: Fix spacing around assignment operator.
Prettier reports a formatting issue: missing space after=. Update the line to:-directLight.intensity =0.21404114048223255; +directLight.intensity = 0.21404114048223255;This ensures consistency with the project's style rules.
🧰 Tools
🪛 ESLint
[error] 33-33: Insert
·(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
e2e/case/animator-crossfade.ts(2 hunks)e2e/case/animator-customAnimationClip.ts(1 hunks)e2e/case/animator-event.ts(1 hunks)e2e/case/animator-play-backwards.ts(1 hunks)e2e/case/animator-play-beforeActive.ts(1 hunks)e2e/case/animator-play.ts(1 hunks)e2e/case/animator-reuse.ts(1 hunks)e2e/case/animator-stateMachine.ts(1 hunks)e2e/case/animator-stateMachineScript.ts(1 hunks)e2e/case/camera-fxaa.ts(1 hunks)e2e/case/camera-opaque-texture.ts(1 hunks)e2e/case/gltf-meshopt.ts(1 hunks)e2e/case/material-pbr.ts(1 hunks)e2e/case/primitive-capsule.ts(1 hunks)e2e/case/primitive-cone.ts(1 hunks)e2e/case/primitive-cuboid.ts(1 hunks)e2e/case/primitive-cylinder.ts(1 hunks)e2e/case/primitive-plane.ts(1 hunks)e2e/case/primitive-sphere.ts(1 hunks)e2e/case/primitive-torus.ts(1 hunks)examples/ambient-light.ts(1 hunks)examples/animation-customAnimationClip.ts(1 hunks)examples/animation-event.ts(1 hunks)examples/animation-stateMachineScript.ts(1 hunks)examples/compressed-texture.ts(1 hunks)examples/pbr-base.ts(1 hunks)examples/primitive-mesh.ts(1 hunks)examples/scene-basic.ts(1 hunks)examples/skeleton-animation-additive.ts(1 hunks)examples/skeleton-animation-crossfade.ts(1 hunks)examples/skeleton-animation-play.ts(1 hunks)examples/skeleton-animation-reuse.ts(1 hunks)
✅ Files skipped from review due to trivial changes (29)
- e2e/case/animator-reuse.ts
- e2e/case/primitive-cone.ts
- e2e/case/animator-play-backwards.ts
- e2e/case/material-pbr.ts
- e2e/case/camera-opaque-texture.ts
- e2e/case/animator-event.ts
- examples/scene-basic.ts
- e2e/case/animator-play-beforeActive.ts
- examples/ambient-light.ts
- e2e/case/primitive-capsule.ts
- examples/animation-event.ts
- examples/skeleton-animation-reuse.ts
- examples/skeleton-animation-additive.ts
- e2e/case/animator-play.ts
- e2e/case/primitive-sphere.ts
- e2e/case/animator-stateMachine.ts
- e2e/case/primitive-cuboid.ts
- examples/skeleton-animation-play.ts
- e2e/case/primitive-cylinder.ts
- examples/skeleton-animation-crossfade.ts
- e2e/case/gltf-meshopt.ts
- examples/animation-customAnimationClip.ts
- e2e/case/animator-stateMachineScript.ts
- examples/animation-stateMachineScript.ts
- examples/primitive-mesh.ts
- e2e/case/primitive-torus.ts
- e2e/case/animator-crossfade.ts
- e2e/case/animator-customAnimationClip.ts
- e2e/case/primitive-plane.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
examples/compressed-texture.ts (1)
packages/core/src/lighting/DirectLight.ts (1)
DirectLight(9-82)
🪛 ESLint
e2e/case/camera-fxaa.ts
[error] 20-20: Insert ;
(prettier/prettier)
examples/compressed-texture.ts
[error] 36-36: Replace ···· with ··
(prettier/prettier)
examples/pbr-base.ts
[error] 33-33: Insert ·
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: codecov
- GitHub Check: e2e (22.x)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
e2e/case/particleRenderer-dream.ts (4)
104-105: Document or extract magic color constants in debris particle
The hard-coded linear-space values1.0and0.004024717018496307may confuse readers. Consider adding a comment explaining their sRGB origin or extracting them into named constants (e.g.DEBRIS_COLOR_MIN,DEBRIS_COLOR_MAX) for clarity.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 104-105: e2e/case/particleRenderer-dream.ts#L104-L105
Added lines #L104 - L105 were not covered by tests
177-178: Precompute alpha normalization instead of inline division
Using64 / 255and128 / 255at runtime obscures intent. Replace with their decimal equivalents (e.g.,0.25098,0.50196) to make the alpha values immediately clear:-main.startColor.constantMin = new Color(0, 0.33716361504833037, 1.0, 64 / 255); +main.startColor.constantMin = new Color(0, 0.33716361504833037, 1.0, 0.25098);🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 177-178: e2e/case/particleRenderer-dream.ts#L177-L178
Added lines #L177 - L178 were not covered by tests
237-237: Simplify full alpha division
255 / 255always equals1.0, so it can be replaced directly for readability:-main.startColor.constant = new Color(0.018500220128379697, 0.23455058216100522, 1.0, 255 / 255); +main.startColor.constant = new Color(0.018500220128379697, 0.23455058216100522, 1.0, 1.0);🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 237-237: e2e/case/particleRenderer-dream.ts#L237
Added line #L237 was not covered by tests
298-299: Extract highlight particle color values to constants
This block contains several magic floats for constantMin/Max and alpha normalization. Defining them as named constants at the top (e.g.HIGHLIGHT_COLOR_MIN,HIGHLIGHT_COLOR_MAX) will centralize these values and improve maintainability.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 298-299: e2e/case/particleRenderer-dream.ts#L298-L299
Added lines #L298 - L299 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e/case/particleRenderer-dream.ts(5 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
e2e/case/particleRenderer-dream.ts
[warning] 34-34: e2e/case/particleRenderer-dream.ts#L34
Added line #L34 was not covered by tests
[warning] 104-105: e2e/case/particleRenderer-dream.ts#L104-L105
Added lines #L104 - L105 were not covered by tests
[warning] 177-178: e2e/case/particleRenderer-dream.ts#L177-L178
Added lines #L177 - L178 were not covered by tests
[warning] 237-237: e2e/case/particleRenderer-dream.ts#L237
Added line #L237 was not covered by tests
[warning] 298-299: e2e/case/particleRenderer-dream.ts#L298-L299
Added lines #L298 - L299 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (1)
e2e/case/particleRenderer-dream.ts (1)
34-34: Ensure E2E baseline screenshot is updated for the new background color
The background is now set to a darker linear-space color (0.004776953 …, 0.01938236 …, 0.006048833 …, 1). Please verify that the screenshot baseline used byinitScreenshot/updateForE2Ehas been updated accordingly to avoid snapshot test failures.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-34: e2e/case/particleRenderer-dream.ts#L34
Added line #L34 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
e2e/case/particleRenderer-force.ts (1)
385-387:⚠️ Potential issueMissed refactoring of color values
These color values haven't been updated to the new linear space representation with precise decimals. They still use the division by 255 format, which is inconsistent with the other color changes in this file.
- main.startColor.constantMin.set(105 / 255, 198 / 255, 255 / 255, 64 / 255); - main.startColor.constantMax.set(13 / 255, 255 / 255, 0 / 255, 32 / 255); + main.startColor.constantMin.set(0.4117647058823529, 0.7764705882352941, 1.0, 0.25098039215686274); + main.startColor.constantMax.set(0.050980392156862744, 1.0, 0.0, 0.12549019607843137);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
e2e/fixtures/originImage/Particle_particleRenderer-dream.jpgis excluded by!**/*.jpge2e/fixtures/originImage/Particle_particleRenderer-force.jpgis excluded by!**/*.jpg
📒 Files selected for processing (1)
e2e/case/particleRenderer-force.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (2)
e2e/case/particleRenderer-force.ts (2)
38-38: Color representation updated to linear spaceThe background colors for both left and right scenes have been updated to use precise floating-point values instead of the previous sRGB-based division representation. This is consistent with the PR's objective to refactor all color properties to linear space.
Also applies to: 43-43
136-138: Standardized color representation for particle start colorsThe startColor values have been updated to use linear space color values with precise decimal representation rather than sRGB-based color representations.
| @@ -306,7 +306,7 @@ | |||
| main.startRotationZ.constantMax = 360; | |||
| main.startRotationZ.mode = ParticleCurveMode.TwoConstants; | |||
|
|
|||
| main.startColor.constant = new Color(37 / 255, 133 / 255, 255 / 255, 255 / 255); | |||
| main.startColor.constant = new Color(0.018500220128379697, 0.23455058216100522, 1.0, 255 / 255); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent alpha component format
The RGB components use precise decimal representation, but the alpha component still uses division format (255/255). For consistency with the PR's refactoring goal, consider using the decimal value directly.
- main.startColor.constant = new Color(0.018500220128379697, 0.23455058216100522, 1.0, 255 / 255);
+ main.startColor.constant = new Color(0.018500220128379697, 0.23455058216100522, 1.0, 1.0);📝 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
| main.startColor.constant = new Color(0.018500220128379697, 0.23455058216100522, 1.0, 255 / 255); | |
| main.startColor.constant = new Color(0.018500220128379697, 0.23455058216100522, 1.0, 1.0); |
Comment on lines
+230
to
232
| main.startColor.constantMin = new Color(0.0, 0.33716361504833037, 1.0, 64 / 255); | ||
| main.startColor.constantMax = new Color(0.004024717018496307, 1.0, 0.0, 128 / 255); | ||
| main.startColor.mode = ParticleGradientMode.TwoConstants; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent color representation format
While the RGB components are now represented as precise decimals in linear space, the alpha component is still using the division format (64/255, 128/255). This is inconsistent with the PR's objective to standardize color representations.
- main.startColor.constantMin = new Color(0.0, 0.33716361504833037, 1.0, 64 / 255);
- main.startColor.constantMax = new Color(0.004024717018496307, 1.0, 0.0, 128 / 255);
+ main.startColor.constantMin = new Color(0.0, 0.33716361504833037, 1.0, 0.25098039215686274);
+ main.startColor.constantMax = new Color(0.004024717018496307, 1.0, 0.0, 0.5019607843137255);📝 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
| main.startColor.constantMin = new Color(0.0, 0.33716361504833037, 1.0, 64 / 255); | |
| main.startColor.constantMax = new Color(0.004024717018496307, 1.0, 0.0, 128 / 255); | |
| main.startColor.mode = ParticleGradientMode.TwoConstants; | |
| main.startColor.constantMin = new Color(0.0, 0.33716361504833037, 1.0, 0.25098039215686274); | |
| main.startColor.constantMax = new Color(0.004024717018496307, 1.0, 0.0, 0.5019607843137255); | |
| main.startColor.mode = ParticleGradientMode.TwoConstants; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/case/particleRenderer-shape-mesh.ts (1)
36-36: Consider adding comments to explain the color space conversion.While the conversion to linear color space is technically correct, these precise floating-point values (0.004776953480693729 and 0.004024717018496307) might confuse future developers who aren't aware of the color space refactoring. Consider adding comments to clarify that these are linear space values.
Also applies to: 94-95
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/case/particleRenderer-shape-mesh.ts(2 hunks)e2e/case/particleRenderer-textureSheetAnimation.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- e2e/case/particleRenderer-textureSheetAnimation.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (2)
e2e/case/particleRenderer-shape-mesh.ts (2)
36-36: Background color converted to linear space.The background color has been updated to a very dark gray using precise floating-point values in linear color space instead of sRGB values, which aligns with the PR objective of refactoring color properties to linear space.
94-96: Particle start color values converted to linear space.The particle system's start color range has been updated to use linear color space values instead of sRGB values. The white color remains visually the same (as 1.0 values are identical in both spaces), while the green tint (second color) now uses a more precise linear space representation for the red channel.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
packages/core/src/lighting/LightManager.ts (1)
180-206: 🛠️ Refactor suggestionUpdate the method documentation to reflect the new brightness calculation.
The method
_getMaxBrightestSunlighthas been changed to use only color brightness instead of intensity*brightness for determining the brightest light. Please update the JSDoc comment to reflect this change in behavior./** * @internal + * Determines the brightest directional light to be used as sunlight. + * Lights with shadows are prioritized. Brightness is determined by the light's color brightness. */ _getMaxBrightestSunlight(): DirectLight | null {packages/loader/src/gltf/extensions/KHR_lights_punctual.ts (1)
17-30: 💡 Verification agent🧩 Analysis chain
Verify that existing GLTF assets will render consistently
While the change itself is well-implemented, it's worth verifying that existing GLTF assets will still render as expected. Since intensity is now directly multiplied into the color values instead of being maintained as a separate property, make sure this approach is compatible with how GLTF files are typically authored.
🏁 Script executed:
#!/bin/bash # Check how many GLTF-related test files might be affected by this change echo "Searching for GLTF test files that might be affected by light intensity changes:" fd -e ts -e js | grep -i test | grep -i gltf | xargs grep -l "intensity" || echo "No files found with intensity in GLTF tests" # Look for example code that demonstrates loading GLTF models with lights echo "Searching for example files that load GLTF models with lights:" fd -e ts -e js | grep -i example | xargs grep -l "KHR_lights_punctual" || echo "No example files found with KHR_lights_punctual"Length of output: 590
I’d like to inspect the GLTF loader tests to confirm how they verify light intensity is handled. Let’s pull the relevant sections from
tests/src/loader/GLTFLoader.test.ts.
🏁 Script executed:
#!/bin/bash echo "Context for KHR_lights_punctual tests:" grep -n -C5 "KHR_lights_punctual" tests/src/loader/GLTFLoader.test.ts || echo "No KHR extension tests found" echo echo "Context for intensity in tests:" grep -n -C5 "intensity" tests/src/loader/GLTFLoader.test.ts || echo "No intensity references found"Length of output: 2728
Restore light.intensity or update tests for KHR_lights_punctual
The loader now bakes
intensityintolight.colorbut no longer assignslight.intensity, which breaks these assertions in
tests/src/loader/GLTFLoader.test.ts(around lines 431–433):
expect(directLight.intensity).to.equal(0.5);expect(directLight.color).to.deep.equal(new Color(1, 0, 0, 1));Choose one of the following fixes:
• Option A: Reintroduce the intensity property in
packages/loader/src/gltf/extensions/KHR_lights_punctual.ts// after `light = entity.addComponent(...)` light.intensity = intensity; if (color) { light.color.set(color[0] * intensity, color[1] * intensity, color[2] * intensity, 1); }• Option B: Update the test to match the new behavior
- expect(directLight.intensity).to.equal(0.5); - expect(directLight.color).to.deep.equal(new Color(1, 0, 0, 1)); + // intensity is baked into color; default intensity remains 1 + // expect(directLight.intensity).to.equal(1); + expect(directLight.color).to.deep.equal(new Color(0.5, 0, 0, 1));Please apply the appropriate change so tests align with the updated loader.
🧹 Nitpick comments (1)
packages/core/src/lighting/LightManager.ts (1)
192-201: RenamemaxIntensitytomaxBrightnessfor consistency.The variable name at line 184 (
maxIntensity) doesn't match the concept it now represents. Since you've changed from using intensity*brightness to using only brightness, and renamed the calculation variable tobrightness, you should also renamemaxIntensitytomaxBrightnessfor consistency.- let maxIntensity = Number.NEGATIVE_INFINITY; + let maxBrightness = Number.NEGATIVE_INFINITY; let hasShadowLight = false; for (let i = 0, n = directLights.length; i < n; i++) { const currentLight = directLights.get(i); if (currentLight.shadowType !== ShadowType.None && !hasShadowLight) { - maxIntensity = Number.NEGATIVE_INFINITY; + maxBrightness = Number.NEGATIVE_INFINITY; hasShadowLight = true; } const brightness = currentLight.color.getBrightness(); if (hasShadowLight) { - if (currentLight.shadowType !== ShadowType.None && maxIntensity < brightness) { - maxIntensity = brightness; + if (currentLight.shadowType !== ShadowType.None && maxBrightness < brightness) { + maxBrightness = brightness; sunlight = currentLight; } } else { - if (maxIntensity < brightness) { - maxIntensity = brightness; + if (maxBrightness < brightness) { + maxBrightness = brightness; sunlight = currentLight; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
e2e/case/animator-additive.ts(2 hunks)e2e/case/animator-crossfade.ts(2 hunks)e2e/case/animator-customAnimationClip.ts(1 hunks)e2e/case/animator-event.ts(3 hunks)e2e/case/animator-play-backwards.ts(2 hunks)e2e/case/animator-play-beforeActive.ts(2 hunks)e2e/case/animator-play.ts(2 hunks)e2e/case/animator-reuse.ts(2 hunks)e2e/case/animator-stateMachine.ts(2 hunks)e2e/case/animator-stateMachineScript.ts(2 hunks)e2e/case/camera-opaque-texture.ts(2 hunks)e2e/case/gltf-meshopt.ts(2 hunks)e2e/case/material-pbr.ts(2 hunks)e2e/case/primitive-capsule.ts(2 hunks)e2e/case/primitive-cone.ts(2 hunks)e2e/case/primitive-cuboid.ts(2 hunks)e2e/case/primitive-cylinder.ts(2 hunks)e2e/case/primitive-plane.ts(2 hunks)e2e/case/primitive-sphere.ts(2 hunks)e2e/case/primitive-torus.ts(2 hunks)examples/ambient-light.ts(4 hunks)examples/animation-customAnimationClip.ts(1 hunks)examples/animation-event.ts(4 hunks)examples/animation-stateMachineScript.ts(5 hunks)examples/compressed-texture.ts(2 hunks)examples/pbr-base.ts(5 hunks)examples/primitive-mesh.ts(3 hunks)examples/scene-basic.ts(2 hunks)examples/skeleton-animation-additive.ts(4 hunks)examples/skeleton-animation-crossfade.ts(3 hunks)examples/skeleton-animation-play.ts(3 hunks)examples/skeleton-animation-reuse.ts(4 hunks)packages/core/src/lighting/DirectLight.ts(2 hunks)packages/core/src/lighting/L 10000 ight.ts(0 hunks)packages/core/src/lighting/LightManager.ts(2 hunks)packages/core/src/lighting/PointLight.ts(1 hunks)packages/core/src/lighting/SpotLight.ts(2 hunks)packages/loader/src/gltf/extensions/KHR_lights_punctual.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/lighting/Light.ts
✅ Files skipped from review due to trivial changes (4)
- examples/primitive-mesh.ts
- e2e/case/animator-crossfade.ts
- examples/animation-event.ts
- examples/animation-stateMachineScript.ts
🚧 Files skipped from review as they are similar to previous changes (31)
- e2e/case/primitive-capsule.ts
- examples/scene-basic.ts
- e2e/case/primitive-torus.ts
- e2e/case/animator-play-backwards.ts
- e2e/case/material-pbr.ts
- e2e/case/primitive-plane.ts
- e2e/case/animator-play-beforeActive.ts
- e2e/case/animator-additive.ts
- e2e/case/animator-stateMachineScript.ts
- e2e/case/primitive-cylinder.ts
- examples/animation-customAnimationClip.ts
- e2e/case/primitive-sphere.ts
- e2e/case/animator-event.ts
- e2e/case/animator-reuse.ts
- packages/core/src/lighting/SpotLight.ts
- e2e/case/camera-opaque-texture.ts
- e2e/case/animator-stateMachine.ts
- e2e/case/animator-customAnimationClip.ts
- packages/core/src/lighting/DirectLight.ts
- e2e/case/animator-play.ts
- examples/ambient-light.ts
- e2e/case/primitive-cuboid.ts
- e2e/case/primitive-cone.ts
- packages/core/src/lighting/PointLight.ts
- examples/skeleton-animation-play.ts
- examples/skeleton-animation-additive.ts
- examples/compressed-texture.ts
- examples/skeleton-animation-crossfade.ts
- examples/skeleton-animation-reuse.ts
- e2e/case/gltf-meshopt.ts
- examples/pbr-base.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/loader/src/gltf/extensions/KHR_lights_punctual.ts (1)
packages/core/src/lighting/Light.ts (2)
color(53-55)color(57-61)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (2)
packages/core/src/lighting/LightManager.ts (1)
5-6: Good refactoring of import statements.Reordering the imports to place interfaces before classes follows good TypeScript practices and improves code organization.
packages/loader/src/gltf/extensions/KHR_lights_punctual.ts (1)
29-31: Looks good: Intensity is now embedded in the color valuesThe change correctly implements the new approach of embedding light intensity directly into the color components, aligning with the broader refactoring to use linear color space consistently throughout the codebase. The intensity value is now multiplied with each RGB component when setting the light color, which eliminates the need for a separate intensity property.
Comment on lines
+192
to
201
| const brightness = currentLight.color.getBrightness(); | ||
| if (hasShadowLight) { | ||
| if (currentLight.shadowType !== ShadowType.None && maxIntensity < intensity) { | ||
| maxIntensity = intensity; | ||
| if (currentLight.shadowType !== ShadowType.None && maxIntensity < brightness) { | ||
| maxIntensity = brightness; | ||
| sunlight = currentLight; | ||
| } | ||
| } else { | ||
| if (maxIntensity < intensity) { | ||
| maxIntensity = intensity; | ||
| if (maxIntensity < brightness) { | ||
| maxIntensity = brightness; | ||
| sunlight = currentLight; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the impact of using only color brightness for sunlight selection.
This change removes the intensity factor from the sunlight selection algorithm, which could affect which light is chosen as the sunlight in existing scenes. Please ensure this behavior change is intentional and properly tested.
🏁 Script executed:
#!/bin/bash
# Find all instances where light intensity was previously set in examples and test cases
# to help identify places that might be affected by the change
echo "Searching for DirectLight intensity usage in examples and test files:"
rg -g "*.ts" -g "*.tsx" "directLight.*intensity.*=" --no-ignoreLength of output: 539
Update tests & examples for sunlight selection logic
The sunlight selection now relies solely on color.getBrightness(), removing the intensity factor. This change will alter which light is chosen as “sunlight” in existing tests and demos. Please update or add assertions in the following locations to reflect the new behavior:
- tests/src/core/Shader.test.ts
- tests/src/core/DeviceLost.test.ts
- tests/src/core/Light.test.ts
- examples/physx-raycast.ts
- e2e/case/animator-blendShape-quantization.ts
Also consider adding a dedicated test case that verifies sunlight is selected by maximum brightness alone.
hhhhkrx
approved these changes
May 13, 2025
zhuxudong
reviewed
May 16, 2025
| Color.linearToSRGBSpace(baseColorFactor[2]), | ||
| baseColorFactor[3] | ||
| ); | ||
| material.baseColor = new Color(baseColorFactor[0], baseColorFactor[1], baseColorFactor[2], baseColorFactor[3]); |
|
add |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
Bug Fixes
Documentation