8000 Color api rename by zhuxudong · Pull Request #2606 · galacean/engine · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Color api rename #2606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 9, 2025
Merged

Color api rename #2606

merged 1 commit into from
Apr 9, 2025

Conversation

zhuxudong
Copy link
Member
@zhuxudong zhuxudong commented Apr 9, 2025 •
edited by coderabbitai bot
Loading

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

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
    • Improved color conversion across lighting, particle effects, post-processing, and material rendering to provide more accurate and consistent color fidelity.
    • Unified the approach for processing color values, resulting in more reliable and visually balanced outputs across all scenes.

@zhuxudong zhuxudong added the math Math library label Apr 9, 2025
@zhuxudong zhuxudong added this to the 1.5 milestone Apr 9, 2025
@zhuxudong zhuxudong requested a review from GuoLei1990 April 9, 2025 08:25
@zhuxudong zhuxudong self-assigned this Apr 9, 2025
@coderabbitai coderabbitai
Copy link
coderabbitai bot commented Apr 9, 2025 •
edited
Loading

Walkthrough

The pull request updates various modules to change how color space conversions are handled. In several lighting, particle, post-processing, shader, and GLTF-related files, calls to convert color values have been updated. Specifically, the conversion from gamma space is now handled via sRGB functions (using Color.sRGBToLinearSpace and Color.linearToSRGBSpace) instead of the old gamma conversion functions. In addition, the underlying math module renames these methods and adds deprecated wrappers for backward compatibility while keeping the overall control flow intact.

Changes

Files Change Summary
packages/core/src/lighting/DirectLight.ts, PointLight.ts, SpotLight.ts, packages/core/src/particle/modules/ParticleGradient.ts Replaced Color.gammaToLinearSpace with Color.sRGBToLinearSpace for processing RGB components before storing in shader/light data.
packages/core/src/postProcess/PostProcessUberPass.ts, packages/core/src/shader/ShaderUniform.ts Updated conversion calls for threshold values and GPU uniform uploads to use Color.sRGBToLinearSpace instead of the gamma variant.
packages/loader/src/gltf/extensions/KHR_materials_pbrSpecularGlossiness.ts, KHR_materials_sheen.ts, KHR_materials_volume.ts, packages/loader/src/gltf/parser/GLTFMaterialParser.ts Changed material color conversions by replacing Color.linearToGammaSpace with Color.linearToSRGBSpace for various material properties.
packages/math/src/Color.ts Renamed conversion methods: gammaToLinearSpace → sRGBToLinearSpace and linearToGammaSpace → linearToSRGBSpace, and introduced deprecated wrappers for the original method names.

Possibly related PRs

  • Remove gamma color space #2565: Addresses similar updates for color space handling by removing conditional gamma color conversions in shaders, aligning with the changes in this pull request.

Suggested labels

enhancement, documentation

Suggested reviewers

  • GuoLei1990
  • hhhhkrx
  • Sway007

Poem

Hopping along the code trail so bright,
I found colors shifting with a fresh new light.
sRGB magic replacing gamma old,
My furry paws dance, my story told.
In every line, a smile I bestow—
CodeRabbit cheers as our colors glow!
🐰✨

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 46aa3e9 and 6e560cb.

