Image pixel-editing class with an enum for colors

Question

I'm creating an image class that can be used by other classes to create visualizations of data. The class doesn't need to be complicated as the classes will be editing the images pixel by pixel.

The image class will be used to create RGB images and export them to any of the netbpm image formats (pbm, pgm, ppm). I may expand the class to export in other formats and maybe load images, but for the moment, I just need the basic functionality of editing and exporting.

I'm mostly looking for feedback on the use of enum values.

---

Image.h

#ifndef IMAGE_H
#define IMAGE_H

#include <string>
#include <vector>

#include <fstream>

#include <cctype>
#include <cmath>

using namespace std;


class Image {
public:
    enum Enum_Colors {WHITE, BLACK, GREY, RED, GREEN, BLUE, YELLOW, CYAN, MAGENTA};

private:
    const unsigned int NUM_PIXEL_VALUES = 3;
    const unsigned int FILE_EXTENSION_LENGTH = 4;

    struct Pixel {
        unsigned int red;
        unsigned int green;
        unsigned int blue;
    };

    unsigned int maxValue;
    vector<vector<Pixel>> imageData;

    /**
     * Creates a pixel representing the given color.
     *
     * @param color an enumerated color
     * @return a pixel representing the given color
    **/
    Pixel ColorPixel(Enum_Colors color) const;

    /**
     * Creates an image file from the image data.
     * 
     * The type of the created file is determined by the function name.
     *
     * @param fileName the name of the created file
     * @return false if an error occurred
     * @return true otherwise
    **/
    bool ExportPBM(string fileName);
    bool ExportPGM(string fileName);
    bool ExportPPM(string fileName);

public:
    Image(unsigned int width, unsigned int height, unsigned int maxValue) : maxValue(maxValue),
        imageData(vector<vector<Pixel>>(height, vector<Pixel>(width, ColorPixel(WHITE)))) {};
    ~Image() {imageData.clear();}

    /**
     * Returns the pixel width of the image.
     *
     * @return the pixel width of the image
    **/
    unsigned int Width();

    /**
     * Returns the pixel height of the image.
     *
     * @return the pixel height of the image
    **/
    unsigned int Height();

    /**
     * Returns the maximum intensity value of the image.
     *
     * @return the maximum intensity value of the image
    **/
    unsigned int MaxValue();

    /**
     * Sets the given pixel value to the given color.
     *
     * @param x the location along the width
     * @param y the location along the height
     * @param color the color to set the pixel to
     * 
     * @return false if the indices are out of bounds
     * @return true otherwise
    **/
    bool SetPixelColor(unsigned int x, unsigned int y, Enum_Colors color);

    /**
     * Sets the given pixel value to the given color.
     *
     * @param x the location along the width
     * @param y the location along the height
     * @param redVal the intensity of the red value to set the pixel to
     * @param greenVal the intensity of the green value to set the pixel to
     * @param blueVal the intensity of the blue value to set the pixel to
     * 
     * @return false if the indices are out of bounds or the color values
        are out of bounds
     * @return true otherwise
    **/
    bool SetPixelValue(unsigned int x, unsigned int y, unsigned int redValue, unsigned int greenValue, unsigned int blueValue);

    /**
     * Creates an image file from the image data.
     * 
     * The type of the created file is determined by the file name.
     * (Supported extensions: .pbm, .pgm, .ppm)
     *
     * @param fileName the name of the created file
     * @return false if the extension is not supported or an error occurred
     * @return true otherwise
    **/
    bool ExportImage(string fileName);

};


#endif

Image.cpp

#include "Image.h"


Image::Pixel Image::ColorPixel(Enum_Colors color) const {
    Pixel pixel;
    switch (color) {
    case WHITE:
        pixel.red = maxValue;
        pixel.green = maxValue;
        pixel.blue = maxValue;
        break;
    case BLACK:
        pixel.red = 0;
        pixel.green = 0;
        pixel.blue = 0;
        break;
    case GREY:
        pixel.red = maxValue / 2;
        pixel.green = maxValue / 2;
        pixel.blue = maxValue / 2;
        break;
    case RED:
        pixel.red = maxValue;
        pixel.green = 0;
        pixel.blue = 0;
        break;
    case GREEN:
        pixel.red = 0;
        pixel.green = maxValue;
        pixel.blue = 0;
        break;
    case BLUE:
        pixel.red = 0;
        pixel.green = 0;
        pixel.blue = maxValue;
        break;
    case YELLOW:
        pixel.red = maxValue;
        pixel.green = maxValue;
        pixel.blue = 0;
        break;
    case CYAN:
        pixel.red = 0;
        pixel.green = maxValue;
        pixel.blue = maxValue;
        break;
    case MAGENTA:
        pixel.red = maxValue;
        pixel.green = 0;
        pixel.blue = maxValue;
        break;
    default:
        pixel.red = maxValue;
        pixel.green = maxValue;
        pixel.blue = maxValue;
        break;
    }

    return pixel;
}


