3
\$\begingroup\$

I am currently creating my own 3D renderer (think something like OpenGL). As a first step, I created a mini program below to render primitive 2D shapes essential to 3D graphics: points, lines, and triangles with z buffering.

I'm hoping for code review on three essential methods in the class Display

  • _DrawFilledTriangle (see also __DrawUpperFilledTriangle and __DrawLowerFilledTriangle)

  • _DrawLine

  • _PutPixel

I'm not sure about parameters these methods should take. The current implementation looks ugly to me because x and y takes int values while z takes double. It is necessary so z buffering will work as intended. I tried int and it produces strange behavior. What are better parameter choices for these methods? I'm thinking about creating a Point struct but it will involve lots of moving and copying. I'm also quite concerned about their performance. Are there any way to speed execution?

Miscellaneous reviews are also welcome!

/* g++ primitives.cpp -o primitives -lSDL2 */

#include <algorithm>
#include <vector>
#include <cmath>

#include <Windows.h>

#include <SDL2/SDL.h>

#define ID_TIMER 1
#define UPDATE_INTERVAL 0

struct Colour
{
    uint8_t r;
    uint8_t g;
    uint8_t b;
    uint8_t a;
};

const int SCREEN_WIDTH = 500;
const int SCREEN_HEIGHT = 500;
const int SCREEN_SCALE_FACTOR = 1;

const Colour RED{255, 0, 0, 255};
const Colour ORANGE{255, 127, 0, 255};
const Colour YELLOW{255, 255, 0, 255};
const Colour GREEN{0, 255, 0, 255};
const Colour BLUE{0, 0, 255, 255};
const Colour INDIGO{75, 0, 130, 255};
const Colour VIOLET{148, 0, 211, 255};

const Colour rainbow[7] = {
    RED,
    ORANGE,
    YELLOW,
    GREEN,
    BLUE,
    INDIGO,
    VIOLET
};

const double ZMAX = 10001.0;

class Display
{
public:
    Display(int width, int height);
    ~Display();

    void Create(HWND);
    void Update();
    void Paint();
    void CleanUp();
    void Destroy();
private:
    SDL_Window *wnd;
    SDL_Renderer *renderer;
    SDL_Texture *texture;

    int width;
    int height;

    std::vector<uint8_t> pixels;
    std::vector<double> zdepth;

    /* Coordinate system:
       x goes right starting from top left corner
       y goes down starting from top left corner
       z goes into page starting from top left corner
     */
    void _DrawFilledTriangle(int x1, int y1, double z1, int x2, int y2, double z2, int x3, int y3, double z3, const Colour& colour);
    void __DrawUpperFilledTriangle(int x1, int y1, double z1, int x2, int y2, double z2, int x3, int y3, double z3, const Colour& colour);
    void __DrawLowerFilledTriangle(int x1, int y1, double z1, int x2, int y2, double z2, int x3, int y3, double z3, const Colour& colour);
    void _DrawLine(int x1, int y1, double z1, int x2, int y2, double z2, const Colour& colour);
    void _PutPixel(int x, int y, double z, const Colour& colour);

    bool _WithinBounds(int x, int y, double z); // for clipping
    void _Swap(int& x1, int& y1, double& z1, int& x2, int& y2, double& z2);
};

Display::Display(int width, int height)
  : width(width), height(height), pixels(width * height * 4), zdepth(width * height * 4)
{
    
}

Display::~Display()
{}

void Display::Create(HWND hWnd)
{
    if (SDL_Init(SDL_INIT_EVERYTHING) < 0)
    {
        SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Couldn't initialize SDL: %s", SDL_GetError());
        std::exit(1);
    }

    wnd = SDL_CreateWindowFrom(hWnd);
    if (wnd == NULL)
    {
        SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Couldn't create window: %s", SDL_GetError());
        std::exit(1);
    }

    renderer = SDL_CreateRenderer(wnd, -1, SDL_RENDERER_ACCELERATED);
    if (renderer == NULL)
    {
        SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Couldn't create renderer: %s", SDL_GetError());
        std::exit(1);
    }

    texture = SDL_CreateTexture(renderer, SDL_PIXELFORMAT_ARGB8888, SDL_TEXTUREACCESS_STREAMING, width, height);
    if (texture == NULL)
    {
        SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Couldn't create texture: %s", SDL_GetError());
        std::exit(1);
    }
}

