10
\$\begingroup\$

As an exercise to learn how OpenGL and image creation worked, as well as to satisfy a curiosity I've developed for Chaos Theory, I decided to create a mandelbrot fractal drawer in C++, which can either draw to an OpenGL context or to a PNG image. I am a beginner at C++, and would much appreciate any constructive feedback. Also, please ridicule me on how I create my images, and how I can make them look better as well.

main.cpp

#include <complex>
#include <iostream>
#include <memory>
#include "Window.h"
#include "Draw_Buffer.h"
#include "Image_Buffer.h"
#include "Buffer_Base.h"

static constexpr float COMPLEX_INCREMENT = 0.005f;

template <typename T>
int iterations_till_escape(const std::complex<T> &c, int max_iterations) {
    std::complex<T> z(0, 0);
    for (int iter = 0; iter < max_iterations; ++iter) {
        z = (z * z) + c;
        if (std::abs(z) > 2) {
            return iter;
        }
    }
    return -1;
}

template <typename T>
RGB calculate_pixel(const std::complex<T> &c) {
    int iterations = iterations_till_escape(c, 255);

    if (iterations == -1) {
        return RGB{0, 0, 0};
    }

    else {
        GLubyte blue = iterations * 5;
        return RGB{0, 0, blue};
    }
}

int main() {
    // Declare window object to represent the complex plane
    Window<float> complex_plane(-2.2, 1.2, -1.7, 1.7);

    // Declare window object to represent the OpenGL window
    Window<int> window(0, ((std::abs(complex_plane.get_x_min()) + complex_plane.get_x_max()) / COMPLEX_INCREMENT),
                       0, ((std::abs(complex_plane.get_y_min()) + complex_plane.get_y_max()) / COMPLEX_INCREMENT));

    std::unique_ptr<Buffer_Base<RGB>> pixel_buffer;


    std::cout << "Running mandelbrot-fractal-drawer...\nWould you like to draw fractal to a window or an image?\n"
              << "Type W for window or I for image" << std::endl;

    char response;
    while (!(std::cin >> response))
        ;
    if (response == 'W' || response == 'w') {
        // Initialise pointer to a draw buffer
        pixel_buffer.reset(new Draw_Buffer(&window, "vertex_shader.glsl", "fragment_shader.glsl"));
    }

    else if (response == 'I' || response == 'i') {
        std::cout << "\nPlease enter the location to where you want the fractal to be drawn" << std::endl;

        std::string src;
        while (!(std::cin >> src))
            ;

        // Initialise pointer to an image buffer
        pixel_buffer.reset(new Image_Buffer(&window, src));
    }

    std::complex<float> pixel_iterator(complex_plane.get_x_min(), complex_plane.get_y_max());
    while (pixel_iterator.imag() > complex_plane.get_y_min()) {
        while (pixel_iterator.real() < complex_plane.get_x_max()) {

            // Calculate the colour of the pixel using the mandelbrot function
            *pixel_buffer << calculate_pixel(pixel_iterator);

            // Increment
            pixel_iterator.real(pixel_iterator.real() + COMPLEX_INCREMENT);
        }

        // Increment
        pixel_iterator.imag(pixel_iterator.imag() - (COMPLEX_INCREMENT));

        // Reset real iterator
        pixel_iterator.real(complex_plane.get_x_min());
    }

    pixel_buffer->flush();


    std::cout << "Closing down..." << std::endl;

}

Buffer_Base.h

#ifndef MANDELBROT_FRACTAL_DRAWER_BUFFER_BASE_H
#define MANDELBROT_FRACTAL_DRAWER_BUFFER_BASE_H

#include <vector>
#include <memory>

#include "Window.h"

template <typename T>
class Buffer_Base {
protected:
    // The buffer itself
    std::vector<T> buffer;

    // Iterator to where in the buffer the appending is happening
    typename std::vector<T>::iterator pos_iter;

    // Represents the size of the window to which the buffer is writing
    std::unique_ptr<Window<int>> window;
public:
    Buffer_Base(Window<int> *win) :
            buffer(win->size()), window(win) { pos_iter = buffer.begin(); }
    virtual ~Buffer_Base() { };
    virtual void flush() = 0;

