Optional and opaque RGB colors #2472

uklotzde · 2020-01-29T14:29:44Z

QColor is too generic for representing opaque RGB colors with 8-bit per channel and a corresponding 24-bit color code. This new class handles all validation, normalization and conversion by wrapping QColor. It is more restrictive than QColor, but preserves the optional (= invalid) state.

This class could be used for all kinds of color metadata, both track colors (#2470) and cue colors. We might even think about making cue colors optional, because QColor is implicitly optional and we would need to handle it anyway! I should have noticed this before 🙈 But my idea of such a wrapper class was rejected when I mentioned that we need such an abstraction and so I didn't dig deeper into the topic.

The unit tests revealed that the handling is trickier than expected and that directly dealing with QColor would be error prone.

daschuer · 2020-01-29T16:58:35Z

Thank you for this work.

When writing the cue color code I did consider to write it as well, but than decided to just use QRgb. Than I have discovered the bug with QColor QRgb and the opacity.
So it is good to have this.

Did you consider to use RgbColorCode as storage variable. This has some benefit:

always valid
Instant conversions to/from double value vor CO
Instant conversions to/from DB and external interfaces.
Single instruction copy (QColor is not implicit shared)

This comes with the cost of constructing QColor when needed. But the construction is not that heavy: https://github.com/mburakov/qt5/blob/93bfa3874c10f6cb5aa376f24363513ba8264117/qtbase/src/gui/painting/qcolor.cpp#L418

I don't have a benchmark yet, but I guess it will over all perform better.

What do you think?

uklotzde · 2020-01-29T18:01:03Z

use RgbColorCode as storage variable

What does that mean, any links? Please elaborate.

daschuer · 2020-01-29T19:58:32Z

src/util/color/rgbcolor.h

+        }
+    }
+
+    QColor m_color;


I mean this:

RgbColorCode m_color

use RgbColorCode as storage variable

What does that mean, any links? Please elaborate.

See my inline comment. You may use 0xFFFFFFFF for invalid. That automatically defaults to transparent, which is what we want in most cases.

Nope. Alpha = 0x00 is transparent.

We agreed on using QColor internally and this is the consequent follow-up and reasonable software design for such an approach. RgbColor allows even less variations than QColor.

We could also store RgbColorCode internally with alpha = 0xFF for all valid opaque colors and 0x00000000 as a special value for invalid/transparent/no color. But this custom mapping must not be exposed publicly! The only conversions that are provided are to/from QColor and std::optional<RgbColorCode>. The public API wouldn't change.

Yes, that was the Idea to keep the whole thing performant.

What is the public API you reffering to?
We have the CO interface that uses the 24 bit color as double, the ES interface that uses tree values for colors, the file tag that uses also a 24 bit value.
So it looks like QColor is only required to feed the Qt painting interface.

invalid/transparent/no color

Let's be clear with language. These are not identical concepts. There are contexts in which transparent is a valid value such as track color; calling it invalid would be a misnomer.

daschuer · 2020-01-29T20:01:52Z

use RgbColorCode as storage variable

What does that mean, any links? Please elaborate.

See my inline comment. You may use 0xFFFFFFFF for invalid. That automatically defaults to transparent, which is what we want in most cases.

uklotzde · 2020-01-29T23:02:48Z

I have turned the class upside down by replacing the internal QColor with RgbColorCode.

I didn't need to change a single test! IMHO a strong indicator that the design is robust and that this is the way to go.

uklotzde · 2020-01-29T23:14:14Z

PS: Let's please stop the discussions about negligible performance improvements and instead get the design right and work done.

src/util/color/rgbcolor.h

ferranpujolcamins

As already expressed, my biggest disagreement is with the fact that RgbColor can hold an invalid color.

Instead, I think it's best to make RgbColor represent only valid colors, then any function converting from QColor to RgbColor can return std::optional<RgbColor>.

ferranpujolcamins · 2020-01-29T17:49:26Z

src/util/color/rgbcolor.h

