Page MenuHome
Differential D10978 Diff 37438 source/blender/blenlib/BLI_color.hh

Changeset View

Standalone View

View Options

source/blender/blenlib/BLI_color.hh

Show All 16 Lines
#pragma once #pragma once
#include <iostream> #include <iostream>
#include "BLI_math_color.h" #include "BLI_math_color.h"
namespace blender { namespace blender {
struct Color4f { /**
float r, g, b, a; * CPP based color structures.
*
* Strongly typed color storage structures with space and alpha association.
* Will increase readability and visibility of typical mistakes when
JacquesLuckeUnsubmitted
Done
Inline Actions

typo (typically)

JacquesLucke: typo (`typically`)
* working with colors.
*
* The storage structs can hold 4 channels (r, g, b and a).
*
* Usage:
*
* Convert a theme byte color to a linearrgb premultiplied.
JacquesLuckeUnsubmitted
Done
Inline Actions

typo (an)

JacquesLucke: typo (`an`)
* ```
* ColorTheme4b theme_color;
* ColorSceneLinear4f<eAlpha::Premultiplied> linearrgb_color =
* BLI_color_convert_to_scene_linear(theme_color).premultiply_alpha();
JacquesLuckeUnsubmitted
Done
Inline Actions

This seems to be outdated, the BLI_color_convert_to_linear function does not exist.

JacquesLucke: This seems to be outdated, the `BLI_color_convert_to_linear` function does not exist.
* ```
*
* The API is structured to make most use of inlining. Most notable are space
* conversions done via `BLI_color_convert_to*` functions.
*
* - Conversions between spaces (theme <=> scene linear) should always be done by
* invoking the `BLI_color_convert_to*` methods.
* - Encoding colors (compressing to store colors inside a less precision storage)
* should be done by invoking the `encode` and `decode` methods.
* - Changing alpha association should be done by invoking `premultiply_alpha` or
JacquesLuckeUnsubmitted
Done
Inline Actions

typo (to_(pre)multiplied_alpha)

JacquesLucke: typo (`to_(pre)multiplied_alpha`)
* `unpremultiply_alpha` methods.
*
* # Encoding.
*
* Color encoding is used to store colors with less precision as in using `uint8_t` in
* stead of `float`. This encoding is supported for `eSpace::SceneLinear`.
* To make this clear to the developer the `eSpace::SceneLinearByteEncoded`
JacquesLuckeUnsubmitted
Done
Inline Actions

typo (a)

JacquesLucke: typo (`a`)
* space is added.
*
* # Precision
*
* Colors can be stored using `uint8_t` or `float` colors. The conversion
* between the two precisions are available as methods. (`to_4b` and
* `to_4f`).
*
brechtUnsubmitted
Done
Inline Actions

This should not be hardcoded, it's not correct if a different OpenColorIO config than the default is used.

When dealing with vertex colors as in this patch, I'd use two color spaces:

  • SceneLinear: float precision color corresponding to the scene linear role in the OpenColorIO config
  • SceneLinearByte: byte precision colors, which are scene linear floats compressed with an sRGB transfer function

Not sure about the best names, it could be "byte", "compressed", something along those lines. But we should not consider vertex colors to be in sRGB space or for there to be some fixed "linear" space.

brecht: This should not be hardcoded, it's not correct if a different OpenColorIO config than the…
jbakkerAuthorUnsubmitted
Done
Inline Actions

That is a good one. In the gpu/draw module it is also confusing when we mean sRGB (themes) and when it is linear, but with byte encoding. What happens to be implemented as srgb.

I prefer the term SceneLinearByteEncoded. A bit long, but gives clarity.

Would this then also be the time to link this to actual ocio config. I do see some challenges there and would suggest to do it as a separate project after this one is completed due to the tight relation with srgb buffers and different goals (code deconfussion vs more flexible color pipeline).

jbakker: That is a good one. In the gpu/draw module it is also confusing when we mean sRGB (themes) and…
brechtUnsubmitted
Done
Inline Actions

ByteEncoded sounds good to me.

I'm not sure what linking to the OCIO config means exactly, but I think just naming the colors after the roles they correspond to in the OCIO config is most important.

brecht: `ByteEncoded` sounds good to me. I'm not sure what linking to the OCIO config means exactly…
* # Alpha conversion
*
* Alpha conversion is only supported in SceneLinear space.
*
* Extending this file:
* - This file can be extended with `ColorHex/Hsl/Hsv` for different representations
* of rgb based colors. `ColorHsl4f<eSpace::SceneLinear, eAlpha::Premultiplied>`
* - Add non RGB spaces/storages ColorXyz.
*/
/* Enumeration containing the different alpha modes. */
enum class eAlpha {
/* Color and alpha are unassociated. */
Straight,
/* Color and alpha are associated. */
Premultiplied,
brechtUnsubmitted
Done
Inline Actions

The reference color space in OpenColorIO configs is not relevant to how Blender works. It's important for the config to define what the transforms are relative to, but there are no colors in Blender in this reference space. I guess this is meant to be SceneLinear instead.

brecht: The reference color space in OpenColorIO configs is not relevant to how Blender works. It's…
};
std::ostream &operator<<(std::ostream &stream, const eAlpha &space);
/* Enumeration containing internal spaces. */
enum class eSpace {
/* Blender theme color space (sRGB). */
Theme,
brechtUnsubmitted
Done
Inline Actions

I would not add an sRGB color space when the implementation is known to not take into account what the scene linear color space is.

Any code using this may introduce bugs, so I would recommend having it.

brecht: I would not add an sRGB color space when the implementation is known to not take into account…
jbakkerAuthorUnsubmitted
Done
Inline Actions

I am a bit between two solutions here. Theme colors are in sRGB. when removing sRGB and not changing the current logic users would do the conversion the old way that is error prone.

