Skip to content

Update color encoding documentation#104666

Merged
Repiteo merged 1 commit into
godotengine:masterfrom
allenwp:color-encoding-docs
Sep 22, 2025
Merged

Update color encoding documentation#104666
Repiteo merged 1 commit into
godotengine:masterfrom
allenwp:color-encoding-docs

Conversation

@allenwp

@allenwp allenwp commented Mar 26, 2025

Copy link
Copy Markdown
Contributor

This PR builds on top of @aaronfranke's merged PR #104410.

This PR adds additional detail to the Color class description, including information about color primaries. It states clearly that Color is expected to use the nonlinear sRGB transfer function for R, G, and B values unless stated otherwise by documentation.

To reviewers:

This PR is somewhat a proposal and somewhat a discussion. I felt that it would be better to present my proposed changes in the form of a PR instead of a proposal.

I have added a note about the srgb parameter of TextureStorage::texture_get_native_handle and TextureStorage::texture_get_rd_texture, but I don't know if I've written this correctly.

The problem with the "color space" term

With upcoming HDR output support and eventual and inevitable wide colour gamut support, I believe it is valuable to be more precise with our terminology regarding colour. Of specific concern is the term "color space", which I have found to be a loose term that often describes both colour gamut (which relates to colour primaries) and colour data encoding/format and/or transfer function. Graphics APIs often combine these attributes into what they call a "color space" (see VkColorSpaceKHR). But sometimes colour scientists will intentionally not refer to data format/encoding or transfer functions when describing what a "color space" is:

image

Source: From Texture to Display: The Color Pipeline of a Pixel in Unreal Engine | Unreal Fest 2024

After quite a few months immersing myself in the field of colour science, I have developed a strong belief that the term "color space" is simply too ambiguous to be useful, especially in technical writing and collaboration between a diverse group of collaborators. I propose that this specific term should generally be avoided and only used with care and an understanding that it is ambiguous. This PR implements this proposed approach.

Acceptable terminology

  • A Color is encoded using the nonlinear sRGB transfer function
  • A Color uses nonlinear sRGB encoding (shorthand of above)
  • A Color uses linear encoding

Why "nonlinear sRGB encoding" instead of "sRGB encoding"?

Using the term "nonlinear" to describe the nonlinear sRGB transfer function helps the reader understand this encoding is related to the linear/nonlinear attribute of RGB values and is not related to the colour gamut. This is especially important with the sRGB standard, which describes both a colour gamut and a transfer function that is used for encoding. Additionally, in Edward Giorgianni and Thomas Madden's book "Digital Color Management" (which provided foundational concepts to the ACES colour system), the term "colour encoding" is sometimes used to describe the the mapping of a colour stimulus (a spectral distribution) to a linear tristimulus value using CIE colour matching functions. This means that "sRGB encoding" could be interpreted to mean "encoding a colour stimulous into linear values using CIE colour matching with the sRGB colour primaries". For this reason, adding "nonlinear" makes it more clear to people from every field that the "nonlinear sRGB encoding" shorthand simply means "encoding using the nonlinear sRGB transfer function".

Gradient and color picker classes

I have left these two classes somewhat unchanged: They still use the "color space" term, and I believe that these are two uncommon scenarios where "color space" is not problematic (for example, OKHSL is very much a "color space" -- it is more than just an encoding and gamut). Once other colour primaries are implemented, this might need to be revisited.

Remaining work

  • Search for other places in the docs that should have details about linear encoding being necessary
  • Ensure Compatibility rendering method behaviour (regarding using nonlinear sRGB values for lighting calculations) is documented somewhere
  • Separate PR: update tutorial documentation to match class documentation terminology
  • Ensure that shader documentation correctly assumes sRGB values and suggests linear when needed for correct simulation
  • Review source_color docs and behaviour with 2D HDR
  • Ensure material vertex color is correctly documented and handled

Comment thread doc/classes/Color.xml Outdated
Comment thread doc/classes/Color.xml Outdated