    Buffer_Base<T> &operator<<(T &&val) {
        if (pos_iter != buffer.end()) {
            *(pos_iter) = std::move(val);
            ++pos_iter;
        }
        return *this;
    }
};

#endif //MANDELBROT_FRACTAL_DRAWER_BUFFER_BASE_H

RGB.h

#ifndef MANDELBROT_FRACTAL_DRAWER_RGB_H
#define MANDELBROT_FRACTAL_DRAWER_RGB_H


struct RGB {
    unsigned char r;
    unsigned char g;
    unsigned char b;
};

#endif //MANDELBROT_FRACTAL_DRAWER_RGB_H

Get_GL.h

#ifndef MANDELBROT_FRACTAL_DRAWER_GET_GL_H
#define MANDELBROT_FRACTAL_DRAWER_GET_GL_H

#ifndef __APPLE__
#include <GL/gl.h>
#else
#include <OpenGL/gl.h>
#endif

#endif //MANDELBROT_FRACTAL_DRAWER_GET_GL_H

Window.h

#ifndef MANDELBROT_FRACTAL_DRAWER_WINDOW_H
#define MANDELBROT_FRACTAL_DRAWER_WINDOW_H

#include <complex>

template<typename T>
class Window {
    T _x_min, _x_max, _y_min, _y_max;
public:
    Window(T x_min, T x_max, T y_min, T y_max) : _x_min(x_min), _x_max(x_max), _y_min(y_min), _y_max(y_max) { }

// Util functions
    T width() const {
        return (_x_max - _x_min);
    }

    T height() const {
        return (_y_max - _y_min);
    }

    T size() const {
        return (height() * width());
    }

// Setters and getters
    T get_y_min() const {
        return _y_min;
    }

    T get_y_max() const {
        return _y_max;
    }

    T get_x_min() const {
        return _x_min;
    }

    T get_x_max() const {
        return _x_max;
    }

    void set_y_min(T _y_min) {
        Window::_y_min = _y_min;
    }

    void set_y_max(T _y_max) {
        Window::_y_max = _y_max;
    }

    void set_x_min(T _x_min) {
        Window::_x_min = _x_min;
    }

    void set_x_max(T _x_max) {
        Window::_x_max = _x_max;
    }

// Reset values
    void reset(T x_min, T x_max, T y_min, T y_max) {
        _y_min(y_min);
        _y_max(y_max);
        _x_min(x_min);
        _x_max(x_max);
    }
};

Image_Buffer.h

#ifndef MANDELBROT_FRACTAL_DRAWER_IMAGE_BUFFER_H
#define MANDELBROT_FRACTAL_DRAWER_IMAGE_BUFFER_H

#include <string>
#include "Buffer_Base.h"
#include "RGB.h"
#include <png.h>

#define PNG_DEBUG 3

class Image_Buffer : public Buffer_Base<RGB> {
    // Location to write image to
    std::string file_src;

    // PNG data
    png_structp png_ptr;
    png_infop info_ptr;
    png_bytep row;


    // File pointer
    FILE *fp;
public:
    Image_Buffer(Window<int> *, const std::string &);

    ~Image_Buffer();

    virtual void flush() override;
};


#endif //MANDELBROT_FRACTAL_DRAWER_IMAGE_BUFFER_H

Image_Buffer.cpp

#include "Image_Buffer.h"
#include <png.h>
#include <fstream>
#include <stdexcept>
#include <string>
#include <sstream>
#include <vector>
#include <algorithm>

Image_Buffer::Image_Buffer(Window<int> *win, const std::string &src) : Buffer_Base(win), file_src(src) { }