void Display::Update()
{
    std::fill(zdepth.begin(), zdepth.end(), ZMAX);
/*
    int centerX = width / 2;
    int centerY = height / 2;

    float radius = 80.0;
    float dr = 20.0;

    double z = 0.0;
    double dz = 10.0;
    int middlez = 35.0;

    for (int i = 0; i < 7; ++i)
    {
        float angle = 0.0;

        while (angle < 360.0)
        {
            int x = centerX + int(radius * std::cos(angle));
            int y = centerY + int(radius * std::sin(angle));

            _DrawLine(centerX, centerY, middlez, x, y, z, rainbow[i]);

            angle += 1.0;
        }

        radius += dr;
        z += dz;
    }
*/
    _DrawFilledTriangle(10, 50, 10, 400, 100, 10, 290, 380, 10, RED);
    _DrawFilledTriangle(50, 350, 2, 130, 40, 20, 380, 200, 5, GREEN);
    /*
    _DrawLine(10, 10, 10, 380, 380, 10, GREEN);
    _DrawLine(10, 10, 1, 380, 380, 100, BLUE);
    _PutPixel(width - 1, height - 1, 0, RED);
    */
}

void Display::Paint()
{
    SDL_SetRenderDrawColor(renderer, 0, 0, 0, 255);
    SDL_RenderClear(renderer);
    SDL_UpdateTexture(texture, NULL, &pixels[0], width * 4);
    SDL_RenderCopy(renderer, texture, NULL, NULL);
    SDL_RenderPresent(renderer);
}

void Display::CleanUp()
{
    SDL_DestroyTexture(texture);
    texture = NULL;

    SDL_DestroyRenderer(renderer);
    renderer = NULL;

    SDL_DestroyWindow(wnd);
    wnd = NULL;

    SDL_Quit();
}

void Display::Destroy()
{
    PostQuitMessage(0);
}

// Nice reference: http://www.sunshine2k.de/coding/java/TriangleRasterization/generalTriangle.png
void Display::_DrawFilledTriangle(int x1, int y1, double z1, int x2, int y2, double z2, int x3, int y3, double z3, const Colour& colour)
{
    // Ensure that each vertex fits in the screen
    if (!_WithinBounds(x1, y1, z1) ||
        !_WithinBounds(x2, y2, z2) ||
        !_WithinBounds(x3, y3, z3))
    {
        return;
    }

    // Sort p1, p2, and p3 so that p1 < p2 < p3
    if (y2 < y1) { _Swap(x2, y2, z2, x1, y1, z1); }
    if (y3 < y1) { _Swap(x3, y3, z3, x1, y1, z1); }
    if (y3 < y2) { _Swap(x3, y3, z3, x2, y2, z2); }

    if (y2 == y3)
    {
        __DrawUpperFilledTriangle(x1, y1, z1, x2, y2, z2, x3, y3, z3, colour);
    }
    else if (y1 == y2)
    {
        __DrawLowerFilledTriangle(x1, y1, z1, x2, y2, z2, x3, y3, z3, colour);
    }
    else
    {
        // Find x4, y4, and z4 using similar triangles: (x3-x1)/(y3-y1)=(x4-x1)/(y4-y1), y4=y2
        int x4 = int(x1 + float(y2 - y1) * ((x3 - x1) / float(y3 - y1)));
        int y4 = y2;
        double z4;

        if (z1 > z3) // p1 is farther than p3
        {
            z4 = z3 + double(y3 - y4) * ((z1 - z3) / double(y3 - y1));
        }
        else
        {
            z4 = z3 - double(y3 - y4) * ((z3 - z1) / double(y3 - y1));
        }

        __DrawUpperFilledTriangle(x1, y1, z1, x2, y2, z2, x4, y4, z4, colour);
        __DrawLowerFilledTriangle(x2, y2, z2, x4, y4, z4, x3, y3, z3, colour);
    }
}

void Display::__DrawUpperFilledTriangle(int x1, int y1, double z1, int x2, int y2, double z2, int x3, int y3, double z3, const Colour& colour)
{
    float dy1 = std::fabs(y2 - y1);
    float dy2 = std::fabs(y3 - y1);

    float curx1 = x1;
    float curx2 = x1 + 0.5;
    float xinc1 = (x2 - x1) / dy1;
    float xinc2 = (x3 - x1) / dy2;

    double curz1 = z1;
    double curz2 = z1 + 0.5;
    double zinc1 = (z2 - z1) / dy1;
    double zinc2 = (z3 - z1) / dy2;

    for (int y = y1; y <= y2; y++)
    {
        _DrawLine((int) curx1, y, curz1, (int) curx2, y, curz2, colour);

        curx1 += xinc1;
        curx2 += xinc2;
        curz1 += zinc1;
        curz2 += zinc2;
    }
}

