0

I have a class Matrix and another class Camera that I want to utilize the Matrix in. My Matrix class looks like this:

class Matrix4f {
    public:
        Matrix4f() {
            this->setMatrix(EMPTY);
        }

        Matrix4f(Matrix4f &m2) {
            this->setMatrix(m2.matrix);
        }

        static Matrix4f& Matrix4f::identity() {
           Matrix4f& identity = Matrix4f() ;
           identity.setMatrix(IDENTITY);
           return identity;
        }

        void setMatrix(float f[4][4]) {
            for (int r = 0; r < 4; r++) {
                 for (int c = 0; c < 4; c++) {
                      this->matrix[r][c] = f[r][c];
                 }
            }
        }

        Matrix4f& operator=(const Matrix4f &m2) {
             this->setMatrix(m2.matrix);
        }
    private:
        float matrix[4][4];
        static float EMPTY[4][4] = {
             { 0, 0, 0, 0 },
             { 0, 0, 0, 0 },
             { 0, 0, 0, 0 },
             { 0, 0, 0, 0 }
        }; // initialize an empty array (all zeros);
        static float IDENTIY[4][4] = {
             { 1, 0, 0, 0 },
             { 0, 1, 0, 0 },
             { 0, 0, 1, 0 },
             { 0, 0, 0, 1 }
        }; // initialize a identity (array)
}

And I have this in my camera class:

class Camera {
    public:
        Camera() {
            this->calculateProjection();
        }

        Matrix4f* getProjection() {
            return this->projection;
        }
    private:
        Matrix4f* projection;

        Matrix4f* calculateProjection() {
             this->projection = &Matrix4f::identity();
             // modify this->projection...

             return this->projection;
        }
  }

When I try to create a Camera instance and then get its projection I get something that looks like a corrupted object (the matrix is filled entirely to large negative numbers).

I am really confused what is causing my code to misbehave like this.
I am fairly certain it deals with a reference being automatically deleted by the compiler, and I think it deals with the identity matrix but it doesn't really make sense.

Shouldn't the identity matrix be copied into the projection matrix, so it wouldn't even matter if the identity matrix gets garbage collected?

I found that I can actually make this code work by either making the identity matrix create a new Matrix4f() and returning that OR making getProjection() return the calculateProjection().
Problem is, I really don't want to do either of those.
I don't want Identity to construct a new Matrix4f because then I have to deal with destroying it, and I don't want getProjection() to call calculateProjection() because that method is expensive,
and should really only be called once as the Projection matrix never changes.

Thomas Paine
  • 303
  • 4
  • 13
  • 1
    What's `identity`? – deviantfan Sep 30 '16 at 04:10
  • 2
    And why are you using pointers in Camera at all? – deviantfan Sep 30 '16 at 04:11
  • 2
    What is the name of your class `Matrix` or `Matrix4f`? – Nawaz Sep 30 '16 at 04:13
  • About identity: That's the first wrong thing. No references. And you need a better copy constructor (const). – deviantfan Sep 30 '16 at 04:14
  • Get rid of the pointers (return objects), get rid of your overloaded copy constructor and assignment operators, and magically you will see your code work. – PaulMcKenzie Sep 30 '16 at 04:15
  • Wow, I can't believe I forgot the identity method, sorry this was a long question, I added it in now. – Thomas Paine Sep 30 '16 at 04:15
  • @ThomasPaine -- Your class uses float arrays as members, and no pointers. Thus the class can be copied with no issues using the compiler default versions of the copy ctor and assignment operator. You're introducing pointers for no reason whatsoever. – PaulMcKenzie Sep 30 '16 at 04:17
  • Well I originally didn't have any copy or = operator code in the Matrix class. I only added those when the code stopped working and I thought they would fix my issue, but they didn't. – Thomas Paine Sep 30 '16 at 04:20
  • Post your real code for `static float EMPTY[4][4] = ` and the following line, what you have there doesn't compile – M.M Sep 30 '16 at 04:24
  • @ThomasPaine Don't return references to local variables. That leads to undefined behavior. All that other code you added is not necessary, and only adds potential for bugs. – PaulMcKenzie Sep 30 '16 at 04:26
  • I added it, but I'm not sure it's relevant to the question, though maybe I'm wrong. – Thomas Paine Sep 30 '16 at 04:26
  • @ThomasPaine -- The comment section on SO is for comments. I am commenting on your code. – PaulMcKenzie Sep 30 '16 at 04:28
  • You're under a fundamental misconception. There is no "projection matrix". There is *a pointer*. Pointers point to other objects. You aren't thinking about where the object is that `this->projection` points to, or when that object is allocated or deallocated. Currently your code sets it to point to an object that is already destroyed, leading to your problems. A simple solution would be to change `Matrix4f* projection;` to `Matrix4f projection;`. Then it is actually a projection matrix. (And change `identity()` to return by value). – M.M Sep 30 '16 at 04:45
  • Thomas, @PaulMcKenzie 's advice may not seem relevant to your current problem, but if you ignore it you are dooming yourself to a buttload of debugging and probably a few more questions in the future. – user4581301 Sep 30 '16 at 04:45
  • Interesting that the OP has snot fixed whether it is `Matrix` or `Matrix4f` , despite so many comments. – Nawaz Sep 30 '16 at 04:51

