AgX-Step3: Refactor OCIO config to be CIE XYZ I-E based #110913

Merged
Sergey Sharybin merged 6 commits from :AgX-Step3 into main 2023-08-08 15:11:17 +02:00
Contributor

This is step 3 of the AgX implementation, as discussed in the original AgX PR

step 3. refactor the config's transform tree to be XYZ I-E based.
The design decision has been discussed in this PR, XYZ I-E is the actual CIE standard, it's to prepare for Spectral Cycles, but this will also make the next step more straight forward.

This includes changing the xyz_D65_to_E.spimtx to use the more accurate Bradford method instead of XYZ Scaling, and also adding xyz_E_to_D65.spimtx since the opposite direction uses a different matrix.

EDIT: Forgot to mention this also involved in renaming of the XYZ space, as mentioned by the comment

This is step 3 of the AgX implementation, as discussed in [the original AgX PR](https://projects.blender.org/blender/blender/pulls/106355#issuecomment-984699) >step 3. refactor the config's transform tree to be XYZ I-E based. The design decision has been discussed in this PR, XYZ I-E is the actual CIE standard, it's to prepare for Spectral Cycles, but this will also make the next step more straight forward. > >This includes changing the xyz_D65_to_E.spimtx to use the more accurate Bradford method instead of XYZ Scaling, and also adding xyz_E_to_D65.spimtx since the opposite direction uses a different matrix. EDIT: Forgot to mention this also involved in renaming of the XYZ space, as mentioned by the comment
Zijun Zhou added 3 commits 2023-08-08 06:12:49 +02:00
Zijun Zhou requested review from Sergey Sharybin 2023-08-08 06:13:51 +02:00
Zijun Zhou requested review from Brecht Van Lommel 2023-08-08 06:14:18 +02:00

@blender-bot build

@blender-bot build
Brecht Van Lommel reviewed 2023-08-08 10:57:59 +02:00
@ -97,4 +124,2 @@
- !<BuiltinTransform> {style: "UTILITY - ACES-AP1_to_CIE-XYZ-D65_BFD", direction: inverse}
- !<ColorSpace>
name: XYZ

This is a significant change that breaks forward compatibility, but it is not mentioned in the PR description.

This is a significant change that breaks forward compatibility, but it is not mentioned in the PR description.
brecht marked this conversation as resolved
@ -62,2 +63,4 @@
colorspaces:
- !<ColorSpace>
name: Linear CIE-XYZ I-E

Slightly shorter name: Linear CIE-XYZ E

Slightly shorter name: `Linear CIE-XYZ E`
brecht marked this conversation as resolved
@ -64,0 +69,4 @@
equalitygroup:
bitdepth: 32f
description: |
1931 CIE XYZ Standard with assumed I-E white point

Standard -> standard
I-E -> illuminant E

Standard -> standard I-E -> illuminant E
brecht marked this conversation as resolved
@ -64,0 +73,4 @@
isdata: false
- !<ColorSpace>
name: Linear CIE-XYZ I-D65

Slightly shorter name: Linear CIE-XYZ D65

As can be seen from the aliases, it's not common to have this I- prefix and not self explanatory anyway.

Slightly shorter name: `Linear CIE-XYZ D65` As can be seen from the aliases, it's not common to have this `I-` prefix and not self explanatory anyway.
brecht marked this conversation as resolved
@ -64,0 +79,4 @@
equalitygroup:
bitdepth: 32f
description: |
1931 CIE XYZ with adapted I-D65 white point

I-D65 -> illuminant D65, here we can actually use the full word

I-D65 -> illuminant D65, here we can actually use the full word
brecht marked this conversation as resolved
@ -64,0 +82,4 @@
1931 CIE XYZ with adapted I-D65 white point
isdata: false
from_scene_reference: !<FileTransform> {src: xyz_E_to_D65.spimtx, interpolation: linear}
to_scene_reference: !<FileTransform> {src: xyz_D65_to_E.spimtx, interpolation: linear}

Is it necessary to provide transforms in two directions?

I wonder if that might actually affect performance negatively, if perhaps in this case OpenColorIO can no longer cancel out the transforms.

Is it necessary to provide transforms in two directions? I wonder if that might actually affect performance negatively, if perhaps in this case OpenColorIO can no longer cancel out the transforms.
brecht marked this conversation as resolved
Zijun Zhou added 1 commit 2023-08-08 11:26:35 +02:00
Zijun Zhou added 1 commit 2023-08-08 11:28:43 +02:00
Fix consequence after renaming
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
806effe40d
Author
Contributor

Comments are adressed.

Comments are adressed.

@blender-bot build

@blender-bot build
Brecht Van Lommel approved these changes 2023-08-08 12:34:01 +02:00
Sergey Sharybin approved these changes 2023-08-08 14:35:06 +02:00
Sergey Sharybin left a comment
Owner

LGTM.

I've created a patch for 3.6 to keep forward compatibility as per our policy: #110924

LGTM. I've created a patch for 3.6 to keep forward compatibility as per our policy: #110924
Sergey Sharybin added 1 commit 2023-08-08 15:10:00 +02:00
Sergey Sharybin merged commit 1bcf31a874 into main 2023-08-08 15:11:17 +02:00
Sergey Sharybin deleted branch AgX-Step3 2023-08-08 15:11:19 +02:00
Member

Sorry I'm a bit late to this. Didn't realize it would get merged so quickly.

A tiny nit: rather than Linear CIE-XYZ I-E or Linear CIE-XYZ E it can just be Linear CIE XYZ. X=Y=Z is already defined to be E by the standard, so mentioning it in the name seems redundant to me. E.g. we don't call sRGB "sRGB D65", etc. It does make sense in the D65 XYZ variant, though, since that deviates from actual CIE XYZ.

(I also wondered briefly if we should drop the Linear prefix as well, for similar reasons. But thinking a bit more, it probably makes sense to have a general linear/non-linear distinction for the spaces, and we're already using the Linear prefix for that anyway.)

Sorry I'm a bit late to this. Didn't realize it would get merged so quickly. A tiny nit: rather than `Linear CIE-XYZ I-E` or `Linear CIE-XYZ E` it can just be `Linear CIE XYZ`. X=Y=Z is already defined to be E by the standard, so mentioning it in the name seems redundant to me. E.g. we don't call sRGB "sRGB D65", etc. It does make sense in the D65 XYZ variant, though, since that deviates from actual CIE XYZ. (I also wondered briefly if we should drop the `Linear` prefix as well, for similar reasons. But thinking a bit more, it probably makes sense to have a general linear/non-linear distinction for the spaces, and we're already using the `Linear` prefix for that anyway.)

We can add an alias if needed, but on the interface I'd rather stick to a fully disambiguated naming. I do not think it is great to require people to know standards by heart to know which illuminant is used. When you see "Linear CIE-XYZ E" you know for sure what it is. If you see "Linear CIE-XYZ" you'll be like "holap, lemme double check my knowledge of standard".

We can add an alias if needed, but on the interface I'd rather stick to a fully disambiguated naming. I do not think it is great to require people to know standards by heart to know which illuminant is used. When you see "Linear CIE-XYZ E" you know for sure what it is. If you see "Linear CIE-XYZ" you'll be like "holap, lemme double check my knowledge of standard".
Member

I could see that. But to expand on my perspective:

To me it's more that CIE XYZ doesn't have a white point in the traditional sense. In RGB color spaces, for example, the white point plays double-duty:

  1. It's the last piece needed to fully define the color space as a coordinate system (by implicitly defining the relative RGB intensities at unity).
  2. And it also defines an assumed perceptual white point for viewing colors defined in that space.

In CIE XYZ, on the other hand, X=Y=Z -> illuminant E is purely about defining the coordinate system colorimetrically, and has none of the perceptual implications usually associated with specifying a white point. It's a bit of a subtle distinction, but in my mind at least it means that CIE XYZ does not have a white point as such—it's just purely a colorimetric coordinate system. And that's also what makes it useful as the foundation upon which other color spaces define their white points.

(XYZ D65, on the other hand, does introduce a white point into XYZ. Making it no longer CIE XYZ per se.)

In any case, I don't feel strongly about it. So I'm fine leaving it as-is. It just feels a little misleading to me, I guess, in terms of what CIE XYZ is.

I could see that. But to expand on my perspective: To me it's more that CIE XYZ doesn't have a white point in the traditional sense. In RGB color spaces, for example, the white point plays double-duty: 1. It's the last piece needed to fully define the color space as a coordinate system (by implicitly defining the relative RGB intensities at unity). 2. And it *also* defines an assumed perceptual white point for viewing colors defined in that space. In CIE XYZ, on the other hand, X=Y=Z -> illuminant E is purely about defining the coordinate system colorimetrically, and has none of the perceptual implications usually associated with specifying a white point. It's a bit of a subtle distinction, but in my mind at least it means that CIE XYZ does not have a white point as such—it's just purely a colorimetric coordinate system. And that's also what makes it useful as the foundation upon which other color spaces define *their* white points. (XYZ D65, on the other hand, *does* introduce a white point into XYZ. Making it no longer CIE XYZ per se.) In any case, I don't feel strongly about it. So I'm fine leaving it as-is. It just feels a little misleading to me, I guess, in terms of what CIE XYZ is.

From that argument I slightly prefer just "Linear CIE-XYZ", but don't feel very strongly about it either way.

From that argument I slightly prefer just "Linear CIE-XYZ", but don't feel very strongly about it either way.
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#110913
No description provided.