AgX-Step4: Add and Rename Colorspaces #110941
No reviewers
Sergey Sharybin
Brecht Van Lommel
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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
7 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#110941
Loading…
Reference in New Issue
No description provided.
Delete Branch ":AgX-Step4"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This is step 4 of the AgX implementation as discussed in the original AgX PR
I also took the chance to edit some spaces description and family tag, which also involves in putting
False Colortoinactive_colorspacesinstead of usingfamily: displayfiltering.The spaces are now:
(Change from
Linear ACEScgtoACEScgsince ACEScg already implied a linear transfer function, otherwise usingLinear AP1 ACESmight make more sense. Same goes for ACES2065-1)This is for AgX's LUT input encoding. It can potentially be useful for interop.
@blender-bot package
Package build started. Download here when ready.
I think native spectral rendering in Cycles should not use illuminant E color spaces on the UI level, and therefore we should not have these illuminant E version of color spaces in our config.
What spectral rendering should do is ignore the illuminant conversion for reflectance colors, but not for emission colors. For the rest of Blender like compositing, file I/O and painting we should use illuminant D65 (or D60 for ACEScg) as usual. It's only the conversion of reflectance colors to/from spectral inside the renderer that needs some special care. From reading papers and course notes, this I believe is also how spectral production renderers do it.
We should then also not have D65 in the name of color spaces where they already have a well defined illuminant.
The reason the ACEScg name exists is exactly to use it in places like this, since AP0 and AP1 are easy to confuse for artists, ACEScg is more recognizable. I'm fine renaming ACES to ACES2065-1, to disambiguate it and follow the same naming as the OpenColorIO cg config.
Ideally we would put all the linear colorspaces in a Linear submenu, and then having Linear in the name is not so important. I don't feel strongly about having Linear in the name for ACES right now or not.
I think we should use "Rec." instead of "BT." in the names. I believe this is a more common, and the OpenColorIO CG config also uses Rec instead of BT.
@ -87,2 +88,3 @@name: Linearname: Linear BT.709 D65aliases: [Linear, Linear BT.709, Linear BT.709 I-D65, Linear Tristimulus, Linear Rec.709, linrec709, Utility - Linear - sRGB, Utility - Linear - Rec.709, lin_srgb, Linear Rec.709 (sRGB), lin_rec709_srgb, lin_rec709, lin_srgb, "CGI: Linear - Rec.709"]family: linearOther families are getting renamed to use capitalization. If we do that we should changes family linear to Linear everywhere as well.
@ -90,3 +92,3 @@bitdepth: 32fdescription: |Rec. 709 (Full Range), Blender native linear spaceOpen Domain (unbounded) Linear BT.709 Tristimulus with illuminant D65 white pointI don't think using the term "Tristimulus" here or other descriptions is helpful, so would leave it out everywhere. OpenColorIO only deals with tristimulus values, it's not making a useful distinction.
Open domain I don't think is a standard term, it might be a term invented by Troy. I don't think we need to mention this at all, in fact the color space itself does not define if it is unbounded or not, also sRGB in Blender permits unbounded values.
@ -125,0 +200,4 @@equalitygroup:bitdepth: 32fdescription: |Open Domain (unbounded) Linear E Gamut Tristimulus with illuminant D65 white point"E Gamut" -> "E-Gamut" also in the description, I have not seen it spelled without the dash elsewhere.
@ -126,2 +211,3 @@name: sRGBfamily:aliases: [sRGB 2.2, sRGB I-D65, srgb_display, sRGB - Display, g22_rec709, Utility - Gamma 2.2 - Rec.709 - Texture, Utility - sRGB - Texture, sRGB - Texture, srgb_tx, srgb_texture, Input - Generic - sRGB - Texture, "sRGB Display: 2.2 Gamma - Rec.709"]family: Displays/SDRCan we keep the family name for all these display to just "Display"?
With upcoming EDR support on macOS this one actually works for HDR too. And as long as we don't have HDR displays there is not much point in making a distinction.
@ -129,3 +229,3 @@bitdepth: 32fdescription: |sRGB display spaceDisplay P3 with sRGB compound (piece-wise) encoding transfer functionIs this DCI-P3? If so it would good to put that there in the description, and also mention that it is commonly used for digital cinema.
This is actually Apple's Display P3, which is common on Mac devices.
@ -138,0 +242,4 @@equalitygroup:bitdepth: 32fdescription: |BT.1886 2.4 Exponent EOTF DisplayIt would be good to mention this is commonly used for TVs.
@ -139,3 +267,3 @@name: Non-Coloraliases: [Generic Data, Non-Colour Data, Raw, Utility - Raw]family: rawfamily: Data/Generic DataData/Generic Data is not a very elegant name. Would go with just Data.
I don't think Imagery is particularly descripitive, all the other color spaces are used for images as well.
It's supposed to refer to the view transforms, should the family just go blank or should it be replaced with some other family name?
We don't use them in the UI right now, so it doesn't matter yet. But we may in the future.
For now perhaps put these in the family "Filmic".
Adapting colors to white point E will definitely be useful for spectral Cycles, yes. It's needed to interpret colors (e.g. from textures) correctly for use as albedo, etc. I'm sure you're aware of that, but for anyone else reading along.
I do wonder what the best approach is in terms of how the transforms are structured, though. And I think the answer to that may depend on how spectral Cycles ends up approaching things. For example, at what point in the chain does Cycles do that white point adaptation? And will it do white point adaptation automatically based on whether the color is used for an albedo parameter, or is it left up to the user to ensure that the white point is adapted appropriately to the use case? And will it do spectral upsampling directly from RGB (like the Jakob 2019 method) , or will it be done from CIE XYZ (like the Meng 2015 method)?
So these illuminant-E spaces might need to be rethought in the future once we figure those questions out. I don't feel strongly about whether we include them for now or not, though.
If we do include them, however, I think it would be good to document in the descriptions the white point adaptation method (Bradford / Hunt / XYZ scaling) used to generate them. And I think that goes for the CIE-XYZ D65 space as well, now that I think of it. (Edit: and any other white-point-adapted spaces as well.)
(Edit 2: I got distracted in the middle of composing this, and Brecht beat me to the punch. Didn't see that before I posted.)
@ -124,1 +156,4 @@- !<ColorSpace>name: Linear E-Gamutaliases: [Linear E-Gamut I-D65, "FilmLight: Linear - E-Gamut"]Is E-Gamut actually used much outside of FilmLight's own tools and configurations? I could very well just be out of the loop on this, but my initial reaction was that it feels kind of specific and maybe not appropriate in a general configuration.
It's needed for AgX's LUT input encoding. Also there are people using E-Gamut for interop, this repository for example:
https://github.com/gralk/images
We could hide it from the UI and only use it internally. But I guess it can be useful to exchange files with BaseLight and it's not really in the way.
Maybe in the description change "Linear E-Gamut" to "Linear FilmLight E-Gamut" to hint what this is for.
Not sure if it should be in this PR or not, but at some point I think it would be good to also add Rec.2100 (both HLG and PQ) as display spaces as well.
I have experimented with Rec.2100 but I don't have a finished version of AgX for it. Still experimenting.
I don't think their inclusion should be blocked on AgX support. The point is to support the output spaces themselves, even if there isn't yet a good tone mapper / image formation chain / whatever you want to call it for them yet. They can just have
Standardfor now, and better transforms can be added later.(Again, maybe that's not for this PR anyway. Just saying AgX isn't a blocker/prerequisite for that IMO.)
There are also complications like we need to have multiple versions that are 1000 nits limited and P3 limited etc., I feel more confortable doing so after all things are figured out.
Yeah, that's a good point. Sounds good.
(Edit: to clarify, I'm assuming AgX isn't part of that "all things". I really think these questions are independent of AgX.)
When we add HDR display spaces, we need to ensure there is actually a way to use them for display or file saving. For display we definitely need significant code changes, for file saving maybe there is a file format that happens to work, but most likely this will also need changes in various places to actually work well.
I'm not sure what the most common file formats are for HDR images like that, I see for example WebP and JPEG 2000 support HDR but I would guess they are not the primary file formats for this. Mainly I see references to encoding HDR video, and I imagine ffmpeg integration will need some changes for that.
This looks ok to me now, but note:
tests/imbuf_io/reference_loadwill need to be updated for this rename.I believe all comments are addressed. Though I left
Linear DCI-P3 D65to haveD65in its name since we are not using the Theater white point here.Love to see we are all in an agreement now :)
The cryptomatte test is failing. It is because the string-based comparison is not enough to see the exr is in scene linear space already. I have a patch for it #110959.
The imbuf tests I think we'll need to update the expected color space.
I'll take care of landing the #110959 (or equivaleent fix to ensure no pixels are touched for cryptomatte), will merge main branch to this PR, and update the expected color space for the imbuf test.
P.S. Will also commit alias to 3.6 for the compatibility.
@Eary Ah, do you mind updating the description to reflect the updated state of added and renamed color spaces? That will help a lot for the final commit message :)
Sure, done.
@blender-bot build
To add a small note on this subject, over the past year there seems to be some convergence around ISO 22028-5 (especially using AVIF or JPEG XL). Apple is adding OS-wide support for HDR images in this format and adding some APIs for general HDR still-image support: https://developer.apple.com/videos/play/wwdc2023/10181/
Adobe has also confirmed that the upcoming HDR mode in Photoshop/Lightroom will also be based around ISO 22028-5 for file I/O: https://community.adobe.com/t5/camera-raw-discussions/p-beta-feature-hdr-output-feedback-thread/m-p/13992463#M22858
This isnt mentioned in the Python API bit of the release notes, despite changing the API in such a way it crashes scripts if not modified.
@Simarilius That is a fair point. We should mention it.
However, if you do "hard-coded" assignments, even prior to the changes we did the script will fail for people with custom OCIO configuration. This is why people shouldn't really assign hard-coded colorspaces. It does seem that we need to expose couple of functionality in the API in order to do things properly:
Can you describe your use-case a bit more, so that we know that changes we are planing to do will support your case?
I'm one of the contributors to the Wolvenkit Cyberpunk Import plugin (https://github.com/WolvenKit/Cyberpunk-Blender-add-on) which imports Cyberpunk assets to Blender and tries to reproduce the in game shaders. One of the shaders was setting an image texture node to LINEAR in the plugin, when I did the update for B4 I initially missed updating this as it wasnt in the release notes so figured I should mention it.
@Simarilius What is the format of the image?
It'll be a PNG, I'm not even 100% sure it needs to be set/changed from default, just already was and the existing code crashed when it wasn't something the release notes warned needed fixing.
PNG is default to sRGB. If it stores some non-color data (maybe direct light) then it needs to eb set to
Non-Color, and it it is somehow contains linear color then its space is to the corresponding linear space (perhaps, scene linear).In any case, it all falls under ability to query roles from the OCIO configuration. While the actual color space name could vary, the name of the role is expected to be the same. Basically, you'd asking "hey, OCIO, what's the color space name for non-color data?". We'll first need to implement this, but it was good to confirm that it actually will solve your use-case in a portable manner.
P.S. I've added a note in the Python API breaking changes about the color space names. And renaming and removal is also mentioned in the Color Management section of the release notes.