void Display::__DrawLowerFilledTriangle(int x1, int y1, double z1, int x2, int y2, double z2, int x3, int y3, double z3, const Colour& colour)
{
    float dy1 = std::fabs(y3 - y1);
    float dy2 = std::fabs(y3 - y2);

    float curx1 = x3;
    float curx2 = x3 + 0.5;
    float xinc1 = (x3 - x1) / dy1;
    float xinc2 = (x3 - x2) / dy2;

    double curz1 = z3;
    double curz2 = z3 + 0.5;
    double zinc1 = (z3 - z1) / dy1;
    double zinc2 = (z3 - z2) / dy2;

    for (int y = y3; y > y1; y--)
    {
        _DrawLine((int) curx1, y, curz1, (int) curx2, y, curz2, colour);

        curx1 -= xinc1;
        curx2 -= xinc2;
        curz1 -= zinc1;
        curz2 -= zinc2;
    }
}

// Adapted from https://github.com/ssloy/tinyrenderer/wiki/Lesson-1:-Bresenham%E2%80%99s-Line-Drawing-Algorithm
void Display::_DrawLine(int x1, int y1, double z1, int x2, int y2, double z2, const Colour& colour)
{
    if (x1 == x2 && y1 == y2 && std::fabs(z1 - z2) < 1e-4)
    {
        _PutPixel(x1, y1, z1, colour);
        return;
    }

    bool steep = false;

    if (std::abs(x1 - x2) < std::abs(y1 - y2))
    {
        _Swap(x1, x2, z1, y1, y2, z1);
        steep = true;
    }

    if (x1 > x2)
    {
        _Swap(x1, y1, z1, x2, y2, z2);
    }

    int dx = x2 - x1;
    int dy = y2 - y1;
    double dz = z2 - z1;

    int derrY = std::abs(dy) * 2;
    int errY = 0;
    int y = y1;
    int yinc = y2 > y1 ? 1 : -1;

    double z = z1;
    double zinc = dz / std::fabs(dx);

    if (steep)
    {
        for (int x = x1; x <= x2; x++)
        {
            _PutPixel(y, x, z, colour);

            errY += derrY;
            if (errY > dx)
            {
                y += yinc;
                errY -= dx * 2;
            }

            z += zinc;
        }
    }
    else
    {
        for (int x = x1; x <= x2; x++)
        {
            _PutPixel(x, y, z, colour);

            errY += derrY;
            if (errY > dx)
            {
                y += yinc;
                errY -= dx * 2;
            }

            z += zinc;
        }
    }
}

void Display::_PutPixel(int x, int y, double z, const Colour& colour)
{
    int offset = y * width * 4 + x * 4;

    if (zdepth[offset] > z)
    {
        zdepth[offset] = z;
        pixels[offset] = colour.b;
        pixels[offset + 1] = colour.g;
        pixels[offset + 2] = colour.r;
        pixels[offset + 3] = colour.a;
    }
}

bool Display::_WithinBounds(int x, int y, double z)
{
    return x >= 0 && x < width &&
           y >= 0 && y < height &&
           z >= 0.0;
}

void Display::_Swap(int& x1, int& y1, double& z1, int& x2, int& y2, double& z2)
{
    int tmpX = x1;
    int tmpY = y1;
    double tmpZ = z1;

    x1 = x2;
    y1 = y2;
    z1 = z2;

    x2 = tmpX;
    y2 = tmpY;
    z2 = tmpZ;
}

LRESULT CALLBACK WndProc(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam)
{
    static Display app(SCREEN_WIDTH, SCREEN_HEIGHT);

    switch (msg)
    {
    case WM_CREATE:
    {
        app.Create(hWnd);

        if(!SetTimer(hWnd, ID_TIMER, UPDATE_INTERVAL, NULL))
        {
            MessageBox(hWnd, "Could not set timer!", "errYor", MB_OK | MB_ICONEXCLAMATION);
            PostQuitMessage(1);
        }
        break;
    }
    case WM_TIMER:
    {
        app.Update();
        InvalidateRect(hWnd, NULL, FALSE);
        break;
    }
    case WM_PAINT:
    {
        app.Paint();
        break;
    }
    case WM_CLOSE:
    {
        app.CleanUp();
        KillTimer(hWnd, ID_TIMER);
        DestroyWindow(hWnd);
        break;
    }
    case WM_DESTROY:
        app.Destroy();
        break;
    default:
        return DefWindowProc(hWnd, msg, wParam, lParam);
    }

    return 0;
}