2 Answers2

2

Your

 Matrix4f& Matrix4f::identity() {
           Matrix4f& identity = Matrix4f() ;
           identity.setMatrix(IDENTITY);
           return identity;
        }

returns a reference to a local object. Once identity() exits the object is gone.

You need to allocate it on the heap then return that.

Alternatively declare a matrix in you class Camera

  ...
  private:
    Matrix4f projection;

    Matrix4f& calculateProjection() { 
         ... modify ...
         return this->projection;
    }
   ...
AndersK
  • 35,813
  • 6
  • 60
  • 86
  • See, the thing I don't understand is, if I put a breakpoint inside of the calculateProjection() method, Visual Studio shows me that the projection matrix is fully initialized to all of the right values... Also if getProjection() simply returns calculateProjection() then the code magically works, which doesn't make sense since shouldn't the identity matrix be dead? – Thomas Paine Sep 30 '16 at 04:18
  • @ThomasPaine u are in undefined behavior territory, there it sometimes work, sometimes not – AndersK Sep 30 '16 at 04:19
  • `Matrix4f& identity = Matrix4f() ;` would give an error in a conforming compiler. OP might be hamstrung by MSVC – M.M Sep 30 '16 at 04:24
  • 2
    Returning reference to a static object would be another solution (perhaps a better one here sine it seems the intent is to always return the same matrix) – M.M Sep 30 '16 at 04:27
  • How could I properly create a static object inside of the Matrix4f class that would be initialized to the identity matrix that I want it to return? – Thomas Paine Sep 30 '16 at 07:24
1

Am I misunderstanding how pointers work?

Maybe. Your code has several problems that I can see:

    Matrix4f& Matrix4f::identity() {
       Matrix4f& identity = Matrix4f() ;
       identity.setMatrix(IDENTITY);
       return identity;
    }

This creates a temporary object, and a reference to it (the reference is called identity). I'm not sure whether the temporary object is destroyed after Matrix4f& identity = Matrix4f() ;, or when the function returns - but either way, the temporary object is destroyed before the function returns and so the function returns a reference to an object that has been destroyed.

this->projection = &Matrix4f::identity();

this->projection is then set to the address of the temporary object that was destroyed.

Later on, you try to access the temporary object that was destroyed, and unsurprisingly get garbage data.

Also, identity (the function) isn't static, so you can't call it as Matrix4f::identity().

user253751
  • 57,427
  • 7
  • 48
  • 90
  • But shouldn't the compiler copy the data from the matrix which identity() returns into the projection matrix? So why should it matter if the identity() matrix gets destroyed, if the data was already copied into the projection? – Thomas Paine Sep 30 '16 at 04:23
  • Returning a reference to a local variable is undefined behavior. Plain and simple. Don't do it. – PaulMcKenzie Sep 30 '16 at 04:27
  • @ThomasPaine `identity` is destroyed before it gets so far as the assignment to `this->projection`. (Which is a pointer assignment so it would not copy any of the matrix data anyway) – M.M Sep 30 '16 at 04:27
  • @ThomasPaine What projection matrix? I see no projection matrix in your code. I see a pointer called `projection` but a pointer to a matrix is not a matrix. (Just like a piece of paper with a house address written on it is not a house) – user253751 Sep 30 '16 at 04:28
  • I may sound like a broken record, but why is Visual Studio's debugger showing me that projection is initialized perfectly then? If I add a break point into the calculateProjection() method, right before the return statement, it shows me that the projection matrix instance is initialized exactly to what I want it to be initialized (an identity matrix that has been modified). It's like the projection instance just resets itself as soon as the method ends. – Thomas Paine Sep 30 '16 at 04:29
  • @ThomasPaine C++ has something that very few other languages has, and that is a concept called *undefined behavior*. Just because your debugger shows "good numbers" means nothing. For example, if you declared an array of 5 integers and set the items to 0, and you "accidentally" had the debugger show you the sixth entry and it happens to be 0, does that mean the code is ok if you access the sixth element in your code? – PaulMcKenzie Sep 30 '16 at 04:31
  • @ThomasPaine see [What is undefined behaviour](http://stackoverflow.com/a/4105123/1505939). One possibility is appearing to work for a while and then breaking. You seem to be confused about what `projection` is too. The projection instance is *a pointer*. Not a matrix. Your debugger is showing you the contents of the memory which that pointer is pointing to, memory that has already been deallocated. Then later on that memory gets reused for some other purpose so the debugger will show that the memory being pointed to has other contents. – M.M Sep 30 '16 at 04:33
  • @ThomasPaine In theory, see above comments about undefined behaviour. In practice, that's because nothing's got around to overwriting the destroyed object yet. But if you were to create another matrix or even call `printf` it would probably overwrite the space where your projection matrix was, which it's free do to because hey, there's nothing there, just garbage data. – user253751 Sep 30 '16 at 04:35