CODEWITHSUNDEEP
c++assignment-operator
Lerp
Lerp

Reputation: 3127

Matrix Assignment Causing Crash

Whenever I assign an already declared Matrix to somethnig else my program exits unexpectedly.

I really don't know why this happens, I was under the impression that leaving assignment operators for the compiler to generate was the best thing to do unless you absolutely had to overload it, which makes me think there's something wrong with my constructor or something?

Example:

Matrix rotation = rotationMatrix3(Y, degToRad(20.0f));
Vector3 left = rotation * direction_;

rotation = rotationMatrix3(Y, degToRad(-20.0f)); //crash
Vector3 right = rotation * direction_;

Here's my Matrix class:

Matrix.h

enum Axis {
    X, Y, Z,
};

class Matrix {
public:
    Matrix(int cols, int rows);
    Matrix(int cols, int rows, const float values[]);
    ~Matrix();

    float& operator()(int col, int row);
    float  operator()(int col, int row) const;
    inline int cols() const { return cols_; }
    inline int rows() const { return rows_; }

private:
    int cols_, rows_;
    float* values_;
};

Vector3 operator*(const Matrix& m, const Vector3& v);

Matrix rotationMatrix3(int axis, float rads);

Matrix.cpp

Matrix::Matrix(int cols, int rows) : cols_(cols), rows_(rows), values_(NULL) {
    values_ = new float[rows_ * cols_];
}

Matrix::Matrix(int cols, int rows, const float values[]) : cols_(cols), rows_(rows), values_(NULL) {
    values_ = new float[rows_ * cols_];

    for(int c = 0; c < cols; ++c) {
        for(int r = 0; r < rows; ++r) {
            (*this)(c, r) = values[r * cols + c];
        }
    }
}

Matrix::~Matrix() {
    delete [] values_;
}

float& Matrix::operator()(int col, int row) {
    return values_[row * cols_ + col];
}

float Matrix::operator()(int col, int row) const {
    return values_[row * cols_ + col];
}

Vector3 operator*(const Matrix& m, const Vector3& v) {
    if(m.cols() != 3) {
        throw std::logic_error("Matrix must have only 3 cols");
    }

    Vector3 result;
    result.x = m(0, 0) * v.x + m(1, 0) * v.y + m(2, 0) * v.z;
    result.y = m(0, 1) * v.x + m(1, 1) * v.y + m(2, 1) * v.z;
    result.z = m(0, 2) * v.x + m(1, 2) * v.y + m(2, 2) * v.z;

    return result;
}

Matrix rotationMatrix3(int axis, float rads) {
    float c = static_cast<float>(cos(rads));
    float s = static_cast<float>(sin(rads));

    if(axis == X) {
        const float mat[] = { 1.0f, 0.0f, 0.0f,
                              0.0f, c,    -s,
                              0.0f, s,    c };
        return Matrix(3, 3, mat);
    } else if(axis == Y) {
        const float mat[] = { c,    0.0f, s,
                              0.0f, 1.0f, 0.0f,
                              -s,   0.0f, c };
        return Matrix(3, 3, mat);
    } else if(axis == Z) {
        const float mat[] = { c,    -s,   0.0f,
                              s,    c,    0.0f,
                              0.0f, 0.0f, 1.0f };
        return Matrix(3, 3, mat);
    } else {
        throw std::logic_error("Unknown axis");
    }
}

Upvotes: 0

Views: 353

Answers (2)

David Hammen
David Hammen

Reputation: 33116

I was under the impression that leaving assignment operators for the compiler to generate was the best thing to do unless you absolutely had to overload it, which makes me think there's something wrong with my constructor or something?

Drop that impression. Those freebies from the language are rarely the "right" thing to do. You have a raw pointer float* values_; in your class. You absolutely have to overload the copy constructor and the assignment operator here.

Upvotes: 2

paddy
paddy

Reputation: 63471

Yep, make a copy constructor that takes a matrix. Otherwise you will be copying the pointer values_ which is subsequently deleted in the original return value that immediately drops out of scope.

My advice to you... You're obviously creating classes to do 3D. DON'T make an arbitrary-sized matrix class for this. Just make a basic 4x4 homogeneous matrix (or 4x3 or 3x3 if you must). Hard-coded. Single array of values - no dynamic allocation. Save you a lot of time, heap fragmentation, and annoying sanity tests. You are never going to run this class with a 5x17 matrix or whatever. So don't code for it.

You commonly need a 2d, 3d, or 4d vector, and a 3d or 4d square matrix. That's all. So make your classes:

Vector2
Vector3
Vector4
Matrix3
Matrix4

Upvotes: 2

Related Questions