bool Image::ExportPBM(string fileName) {
    ofstream fout;

    /* Attempt to create the file and check for errors */
    fout.open(fileName);
    if (fout.fail()) {
        return false;
    }

    /* Write heading */
    fout << "P1" << endl;   // file type identifier
    fout << "# CREATOR: C++ Image Class, by Jacob Bischoff" << endl;
    fout << imageData.at(0).size() << " " << imageData.size() << endl;  // width height

    /* Write body */
    for (unsigned int i = 0; i < imageData.size(); i++) {
        /* Add end line in proper locations */
        if (i > 0) {
            fout << endl;
        }

        /* Output pixel values; 0 - White, 1 - Black */
        for (unsigned int j = 0; j < imageData.at(i).size(); j++) {
            Pixel pixel = imageData.at(i).at(j);
            if (j > 0) {
                fout << " ";
            }
            fout << 1 - (int)roundf((pixel.red + pixel.green + pixel.blue) / (double)NUM_PIXEL_VALUES / (double)maxValue);
        }
    }

    return true;
}


bool Image::ExportPGM(string fileName) {
    ofstream fout;

    /* Attempt to create the file and check for errors */
    fout.open(fileName);
    if (fout.fail()) {
        return false;
    }

    /* Write heading */
    fout << "P2" << endl;   // file type identifier
    fout << "# CREATOR: C++ Image Class, by Jacob Bischoff" << endl;
    fout << imageData.at(0).size() << " " << imageData.size() << endl;  // width height
    fout << maxValue << endl;

    /* Write body */
    for (unsigned int i = 0; i < imageData.size(); i++) {
        /* Add end line in proper locations */
        if (i > 0) {
            fout << endl;
        }

        /* Output pixel values; 0 - black, maxValue - white */
        for (unsigned int j = 0; j < imageData.at(i).size(); j++) {
            Pixel pixel = imageData.at(i).at(j);
            if (j > 0) {
                fout << " ";
            }
            fout << (int)roundf((pixel.red + pixel.green + pixel.blue) / (double)NUM_PIXEL_VALUES);
        }
    }

    return true;
}


bool Image::ExportPPM(string fileName) {
    ofstream fout;

    /* Attempt to create the file and check for errors */
    fout.open(fileName);
    if (fout.fail()) {
        return false;
    }

    /* Write heading */
    fout << "P3" << endl;   // file type identifier
    fout << "# CREATOR: C++ Image Class, by Jacob Bischoff" << endl;
    fout << imageData.at(0).size() << " " << imageData.size() << endl;  // width height
    fout << maxValue << endl;

    /* Write body */
    for (unsigned int i = 0; i < imageData.size(); i++) {
        /* Add end line in proper locations */
        if (i > 0) {
            fout << endl;
        }

        /* Output pixel values; 0 0 0 - black, maxValue maxValue maxValue - white */
        for (unsigned int j = 0; j < imageData.at(i).size(); j++) {
            Pixel pixel = imageData.at(i).at(j);
            if (j > 0) {
                fout << "\t";
            }
            fout << pixel.red << " " << pixel.green << " " << pixel.blue;
        }
    }

    return true;
}


unsigned int Image::Width()  {
    /* prevent out of bounds error */
    if (imageData.size() == 0) {
        /* no second order elements */
        return 0;
    }
    else {
        /* return the second order size */
        return imageData.at(0).size();
    }
}


unsigned int Image::Height() {
    /* return the first order size */
    return imageData.size();
}


unsigned int Image::MaxValue() {
    /* return the maximum intensity value */
    return maxValue;
}


bool Image::SetPixelColor(unsigned int x, unsigned int y, Enum_Colors color) {
    /* check if indices are withing bounds */
    if ((y < imageData.size()) && (x < imageData.at(y).size())) {
        imageData.at(y).at(x) = ColorPixel(color);
        return true;
    }
    else {
        return false;
    }
}


bool Image::SetPixelValue(unsigned int x, unsigned int y, unsigned int redValue, unsigned int greenValue, unsigned int blueValue) {
    /* check if indices are withing bounds */
    if ((y < imageData.size()) && (x < imageData.at(y).size())) {
        /* check if intensity values are larger than max */
        if ((redValue <= maxValue) || (greenValue <= maxValue) || (blueValue <= maxValue)) {
            imageData.at(y).at(x).red = redValue;
            imageData.at(y).at(x).green = greenValue;
            imageData.at(y).at(x).blue = blueValue;
            return true;
        }
        else {return false;}
    }
    else {return false;}
}


bool Image::ExportImage(string fileName) {
    string fileExtension = fileName.substr(fileName.size() - FILE_EXTENSION_LENGTH, FILE_EXTENSION_LENGTH);

    /* Make extension all lowercase for ease of comparison */
    for (unsigned int i = 0; i < fileExtension.size(); ++i) {
        fileExtension.at(i) = tolower(fileExtension.at(i));
    }

    /* Evaluate extension to determine which file type to create. */
    if (fileExtension == ".pbm") {
        return ExportPBM(fileName);
    }
    else if (fileExtension == ".pgm") {
        return ExportPGM(fileName);
    }
    else if (fileExtension == ".ppm") {
        return ExportPPM(fileName);
    }
    else {
        return false;
    }
}

