dwmkerr / glmnet

GlmNet is a .NET version of the excellent OpenGL Mathematics library (GLM).
MIT License
53 stars 20 forks source link

Matrices do not perform a deep copy (inconsistent with vector) #1

Open chrispepper1989 opened 9 years ago

chrispepper1989 commented 9 years ago

There is some inconsistencies with GLMnet in the way it treats matrices and vertices.

vec2 bob = new vec2(1,1); vec2 fred = bob; // fred is now a copy of bob fred = fred * new vec(2,2,); //bob is still 1,1

mat4 bob= mat4.identity(); mat4 fred = bob; //both bob and fred point to the same internal array

glm.translate(fred , by); //bob & fred have now changed

I suggest that you overload the "=" operator so that matrices perform a deep copy. Or provide a clone function like:

    public static mat4 CloneMatrix(mat4 matrix)
    {
        vec4[] cols = new[]
        {
            matrix[0],
            matrix[1],
            matrix[2],
            matrix[3]

        };
        return new mat4(cols);
    }
chrispepper1989 commented 9 years ago

just remembered you can not overload "=" operator in c#

Something still needs to be done to ensure consistency though

dwmkerr commented 9 years ago

Damn, this is a nasty issue. You are quite right - the matrix is a struct, and semantically a copy to a new variable should copy the struct. The issue is that it does copy the struct, however it copies the internally used reference to an array which holds the structs values as well, meaning although the struct is copied, and there are two distinct references, they point to the same array. I'll look into this as a matter of priority.

I can only see three ways around this issue:

  1. We understand this as a limitation of C#. We make all types ICloneable, and provide a type safe Clone method, updating the documentation as saying this is one of the things that you have to be aware of with GLM.NET vs GLM - things need to be cloned.
  2. We make the arrays fixed size. This means we get deep copy 'out of the box', however, every project needs to then set the unsafe flag, which seems pretty nasty.
  3. We make the matrices internally use 4, 9 or 16 actual double values (named a, b, c etc). This would lead to proper copies but would greatly increase the complexity of the code.

I'm leaning towards point 1, any thoughts?

chrispepper1989 commented 9 years ago

I think 1 is definitely the way to go, it seems to be C#s answer to overloading "=". This issue was really the first time I felt C# was a potential minefield when it comes to ref vs value. At least with C++ its really clear when something is a pointer :p

But, I wonder if there is a way to make this clearer in the API, either by making matrix a class, so its slightly clearer what "=" will do (but then I would recommend doing the same for vector), or blocking the "=" operator somehow.

A bit of googling shows how often people try to work around this in vain http://stackoverflow.com/questions/11336935/c-sharp-automatic-deep-copy-of-struct

Interesting limitation of the language

chrispepper1989 commented 9 years ago

Another thing that needs to be considered is "identity"

i.e.

myMat = mat4.identity();

At the moment that effectively changes the pointer of myMat to the pointer of "mat4.identity()" (i think)

Which if im not mistaken is a "new" so we don't change the identity matrix. But it does mean that if mat is left as a clonable reference users are going to experience inconsistent behavior e.g.:

mat4 camera;

mat4 GetCamera() { return camera; //users holding a ref to this would expect it to change on "UpdateCamera" }

UpdateCamera() { camera = mat4.identy() //now "camera" is pointing to something else //make changes to camera

//user with "camera" ref wonders why its not updated

}

dwmkerr commented 9 years ago

Yes it's certainly non-trivial to get this right, I think we'll have to document a consistent approach and describe it clearly in the readme as something which deviates from the behaviour of the C++ GLM