HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

2D Points painting program in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
pointspaintingprogram

Problem

I've written a simple program in C++ to practice OpenGL. It takes a set of numbers through stdin and then plots it in Decartes coordinate system with x for position in input, and y for the value itself.

Example

Input:

1 4 9 16 25 36 49 64 81 100 121 144 169 196 225 256 289 324 361 400


Points to paint:

(0, 1) (1, 4) (2, 9) (3, 16) ... (19, 400)


Code


Mind the headers when compiling.

main.cpp

#include 
#include 
#include 

#include 
#include 

#include 

typedef float real_t;

std::vector  DATA;

real_t
    size_x = 800,
    size_y = 800,

    width(size_x * 4),
    height(size_y * 4),

    change = 0.05,

    BOLD = 3,
    bold_x(BOLD * width / size_x),
    bold_y(BOLD * height / size_y),

    shift_x = 0,
    shift_y = 0;

void DrawText(real_t x, real_t y, const char *string) {
    glRasterPos2f(x, y);
    for (const char *c = string; *c != '\0'; ++c)
        glutBitmapCharacter(GLUT_BITMAP_TIMES_ROMAN_24, *c);
}

inline void DrawPoint(real_t &x, real_t &y) {
    real_t
        point_x = 2 * x - shift_x,
        point_y = 2 * y - shift_y;
    if(
            point_x >= width
        ||
            point_x = height
        ||
            point_y 
void fill_data(std::vector  &data) {
    real_t value;
    while(std::cin >> value)
        data.push_back(value);
}

int main(int argc, char **argv) {
    fill_data(DATA);
    glutInit(&argc, argv);      
    glutInitDisplayMode(GLUT_DOUBLE | GLUT_RGBA | GLUT_DEPTH);  
    glutInitWindowSize(size_x, size_y);
    glutCreateWindow("gl_world");

    glEnable(GL_BLEND);
    glBlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA);

    glutDisplayFunc(Display);
    glutReshapeFunc(Reshape);
    glutKeyboardFunc(Keyboard);
    glutSpecialFunc(Special);

    Display();

    glutMainLoop();

    return 0;
}


Could you, please, help me to improve it's readability, performance and style? What are the better practices?

Solution

First, a note on legacy OpenGL

This OpenGL+GLUT API that you're using is the legacy, AKA fixed-function, OpenGL. It is an alright option for learning the basics of 2D/3D hardware accelerated graphics, but... nobody uses it anymore, that's why it's called legacy :P. Graphics hardware has evolved A LOT since the first release of OpenGL, some 20-ish years ago, so it is only natural that the API has evolved and changed a lot in the process too. Todays graphics take advantage of the so called "programable pipeline", meaning you actually write programs in a shading language that run directly on the GPU.

Once you get more or less familiar with the basics, I suggest that you take the leap to modern GL at once (aim OpenGL version 3 and above), since there's not much point in learning the finer aspects of an outdated technology (apart from retro-programming exercises, of course). At first you might find it a bit daunting to see the amount of extra work needed to get a simple triangle to draw on screen with OpenGL 3+, and that's the way it is. GL has become a much lower-level API for the sake of perfomance. That's not a bad thing really, because there are dozens of high-level libraries built on top of it to choose from if you just want some quick 2D/3D drawing, leaving GL for the high-perf, low-level crew. Nevertheless, I suggest you do take a look at a couple tutorials of modern OpenGL to get a feel for it and better understand the 3D pipeline, even if in the end you just want something simple and easy to set up and switch to some high-level framework.

Code Review

It would be nice to use a class, even if this is a main.cpp-only kind of program. That avoids the bad habit of global variables and keeps the code (hopefully) better organized.

DrawPoint:

It takes x and y by reference, but that's not necessary, since the function doesn't change those parameters.

Also this formatting is not very practical:

if(
        point_x >= width
    ||
        point_x = height
    ||
        point_y < 0
)


That statement isn't even that long, it could be all in the same line, or breaking once at most.

All that stringification here is only creating temporary instances of strings that get discarded right away.

DrawText(width - 230 * width/size_x, height - 30 * height/size_y, std::string(std::string() + "Width:  " + std::to_string(width)).c_str());


This should be enough:

DrawText(width - 230 * width/size_x, 
         height - 30 * height/size_y, 
        ("Width:  " + std::to_string(width)).c_str());


Also, that's one place where breaking the lines actually helps readability, unlike the if previously mentioned.

By-the-way, why not just take a const std::string & in DrawText? Both usage instances of that function are dealing with C++ strings, so the char * parameter seems inadequate here.

Not much reason to make fill_data a template function. In the body, you read a real_t value, so template type anything_t has to be implicitly convertible to real_t, plus, in the only usage case we have a std::vector, so just keep it simple as a non-template function taking a ref to std::vector.

Be consistent with your naming conventions. You have variables named using snake_case, but also ALL_UPPERCASE. The uppercase notation is usually associated with macro constants. Your functions are all MultiCase, with the exception of fill_data. A consistent naming convention will help you and readers of the code to remember names more easily.

Code Snippets

if(
        point_x >= width
    ||
        point_x < 0
    ||
        point_y >= height
    ||
        point_y < 0
)
DrawText(width - 230 * width/size_x, height - 30 * height/size_y, std::string(std::string() + "Width:  " + std::to_string(width)).c_str());
DrawText(width - 230 * width/size_x, 
         height - 30 * height/size_y, 
        ("Width:  " + std::to_string(width)).c_str());

Context

StackExchange Code Review Q#105415, answer score: 3

Revisions (0)

No revisions yet.