Answer 1

Code:

• Don't use using namespace std;.

• It looks like you're including more dependencies in the header than necessary (cmath, fstream, cctype). These are only used in the .cpp file, so they should be included there.

• Use enum class for type safety instead of a plain enum.

• We don't need to call imageData.clear() in the destructor, or even define a destructor. (The compiler generated destructor will call the std::vector destructor automatically).

• It's easier to store width and height directly in the image class, instead of getting them from the vectors.

• It's more efficient to store data in a one-dimensional vector of width * height elements, as the allocated memory is all in one place, and we aren't storing a size for each individual row vector. The index in this vector can be calculated as x + width * y.

• The file formats seem to indicate a max component value of 65535. This can be represented by a std::uint16_t (i.e. short unsigned int), so it might be better to use that for the pixel component type and for maxValue.

• ColorPixel() can use braced init-list to return values, which is rather more concise:

  Image::Pixel Image::ColorPixel(Enum_Colors color) const {
      switch (color) {
      case Enum_Colors::WHITE: return{ maxValue, maxValue, maxValue };
      case Enum_Colors::BLACK: return{ 0, 0, 0 };
      // ...
      }
  }
  

• ColorPixel() should perhaps assert or throw an exception if the enum value is invalid, rather than returning white.

---

Design:

I'd suggest a slightly different architecture.

Currently this class is doing 3 separate things:

• Storing image data.
• Converting image data from one format to another (for export to each file format).
• Exporting image data.

It would be cleaner to split this up accordingly. Something like the following:

#include <array>
#include <cstdint>
#include <string>
#include <vector>

// IMAGE STORAGE:
template<class T, std::size_t Size>
struct Pixel
{
    std::array<T, Size> Data;
};

using Pixel_BW = Pixel<bool, 1u>;
using Pixel_Gray8 = Pixel<std::uint8_t, 1u>;
using Pixel_RGB8 = Pixel<std::uint8_t, 3u>;

template<class PixelT>
class Image
{
public:

    Image(std::size_t width, std::size_t height, PixelT initialValue):
        m_width(width),
        m_height(height),
        m_data(width * height)
    {

    }

    PixelT const& At(std::size_t x, std::size_t y) const
    {
        return m_data.at(GetIndex(x, y));
    }

    PixelT& At(std::size_t x, std::size_t y)
    {
        return m_data.at(GetIndex(x, y));
    }

    // ...

private:

    std::size_t GetIndex(std::size_t x, std::size_t y) const
    {
        return x + m_width * y;
    }

    std::size_t m_width;
    std::size_t m_height;
    std::vector<PixelT> m_data;
};

// CONVERT:
Image<Pixel_BW> ConvertRGBToBW(Image<Pixel_RGB8> const& image)
{
    auto result = Image<Pixel_BW>(image.GetWidth(), image.GetHeight(), false);

    // ... convert
    // ... for each (x, y): result.At(x, y) = ConvertRGBPixelToBWPixel(image.At(x, y));

    return result;
}

// EXPORT:
void ExportPBM(std::string const& fileName, Image<Pixel_BW> const& data)
{
    // ... export
}

void ExportPGM(std::string const& fileName, Image<Pixel_Gray> const& data)
{
    // ... export
}

void ExportPPM(std::string const& fileName, Image<Pixel_RGB> const& data)
{
    // ... export
}

Making Pixel an externally visible class and templating Image on it is much more flexible, and allows us to create whatever image types we need. We can access elements of the array by index, or we could use inheritance instead of typedefs to get named accessors for each component, e.g.:

struct Pixel_RGB8 : Pixel<std::uint8_t, 3u>
{
    std::uint8_t const& R() const
    {
        return Data[0];
    }

    std::uint8_t& R()
    {
        return Data[0];
    }

    // ... G(), B()
};

This would also provide type safety for storing images with the same component types, but different meanings (e.g. RGB vs YUV).

Since there are lots of different ways to convert an image from RGB to Grayscale (e.g. specifying different multipliers for each color component) it's more flexible to separate the conversion from the image export. Using separate functions like this does create an extra copy of the image data, but that probably isn't a concern unless working with very large images. (And we could solve that issue by creating an ImageView class that performs the conversion on accessing a pixel).

---

This does make creating colors a little more difficult, since something like "magenta" doesn't really make sense for a black and white image. The simplest thing to do is probably to define the constants that do make sense for each pixel type:

namespace Colors
{

    constexpr auto Black_BW = Pixel_BW{ false };
    constexpr auto White_BW = Pixel_BW{ true };

    constexpr auto Black_RGB8 = Pixel_RGB8{ 0, 0, 0 };
    constexpr auto White_RGB8 = Pixel_RGB8{ 255, 255, 255 };
    // ...

    constexpr auto Black_RGB32F = Pixel_RGB32F{ 0.f, 0.f, 0.f };
    constexpr auto White_RGB32F = Pixel_RGB32F{ 1.f, 1.f, 1.f };
    // ...

} // Colors

We could perhaps use templates or multiplication to reduce duplication between different RGB pixel types.

---