Specifics (Line-by-Line)
struct color_RGB
{
float R;
float G;
float B;
float a;
};
Inconsistent capitalization of members: A should be capitalized here, just like R, G, and B.
You also have strange naming conventions. You capitalize namespace and function names, but you lowercase structure names? You use underscores in structure names, but CamelCase in namespace and function names? Maybe you actually have a naming convention that calls for that, but it smells funny to me.
float Hue2RGB(float m1, float m2, float hue)
- The naming is too cutesy. Don't use "2" to mean "to". Spell it out:
HueToRGB
- What are
m1 and m2? Use descriptive names for your parameters.
if (hue < 0.0) { hue += 1.0; };
if (hue > 1.0) { hue -= 1.0; };
Are you sure that this logic is correct?
Consider instead:
hue = std::clamp(hue, 0.0, 1.0);
This is not only easier to read and may be faster (depending on your optimizer), but more importantly, seems to be more correct.
You can also replace the implementations of HueClamp and SLClamp with this call to std::clamp.
If you're not targeting C++17, then define it yourself in a header and use it. The definition is simple, and the boon to code readability is substantial:
template <class T>
constexpr const T& clamp(const T& value, const T& lowerBound, const T& upperBound)
{
return std::max(lowerBound, std::min(value, upperBound));
}
Of course, you don't even use HueClamp and SLClamp in the code, so I'm not sure what they're for. The client of the interface? Don't push those implementation details onto the client. Instead, handle whatever clamping needs to be done internally. The only reason you'd ever want to expose that complexity is if you were worried about the performance cost of possibly clamping a value that is already known to be within the appropriate bounds. You shouldn't worry about this, though. The cost of a simple clamp operation is extremely low, and, on the first attempt, correctness and simplicity matter more than speed.
else
return m1;
You should use curly braces here to create an explicit scope. You do everywhere else, so you should be consistent. Even if you weren't already doing it everywhere else in your code, I would highly recommend doing it everywhere to avoid introducing bugs during maintenance.
if (hue * 6.0 < 1.0)
(…and other places where the same pattern appears)
I recommend using parentheses to make operator precedence and associativity explicit. The comparison is done after the arithmetic operation here, so write it that way:
if ((hue * 6.0) < 1.0)
if (value <= 0.0) { return_value = given_s + 0.1; }
else if (value >= 1.0) { return_value = 1.0; }
Line up similar expressions using horizontal whitespace. This not only makes the code look neater, but it helps to catch inadvertent bugs.
if (value <= 0.0) { return_value = given_s + 0.1; }
else if (value >= 1.0) { return_value = 1.0; }
Same thing here:
if (max == color.R && max != color.G) { H += (color.G - color.B) / delta; }
if (max == color.G && max != color.B) { H += 2 + (color.B - color.R) / delta; }
if (max == color.R && max != color.R) { H += 4 + (color.R - color.G) / delta; }
Line 'em up:
if ((max == color.R) && (max != color.G)) { H += 0 + (color.G - color.B) / delta; }
if ((max == color.G) && (max != color.B)) { H += 2 + (color.B - color.R) / delta; }
if ((max == color.R) && (max != color.R)) { H += 4 + (color.R - color.G) / delta; }
I even go so far as to add no-ops sometimes to ensure that things line up perfectly. Why? Because now you can see exactly what's happening, and immediately spot the differences between them. If one was incorrectly different, you'd be able to spot that at a glance. It's caught innumerable bugs for me.
H = H / 6.0;
Use compound-assignment operators when possible to simplify expressions and reduce duplication:
H /= 6.0;
float SLClamp(float value, float given_s)
{
float return_value = 0.0;
if (value <= 0.0) { return_value = given_s + 0.1; }
else if (value >= 1.0) { return_value = 1.0; }
return return_value;
}
Too much whitespace here: what are all these blank lines for? Tighten this up, and be consistent.
float min = std::fmin(std::fmin(color.R, color.G), color.B);
float max = std::fmax(std::fmax(color.R, color.G), color.B);
Also, this is C++, so don't use the C-style f-prefixed floating-point functions. They are necessary in C because that language doesn't support overloading. C++ does. It also supports templates for automatic type deduction, and so that's how the std::min and std::max functions are implemented. Lose the prefixes; retain the type-safety.
float H = 0, S = 0, L = ((min + max) / 2.0);
Prefer not initializing multiple variables on a single line. Unless you're getting paid a bonus for writing the fewest lines of code, there is never a reason to write the above over:
float H = 0;
float S = 0;
float L = ((min + max) / 2.0);
color_HSL HueShift(color_HSL color, float delta)
{
return { color.H + delta, color.S, color.L };
}
You should almost certainly be implementing wrap-around logic here, to make sure that the hue doesn't get out of bounds. If you don't want to wrap around, at least clamp the hue to be within range. Never return an object that represents an invalid value of that type.
General
It looks more like you've translated the code from Lua into C. In C++, you can create objects that represent colors in a particular color space. Those objects can not only hold their own data members, but can also know how to create and convert themselves.
Why are you working with floats here? Single-precision floating-point values are almost never faster than double-precision floating-point values, so there's no advantage in paying the price for their reduced precision. This is especially a concern when you're doing a series of arithmetic manipulations. Prefer to use double by default in C or C++, unless you're targeting a specific embedded microcontroller where you know that single-precision floating-point operations are faster, or you are more concerned about packing values into memory in as efficient a representation as possible (and even in that latter case, do your calculations as doubles and only convert to float at the end for the storage representation).
If you really couldn't decide which type to use, you could typedef it, or even template it. (Although the templates would make your code's implementation uglier and more difficult to write. It is probably not useful in this case, because I doubt you'll want to represent color values in a variety of ways.)
Your code completely lacks explanatory comments. Most of it is nicely self-documenting, since you've chosen the symbol names well (except for m1 and m2—I still don't know what those refer to). However, the algorithms that actually do the conversion are not self-explanatory, and should have some explanation of why you're doing what you're doing, especially in functions like Hue2RGB.
Reworked Version
struct HSLColor
{
static const double MinH = 0.0;
static const double MaxH = 360.0;
static const double MinS = 0.0;
static const double MaxS = 1.0;
static const double MinL = 0.0;
static const double MaxL = 1.0;
double H;
double S;
double L;
HSLColor(double h, double s, double l);
HSLColor(const HSLColor& hsl) = default;
HSLColor(const RGBColor& rgb);
void HueShift(double delta);
void MakeComplementary()
// ...etc.
};
struct RGBColor
{
static const double MinR = 0.0;
static const double MaxR = 1.0;
static const double MinG = 0.0;
static const double MaxG = 1.0;
static const double MinB = 0.0;
static const double MaxB = 1.0;
static const double MinA = 0.0;
static const double MaxA = 1.0;
double R;
double G;
double B;
double A;
RGBColor(double r, double g, double b, double a = MaxA);
RGBColor(const RGBColor& rgb) = default;
RGBColor(const HSLColor& hsl);
};
/////////////
// HSLColor
/////////////
HSLColor::HSLColor(double h, double s, double l);
: H(h) // NOTE: Could also consider clamping and/or
, S(s) // range-checking these values
, L(l) // here in the constructor.
{ }
HSLColor::HSLColor(const RGBColor& rgb)
{
const auto min = std::min(std::min(rgb.R, rgb.G), rgb.B);
const auto max = std::max(std::max(rgb.R, rgb.G), rgb.B);
const auto delta = (max - min);
this->L = ((min + max) / 2.0);
if ((this->L > 0.0) && (this->L < 0.5))
{
this->S = (delta / (min + max));
}
else if ((this->L > 0.5) && (this->L < 1.0))
{
this->S = (delta / (2.0 - max - min));
}
else
{
this->S = 0.0;
}
if (delta > 0.0)
{
if ((max == rgb.R) && (max != rgb.G)) { this->H += (0 + (rgb.G - rgb.B) / delta); }
if ((max == rgb.G) && (max != rgb.B)) { this->H += (2 + (rgb.B - rgb.R) / delta); }
if ((max == rgb.R) && (max != rgb.R)) { this->H += (4 + (rgb.R - rgb.G) / delta); }
this->H /= 6.0;
this->H *= HSLColor::MaxH;
this->H = std::clamp(this->H, HSLColor::MinH, HSLColor::MaxH);
}
else
{
this->H = 0.0;
}
}
void HSLColor::HueShift(double delta)
{
// TODO: Decide whether this should implement wrap-around semantics for hue,
// or whether clamping to be within the allowed range is appropriate.
this->H = std::clamp((this->H + delta), HSLColor::MinH, HSLColor::MaxH);
}
void HSLColor::MakeComplementary()
{
this->HueShift(180.0);
}
// ...etc.
/////////////
// RGBColor
/////////////
namespace {
double HueToRGB(double m1, double m2, double hue)
{
hue = std::clamp(hue, HSLColor::MinH, HSLColor::MaxH);
if ((hue * 6.0) < 1.0)
{
return (m1 + (m2 - m1) * hue * 6.0);
}
else if ((hue * 2.0) < 1.0)
{
return m2;
}
else if ((hue * 3.0) < 2.0)
{
return (m1 + (m2 - m1) * ((2.0 / 3.0) - hue) * 6.0);
}
else
{
return m1;
}
}
}
RGBColor::RGBColor(double r, double g, double b, double a /* = MaxA */)
: R(r) // NOTE: Could also consider clamping and/or
, G(g) // range-checking these values
, B(b) // here in the constructor.
, A(a)
{ }
RGBColor::RGBColor(const HSLColor& hsl)
{
const auto hue = (hsl.H / 360.0);
const auto m2 = (hsl.L <= 0.5) ? (color.L * (color.S + 1.0))
: (color.L + color.S - (color.L * color.S));
const auto m1 = ((hsl.L * 2.0) - m2);;
this->R = HueToRGB(m1, m2, hsl.H + (1.0 / 3.0));
this->G = HueToRGB(m1, m2, hsl.H);
this->B = HueToRGB(m1, m2, hsl.H - (1.0 / 3.0));
this->A = 1.0;
}
I haven't implemented all the mutating member functions here that you have in your code, but I think you can get the idea on how to do that.
If you want to implement ToXxx functions that return a new, mutated object (rather than modifying the current one), you can easily expand this to do so.
You could also add "named constructors" (static FromXxx functions) that allow you to create an object of a particular type. The standard C++ constructors work well for creating an RGBColor by converting an HSLColor, for example, but they don't work as well for creating an HSLColor that is hue-shifted. This is where named constructors are useful.
There are certainly ways of optimizing the performance of this code, as there always is for branchy arithmetic code. But hold off on that until your performance profiler tells you that there is an actual bottleneck. Compilers are surprisingly good at optimizing this stuff nowadays anyway, so unless you have a really advanced understanding of machine-level implementation details, you are unlikely to be able to beat it.