int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow)
{
    const TCHAR szClassName[] = TEXT("MyClass");

    WNDCLASS wc;
    HWND hWnd;
    MSG msg;
    RECT rcClient;
    UINT style = WS_OVERLAPPED | WS_CAPTION | WS_SYSMENU | WS_MINIMIZEBOX | WS_VISIBLE; // no maximize box and resizing

    wc.style         = CS_HREDRAW | CS_VREDRAW;
    wc.lpfnWndProc   = WndProc;
    wc.cbClsExtra    = 0;
    wc.cbWndExtra    = 0;
    wc.hInstance     = hInstance;
    wc.hIcon         = LoadIcon(NULL, IDI_APPLICATION);
    wc.hCursor       = LoadCursor(NULL, IDC_ARROW);
    wc.hbrBackground = (HBRUSH)(COLOR_WINDOW+1);
    wc.lpszMenuName  = NULL;
    wc.lpszClassName = szClassName;
    wc.hIcon         = LoadIcon(NULL, IDI_APPLICATION);

    if (!RegisterClass(&wc))
    {
        MessageBox(NULL, TEXT("Window Registration Failed!"), TEXT("errYor!"),
            MB_ICONEXCLAMATION | MB_OK);
        return 0;
    }

    rcClient.left = 0;
    rcClient.top = 0;
    rcClient.right = SCREEN_WIDTH * SCREEN_SCALE_FACTOR;
    rcClient.bottom = SCREEN_HEIGHT * SCREEN_SCALE_FACTOR;

    AdjustWindowRectEx(&rcClient, style, TRUE, 0);

    hWnd = CreateWindow(szClassName,
        TEXT("SDLWin1"),
        style,
        CW_USEDEFAULT,
        CW_USEDEFAULT,
        rcClient.right - rcClient.left,
        rcClient.bottom - rcClient.top,
        NULL,
        NULL,
        hInstance,
        NULL);

    if (hWnd == NULL)
    {
        MessageBox(NULL, TEXT("Window Creation Failed!"), TEXT("errYor!"),
            MB_ICONEXCLAMATION | MB_OK);
        return 0;
    }

    ShowWindow(hWnd, nCmdShow);
    UpdateWindow(hWnd);

    while (GetMessage(&msg, NULL, 0, 0))
    {
        TranslateMessage(&msg);
        DispatchMessage(&msg);
    }

    return (int) msg.wParam;
}

Output

out

\$\endgroup\$

2 Answers 2

8
\$\begingroup\$
  • Your use of underscores to prefix methods is not a good idea since they are reserved.
  • The lack of comments through out the code mean that it is more difficult to read than it should be.
  • You should look at making variables and arguments const where they can be.
  • Use of a struct/class to amalgamate the three co-ordinate values for a point would help.
  • You might want to look at cleaning the level 3 warnings (at least) up from the compiler. There should be some about implicit casting (double to float) and C Style casts.
  • In __DrawLowerFilledTriangles you are doing a std::fabs() where the parameter is an int. I feel this might be a performance hit, it might be better to use abs and then cast to a float. If you are going to use this function a lot it might make a difference.
  • Again this is just a feeling, it might be more performant in _PutPixel to write the colour as a 32bit uint rather than 4 8bit uints. So you could add an operator to the Colour class that wrote it to a uint32 in the bgra order using the bitwise shift operator.
  • I've just read your comment about the Point. If you use const ref arguments to you function and emplace on your vectors you should be OK about moving and copying.
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for suggestions! When amalgamating coordinates, I want all of them take the same type (either int or double). But if I use double for x and y, there will be lots of conversions to int. I wonder if it will make the program slow. \$\endgroup\$
    – Jimmy Yang
    May 26 at 7:58
  • \$\begingroup\$ I've always wondered about double/fp pixel co-ordinates. How many screens let you print to 1/2 a pixel? You could use a template and let the compiler do all the work, you code could/should be agnostic. \$\endgroup\$ May 27 at 9:16
2
\$\begingroup\$

Small things

  • The first line comment appears to be instructions on how to compile. If you plan on expanding the project beyond this, maybe it is worth making compilation commands into a Makefile.
  • Don't leave around commented-out code if you can. Refactor to demo functions if they do demo functionality.
  • You don't have to implement swaps with a temp variable in c++ because we have std::swap.
  • Maybe it's worth having a Triangle class instead of taking in lots of arguments for triangle drawing. Stylistically I don't like functions with 10 arguments. It's more natural to represent a triangle as 3 individual vertices.
  • For a Point struct, the compiler should be able to figure out when to move or copy with using const refs. And I would guess it's not a performance issue versus the computations for drawing but that would be revealed through profiling.
  • If you do a lot of data shuffling, maybe it is faster to be able to access the 4 8-bit channels as a single 32-bit value in a union with type punning or have proper getters and setters. This should be profiled too.
\$\endgroup\$

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.