Page MenuHome
Differential D10978 Diff 36166 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 { /* Spaces are defined as classes to be extended with meta-data in the future.
* The meta data could contain CIE mapping and whitepoints. */
class Srgb {
};
class LinearRGB {
};
/* Enumeration containing the different alpha modes. */
enum class eAlpha : uint8_t {
/* Alpha is unassociated (color is straight). */
Straight,
/* Alpha is associated (color is premultiplied with alpha). */
Premultiplied,
};
template<typename Space, eAlpha Alpha> struct Color4f;
template<typename Space, eAlpha Alpha> struct Color4b;
/* Internal roles. To shorten the type names and hide complexity in areas where transformations are
* unlikely to happen. */
using ColorRender = Color4f<LinearRGB, eAlpha::Premultiplied>;
using ColorReference = Color4f<LinearRGB, eAlpha::Premultiplied>;
using ColorCompositor = Color4f<LinearRGB, eAlpha::Premultiplied>;
using ColorTheme = Color4b<Srgb, eAlpha::Straight>;
using ColorGeometry = Color4f<LinearRGB, eAlpha::Premultiplied>;
MINLINE void associate_alpha(const Color4f<LinearRGB, eAlpha::Straight> &src,
Color4f<LinearRGB, eAlpha::Premultiplied> &r_out);
MINLINE void unassociate_alpha(const Color4f<LinearRGB, eAlpha::Premultiplied> &src,
Color4f<LinearRGB, eAlpha::Straight> &r_out);
MINLINE void transfer_color(const Color4b<Srgb, eAlpha::Straight> &src,
Color4f<Srgb, eAlpha::Straight> &r_out);
MINLINE void transfer_color(const Color4b<Srgb, eAlpha::Straight> &src,
Color4f<LinearRGB, eAlpha::Straight> &r_out);
MINLINE void transfer_color(const Color4b<Srgb, eAlpha::Straight> &src,
Color4f<LinearRGB, eAlpha::Premultiplied> &r_out);
MINLINE void transfer_color(const Color4b<Srgb, eAlpha::Straight> &src,
Color4b<Srgb, eAlpha::Straight> &r_out);
MINLINE void transfer_color(const Color4f<LinearRGB, eAlpha::Premultiplied> &src,
Color4b<Srgb, eAlpha::Straight> &r_out);
template<typename Space, eAlpha Alpha> struct Color4f {
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…
float r, g, b, a; float r, g, b, a;
Color4f() = default; Color4f() = default;
Color4f(const float *rgba) : r(rgba[0]), g(rgba[1]), b(rgba[2]), a(rgba[3]) Color4f(const float *rgba) : r(rgba[0]), g(rgba[1]), b(rgba[2]), a(rgba[3])
JacquesLuckeUnsubmitted
Done
Inline Actions

typo (typically)

JacquesLucke: typo (`typically`)
{ {
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…
} }
Color4f(float r, float g, float b, float a) : r(r), g(g), b(b), a(a) Color4f(float r, float g, float b, float a) : r(r), g(g), b(b), a(a)
{ {
} }
JacquesLuckeUnsubmitted
Done
Inline Actions

typo (an)

JacquesLucke: typo (`an`)
template<typename OtherSpace, eAlpha OtherAlpha>
Color4f(const Color4b<OtherSpace, OtherAlpha> &src)
{
transfer_color(src, *this);
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…
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.
}
operator float *() operator float *()
{ {
return &r; return &r;
} }
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…
operator const float *() const operator const float *() const
{ {
return &r; return &r;
JacquesLuckeUnsubmitted
Done
Inline Actions

typo (to_(pre)multiplied_alpha)

JacquesLucke: typo (`to_(pre)multiplied_alpha`)
} }
friend std::ostream &operator<<(std::ostream &stream, Color4f c) friend std::ostream &operator<<(std::ostream &stream, Color4f c)
{ {
stream << "(" << c.r << ", " << c.g << ", " << c.b << ", " << c.a << ")"; stream << "(" << c.r << ", " << c.g << ", " << c.b << ", " << c.a << ")";
return stream; return stream;
} }
JacquesLuckeUnsubmitted
Done
Inline Actions

typo (a)

JacquesLucke: typo (`a`)
friend bool operator==(const Color4f &a, const Color4f &b) friend bool operator==(const Color4f<Space, Alpha> &a, const Color4f<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 Color4f<Space, Alpha> &a, const Color4f<Space, Alpha> &b)
{ {
return !(a == b); return !(a == b);
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…
} }
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 { template<typename Space, eAlpha Alpha> struct Color4b {
uint8_t r, g, b, a; uint8_t r, g, b, a;
Color4b() = default; Color4b() = default;
Color4b(uint8_t r, uint8_t g, uint8_t b, uint8_t a) : r(r), g(g), b(b), a(a) Color4b(uint8_t r, uint8_t g, uint8_t b, uint8_t a) : r(r), g(g), b(b), a(a)
{ {
} }
Color4b(Color4f other) template<typename OtherSpace, eAlpha OtherAlpha>
Color4b(const Color4f<OtherSpace, OtherAlpha> &src)
{ {
rgba_float_to_uchar(*this, other); transfer_color(src, *this);
}
operator Color4f() const
{
Color4f result;
rgba_uchar_to_float(result, *this);
return result;
} }
operator uint8_t *() operator uint8_t *()
{ {
return &r; return &r;
} }
operator const uint8_t *() const operator const uint8_t *() const
{ {
return &r; return &r;
} }
friend std::ostream &operator<<(std::ostream &stream, Color4b c) friend std::ostream &operator<<(std::ostream &stream, Color4b c)
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…
{ {
stream << "(" << c.r << ", " << c.g << ", " << c.b << ", " << c.a << ")"; stream << "(" << c.r << ", " << c.g << ", " << c.b << ", " << c.a << ")";
return stream; return stream;
JacquesLuckeUnsubmitted
Done
Inline Actions

typo (..)

JacquesLucke: typo (`..`)
} }
friend bool operator==(const Color4b &a, const Color4b &b) friend bool operator==(const Color4b<Space, Alpha> &a, const Color4b<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 Color4b &a, const Color4b &b) friend bool operator!=(const Color4b<Space, Alpha> &a, const Color4b<Space, Alpha> &b)
{ {
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?
return !(a == b); return !(a == b);
} }
uint64_t hash() const uint64_t hash() const
{ {
return static_cast<uint64_t>(r * 1283591) ^ static_cast<uint64_t>(g * 850177) ^ return static_cast<uint64_t>(r * 1283591) ^ static_cast<uint64_t>(g * 850177) ^
JacquesLuckeUnsubmitted
Done
Inline Actions

typo (**)

JacquesLucke: typo (`**`)
static_cast<uint64_t>(b * 735391) ^ static_cast<uint64_t>(a * 442319); static_cast<uint64_t>(b * 735391) ^ static_cast<uint64_t>(a * 442319);
} }
}; };
/* These functions should invoke a BLI_math_color macro or perform a OCIO color transfer. */
MINLINE void associate_alpha(const Color4f<LinearRGB, eAlpha::Straight> &src,
Color4f<LinearRGB, eAlpha::Premultiplied> &r_out)
{
straight_to_premul_v4_v4(r_out, src);
}
MINLINE void unassociate_alpha(const Color4f<LinearRGB, eAlpha::Premultiplied> &src,
JacquesLuckeUnsubmitted
Done
Inline Actions

Not sure I understand that last sentence.

JacquesLucke: Not sure I understand that last sentence.
Color4f<LinearRGB, eAlpha::Straight> &r_out)
{
premul_to_straight_v4_v4(r_out, src);
}
MINLINE void transfer_color(const Color4b<Srgb, eAlpha::Straight> &src,
Color4f<Srgb, eAlpha::Straight> &r_out)
{
rgba_uchar_to_float(r_out, src);
}
MINLINE void transfer_color(const Color4b<Srgb, eAlpha::Straight> &src,
Color4f<LinearRGB, eAlpha::Premultiplied> &r_out)
{
Color4f<LinearRGB, eAlpha::Straight> intermediate(src);
associate_alpha(intermediate, r_out);
}
MINLINE void transfer_color(const Color4b<Srgb, eAlpha::Straight> &src,
Color4b<Srgb, eAlpha::Straight> &r_out)
{
r_out.r = src.r;
r_out.b = src.b;
r_out.g = src.g;
r_out.a = src.a;
}
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>));`…
MINLINE void transfer_color(const Color4f<LinearRGB, eAlpha::Premultiplied> &src,
Color4f<LinearRGB, eAlpha::Straight> &r_out)
{
unassociate_alpha(src, r_out);
}
MINLINE void transfer_color(const Color4f<LinearRGB, eAlpha::Premultiplied> &src,
Color4b<Srgb, eAlpha::Straight> &r_out)
{
Color4f<LinearRGB, eAlpha::Straight> intermediate(src);
transfer_color(intermediate, r_out);
}
MINLINE void transfer_color(const Color4b<Srgb, eAlpha::Straight> &src,
Color4f<LinearRGB, eAlpha::Straight> &r_out)
{
srgb_to_linearrgb_uchar4(r_out, src);
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