vector3 math module

Question

I'm writing a little math module in C to handle vectors and matrices. This will be column-major style but right now I've only finished the basics of the vector functions and wanted some feedback on optimization tips, naming conventions, etc. This is C so no real function overloading. I would like to keep it C style.

vec3f.h

#include "stdlib.h"
#include "math.h"

const double m_PI = 3.14159265359;

typedef struct vec3f
{
    double p[3];
}vec3f;

vec3f* vec3fx(double x, double y, double z);
vec3f* vec3fv(vec3f* v3);
vec3f  vec3fs(double x, double y, double z);
void vec3fadd(vec3f* out, struct vec3f* a, struct vec3f* b);
void vec3fsub(vec3f* out, struct vec3f* a, struct vec3f* b);
void vec3fmul(vec3f* out, struct vec3f* a, struct vec3f* b);

void vec3fnegate(vec3f* out);

void vec3fscale(vec3f* out, double a);
void vec3fscaleu(vec3f* out, double x, double y, double z);
void vec3fscalex(vec3f* out, double a);
void vec3fscaley(vec3f* out, double a);
void vec3fscalez(vec3f* out, double a);

void vec3flen(double* out, vec3f* in);

void toDegrees(double* out, double angleInRadians);
void toRadians(double* out, double angleInDegrees);

void vec3fdot(double* out, vec3f* a, vec3f* b);
void vec3fcross(vec3f* out, vec3f* a, vec3f* b);

void vec3fnorm(vec3f* out, vec3f* in);
void vec3fdist(double* out, vec3f* a, vec3f* b);

void vec3fangle(double* out, vec3f* a, vec3f* b);

vec3f.c

#include "vec3f.h"

vec3f* vec3fx(double x, double y, double z)
{
    vec3f* v = (vec3f*)malloc(sizeof(vec3f));
    v->p[0] = x;
    v->p[1] = y;
    v->p[2] = z;
    return v;
}

vec3f* vec3fv(vec3f* v3)
{
    vec3f* v = (vec3f*)malloc(sizeof(vec3f));
    v->p[0] = v3->p[0];
    v->p[1] = v3->p[1];
    v->p[2] = v3->p[2];
    return v;
}

void vec3fadd(vec3f* out, vec3f* a, vec3f* b)
{
    out->p[0] = a->p[0] + b->p[0];
    out->p[1] = a->p[1] + b->p[1];
    out->p[2] = a->p[2] + b->p[2];
}

void vec3fsub(vec3f* out, vec3f* a, vec3f* b)
{
    out->p[0] = a->p[0] - b->p[0];
    out->p[1] = a->p[1] - b->p[1];
    out->p[2] = a->p[2] - b->p[2];
}

void vec3fmul(vec3f* out, vec3f* a, vec3f* b)
{
    out->p[0] = a->p[0] * b->p[0];
    out->p[1] = a->p[1] * b->p[1];
    out->p[2] = a->p[2] * b->p[2];
}

void vec3fnegate(vec3f* out)
{
    out->p[0] = -out->p[0];
    out->p[1] = -out->p[1];
    out->p[2] = -out->p[2];
}

void vec3fscale(vec3f* out, double a)
{
    out->p[0] *= a;
    out->p[1] *= a;
    out->p[2] *= a;
}

void vec3fscaleu(vec3f* out, double x, double y, double z)
{
    out->p[0] *= x;
    out->p[1] *= y;
    out->p[2] *= z;
}

void vec3fscalex(vec3f* out, double a)
{
    out->p[0] *= a;
}

void vec3fscaley(vec3f* out, double a)
{
    out->p[1] *= a;
}

void vec3fscalez(vec3f* out, double a)
{
    out->p[2] *= a;
}

void vec3flen(double* out, vec3f* in)
{
    double res = 0;

    res += in->p[0] * in->p[0];
    res += in->p[1] * in->p[1];
    res += in->p[2] * in->p[2];

    *out = sqrt(res);
}


void toDegrees(double* out, double angleInRadians)
{
    *out = angleInRadians * (double)(180.0/m_PI);
}

void toRadians(double* out, double angleInDegrees)
{
    *out = angleInDegrees * (double)(m_PI/180.0);
}