+class RgbColor {
+  public:
+    RgbColor() {
+        DEBUG_ASSERT(!isValid());


Do we need the debug assertion? This is already covered by the tests.

Furthermore, we could discuss whether we want the "optionality" to be baked in RgbColor. From my point of view, it'd more flexible to have a type that represents a valid color. If you want an optional color, that's what std::optional is for.

We must handle it, because QColor requires us to do so. Otherwise this class could not be agnostic and would require use case specific quirks.

The mapping to non-optional values has to be done on the next abstraction level, not here.

Debug assertions might reveal bugs that we didn't anticipate in the tests. Yes, these are helpful and intended!

What I mean is:

If I understood correctly, RgbColor is just RgbColorCode with a special value to indicate an invalid color (and some validation to make sure we don't accidentally set an invalid color).

Instead, I propose to use std::optional<RgbColorCode>. Here we don't treat any value of RgbColorCode as "invalid", so we can just ignore the upper bits.

With my suggested approach, we get !=, == and bool() for free from std::optional, while you needed to implement them for RgbColor.

Furthermore, the following methods are no longer needed:

optional()

optionalCodeToInternalCode (any RgbColorCode is valid, we just ignore the upper bits)

The only code you actually need to write is a bunch of free functions to convert back and forth QColor:

QColor toQColor(RgbColorCode color) { return QColor::fromRgb(m_internalCode & kRgbCodeMask); } QColor toQColor(std::optional<RgbColorCode> color) { if (color) { return toQColor(*color); } else { return QColor(); } } std::optional<RgbColorCode> toRgb(QColor color) { if (anyColor.isValid()) { return std::make_optional<RgbColorCode>(anyColor.rgb()) } else { return std::optional<RgbColorCode>() } }

Another advantage of this approach is that you can express whether you accept invalid colors or not in a function, by asking for a std::optional<RgbColorCode> or a RgbColorCode. This is not possible with your proposed approach.

All this being said, there's no need for more discussion unless you have some questions about what I've written here.

If you feel you approach is best, I'm fine. Just wanted you to consider this other one :)

@ferranpujolcamins Hm, I need to think about this again. Your remarks about optionality are valid.

I don't have any stakes in here and just wanted to demonstrate that we need some kind of abstraction. But it quickly escalated in terms of work effort ;)

Agree, sometimes just happens that your abstraction can just be a type alias, because what you need is already there ;)

src/util/color/rgbcolor.h

uklotzde · 2020-01-30T10:22:58Z

If you have different ideas, just do it.

I can only strongly recommend to add appropriate abstraction layers. This tiny class in conjunction with the tests revealed how complex the situation really is. Be warned, there are a many pitfalls that might not be obvious! ...including dirty and time consuming struggles with ancient C++ compilers :((

uklotzde · 2020-01-31T13:49:50Z

@Holzhaus
Proof of concept: Optional track colors

ferranpujolcamins · 2020-01-31T17:28:46Z

src/util/color/rgbcolor.h

+    // without an an alpha channel.
+    // We are using a separate typedef, because QRgb implicitly
+    // includes an alpha channel whereas this type does not!
+    typedef quint32 code_t;


What does the t stand for?

Shorthand for "type", common suffix in C/C++ for types and typedefs. The lowercase type names also pickup the spelling of the corresponding C++ types, i.e. optional. Lowercase type names for primitive types or PODs are also not uncommon. This keeps the definitions consistent and intuitive.

ferranpujolcamins · 2020-01-31T17:41:30Z

src/util/color/rgbcolor.h

+
+inline
+QColor toQColor(RgbColor color) {
+    return QColor::fromRgb(color);


Qt will ignore the alpha bits, so you can remove the bitmask. Is there any other reason to have it?

QColor::fromRgb() implicitly adds alpha bits. But QColor::rgb() doesn't remove them. Try it without the bitmask and the tests will fail.

Holzhaus · 2020-01-31T18:52:46Z

Can we still store the alpha bits in the Database though? Just in case we do want to allow transparency later on without rewriting the DB. We can just force them to 0xFF for now.

uklotzde · 2020-01-31T23:03:29Z

Can we still store the alpha bits in the Database though? Just in case we do want to allow transparency later on without rewriting the DB. We can just force them to 0xFF for now.

Can we imagine any use case for alpha bits? Why should track or cue colors have differing alpha values? Doing a database migration later when we actually need this feature is much easier than complicating the code by adding them now.

Holzhaus · 2020-02-02T22:47:41Z

Doing a database migration later when we actually need this feature is much easier than complicating the code by adding them now.

Yes, you're right. Something like UPDATE tracks SET color = (color ^ 0xFF000000); should work fine in case we need it. So just ignore my comment above ;-)

uklotzde · 2020-02-03T18:19:27Z

The track color PoC demonstrates how convenient this class and the associated utility functions are for adding (optional) color support. Anything else to do?

daschuer · 2020-02-03T22:37:05Z

src/util/color/rgbcolor.h

+}
+
+// Explicit conversion of both non-optional and optional
+// RgbColor values to QColor as overloaded free functions.


Why you decide her for a free function? I think it is more QT like to have a member function for this.

We cannot add new functions to std::optional. The conversion functions for RgbColor and std::optional<RgbColor should use the same invocation pattern. In C++ this is only possible with a free function.

Why is std::optional relevant here?
Can't we just introduce
RgbColor::toQColor()

No. Because std::nullopt -> QColor() (= invalid) is not covered by just RgbColor.

Sorry, I don't understand the point here.
I think I have missed something.

We have here the toQColor() function taking a RgbColor. There is no std::nullopt involved.

RgbColor rgb QColor c1 = toQColor(rgb); QColor c2 = rgb.toQColor();

What is the issue with the later?

oh I have some ideas, hit me up on zulip

ok sounds like we have consensus on this PR!

The decision to map std::nullopt to QColor() should be done here, only once and it should be tested. Instead of hoping that developers use this convention at various places.

The same arguments apply for the conversion to QVariant. Let's not repeat those bad practices in the current code base again and again.

daschuer · 2020-02-03T22:37:31Z

It still LGTM

ferranpujolcamins · 2020-02-04T17:20:39Z

LGTM

ywwg · 2020-02-04T18:38:19Z

Are we waiting for anything? Or can I push the big button?

uklotzde · 2020-02-04T19:07:06Z

Pending discussion #2472 (comment)

I strongly recommend to keep toQColor() and toQVariant() as free functions to avoid inconsistent invocation patterns. Developers should not need to think about the actual type for this use case. This is the price we have to pay for using std::optional instead of a custom class. IMHO a fair trade off.

Holzhaus · 2020-02-06T14:06:32Z

Merge?

Holzhaus · 2020-02-07T23:26:36Z

@uklotzde Are the any more changes needed or shall we merge?

uklotzde · 2020-02-08T09:04:51Z

@Holzhaus Ready. With the last commit I only added the missing tests to cover all possible conversions.

Holzhaus · 2020-02-10T23:32:55Z

Thanks!

ferranpujolcamins · 2020-02-13T21:38:58Z

src/util/color/rgbcolor.h

+#include "util/assert.h"
+#include "util/optional.h"
+
+namespace mixxx {


@uklotzde Is it ok if I move this class into the Color namespace in my cue color branch?

Please coordinate this with @Holzhaus to avoid merge conflicts. Better do it in a separate PR that is expected to be merged quickly.`

Please also note that namespace names should be lowercase:

https://google.github.io/styleguide/cppguide.html#Namespace_Names

The Color namespace violates this rule.

@ferranpujolcamins Do we actually need a separate color namespace within mixxx for just a bunch of classes, all with color in their class name?

The color namespace was there to group some free functions. We can reuse it to include your class. But it doesn't really matter to me. Just let me know what you think it's best.

With argument dependent lookup and function overloading available in C++ there is no need to hide free functions in artificial namespaces. Or do I miss a use case?

We are free to chose whatever works best for us. However keep an eye on discoverability and developer experience in general.

uklotzde added 5 commits January 29, 2020 15:29

Add RgbColor class

5055657

Add equality comparison operators

a831e39

Add note about RgbColorCode vs. QRgb

2144b51

Add note about immutability

e3b5353

Add more documentation

cfd14f1

uklotzde changed the title ~~Optional, opaque RGB colors~~ Optional and opaque RGB colors Jan 29, 2020

uklotzde changed the title ~~Optional and opaque RGB colors~~ [WiP] Optional and opaque RGB colors Jan 29, 2020

uklotzde added 2 commits January 29, 2020 18:46

Use std::optional for color code conversions

c657a9f

Reduce variations of conversion operations

af1fdb1

Declare Qt type information

7b6acf0

daschuer reviewed Jan 29, 2020

View reviewed changes

Replace internal QColor with RgbColorCode

ef4ce69

uklotzde added 2 commits January 30, 2020 00:06

Treat RgbColor as a POD

cc2e262

Register global meta type for RgbColor

a991d23

uklotzde changed the title ~~[WiP] Optional and opaque RGB colors~~ Optional and opaque RGB colors Jan 29, 2020

uklotzde changed the title ~~Optional and opaque RGB colors~~ [WiP] Optional and opaque RGB colors Jan 29, 2020

Fix build for ancient GCC version 5.4.0

c298ab3

daschuer requested changes Jan 30, 2020

View reviewed changes

src/util/color/rgbcolor.h Outdated Show resolved Hide resolved

src/util/color/rgbcolor.h Outdated Show resolved Hide resolved

src/util/color/rgbcolor.h Outdated Show resolved Hide resolved

uklotzde added 3 commits January 30, 2020 08:46

Update documentation

1b52a67

Replace isValid() with operator bool()

cac064b

Prefer explicit conversion and fix various issues

3326c92

ferranpujolcamins requested changes Jan 30, 2020

View reviewed changes

Remove constexpr

ec9a027

Fix type inference in tests

30f203d

uklotzde added 7 commits January 31, 2020 11:25

Move RgbColorCode typedef into RgbColor

b31edfd

Add missing constexpr

bc2fed7

Add comment about constexpr ctor

a864454

Implicitly validate color codes in RgbColor ctor

18b6ffe

Update class documentation

fc6a36a

Add debug output stream operators

3df8889

Fix invalid constexpr

dc597b0

Add conversions from/to QVariant

14a29f8

ferranpujolcamins reviewed Jan 31, 2020

View reviewed changes

Run clang-format

f6b781c

daschuer reviewed Feb 3, 2020

View reviewed changes

Holzhaus approved these changes Feb 5, 2020

View reviewed changes

Add missing RgbColor tests

06d393e

Holzhaus merged commit f7f52f6 into mixxxdj:master Feb 10, 2020

uklotzde deleted the rgbcolor branch February 11, 2020 00:04

ferranpujolcamins reviewed Feb 13, 2020

View reviewed changes

Optional and opaque RGB colors #2472

Optional and opaque RGB colors #2472

Conversation

uklotzde commented Jan 29, 2020 • edited Loading

daschuer commented Jan 29, 2020

uklotzde commented Jan 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jan 29, 2020

uklotzde commented Jan 29, 2020

uklotzde commented Jan 29, 2020

ferranpujolcamins left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferranpujolcamins Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Jan 30, 2020

uklotzde commented Jan 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus commented Jan 31, 2020

uklotzde commented Jan 31, 2020

Holzhaus commented Feb 2, 2020

uklotzde commented Feb 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Feb 3, 2020

ferranpujolcamins commented Feb 4, 2020

ywwg commented Feb 4, 2020

uklotzde commented Feb 4, 2020

Holzhaus commented Feb 6, 2020

Holzhaus commented Feb 7, 2020

uklotzde commented Feb 8, 2020

Holzhaus commented Feb 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Jan 29, 2020 •

edited

Loading

ferranpujolcamins left a comment •

edited

Loading

ferranpujolcamins Jan 30, 2020 •

edited

Loading

uklotzde Feb 4, 2020 •

edited

Loading