@Calinou Calinou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@allenwp allenwp force-pushed the color-encoding-docs branch 3 times, most recently from 4735943 to da94f96 Compare August 27, 2025 21:22
@allenwp allenwp force-pushed the color-encoding-docs branch 3 times, most recently from 2c5ac14 to 3c28560 Compare August 28, 2025 17:51
@allenwp allenwp changed the title Add encoding documentation to Color class Update color encoding documentation Aug 28, 2025
@allenwp allenwp force-pushed the color-encoding-docs branch from 3c28560 to ca7f90e Compare August 28, 2025 19:22
@allenwp allenwp marked this pull request as ready for review August 28, 2025 19:23
@allenwp allenwp requested a review from a team as a code owner August 28, 2025 19:23
@allenwp

allenwp commented Aug 28, 2025

Copy link
Copy Markdown
Contributor Author

@Calinou @AThousandShips I have finished work on this PR and it is now ready for review. It has changed quite a bit since its initial draft that you previously looked at, so I figured I'd mention you here to let you know of the changes.

I've marked this PR as a discussion because I believe that this is as much a proposal as it is a PR, but I think it's easiest to talk about my proposed changes when they're all visible in the form of this PR.

Comment thread doc/classes/Viewport.xml Outdated
Comment thread doc/classes/Viewport.xml Outdated
Comment thread doc/classes/RenderingServer.xml Outdated
Comment thread doc/classes/ProjectSettings.xml Outdated
Comment thread doc/classes/Color.xml Outdated
Comment thread doc/classes/RenderingDevice.xml Outdated
Comment thread doc/classes/RenderingDevice.xml Outdated
Comment thread doc/classes/RenderingDevice.xml Outdated
Comment thread doc/classes/Image.xml Outdated
Comment thread doc/classes/Color.xml Outdated
@allenwp allenwp force-pushed the color-encoding-docs branch from d0c65c5 to 68b2f53 Compare August 29, 2025 18:04
@allenwp

allenwp commented Aug 29, 2025

Copy link
Copy Markdown
Contributor Author

Thanks, @AThousandShips! I've incorporated all of your suggested changes.

Comment thread doc/classes/Image.xml Outdated
Comment thread doc/classes/ProjectSettings.xml Outdated
@allenwp allenwp force-pushed the color-encoding-docs branch from 267154a to d1c94ec Compare September 2, 2025 15:16
@allenwp

allenwp commented Sep 2, 2025

Copy link
Copy Markdown
Contributor Author

I've applied the suggested fixes, thanks @Calinou!

@Calinou Calinou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good to merge after rebasing.

@allenwp allenwp force-pushed the color-encoding-docs branch from d1c94ec to 12ccc39 Compare September 2, 2025 18:11
@allenwp

allenwp commented Sep 2, 2025

Copy link
Copy Markdown
Contributor Author

Before this is merged, I would like @clayjohn to comment on the sentiment of this PR (the description text of the PR) to ensure this is a good change.

@aaronfranke aaronfranke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked over these changes, it looks good to me overall, but I have one problem with it.

Comment thread doc/classes/Color.xml Outdated
@allenwp allenwp force-pushed the color-encoding-docs branch from 12ccc39 to 7769a71 Compare September 6, 2025 22:17
@allenwp

allenwp commented Sep 6, 2025

Copy link
Copy Markdown
Contributor Author

I changed the nonlinear sRGB transfer function link to point to the transfer function heading on the wikipedia page.

@Repiteo Repiteo modified the milestones: 4.x, 4.6 Sep 10, 2025
Comment thread doc/classes/ProjectSettings.xml Outdated
…detail to `Color` class description. State clearly that `Color` is expected to use the nonlinear sRGB transfer function.
@allenwp allenwp force-pushed the color-encoding-docs branch from 0d10144 to 46e4096 Compare September 15, 2025 22:12
@Repiteo Repiteo merged commit d7565ff into godotengine:master Sep 22, 2025
20 checks passed
@Repiteo

Repiteo commented Sep 22, 2025

Copy link
Copy Markdown
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants