Remove gamma color space #2565
Remove gamma color space #2565
Conversation
WalkthroughThe changes remove conditional shader logic for gamma color space conversion across multiple modules. Shader code now always applies gamma-to-linear and linear-to-gamma conversions, eliminating preprocessor checks. This update spans end-to-end tests, examples, lighting, particle systems, and shader uniforms. In addition, engine configuration has been simplified by removing the configurable color space property and associated enumeration from core engine modules. Changes
Sequence Diagram(s)sequenceDiagram
participant S as Shader
participant G as Gamma Converter
S->>G: Apply gammaToLinear(baseColor)
G-->>S: Return linear baseColor
S->>G: Apply linearToGamma(finalColor)
G-->>S: Return gamma-corrected finalColor
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (3)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
the
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
packages/core/src/shader/ShaderUniform.ts (3)
41-60:⚠️ Potential issueFix caching logic for gamma-corrected values.
While the gamma correction changes align with the PR objective, the caching logic is inconsistent. The cache stores the original values but compares them against gamma-corrected values, which could lead to unnecessary uploads.
Apply this diff to fix the caching logic:
if ((<Color>value).r !== undefined) { - if (cacheValue.x !== (<Color>value).r || cacheValue.y !== (<Color>value).g) { + const r = Color.gammaToLinearSpace((<Color>value).r); + const g = Color.gammaToLinearSpace((<Color>value).g); + if (cacheValue.x !== r || cacheValue.y !== g) { this._gl.uniform2f( shaderUniform.location, - Color.gammaToLinearSpace((<Color>value).r), - Color.gammaToLinearSpace((<Color>value).g) + r, + g ); - cacheValue.x = (<Color>value).r; - cacheValue.y = (<Color>value).g; + cacheValue.x = r; + cacheValue.y = g; } }
66-92:⚠️ Potential issueFix caching logic for gamma-corrected values.
The caching logic is inconsistent. The cache stores the original values but compares them against gamma-corrected values.
Apply this diff to fix the caching logic:
if ((<Color>value).r !== undefined) { - if (cacheValue.x !== (<Color>value).r || cacheValue.y !== (<Color>value).g || cacheValue.z !== (<Color>value).b) { + const r = Color.gammaToLinearSpace((<Color>value).r); + const g = Color.gammaToLinearSpace((<Color>value).g); + const b = Color.gammaToLinearSpace((<Color>value).b); + if (cacheValue.x !== r || cacheValue.y !== g || cacheValue.z !== b) { this._gl.uniform3f( shaderUniform.location, - Color.gammaToLinearSpace((<Color>value).r), - Color.gammaToLinearSpace((<Color>value).g), - Color.gammaToLinearSpace((<Color>value).b) + r, + g, + b ); - cacheValue.x = (<Color>value).r; - cacheValue.y = (<Color>value).g; - cacheValue.z = (<Color>value).b; + cacheValue.x = r; + cacheValue.y = g; + cacheValue.z = b; } }
98-139:⚠️ Potential issueFix caching logic for gamma-corrected values.
The caching logic is inconsistent. The cache stores the original values but compares them against gamma-corrected values.
Apply this diff to fix the caching logic:
if ((<Color>value).r !== undefined) { if ( - cacheValue.x !== (<Color>value).r || - cacheValue.y !== (<Color>value).g || - cacheValue.z !== (<Color>value).b || + cacheValue.x !== Color.gammaToLinearSpace((<Color>value).r) || + cacheValue.y !== Color.gammaToLinearSpace((<Color>value).g) || + cacheValue.z !== Color.gammaToLinearSpace((<Color>value).b) || cacheValue.w !== (<Color>value).a ) { + const r = Color.gammaToLinearSpace((<Color>value).r); + const g = Color.gammaToLinearSpace((<Color>value).g); + const b = Color.gammaToLinearSpace((<Color>value).b); this._gl.uniform4f( shaderUniform.location, - Color.gammaToLinearSpace((<Color>value).r), - Color.gammaToLinearSpace((<Color>value).g), - Color.gammaToLinearSpace((<Color>value).b), + r, + g, + b, (<Color>value).a ); - cacheValue.x = (<Color>value).r; - cacheValue.y = (<Color>value).g; - cacheValue.z = (<Color>value).b; + cacheValue.x = r; + cacheValue.y = g; + cacheValue.z = b; cacheValue.w = (<Color>value).a; } }
🧹 Nitpick comments (2)
packages/core/src/EngineSettings.ts (1)
4-4: Consider using a type alias instead of an empty interface.Since the interface is now empty after removing the
colorSpaceproperty, it would be more appropriate to use a type alias.-export interface EngineSettings {} +export type EngineSettings = {};🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
packages/core/src/particle/ParticleGenerator.ts (1)
734-734: Remove extra whitespace.There is an extra whitespace line that should be removed.
main.startColor.evaluate(undefined, main._startColorRand.random(), startColor); - startColor.toLinear(startColor);🧰 Tools
🪛 ESLint
[error] 734-734: Delete
····(prettier/prettier)
🪛 GitHub Check: lint
[failure] 734-734:
Delete····
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (28)
packages/core/src/postProcess/shaders/Bloom/BloomBlurH.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/Bloom/BloomBlurV.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/Bloom/BloomPrefilter.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/Bloom/BloomUpsample.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/PostCommon.glslis excluded by!**/*.glslpackages/core/src/postProcess/shaders/UberPost.glslis excluded by!**/*.glslpackages/core/src/shaderlib/begin_mobile_frag.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/SkyProcedural.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/SkyProcedural.vs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/blinn-phong.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/particle.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/pbr-specular.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/pbr.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/skybox.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/extra/unlit.fs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/pbr/btdf.glslis excluded by!**/*.glslpackages/core/src/shaderlib/pbr/ibl_frag_define.glslis excluded by!**/*.glslpackages/core/src/shaderlib/pbr/pbr_frag.glslis excluded by!**/*.glslpackages/core/src/shaderlib/pbr/pbr_helper.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/BTDF.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/FragmentPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/LightIndirectFunctions.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glslis excluded by!**/*.glsltests/src/shader-lab/shaders/compilation-error.shaderis excluded by!**/*.shadertests/src/shader-lab/shaders/demo.shaderis excluded by!**/*.shadertests/src/shader-lab/shaders/macro-pre.shaderis excluded by!**/*.shadertests/src/shader-lab/shaders/unlit.shaderis excluded by!**/*.shader
📒 Files selected for processing (17)
e2e/case/camera-opaque-texture.ts(2 hunks)e2e/case/shaderLab-mrt.ts(1 hunks)examples/camera-depth-texture.ts(1 hunks)examples/model-mesh.ts(1 hunks)examples/transparent-shadow.ts(1 hunks)packages/core/src/Engine.ts(0 hunks)packages/core/src/EngineSettings.ts(1 hunks)packages/core/src/enums/ColorSpace.ts(0 hunks)packages/core/src/index.ts(0 hunks)packages/core/src/lighting/DirectLight.ts(1 hunks)packages/core/src/lighting/PointLight.ts(1 hunks)packages/core/src/lighting/SpotLight.ts(1 hunks)packages/core/src/particle/ParticleGenerator.ts(1 hunks)packages/core/src/particle/modules/ColorOverLifetimeModule.ts(1 hunks)packages/core/src/particle/modules/ParticleGradient.ts(2 hunks)packages/core/src/shader/ShaderUniform.ts(3 hunks)tests/src/core/Shader.test.ts(2 hunks)
💤 Files with no reviewable changes (3)
- packages/core/src/enums/ColorSpace.ts
- packages/core/src/index.ts
- packages/core/src/Engine.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/EngineSettings.ts
[error] 4-4: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🪛 ESLint
packages/core/src/particle/ParticleGenerator.ts
[error] 734-734: Delete ····
(prettier/prettier)
🪛 GitHub Check: lint
packages/core/src/particle/ParticleGenerator.ts
[failure] 734-734:
Delete ····
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: codecov
- GitHub Check: e2e (22.x)
🔇 Additional comments (12)
packages/core/src/lighting/PointLight.ts (1)
51-53: LGTM! Consistent color space handling.The changes ensure that light colors are always converted from gamma to linear space, aligning with the PR's objective of removing conditional gamma color space handling.
packages/core/src/lighting/DirectLight.ts (1)
60-62: LGTM! Consistent color space handling.The changes ensure that light colors are always converted from gamma to linear space, aligning with the PR's objective of removing conditional gamma color space handling.
e2e/case/camera-opaque-texture.ts (1)
98-98: LGTM! Consistent color space handling in shaders.The changes ensure consistent color space conversion by:
- Converting the base color from gamma to linear space
- Converting the final color back to gamma space before output
This aligns with the PR's objective of removing conditional gamma color space handling.
Also applies to: 108-108
e2e/case/shaderLab-mrt.ts (1)
65-65: LGTM! Consistent color space handling.The unconditional gamma-to-linear conversion aligns with the PR's objective to standardize color space handling.
examples/camera-depth-texture.ts (1)
132-132: LGTM! Proper depth visualization.The unconditional linear-to-gamma conversion ensures correct display of depth values.
packages/core/src/particle/modules/ColorOverLifetimeModule.ts (1)
56-57:✅ Verification successful
LGTM! Simplified particle color handling.
The removal of colorSpace parameter streamlines the color conversion process while maintaining the core functionality of particle color gradients.
Also applies to: 62-63
🏁 Script executed:
#!/bin/bash # Verify that all _getColorTypeArray calls have been updated consistently rg -A 2 "_getColorTypeArray\(" --type tsLength of output: 1268
LGTM! Simplified particle color handling.
The updated calls to
_getColorTypeArrayand_getAlphaTypeArrayin both the max (lines 56-57) and min (lines 62-63) gradient sections correctly remove the unusedcolorSpaceparameter. This streamlines color conversion while preserving the intended gradient behavior, as verified by the consistent updates across the module.examples/model-mesh.ts (1)
171-171: LGTM! Consistent color space conversion with fog.The unconditional linear-to-gamma conversion ensures correct color display while maintaining proper fog effect visualization.
packages/core/src/lighting/SpotLight.ts (1)
83-85: LGTM! Consistent gamma correction.The changes align with the PR objective by ensuring gamma to linear conversion is always applied to light color components.
examples/transparent-shadow.ts (1)
66-66: LGTM! Consistent gamma correction.The changes align with the PR objective by ensuring linear to gamma conversion is always applied to the fragment color.
packages/core/src/particle/modules/ParticleGradient.ts (1)
156-156: LGTM! Consistent gamma correction.The changes align with the PR objective by:
- Removing the
colorSpaceparameter from the method signature.- Ensuring gamma to linear conversion is always applied to color components.
Also applies to: 165-167
tests/src/core/Shader.test.ts (1)
379-379: LGTM! Gamma color space conversion is now unconditional.The changes correctly implement unconditional gamma color space conversion by:
- Converting texture color from gamma to linear space
- Converting final color from linear to gamma space
Also applies to: 397-397
packages/core/src/particle/ParticleGenerator.ts (1)
734-735: LGTM! Particle color now always uses linear color space.The change correctly implements unconditional conversion of particle start color to linear space, aligning with the removal of gamma color space.
🧰 Tools
🪛 ESLint
[error] 734-734: Delete
····(prettier/prettier)
🪛 GitHub Check: lint
[failure] 734-734:
Delete····
GuoLei1990
added
Rendering
github-actions
bot
added
the
documentation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.5 #2565 +/- ##
========================================
Coverage 68.93% 68.93%
========================================
Files 961 960 -1
Lines 100271 100199 -72
Branches 8654 8643 -11
========================================
- Hits 69124 69075 -49
+ Misses 30887 30864 -23
Partials 260 260
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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
Refactor
Chore
Tests