void vec3fdot(double* out, vec3f* a, vec3f* b)
{
    double total = 0;
    total += a->p[0] * b->p[0];
    total += a->p[1] * b->p[1];
    total += a->p[2] * b->p[2];
    *out = total;
}
void vec3fcross(vec3f* out, vec3f* a, vec3f* b)
{
    out->p[0] = a->p[1] * b->p[2] - b->p[1] * a->p[2];
    out->p[1] = a->p[2] * b->p[0] - b->p[2] * a->p[0];
    out->p[2] = a->p[0] * b->p[1] - b->p[0] * a->p[1];
}

void vec3fnorm(vec3f* out, vec3f* in)
{
    double t = 0;
    vec3flen(&t, in);
    out->p[0] = in->p[0]/t;
    out->p[1] = in->p[1]/t;
    out->p[2] = in->p[2]/t;
}

void vec3fdist(double* out, vec3f* a, vec3f*b)
{
    vec3f tmp;
    vec3fsub(&tmp, b, a);
    vec3flen(out, &tmp);
}

void vec3fangle(double* out, vec3f* a, vec3f* b)
{
    double tmp;
    vec3fdot(&tmp, a, b);
    *out = acos(tmp);
}

Answer 1

This seems to be generally well-written code. I have a few observations which may help you improve your code.

Separate interface from implementation

The interface is the part in the vec3f.h file and the implementation is in the vec3f.c file. Users of this code should be able to read and understand everything they need from the implementation file. That means that only #includes essential to being able to understand the interface should be in the .h file. In this case, none are needed, so the #include lines should be deleted from the .h file. Also...

Use all appropriate #includes

The implementation file (the .c file) does need some #include files but they should be in angle brackets and not quotes. When you write #include "math.h" it is different from #include <math.h>. For standard headers, you should use the <> form. If you're not sure about the difference, see this question for more detail. In this case, vec3f.c should start with:

#include "vec3f.h"
#include <stdlib.h>
#include <math.h>

Use include guards

There should be an include guard in the .h file. That is, start the file with:

#ifndef VEC3F_H
#define VEC3F_H
// file contents go here
#endif // VEC3F_H

Omit spurious declarations

The vec3f.h file includes a declaration for vec3fs but no implementation exists in the vec3f.c file. The declaration should be omitted.

Use const where practical

For a number of the operations, the vec3f structures passed in are unaltered. That's good design, but you should make it explicit. So for example instead of this:

void vec3fadd(vec3f* out, struct vec3f* a, struct vec3f* b);

Use this instead:

void vec3fadd(vec3f* out, const vec3f* a, const vec3f* b);

Note that this change also uses the next suggestion.

Use defined typedef where practical

The declarations for vec3fadd, vec3fsub and vec3fmul should use the the typedef'd vec3f rather than the longer struct vec3f. Both are correct, but since you've created the typedef, it makes sense to also use it consistently.

Provide a means for non-heap allocation

Perhaps your unwritten vec3fs function was intended to do this, but there is currently no way to initialize values for an stack-based vec3f. I'd recommend adding one like this:

vec3f* vec3fs(vec3f* v, double x, double y, double z)
{
    if (v != NULL) {
        v->p[0] = x;
        v->p[1] = y;
        v->p[2] = z;
    }
    return v;
}

Check the return value of malloc

If the program runs out of memory, a call to malloc can fail. The indication for this is that the call will return a NULL pointer. You should check for this and avoid dereferencing a NULL pointer (which typically causes a program crash). For example, using the vec3fs above, you could rewrite vec3fx as:

vec3f* vec3fx(double x, double y, double z)
{
    vec3f* v = malloc(sizeof(vec3f));
    return vec3fs(v, x, y, z);
}

Note also that the cast is not required since malloc returns a void *.

Don't pointlessly initialize variables

The current code includes this function:

void vec3fdot(double* out, vec3f* a, vec3f* b)
{
    double total = 0;
    total += a->p[0] * b->p[0];
    total += a->p[1] * b->p[1];
    total += a->p[2] * b->p[2];
    *out = total;
}

Consider instead this minor rewrite of vec3fdot:

double vec3fdot(const vec3f* a, const vec3f* b)
{
    return a->p[0] * b->p[0]
         + a->p[1] * b->p[1]
         + a->p[2] * b->p[2];
}

Return something useful from functions

