Game engine/C++11 software 3D renderer/WebGL library

I am a graphics programmer and was wondering about my code quality.

I selected these three examples:

• Game engine, which uses OpenGL and has a lot of other non-graphics code

• A 3D software renderer, uses the basic X11 libraries for drawing

• A WebGL framework (found under felipetavares/webgl.js)

Here is the code for the triangle rasterizer in the 3D renderer (it uses some functions/types but they should be obvious by the context):

```
void tRas::rasterTriangle (vec2& _a, vec2& _b, vec2& _c) {
vec2 a,b,c;

// First, sort vertices by y
vector v {_a,_b,_c};
int i,smaller=0;
for (i=0;i= ap)
bp = smaller+1;
else
bp = smaller;
b = v[smaller];
v.erase(v.begin()+smaller);

c = v[0];
for (i=0;i= pixelBuffer->width ||
a.y = pixelBuffer->height)
return;

FloatToUInt conversor;

conversor.i = zBuffer->getPixel(a.x,a.y);

if (tri.vertices[0].position.z >= conversor.f) {
conversor.f = tri.vertices[0].position.z;

vec2 fcoord = tri.vertices[0].texCoord*tri.vertices[0].position.z;
vec3 fcolor = fragmentShader(tri.vertices[0].color, fcoord, tri.texture);
unsigned char r=fcolor.x255,g=fcolor.y255,b=fcolor.z*255;
unsigned int fc = (0xFFsetPixel ((int)a.x,(int)a.y,fc);
zBuffer->setPixel ((int)a.x, (int)a.y, conversor.i);
}
return;
}

if (b.y-a.y > 0) {
dx1=(b.x-a.x)/(b.y-a.y);
dc1=(tri.vertices[bp].color-tri.vertices[ap].color)/(b.y-a.y);
dt1=(tri.vertices[bp].texCoord/tri.vertices[bp].position.z-tri.vertices[ap].texCoord/tri.vertices[ap].position.z)/(b.y-a.y);
dz1=(1/tri.vertices[bp].position.z-1/tri.vertices[ap].position.z)/(b.y-a.y);
drz1=(tri.vertices[bp].position.z-tri.vertices[ap].position.z)/(b.y-a.y);
} else
dx1=0;
if (c.y-a.y > 0) {
dx2=(c.x-a.x)/(c.y-a.y);
dc2=(tri.vertices[cp].color-tri.vertices[ap].color)/(c.y-a.y);
dt2=(tri.vertices[cp].texCoord/tri.vertices[cp].position.z-tri.vertices[ap].texCoord/tri.vertices[ap].position.z)

This code is a little bit hard to review, as you've only included a single function. It's difficult to tell whether undeclared variables are members or globals, and other things like that. Having said that, I'll tell you what things I've noticed in what you've posted.

Naming is Hard

It's been said that naming things effectively is one of the hardest things in programming. It's even harder when you're working with something as abstract as a triangle. There's no real-world thing it represents. But we can still do better than a, b, and c for the vertices. I recently wrote some bad code with these exact variable names! I was doing something that involved the quadratic equation, and when I learned it, it was always written with the variables a, b, and c. Well it turns out the class I was adding the code to did exactly what you did and had 3 vertices with the same name. I failed to notice and had a very difficult time debugging some very strange results!

I recommend using something like v0, v1 and v2 for "vertex" 0, 1 and 2. Or p0, p1 and p2 for "point" 0, 1 and 2.

Also, it's not a great idea to have function arguments start with an underscore. It's the opposite of the typical convention of member variables starting with an underscore and is likely to confuse future readers of this code. (I believe there may also be cases where C++ reserves symbols beginning with an underscore, though I may be remembering wrong.)

I'm also not fond of vec2 and vec3. I get that they are 2 or 3 element vectors, but what do they contain? floats? ints? shapes? pizzas? In most places where I've seen such classes they are name vec2f and vec3f to indicate that they hold floats.

Break Things Into Functions

It looks like there are (at least) 2 steps to your algorithm - sort the vertices by Y coordinate, then test for degenerate triangles, do the rasterization. Those should be 3 different functions.

Don't Do Things The Hard Way

Your method of sorting the points by their y coordinates looks really complicated and slow. You could just do something like this:

vec2 low, mid, high;
if (_a.y < _b.y)
{
    low = _a;
    mid = _b;
}
else
{
    low = _b;
    mid = _a;
}

if (_c.y < low.y)
{
    high = mid;
    mid = low;
    low = _c;
}
else if (_c.y < mid._y)
{
    high = mid;
    mid = _c;
}
else
{
    high = _c;
}

Don't Repeat Yourself

In the last section of the method, it looks like you're finding the some slopes. It looks like you're doing the same thing 3 times in a row. It would be better to have a method that you call 3 times with the appropriate parameters. Something like this:

void findSlopes(const vec2& v0, const vec2& v1, const int v0Index, const int v1Index, float& dx, float& dc, float& dt, float& dz, float& drz)
{
  if (v1.y - v0.y > 0) {
    dx = (v1.x - v0.x) / (v1.y - v0.y);
    dc = (tri.vertices[v1Index].color - tri.vertices[v0Index].color) / (b.y - a.y);
    dt = (tri.vertices[v1Index].texCoord / tri.vertices[v0Index].position.z - tri.vertices[v0Index].texCoord / tri.vertices[v0Index].position.z) / (b.y - a.y);
    dz = (1 / tri.vertices[v1Index].position.z - 1 / tri.vertices[v0Index].position.z) / (b.y - a.y);
    dry = (tri.vertices[v1Index].position.z - tri.vertices[v0Index].position.z) / (b.y - a.y);
  } else
    dx = 0;
}

StackExchange Code Review Q#118826, answer score: 4