📒 Files selected for processing (11)
  • 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/modules/ParticleGradient.ts (1 hunks)
  • packages/core/src/postProcess/PostProcessUberPass.ts (1 hunks)
  • packages/core/src/shader/ShaderUniform.ts (3 hunks)
  • packages/loader/src/gltf/extensions/KHR_materials_pbrSpecularGlossiness.ts (2 hunks)
  • packages/loader/src/gltf/extensions/KHR_materials_sheen.ts (1 hunks)
  • packages/loader/src/gltf/extensions/KHR_materials_volume.ts (1 hunks)
  • packages/loader/src/gltf/parser/GLTFMaterialParser.ts (2 hunks)
  • packages/math/src/Color.ts (5 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/loader/src/gltf/extensions/KHR_materials_volume.ts

[warning] 18-20: packages/loader/src/gltf/extensions/KHR_materials_volume.ts#L18-L20
Added lines #L18 - L20 were not covered by tests

packages/core/src/shader/ShaderUniform.ts

[warning] 47-48: packages/core/src/shader/ShaderUniform.ts#L47-L48
Added lines #L47 - L48 were not covered by tests

packages/core/src/particle/modules/ParticleGradient.ts

[warning] 165-167: packages/core/src/particle/modules/ParticleGradient.ts#L165-L167
Added lines #L165 - L167 were not covered by tests

packages/loader/src/gltf/extensions/KHR_materials_sheen.ts

[warning] 16-18: packages/loader/src/gltf/extensions/KHR_materials_sheen.ts#L16-L18
Added lines #L16 - L18 were not covered by tests

packages/math/src/Color.ts

[warning] 364-365: packages/math/src/Color.ts#L364-L365
Added lines #L364 - L365 were not covered by tests

[warning] 369-370: packages/math/src/Color.ts#L369-L370
Added lines #L369 - L370 were not covered by tests

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: e2e (22.x)
🔇 Additional comments (19)
packages/loader/src/gltf/extensions/KHR_materials_pbrSpecularGlossiness.ts (2)

23-25: API update: Changed color space conversion from gamma to sRGB

The conversion is now using Color.linearToSRGBSpace instead of Color.linearToGammaSpace for the diffuseFactor RGB values, maintaining consistency with the overall color API renaming in the project.

39-41: API update: Changed color space conversion from gamma to sRGB

The conversion is now using Color.linearToSRGBSpace instead of Color.linearToGammaSpace for the specularFactor RGB values, maintaining consistency with the overall color API renaming in the project.

packages/loader/src/gltf/extensions/KHR_materials_volume.ts (1)

18-20: API update: Changed color space conversion from gamma to sRGB

The conversion is now using Color.linearToSRGBSpace instead of Color.linearToGammaSpace for the attenuationColor RGB values, maintaining consistency with the overall color API renaming in the project.

The static analysis indicates these lines are not covered by tests. Consider adding test coverage for the KHR_materials_volume extension to ensure proper functionality.

#!/bin/bash
# Check for existing tests covering the KHR_materials_volume extension
rg -i "test.*KHR_materials_volume" --type ts
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 18-20: packages/loader/src/gltf/extensions/KHR_materials_volume.ts#L18-L20
Added lines #L18 - L20 were not covered by tests

packages/core/src/postProcess/PostProcessUberPass.ts (1)

120-120: API update: Changed color space conversion from gamma to sRGB

The conversion is now using Color.sRGBToLinearSpace instead of Color.gammaToLinearSpace for the threshold value in the _setupBloom method, maintaining consistency with the overall color API renaming in the project.

packages/core/src/lighting/SpotLight.ts (1)

83-85: API update: Changed color space conversion from gamma to sRGB

The conversion is now using Color.sRGBToLinearSpace instead of Color.gammaToLinearSpace for the lightColor RGB components, maintaining consistency with the overall color API renaming in the project.

packages/core/src/lighting/PointLight.ts (1)

51-53: Consistent conversion to sRGB color space.

The changes replace Color.gammaToLinearSpace with Color.sRGBToLinearSpace for converting RGB color components. This change aligns with the broader effort to standardize color space conversion across the codebase.

packages/loader/src/gltf/extensions/KHR_materials_sheen.ts (1)

16-18: Consistent renaming to sRGB color space conversion.

The methods for color space conversion have been renamed from linearToGammaSpace to linearToSRGBSpace, maintaining consistent terminology across the codebase.

These changes aren't covered by tests according to the codecov report. Consider adding tests to verify the color conversion behavior hasn't changed unexpectedly:

<
8000
span class="pl-c">#!/bin/bash
# Find existing tests for the KHR_materials_sheen component
rg -i "test.*KHR_materials_sheen" --type ts
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 16-18: packages/loader/src/gltf/extensions/KHR_materials_sheen.ts#L16-L18
Added lines #L16 - L18 were not covered by tests

packages/core/src/shader/ShaderUniform.ts (3)

72-74: Update color space conversion in upload3f method.

Changed color conversion from gamma space to sRGB space for 3-component color vectors, consistent with other changes.

109-112: Update color space conversion in upload4f method.

Changed color conversion from gamma space to sRGB space for 4-component color vectors, while correctly keeping the alpha channel unmodified.

47-48: Update color space conversion in upload2f method.

Changed color conversion from gamma space to sRGB space for 2-component color vectors.

These changes aren't covered by tests according to codecov. Consider verifying the visual output:

#!/bin/bash
# Look for unit tests that might be relevant to ShaderUniform color handling
rg -i "test.*ShaderUniform.*color" --type ts
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 47-48: packages/core/src/shader/ShaderUniform.ts#L47-L48
Added lines #L47 - L48 were not covered by tests

packages/core/src/particle/modules/ParticleGradient.ts (1)

165-167: Update color space conversion in particle gradient.

Changed color conversion from gamma space to sRGB space for particle gradient colors, keeping the implementation consistent with other changes in the codebase.

These changes aren't covered by tests according to codecov. Consider adding tests to verify that particle gradient colors render correctly:

#!/bin/bash
# Find existing tests for the ParticleGradient component
rg -i "test.*ParticleGradient" --type ts
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 165-167: packages/core/src/particle/modules/ParticleGradient.ts#L165-L167
Added lines #L165 - L167 were not covered by tests

packages/core/src/lighting/DirectLight.ts (1)

60-62: API naming change: gammaToLinearSpace → sRGBToLinearSpace

The color conversion method has been updated from gammaToLinearSpace to sRGBToLinearSpace. This aligns with standard terminology in computer graphics, as sRGB is the standard color space used in digital images and displays.

packages/loader/src/gltf/parser/GLTFMaterialParser.ts (2)

59-65: API naming change: linearToGammaSpace → linearToSRGBSpace

The color conversion method has been updated from linearToGammaSpace to linearToSRGBSpace. This is part of the consistent renaming across the codebase to use sRGB terminology instead of gamma, which is more accurate for the color space transformation being performed.

100-106: API naming change: linearToGammaSpace → linearToSRGBSpace

The color conversion method has been updated from linearToGammaSpace to linearToSRGBSpace, consistent with the changes elsewhere in the codebase.

packages/math/src/Color.ts (5)

10-22: Method rename: gammaToLinearSpace → sRGBToLinearSpace

The method has been renamed from gammaToLinearSpace to sRGBToLinearSpace to more accurately reflect the standard color space transformation being performed. The documentation has been updated accordingly to reference sRGB space instead of gamma space.

25-37: Method rename: linearToGammaSpace → linearToSRGBSpace

The method has been renamed from linearToGammaSpace to linearToSRGBSpace to more accurately reflect the standard color space transformation being performed. The documentation has been updated accordingly to reference sRGB space instead of gamma space.

307-309: Updated method calls in toLinear method

The method calls have been updated to use the renamed sRGBToLinearSpace method instead of the old gammaToLinearSpace method.

321-324: Updated method calls in toGamma method

The method calls have been updated to use the renamed linearToSRGBSpace method instead of the old linearToGammaSpace method.

362-370: Added deprecated methods for backward compatibility

Deprecated versions of the old methods have been added that simply call the new methods, providing backward compatibility for existing code that might be using the old API.

The static analysis indicates these methods are not covered by tests. Consider adding tests to ensure they work as expected, even though they are simple wrappers.

#!/bin/bash
# Check if there are any tests for the deprecated methods

rg -A 2 "gammaToLinearSpace" --type ts --glob "**/*test*.ts"
rg -A 2 "linearToGammaSpace" --type ts --glob "**/*test*.ts"
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 364-365: packages/math/src/Color.ts#L364-L365
Added lines #L364 - L365 were not covered by tests

[warning] 369-370: packages/math/src/Color.ts#L369-L370
Added lines #L369 - L370 were not covered by tests

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@codecov Codecov
Copy link
codecov bot commented Apr 9, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 75.40984% with 15 lines in your changes missing coverage. Please review.

Project coverage is 68.59%. Comparing base (46aa3e9) to head (6e560cb).
Report is 1 commits behind head on dev/1.5.

Files with missing lines Patch % Lines
packages/math/src/Color.ts 81.81% 4 Missing ⚠️
...ages/core/src/particle/modules/ParticleGradient.ts 0.00% 3 Missing ⚠️
.../loader/src/gltf/extensions/KHR_materials_sheen.ts 0.00% 3 Missing ⚠️
...loader/src/gltf/extensions/KHR_materials_volume.ts 0.00% 3 Missing ⚠️
packages/core/src/shader/ShaderUniform.ts 75.00% 2 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff            @@
##           dev/1.5    #2606   +/-   ##
========================================
  Coverage    68.59%   68.59%           
========================================
  Files          966      966           
  Lines       101428   101438   +10     
  Branches      8714     8717    +3     
==========
8000
==============================
+ Hits         69576    69586   +10     
  Misses       31589    31589           
  Partials       263      263
Flag Coverage Δ
unittests 68.59% <75.40%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@GuoLei1990 GuoLei1990 merged commit 958265a into galacean:dev/1.5 Apr 9, 2025
8 of 9 checks passed
@GuoLei1990 GuoLei1990 added the ignore for release ignore for release label Apr 9, 2025
This was referenced Apr 9, 2025
Merged
Merged
@coderabbitai coderabbitai bot mentioned this pull request May 12, 2025
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuoLei1990 GuoLei1990 GuoLei1990 approved these changes

Assignees

@zhuxudong zhuxudong

Labels
ignore for release ignore for release math Math library
Projects
None yet
Milestone
1.5
Development

Successfully merging this pull request may close these issues.
2 participants
@zhuxudong @GuoLei1990
0