 char theme_color[4];
 float linear_color[4];
 srgb_to_linearrgb_v4(linear_color, srgb_color)
ColorSceneLinear4f<eAlpha::Straight> scene_linear(linear_color)

A future proof solution would be to use OCIO for the conversion between known color spaces and roles.

I think you're right to remove the confusion from start and only add it when we have connected it to color management.

jbakker: I am a bit between two solutions here. Theme colors are in sRGB. when removing sRGB and not…
brechtUnsubmitted
Done
Inline Actions

I think it's fine to have a color space for theme colors. And a case I forgot is the usage of theme colors in the viewport background, which needs a conversion to scene linear.

Maybe we can rename ColorSrgb to ColorTheme or ColorUI?

brecht: I think it's fine to have a color space for theme colors. And a case I forgot is the usage of…
/* Blender internal scene linear color space (maps to SceneReference role in OCIO). */
SceneLinear,
/* Blender internal scene linear color space compressed to be stored in 4 uint8_t. */
SceneLinearByteEncoded,
};
std::ostream &operator<<(std::ostream &stream, const eSpace &space);
Color4f() = default; /* Template class to store RGBA values with different precision, space and alpha association. */
template<typename ChannelStorageType, eSpace Space, eAlpha Alpha> class ColorRGBA {
public:
ChannelStorageType r, g, b, a;
constexpr ColorRGBA() = default;
Color4f(const float *rgba) : r(rgba[0]), g(rgba[1]), b(rgba[2]), a(rgba[3]) constexpr ColorRGBA(const ChannelStorageType rgba[4])
: r(rgba[0]), g(rgba[1]), b(rgba[2]), a(rgba[3])
{ {
} }
Color4f(float r, float g, float b, float a) : r(r), g(g), b(b), a(a) constexpr ColorRGBA(const ChannelStorageType r,
JacquesLuckeUnsubmitted
Done
Inline Actions

I think this name should be a bit more specific, like convert_color_space. Also it might make sense to take src as a reference, just for consistency.

JacquesLucke: I think this name should be a bit more specific, like `convert_color_space`. Also it might make…
const ChannelStorageType g,
const ChannelStorageType b,
const ChannelStorageType a)
: r(r), g(g), b(b), a(a)
{ {
} }
operator float *() operator ChannelStorageType *()
{ {
return &r; return &r;
} }
operator const float *() const operator const ChannelStorageType *() const
{ {
return &r; return &r;
} }
friend std::ostream &operator<<(std::ostream &stream, Color4f c) friend std::ostream &operator<<(std::ostream &stream,
brechtUnsubmitted
Done
Inline Actions

I don't think this kind of automatic conversion in a constructor is good.

These conversions can be lossy. For example you would not want a function that has a temporary variable with a different alpha mode than the data it is operating on, because you will lose data.

What I think is helpful is communicating intent and type safety. I'd rather have specific functions that associate/unassociate alpha, decode/encode byte colors, etc.

brecht: I don't think this kind of automatic conversion in a constructor is good. These conversions…
jbakkerAuthorUnsubmitted
Done
Inline Actions

Although I tried to do no conversion in constructor at all, it wasn't working for me. The compiler wasn't smart enough to select the correct template function. Hence it is commented out.

When using specific conversion methods introduces more bugs as you really need to specify the correct constructor. When this isn't done the compiler will select float* or uint8_t* constructor what isn't expected by the developer. See SrgbStraightToSceneLinearPremultiplied for example where every step needs to be specified.

We could simplify this for common transformations, but I think the current setup is too error-prone.

Another solution would be to make the color space + storage as a single class with concrete methods eg Srgb4b/SceneLinear4f as subclasses from abstract classes Color4b and Color4f

jbakker: Although I tried to do no conversion in constructor at all, it wasn't working for me. The…
JacquesLuckeUnsubmitted
Done
Inline Actions

I think it's fine to do no color space conversion in constructors and only use standalone functions with descriptive names instead. That should make things more explicit indeed.

I can see the issue with the float/uint8_t * constructor. The issue here is not really the constructor though but the implicit conversion to a pointer. This was causing issues for float3 as well sometimes. The best solution might be to just remove operator float *() and similar conversion operators. Instead people could use &color.r or something like color.ptr(). It's not pretty but at least it does not have these issues.

JacquesLucke: I think it's fine to do no color space conversion in constructors and only use standalone…
const ColorRGBA<ChannelStorageType, Space, Alpha> &c)
{ {
stream << "(" << c.r << ", " << c.g << ", " << c.b << ", " << c.a << ")";
stream << Space << Alpha << "(" << c.r << ", " << c.g << ", " << c.b << ", " << c.a << ")";
return stream; return stream;
} }
friend bool operator==(const Color4f &a, const Color4f &b) friend bool operator==(const ColorRGBA<ChannelStorageType, Space, Alpha> &a,
const ColorRGBA<ChannelStorageType, Space, Alpha> &b)
{ {
return a.r == b.r && a.g == b.g && a.b == b.b && a.a == b.a; return a.r == b.r && a.g == b.g && a.b == b.b && a.a == b.a;
} }
friend bool operator!=(const Color4f &a, const Color4f &b) friend bool operator!=(const ColorRGBA<ChannelStorageType, Space, Alpha> &a,
const ColorRGBA<ChannelStorageType, Space, Alpha> &b)
{ {
return !(a == b); return !(a == b);
} }
uint64_t hash() const uint64_t hash() const
{ {
uint64_t x1 = *reinterpret_cast<const uint32_t *>(&r); uint64_t x1 = *reinterpret_cast<const uint32_t *>(&r);
uint64_t x2 = *reinterpret_cast<const uint32_t *>(&g); uint64_t x2 = *reinterpret_cast<const uint32_t *>(&g);
uint64_t x3 = *reinterpret_cast<const uint32_t *>(&b); uint64_t x3 = *reinterpret_cast<const uint32_t *>(&b);
uint64_t x4 = *reinterpret_cast<const uint32_t *>(&a); uint64_t x4 = *reinterpret_cast<const uint32_t *>(&a);
return (x1 * 1283591) ^ (x2 * 850177) ^ (x3 * 735391) ^ (x4 * 442319); return (x1 * 1283591) ^ (x2 * 850177) ^ (x3 * 735391) ^ (x4 * 442319);
} }
}; };
struct Color4b { /* Forward declarations of concrete color classes. */
uint8_t r, g, b, a; template<eAlpha Alpha> class ColorSceneLinear4f;
template<eAlpha Alpha> class ColorSceneLinearByteEncoded4b;
template<typename ChannelStorageType> class ColorTheme4;
Color4b() = default; /* Forward declation of precision conversion methods. */
BLI_INLINE ColorTheme4<float> BLI_color_convert_to_theme4f(const ColorTheme4<uint8_t> &srgb4b);
BLI_INLINE ColorTheme4<uint8_t> BLI_color_convert_to_theme4b(const ColorTheme4<float> &srgb4f);
Color4b(uint8_t r, uint8_t g, uint8_t b, uint8_t a) : r(r), g(g), b(b), a(a) template<eAlpha Alpha>
class ColorSceneLinear4f final : public ColorRGBA<float, eSpace::SceneLinear, Alpha> {
public:
constexpr ColorSceneLinear4f<Alpha>() : ColorRGBA<float, eSpace::SceneLinear, Alpha>()
{ {
} }
Color4b(Color4f other) constexpr ColorSceneLinear4f<Alpha>(const float *rgba)
: ColorRGBA<float, eSpace::SceneLinear, Alpha>(rgba)
{ {
rgba_float_to_uchar(*this, other);
} }
operator Color4f() const constexpr ColorSceneLinear4f<Alpha>(float r, float g, float b, float a)
: ColorRGBA<float, eSpace::SceneLinear, Alpha>(r, g, b, a)
{ {
Color4f result;
rgba_uchar_to_float(result, *this);
return result;
} }
operator uint8_t *() /**
* Convert to its byte encoded counter space.
**/
ColorSceneLinearByteEncoded4b<Alpha> encode() const
{ {
return &r; ColorSceneLinearByteEncoded4b<Alpha> encoded;
linearrgb_to_srgb_uchar4(encoded, *this);
return encoded;
} }
operator const uint8_t *() const /**
* Convert color and alpha association to premultiplied alpha.
*
* Does nothing when color has already a premultiplied alpha.
*/
ColorSceneLinear4f<eAlpha::Premultiplied> premultiply_alpha() const
JacquesLuckeUnsubmitted
Done
Inline Actions

It feels like either (1) this method should not exist when when Alpha != Straight or (2) it should just return the value unchanged in this case.