void Image_Buffer::flush() {
    fp = fopen(file_src.c_str(), "wb");
    if (!fp) {
        std::ostringstream ss;
        ss << "error: Unable to open file " << file_src << " for writing";
        throw std::runtime_error(ss.str());
    }

    png_ptr = png_create_write_struct(PNG_LIBPNG_VER_STRING, NULL, NULL, NULL);

    if (!png_ptr) {
        throw std::runtime_error("error: png_create_write_struct failed");
    }

    info_ptr = png_create_info_struct(png_ptr);
    if (!info_ptr) {
        throw std::runtime_error("error: png_create_info_struct failed");
    }

    if (setjmp(png_jmpbuf(png_ptr))) {
        throw std::runtime_error("Error during init_io");
    }

    png_init_io(png_ptr, fp);

    // Write header (8 bit colour depth)
    png_set_IHDR(png_ptr, info_ptr, window->width(), window->height(),
                 8, PNG_COLOR_TYPE_RGB, PNG_INTERLACE_NONE,
                 PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE);

    png_text title_text;
    title_text.compression = PNG_TEXT_COMPRESSION_NONE;
    title_text.key = "Title";
    title_text.text = (char *)file_src.c_str();
    png_set_text(png_ptr, info_ptr, &title_text, 1);

    png_write_info(png_ptr, info_ptr);

    std::vector<RGB> row(3 * window->width());
    auto first = buffer.begin();
    auto last = buffer.begin() + window->width();

    while (first != buffer.end()) {
        std::copy(first, last, row.begin());
        png_write_row(png_ptr, (png_bytep)&row[0]);
        first = last;
        last += window->width();
    }



    png_write_end(png_ptr, NULL);

    png_init_io(png_ptr, fp);
}

Image_Buffer::~Image_Buffer() {
    if (fp) fclose(fp);
    if (info_ptr) png_free_data(png_ptr, info_ptr, PNG_FREE_ALL, -1);
    if (png_ptr) png_destroy_write_struct(&png_ptr, static_cast<png_infopp>(NULL));
}

Draw_Buffer.h

#ifndef MANDELBROT_FRACTAL_DRAWER_DRAW_BUFFER_H
#define MANDELBROT_FRACTAL_DRAWER_DRAW_BUFFER_H

#define GLEW_STATIC

#include <GL/glew.h>
#include <GLFW/glfw3.h>
#include "Get_GL.h"
#include "Buffer_Base.h"
#include "RGB.h"

class Draw_Buffer : public Buffer_Base<RGB> {
    // Pointer to glfw screen
    GLFWwindow *screen;

    // Texture where pixels are written to
    GLuint mandelbrot_tex;

    // GLSL Shader program
    GLuint shader_prog;

    // Vertex shader
    GLuint vertex_shader;

    // Fragment shader
    GLuint frag_shader;

    // VAO
    GLuint vao;

    // Element buffer object
    GLuint ebo;

    // Vertex buffer object
    GLuint vbo;

    // Util function to compile shader
    static void compile_shader(GLuint &shader, const std::string &src);
public:
    Draw_Buffer(Window<int> *, const std::string &, const std::string &);
    virtual ~Draw_Buffer() override;

    void make_current() {
        glfwMakeContextCurrent(screen);
    }

    virtual void flush() override;
};


#endif //MANDELBROT_FRACTAL_DRAWER_DRAW_BUFFER_H

Draw_Buffer.cpp

#include "Draw_Buffer.h"
#include "Buffer_Base.h"
#include <memory>
#include <algorithm>
#include <stdexcept>
#include <vector>
#include <sstream>
#include <fstream>
#include <string>
#include <iostream>

// Util function to compile a shader from source
void Draw_Buffer::compile_shader(GLuint &shader, const std::string &src) {
    std::ifstream is(src);
    std::string code;

    std::string temp_str;
    while (std::getline(is, temp_str)) {
        code += temp_str + '\n';
    }

    const char *c_code = code.c_str();
    glShaderSource(shader, 1, &c_code, NULL);
    glCompileShader(shader);
}

Draw_Buffer::Draw_Buffer(Window<int> *win, const std::string &vertex_shader_src, const std::string &frag_shader_src) :
        Buffer_Base(win) {
// Initialise GLFW
    if (!glfwInit()) {
        throw std::runtime_error("error: GLFW unable to initialise");
    }

// Set up the window
    glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 3);
    glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 2);
    glfwWindowHint(GLFW_OPENGL_PROFILE, GLFW_OPENGL_CORE_PROFILE);
    glfwWindowHint(GLFW_OPENGL_FORWARD_COMPAT, GL_TRUE);

    glfwWindowHint(GLFW_RESIZABLE, GL_FALSE);

    screen = (glfwCreateWindow(win->width(), win->height(), "Mandelbrot Fractal", nullptr, nullptr));

    make_current();