The way the functions are currently written, most return void but this prevents one from writing useful construct such as:

toDegrees(vec3fangle(vec1, vec2));

To support this, you could rewrite those two functions as:

double toDegrees(double angleInRadians)
{
    return angleInRadians * (double)(180.0/m_PI);
}
double vec3fangle(const vec3f* a, const vec3f* b)
{
    return acos(vec3fdot(a, b)/(vec3flen(a)*vec3flen(b)));
}

Similarly, I'd recommend that each function that takes a vec3f *out as the first parameter should also return it. This would make your vec3f.h file look like this:

vec3f.h

#ifndef VEC3F_H
#define VEC3F_H
const double m_PI = 3.14159265359;

typedef struct vec3f
{
    double p[3];
}vec3f;

vec3f* vec3fx(double x, double y, double z);
vec3f* vec3fv(vec3f* v3);
vec3f* vec3fs(vec3f* v, double x, double y, double z);
vec3f* vec3fadd(vec3f* out, const vec3f* a, const vec3f* b);
vec3f* vec3fsub(vec3f* out, const vec3f* a, const vec3f* b);
vec3f* vec3fmul(vec3f* out, const vec3f* a, const vec3f* b);

vec3f* vec3fnegate(vec3f* out);

vec3f* vec3fscale(vec3f* out, double a);
vec3f* vec3fscaleu(vec3f* out, double x, double y, double z);
vec3f* vec3fscalex(vec3f* out, double a);
vec3f* vec3fscaley(vec3f* out, double a);
vec3f* vec3fscalez(vec3f* out, double a);

double vec3flen(const vec3f* in);

double toDegrees(double angleInRadians);
double toRadians(double angleInDegrees);

double vec3fdot(const vec3f* a, const vec3f* b);
vec3f* vec3fcross(vec3f* out, const vec3f* a, const vec3f* b);

vec3f* vec3fnorm(vec3f* out, const vec3f* in);
double vec3fdist(const vec3f* a, const vec3f* b);

double vec3fangle(const vec3f* a, const vec3f* b);
#endif // VEC3F_H

Consider providing "in-place" operations as well

Your interface already includes the more generic form in which the output vector is different from either input vector, but it would likely be handy to have an explicit "in-place" version for your operators as well. For example,

vec3f* vec3fadd(vec3f* a, const vec3f* b) 
{
    return vec3fadd(a, a, b);
}

Similarly, you may want to provide non-in-place versions of functions such as the scaling functions:

vec3f* vec3fscale(vec3f* out, const vec3f* a, double f);

Consider making the dimensionality generic

If you ever wanted to make a 2D or 4D point, consider how it might be done:

double vec2flen(const vec2f* in)
{
    return sqrt(in->p[0] * in->p[0]
              + in->p[1] * in->p[1]);
}
double vec3flen(const vec3f* in)
{
    return sqrt(in->p[0] * in->p[0]
              + in->p[1] * in->p[1]
              + in->p[2] * in->p[2]);
}
double vec4flen(const vec4f* in)
{
    return sqrt(in->p[0] * in->p[0]
              + in->p[1] * in->p[1]
              + in->p[2] * in->p[2]
              + in->p[3] * in->p[3]);
}

It might be useful to consider writing these generically in terms of the dimensionality of p. That might be either a fixed compile-time value as shown below or potentially as a member of a generic vecf structure.

double vecflen(const vecf* in)
{
    double sum = 0;
    for (int i=0; i < DIMENSIONS; ++i) {
        sum += in->p[i] * in->p[i];
    }
    return sqrt(sum);
}

Consider removing allocating functions

Instead of the functions which allocate memory and then initialize, I think it may be better to leave out the allocation, since it's nothing more than malloc(sizeof(vec3f)) anyway. Doing so will prevent "hidden" memory allocations since both the allocation and freeing of that memory will happen in some calling function using the usual memory allocation/deallocation calls.

Consider adding a projection function

One handy function that does not appear in your list is a projection function:

//  calculate the projection of b onto a 
vec3f* vec3fproj(vec3f* out, const vec3f* a, const vec3f* b)
{
    vec3fcopy(out, a);
    double alen = vec3flen(a);
    return vec3fscale(out, vec3fdot(a, b)/(alen*alen));
}