  1. This could be done by moving the method out of ColorSceneLinear4f into a standalone function. Alternatively, one could use some SFINAE to disable the method based on template parameters, this would look quite ugly though.
  2. This could be done with a simple if constexpr (Alpha == eAlpha::Straight) ....

The same applies to the method below of course.

JacquesLucke: It feels like either (1) this method should not exist when when `Alpha != Straight` or (2) it…
{ {
return &r; if constexpr (Alpha == eAlpha::Straight) {
ColorSceneLinear4f<eAlpha::Premultiplied> premultiplied;
straight_to_premul_v4_v4(premultiplied, *this);
return premultiplied;
}
else {
return *this;
}
} }
friend std::ostream &operator<<(std::ostream &stream, Color4b c) /**
* Convert color and alpha association to straight alpha.
*
* Does nothing when color has straighten alpha.
JacquesLuckeUnsubmitted
Done
Inline Actions

typo (..)

JacquesLucke: typo (`..`)
*/
ColorSceneLinear4f<eAlpha::Straight> unpremultiply_alpha() const
{ {
stream << "(" << c.r << ", " << c.g << ", " << c.b << ", " << c.a << ")"; if constexpr (Alpha == eAlpha::Premultiplied) {
return stream; ColorSceneLinear4f<eAlpha::Straight> straighten;
premul_to_straight_v4_v4(straighten, *this);
return straighten;
}
else {
return *this;
}
} }
};
template<eAlpha Alpha>
class ColorSceneLinearByteEncoded4b final
: public ColorRGBA<uint8_t, eSpace::SceneLinearByteEncoded, Alpha> {
public:
constexpr ColorSceneLinearByteEncoded4b() = default;
friend bool operator==(const Color4b &a, const Color4b &b) constexpr ColorSceneLinearByteEncoded4b(const uint8_t *rgba)
JacquesLuckeUnsubmitted
Done
Inline Actions

Should this constructor and the one below take uint8_ts as argument?

JacquesLucke: Should this constructor and the one below take `uint8_t`s as argument?
: ColorRGBA<uint8_t, eSpace::SceneLinearByteEncoded, Alpha>(rgba)
{ {
return a.r == b.r && a.g == b.g && a.b == b.b && a.a == b.a;
} }
friend bool operator!=(const Color4b &a, const Color4b &b) constexpr ColorSceneLinearByteEncoded4b(uint8_t r, uint8_t g, uint8_t b, uint8_t a)
: ColorRGBA<uint8_t, eSpace::SceneLinearByteEncoded, Alpha>(r, g, b, a)
{ {
return !(a == b);
} }
uint64_t hash() const /**
* Convert to back to float color.
*/
JacquesLuckeUnsubmitted
Done
Inline Actions

typo (**)

JacquesLucke: typo (`**`)
ColorSceneLinear4f<Alpha> decode() const
{ {
return static_cast<uint64_t>(r * 1283591) ^ static_cast<uint64_t>(g * 850177) ^ ColorSceneLinear4f<Alpha> decoded;
static_cast<uint64_t>(b * 735391) ^ static_cast<uint64_t>(a * 442319); srgb_to_linearrgb_uchar4(decoded, *this);
return decoded;
} }
}; };
/**
* Theme color template class.
*
* Don't use directly, but use `ColorTheme4b/ColorTheme4b`.
*
* This has been implemented as a template to improve inlining. When implemented as concrete
* classes (ColorTheme4b/f) the functions would be hidden in a compile unit what wouldn't be
* inlined.
*/
template<typename ChannelStorageType>
class ColorTheme4 final : public ColorRGBA<ChannelStorageType, eSpace::Theme, eAlpha::Straight> {
public:
constexpr ColorTheme4() : ColorRGBA<ChannelStorageType, eSpace::Theme, eAlpha::Straight>(){};
constexpr ColorTheme4(const ChannelStorageType *rgba)
: ColorRGBA<ChannelStorageType, eSpace::Theme, eAlpha::Straight>(rgba)
{
}
constexpr ColorTheme4(ChannelStorageType r,
ChannelStorageType g,
ChannelStorageType b,
ChannelStorageType a)
: ColorRGBA<ChannelStorageType, eSpace::Theme, eAlpha::Straight>(r, g, b, a)
{
}
/**
* Change precision of color to float.
*/
ColorTheme4<float> to_4f() const
{
if constexpr ((std::is_same_v<ChannelStorageType, uint8_t>)) {
return BLI_color_convert_to_theme4f(*this);
}
else {
return *this;
}
}
/**
* Change precision of color to uint8_t.
*/
ColorTheme4<uint8_t> to_4b() const
{
if constexpr ((std::is_same_v<ChannelStorageType, float>)) {
return BLI_color_convert_to_theme4b(*this);
}
else {
return *this;
}
}
};
using ColorTheme4b = ColorTheme4<uint8_t>;
using ColorTheme4f = ColorTheme4<float>;
BLI_INLINE ColorTheme4b BLI_color_convert_to_theme4b(const ColorTheme4f &theme4f)
{
ColorTheme4b theme4b;
rgba_float_to_uchar(theme4b, theme4f);
return theme4b;
JacquesLuckeUnsubmitted
Done
Inline Actions

Not sure I understand that last sentence.

JacquesLucke: Not sure I understand that last sentence.
}
BLI_INLINE ColorTheme4f BLI_color_convert_to_theme4f(const ColorTheme4b &theme4b)
{
ColorTheme4f theme4f;
rgba_uchar_to_float(theme4f, theme4b);
return theme4f;
}
BLI_INLINE ColorSceneLinear4f<eAlpha::Straight> BLI_color_convert_to_scene_linear(
const ColorTheme4f &theme4f)
{
ColorSceneLinear4f<eAlpha::Straight> scene_linear;
srgb_to_linearrgb_v4(scene_linear, theme4f);
return scene_linear;
}
BLI_INLINE ColorSceneLinear4f<eAlpha::Straight> BLI_color_convert_to_scene_linear(
const ColorTheme4b &theme4b)
{
ColorSceneLinear4f<eAlpha::Straight> scene_linear;
srgb_to_linearrgb_uchar4(scene_linear, theme4b);
return scene_linear;
}
BLI_INLINE ColorTheme4f
BLI_color_convert_to_theme4f(const ColorSceneLinear4f<eAlpha::Straight> &scene_linear)
JacquesLuckeUnsubmitted
Done
Inline Actions

This doesn't compile for me.
Use BLI_assert((std::is_same_v<ChannelStorageType, uint8_t>)); instead (note the double parenthesis).
The same applies to the method below.
Also the same arguments I mentioned in to_premultiplied_alpha apply here.

JacquesLucke: This doesn't compile for me. Use `BLI_assert((std::is_same_v<ChannelStorageType, uint8_t>));`…
{
ColorTheme4f theme4f;
linearrgb_to_srgb_v4(theme4f, scene_linear);
return theme4f;
}
BLI_INLINE ColorTheme4b
BLI_color_convert_to_theme4b(const ColorSceneLinear4f<eAlpha::Straight> &scene_linear)
{
ColorTheme4b theme4b;
linearrgb_to_srgb_uchar4(theme4b, scene_linear);
return theme4b;
}
/* Internal roles. For convenience to shorten the type names and hide complexity. */
using ColorGeometry4f = ColorSceneLinear4f<eAlpha::Premultiplied>;
using ColorGeometry4b = ColorSceneLinearByteEncoded4b<eAlpha::Premultiplied>;
brechtUnsubmitted
Done
Inline Actions

I would not use "scene reference" terminology. "Reference" has a specific meaning in OpenColorIO, and it's different.

Maybe giving the ColorSceneLinear4f template a default value of eAlpha::Premultiplied works? Then we don't have to introduce a new term.

brecht: I would not use "scene reference" terminology. "Reference" has a specific meaning in…
jbakkerAuthorUnsubmitted
Done
Inline Actions

Both premultiplied and straight is used in blender (compositor and selected operations).
My proposal would be to remove ColorSceneReference4f and ColorSceneReference4b and use the actual classes

using ColorGeometry4f = ColorSceneLinear4f<eAlpha::Premultiplied>;
using ColorGeometry4b = ColorSceneLinearByteEncoded4b<eAlpha::Premultiplied>;
jbakker: Both premultiplied and straight is used in blender (compositor and selected operations). My…
brechtUnsubmitted
Done
Inline Actions

Fine with me too.

brecht: Fine with me too.
} // namespace blender } // namespace blender
jbakkerAuthorUnsubmitted
Done
Inline Actions

new line

jbakker: new line