// Initialise glew
    glewExperimental = GL_TRUE;
    GLenum glewinit = glewInit();

    if (glewinit != GLEW_OK) {
        std::ostringstream ss;
        ss << "error: Glew unable to initialise" << glewinit;
        throw std::runtime_error(ss.str());
    }

// Clear
    glClearColor(0, 0, 0, 0);
    glClear(GL_COLOR_BUFFER_BIT);

// Generate shaders
    vertex_shader = glCreateShader(GL_VERTEX_SHADER);
    frag_shader = glCreateShader(GL_FRAGMENT_SHADER);

    GLint compile_status;
    compile_shader(vertex_shader, vertex_shader_src);
    glGetShaderiv(vertex_shader, GL_COMPILE_STATUS, &compile_status);
    if (compile_status != GL_TRUE) {
        char buffer[512];
        glGetShaderInfoLog(vertex_shader, 512, NULL, buffer);
        throw std::runtime_error(buffer);
    }

    compile_shader(frag_shader, frag_shader_src);
    glGetShaderiv(frag_shader, GL_COMPILE_STATUS, &compile_status);
    if (compile_status != GL_TRUE) {
        char buffer[512];
        glGetShaderInfoLog(frag_shader, 512, NULL, buffer);
        throw std::runtime_error(buffer);
    }

// Put shaders into shader program
    shader_prog = glCreateProgram();
    glAttachShader(shader_prog, vertex_shader);
    glAttachShader(shader_prog, frag_shader);
    glBindFragDataLocation(shader_prog, 0, "outColor");
    glLinkProgram(shader_prog);
    glUseProgram(shader_prog);

// Create VAO
    glGenVertexArrays(1, &vao);
    glBindVertexArray(vao);

// Create vertex and element buffers
    const static GLfloat vertices[] = {
            // Position   Tex-coords
            -1.0f,  1.0f, 0.0f, 0.0f, // Top-left
             1.0f,  1.0f, 1.0f, 0.0f, // Top-right
             1.0f, -1.0f, 1.0f, 1.0f, // Bottom-right
            -1.0f, -1.0f, 0.0f, 1.0f  // Bottom-left
    };

    glGenBuffers(1, &vbo);
    glBindBuffer(GL_ARRAY_BUFFER, vbo);
    glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW);

    const static GLuint elements[] = {
            0, 1, 2,
            2, 3, 0
    };

    glGenBuffers(1, &ebo);
    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, ebo);
    glBufferData(GL_ELEMENT_ARRAY_BUFFER, sizeof(elements), elements, GL_STATIC_DRAW);

// Set shader attributes
    GLint pos_attrib = glGetAttribLocation(shader_prog, "position");
    glVertexAttribPointer(pos_attrib, 2, GL_FLOAT, GL_FALSE, 4 * sizeof(GLfloat), 0);
    glEnableVertexAttribArray(pos_attrib);

    GLint tex_coord_attrib = glGetAttribLocation(shader_prog, "tex_coord");
    glEnableVertexAttribArray(tex_coord_attrib);
    glVertexAttribPointer(tex_coord_attrib, 2, GL_FLOAT, GL_FALSE,
                        4 * sizeof(GLfloat), (void*)(2 * sizeof(GLfloat)));

// Generate texture
    glGenTextures(1, &mandelbrot_tex);

// Bind the texture information

    glActiveTexture(GL_TEXTURE0);
    glBindTexture(GL_TEXTURE_2D, mandelbrot_tex);
    glUniform1i(glGetUniformLocation(shader_prog, "tex"), 0);
}

