Details
Diff Detail
- Repository
- rB Blender
Event Timeline
I'll probably add to_linear_rec709 and to_aces as well, if this API structure makes sense.
We could also add general OpenColorIO colorspace conversions to the API at some later point, but that would be solving a different problem. For importers and exporters it's important to be able to know what the colorspaces is in an absolute sense, whereas color space names in OpenColorIO configs can be arbitrary.
While most colors (by default are in linear color space), this API is returning colors in other spaces, meaning it's possible to have mathutils.Color in other color-spaces.
The down side of this is any further operations on them might not work as expected.
Would it be better to have mathutils.Color to store a space? This would simplify conversion functions since you could call to_xyz(...) knowing that whatever color-space the color is in, it will use the correct conversion, removing the need for from_* & the assumption that all colors are linear.
I am not really convinced making Color aware of its space is something what really makes life easier. For the colors coming from RNA we know the color space, but Color object might be created by a script. This means that either we accept Color without a space attached to it anyway (so that we do not break API) or we will introduce a huge breaking change and introduce an entire complication of creating colors.
I also believe it is not a new "problem" that there is no space attached to the Color: IO scripts already need to take care of conversion from either scene linear or sRGB when dealing with matertials/legacy vertex colors.
Moving the burden of implementing color space conversion logic from scripts to BPY is something we indeed need to do. The proposed patch mainly seems good to me. It does clarify a lot of things docstrings. The only thing I'd do different is naming to solve the ambiguity: to_xyz -> linear_to_xyz (or scene_linear_to_xyz if we do not want to have linear imply scene linear). This will help catching semantic issues from just seeing or typing the code (avoiding thought process indirection). I believe it will also solve the concern of Campbell.
Yes explicitly stating ‘scene linear’ is a good idea especially since CIE XYZ D65 is the display referred exchange color space.
If having the color space stored in the color complicates things to much, having more explicit functions seem reasonable, I find the to/from in this patch a bit confusing though.
These are straightforward, at the expense of being more verbose.
- .to_srgb_from_linear()
- .to_srgb_from_xyz()
- `.to_linear_from_srgb()' ... etc.
Another option could be to have the from as an argument, defaulting to linear.
- .to_srgb(from='XYZ') for e.g, that has the advantage that if we support more color spaces in the API the number of methods wont increase so much, although in this case I doubt that would be a problem.
Just for the records, this works although it looses a bit of precision.
For instance: if the byte color is 179, it becomes 178 after the conversion. I'm not sure if it is relevant.
[EDIT: it turns out to be 178.9 so it is fine]
Color(C.object.data.attributes.active_color.data[0].color[:3]).scene_linear_to_srgb()[0] * 255
178.97117167711258
| intern/opencolorio/fallback_impl.cc | ||
|---|---|---|
| 246–247 | Without specifying the LINEAR component, it appears that this refers to the HDTV color space. | |
| intern/opencolorio/ocio_capi.h | ||
| 40 | Needs the AP0 to avoid confusion with the AP1 ACEScg color space. | |
Having all these methods with color space names as top-level methods clutters the namespace. Using a single prefix that makes it clear these functions are conversions and groups them more usefully:
With this patch auto-completion shows:
>>> color = Color()
>>> color.
aces_to_scene_linear(
b
copy(
freeze(
g
h
hsv
is_frozen
is_valid
is_wrapped
owner
r
rec709_linear_to_scene_linear(
s
scene_linear_to_aces(
scene_linear_to_rec709_linear(
scene_linear_to_srgb(
scene_linear_to_xyz_d65(
srgb_to_scene_linear(
v
xyz_d65_to_scene_linear(Using to/from terms would group them more usefully, eg:
>>> color.
-- snip ---
to_aces_from_scene_linear(
to_rec709_linear_from_scene_linear(
to_scene_linear_from_aces(
to_scene_linear_from_rec709_linear(
to_scene_linear_from_srgb(
to_scene_linear_from_xyz_d65(
to_srgb_from_scene_linear(
to_xyz_d65_from_scene_linear(Other names could work too but .convert_* for e.g. but end up being more verbose.
I find the "reversed" order of "from" and "to" component confusing and harder to follow than a more natural semantic "from FOO to BAR". If we are to name functions in a way that visually groups them then I'd say lets use "from_" prefix. For example from_scene_linear_to_xyz.
Out of curiosity: the new functions all do two conversions, via XYZ; is there no need to expose that as IMB_IMB_colormanagement_…_to_xyz and IMB_colormanagement_xyz_to_… functions?
Another thing, which is very minor and feel free to completely ignore. The two matrix multiplications in each function feel a bit weird to me. From a clarity of code perspective, and from the single responsibility principle, I would expect each conversion to be its own function, and then RGB → REC709 would do two function calls for RGB → XYZ and XYZ → REC709. I can understand the need to save some function calls and get these functions more performant, but in that case, wouldn't it be better to also halve the number of matrix multiplications, by precomputing the RGB → REC709 matrix and using that directly?
About the naming: I agree that from_…_to_… is clearer.
Ok, will rename to from_scene_linear_to_xyz.
The fact that XYZ is used as an intermediate space internally is an implementation detail, which might change at some point. Conceptually we should be thinking of scene linear as the main color space that you can convert to/from for file I/O, or convert from for display.
I'm going to do cleanup commit after this with some renaming for clarity, I can also precompute the matrices then to avoid having to do two matrix multiplications.
| intern/opencolorio/fallback_impl.cc | ||
|---|---|---|
| 246–247 | The fact that it's a matrix transform to me implies it's linear. | |
| intern/opencolorio/ocio_capi.h | ||
| 40 | The comment clarifies that. The proper name as suggested by the standard really is "ACES2065-1", but I think using "ACES" and "ACEScg" in API names is enough to distinguish them. | |
| source/blender/python/mathutils/mathutils_Color.c | ||
|---|---|---|
| 1018 | Looks like a typo? | |