Limit values of struct member

Question

I want to create a simple struct that stores the RGB-values of a color. r, g and b are supposed to be double numbers in [0,1].

struct Color 
{
  Color(double x): r{x}, g{x}, b{x} {
    if (r < 0.0) r = 0.0;
    if (r > 1.0) r = 1.0;
    if (g < 0.0) g = 0.0;
    if (g > 1.0) g = 1.0;
    if (b < 0.0) b = 0.0;
    if (b > 1.0) b = 1.0;
  }
}

Is there a better way than using those if statements?

Probable duplicate of http://stackoverflow.com/q/427477/560648 — Lightness Races in Orbit, May 03 '15 at 16:11
Almost identical to http://stackoverflow.com/q/29037854/560648 which is a dupe of the question I previously mentioned — Lightness Races in Orbit, May 03 '15 at 16:12
The only improvement for those statements (though Barry answered with a better solution,) is to turn every second `if` into an `else if` — Evan Carslake, May 03 '15 at 16:12
Eh, I was just meaning that you could technically do it like that. Turning those 6 if statements on 6 lines, into 3 if statements and 3 else if statements (and formatted to 3 lines.) Hmm now seeing, it would only save you one less call, 6 to 5 so yeah pointless. — Evan Carslake, May 03 '15 at 16:18

score 8 · Answer 1 · answered May 03 '15 at 16:09

Just write a function to clamp:

double clamp(double val, double left = 0.0, double right = 1.0) {
    return std::min(std::max(val, left), right);
}

And use that in your constructor:

Color(double x)
    : r{clamp(x)}
    , g{clamp(x)}
    , b{clamp(x)} 
{ }

score 5 · Answer 2 · answered May 03 '15 at 16:09

You, can can use min and max, ideally combining them into a clamp function:

template <class T>
T clamp(T val, T min, T max)
{
  return std::min(max, std::max(min, val));
}

struct Color
{
  Color(double x) : r{clamp(x, 0., 1.)}, g{clamp(x, 0., 1.)}, b{clamp(x, 0., 1.)}
  {}
};

score 2 · Answer 3 · 2015-05-03T17:04:24.253

For a first pass iteration, we have min/max functions we can and should use:

struct Color 
{
    explicit Color(double x): r{x}, g{x}, b{x} 
    {
        r = std::max(r, 0.0);
        r = std::min(r, 1.0);

        g = std::max(g, 0.0);
        g = std::min(g, 1.0);

        b = std::max(b, 0.0);
        b = std::min(b, 1.0);
    }

    double r, g, b;
};

I'd also suggest making that constructor explicit, as it's rather confusing for a scalar to implicitly convert to a Color.

The reason this is arguably an upgrade even with roughly the same amount of code and arguably not the biggest improvement in readability is because, while optimizing compilers might emit faster branchless code here, min and max can often guarantee an efficient implementation. You're also expressing what you're doing in a slightly more direct way.

There is some truth to this somewhat counter-intuitive idea that writing higher level code helps you achieve efficiency, if only for the reason that the low-level logic used to implement the high-level function is more likely to be efficient than what people would repeatedly write otherwise in their more casual, daily kind of code. It also helps direct your codebase towards more central targets for optimization.

As a second pass, this may not improve things for your particular use cases, but in general I've found it's useful to represent color and vector components using an array to allow you to access them with loops. This is because if you start doing somewhat complex things with colors like blending them together, the logic for each color component is non-trivial but identical for all components, so you don't want to end up writing such code three times all the time or always be forced into writing the per-component logic in a separate function or anything like that.

So we might do this:

class Color 
{
public:
    explicit Color(double x)
    {
        for (int j=0; j < 3; ++j)
        {
            rgb[j] = x;
            rgb[j] = std::max(rgb[j], 0.0);
            rgb[j] = std::min(rgb[j], 1.0);
        }
    }

    // Bounds-checking assertions in these would also be a nice idea.
    double& operator[](int n) {return rgb[n]};
    double operator[](int n) const {return rgb[n]};

    double& red() {return rgb[0];}
    double red() const {return rgb[0];}

    double& green() {return rgb[1];}
    double green() const {return rgb[1];}

    double& blue() {return rgb[2];}
    double blue() const {return rgb[2];}

    // Somewhat excess fluff, but such methods can be useful when
    // interacting with a low-level C-style API (OpenGL, e.g.) as 
    // opposed to using &color.red() or &color[0].
    double* data() {return rgb;}
    const double* data() const {return rgb;}

private:
    double rgb[3];
};

Finally, as others have mentioned, this is where a function to clamp values to a range is useful, so as a final pass:

template <class T>
T clamp(T val, T low, T high)
{
    assert(low <= high);
    return std::max(std::min(val, high), low);
}

// New constructor using clamp:
explicit Color(double x)
{
    for (int j=0; j < 3; ++j)
        rgb[j] = clamp(x, 0.0, 1.0);
}

Limit values of struct member

3 Answers3