Draw_Buffer::~Draw_Buffer() {

// Unbind buffer
    glBindVertexArray(NULL);

// Delete shaders
    glDeleteProgram(shader_prog);
    glDeleteShader(vertex_shader);
    glDeleteShader(frag_shader);

// Delete buffers
    glDeleteBuffers(1, &vbo);
    glDeleteBuffers(1, &ebo);
    glDeleteVertexArrays(1, &vao);

// Terminate GLFW
    glfwDestroyWindow(screen);
    glfwTerminate();
}

void Draw_Buffer::flush() {
    glClear(GL_COLOR_BUFFER_BIT);

    // Reset texture
    glBindTexture(GL_TEXTURE_2D, mandelbrot_tex);
    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB, window->width(), window->height(), 0, GL_RGB, GL_BYTE, &buffer[0]);

    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_REPEAT);
    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_REPEAT);
    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);

    // Draw rectangle
    glDrawElements(GL_TRIANGLES, 6, GL_UNSIGNED_INT, 0);

    // Sends message if there is an OpenGL bug
    GLenum err = glGetError();
    if (err) {
        std::stringstream ss;
        ss << "GL Error: " << err;
        throw std::runtime_error(ss.str());
    }

    // Swap buffers
    glfwSwapBuffers(screen);

    // Reset iterator
    pos_iter = buffer.begin();

    while(!glfwWindowShouldClose(screen)) {
        glfwPollEvents();
    }
}

fragment_shader.glsl

#version 150

in vec2 Tex_coord;

out vec4 outColor;

uniform sampler2D tex;

void main() {
    outColor = texture(tex, Tex_coord);
}

vertex_shader.glsl

#version 150

in vec2 position;
in vec2 tex_coord;

out vec2 Tex_coord;

void main() {
    gl_Position = vec4(position, 0.0, 1.0);
    Tex_coord = tex_coord;
}
\$\endgroup\$
5
\$\begingroup\$

This is a lot of code to cover, so I'll touch on the highlights. Overall the code is pretty straightforward and easy to read. I like that you used std::complex instead of writing your own complex class or passing around 2 floats or something like that. Here are a few things I think could be improved:

Break Things Into Functions

For the most part your functions seem to be a decent size, but I see a few that really should be broken up a little more. Your main() function does a number of things: Creates the areas for the complex plane and the OpenGL window, displays instructions to the user, gets user input, draws the image, outputs the image. Those should each be different functions.

If you had the user input in its own function, you might have noticed that you aren't handling invalid input at all. Valid inputs are W, w, I and i. What happens if the user inputs x? Most likely a crash.

By the same token, your constructor for Draw_Buffer() has comments detailing what each section does. This is a good hint that you need to move those sections into their own functions named something similar to the comment. For example, you could have a setupWindow() function, an InitialiseGLEW() function, a generateShaders() function, and a createGeometry() function for the VAO and VBO, etc.

Naming

I think your naming could use improvement. You have a class named Window, but it doesn't do what we typically think of a window on a computer doing. It's in a file named Window.h which is one letter away from Windows.h, which is a fairly important header when building for Windows. You could at least prefix it with something specific to your program, such as MFDWindow for "Mandelbrot Fractal Drawer Window." But as I said, it's not even a window in the traditional sense. It seems to actually represent the boundary of an area of the complex or real plane. You could rename it to something like CalculationBounds or Bounds2D, or something along those lines.

Also, be wary of general terms like Buffer. Terms like Buffer, Record, Data, and Info tend to convey no information at all. It looks like they always hold pixel data or image data, so maybe that could guide your naming? (Also why is it a templated class? You only ever use it with RGB pixels. It could be an RGBBuffer for example if it weren't templated.)

It's not clear to me what purpose your buffer serves that isn't handled just fine by a raw std::vector<>. It takes a Window in the constructor, but none of its code uses it. The subclasses use it, which is a mistake in my opinion. It doesn't generally make sense to have protected data. (protected methods are usually fine.) What you're basically telling the compiler when you do that is "Any class that inherits from this class can change this data." That's only 1 small step above global data in terms of being able to understand the state of your application's data at any given point. With protected data you can't be sure the code in the class that contains it was the one who set it to its current value, and you can't be sure it won't get changed out from under you later.

I noticed that you named the subclasses Image_Buffer and Draw_Buffer. This may sound odd at first, but Image is a noun and Draw is a verb. It's odd to have 2 classes that inherit from the same base class differ in that way. The class represents a thing (a buffer) rather than an action, so I would expect both to be nouns. Draw_Buffer might be better named something like Screen_Buffer or Display_Buffer (where I'm using "display" as a noun, not a verb).

Also, you should decide on one style of naming and stick with it. You have names that are written using underscores to separate words, and other names that use camel case to show word boundaries. For example, in your fragment shader, you use Tex_coord, but outColor. Also, your capitalization is reversed on those two names. It doesn't really matter which one you pick, but you should be consistent in your choice.

Underscores are generally a bad idea as a prefix for variable names, like you have in the Window class. Such names are reserved by the C++ spec for specific use by the compiler. It's very odd to see them as parameter names to methods. And it's even more odd that you'd name an argument to a method the same thing as your member variable. It necessitates the use of the className:: prefix, which also looks funny, since that's usually used for either referencing a static member, or for superclass disambiguation. (In my experience it's more common to see this-> instead of className:: in cases like this.) I recommend removing the leading underscores from everything, and making the method arguments have different names, such as new_x_min.

Future Directions

I realize that this is an exercise to help you learn about OpenGL, so I don't expect you to have already done what I'm going to suggest. This is just some ideas for where to go with this. Now that you've had a taste of how to make OpenGL draw stuff, you can try moving the calculation of the set into the fragment shader so that it's done quickly in parallel by the GPU. It's a little tricky, but not impossible. And then, if you're feeling really ambitious, you can try using a compute shader to do the calculations. Good luck!

\$\endgroup\$
  • \$\begingroup\$ Thank you for writing this review. I appreciate the time you have put into this. \$\endgroup\$ – James Balajan Apr 13 '17 at 8:35
  • \$\begingroup\$ Can you point me in the right direction for concurrency? Should I use the STL, OpenMP or OpenCL? \$\endgroup\$ – James Balajan Apr 13 '17 at 9:18
  • \$\begingroup\$ For this particular problem, OpenCL is likely to be the fastest. GPUs have hundreds to thousands of cores to work on problems, and this problem is a very good fit for them. CPUs currently have 10s of cores, so can't do as much at once. Calculating Mandelbrot sets is "embarrassingly parallelizable." It's compute bound rather than fill-rate bound, so lots of cores doing calculations wins for this. The others are good to know about also, but might work better for a different set of problems. \$\endgroup\$ – user1118321 Apr 13 '17 at 16:34
6
\$\begingroup\$

I see a number of things that may help you improve your code.

Fix the bugs

There are a couple of bugs in this program that should be fixed. First, the include guard in Window.h is missing an #endif, but that's an easy fix. Second, there is an object ownership problem that's a little more subtle. Within main, the window object is created as a regular variable which means that the destructor will be called when it goes out of scope at the end of main. However, the Buffer_Base class gets a std::unique_ptr to the same object, resulting, eventually, in a double delete. Either use a plain pointer within the Buffer_Base class or, better, use std::make_unique within main when constructing the window.

Check for errors sooner

With some versions of GLEW and GL, even a successful call to glewInit() can cause a GL_INVALID_ENUM error. My machine happens to have one of the combinations that causes this. Unfortunately, the only place in the current code that calls glGetError() is in Draw_Buffer::flush(), by which time the actual cause of the error is ancient history. It took some time for me to trace this error back to its source, but if each operation (or group of related operations) had been followed by a call to glGetError(), this would have been much easier to find.

NULL is not 0

This is a call in the Draw_Buffer destructors:

glBindVertexArray(NULL);

However, the argument is a GLuint and not a pointer, so 0 should be used and not NULL.

\$\endgroup\$
  • 1
    \$\begingroup\$ Now that there's nullptr, I'd give simpler advice.If you need a null pointer, use nullptr. Otherwise, use 0. Never use NULL. \$\endgroup\$ – Jerry Coffin Apr 12 '17 at 23:11
  • \$\begingroup\$ Thank you for putting time into writing this review. It was extremely helpful. \$\endgroup\$ – James Balajan Apr 13 '17 at 8:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.