adamlwgriffiths / Pyrr

3D mathematical functions using NumPy
Other
401 stars 54 forks source link

Matrix44 multiplication -> projection * lookat #51

Closed szabolcsdombi closed 7 years ago

szabolcsdombi commented 7 years ago

I am using pyrr to create a projection and modelview matrix.

Why do I have to transpose the matrices before multiplication?

mvp = perspective.transpose() * lookat.transpose()

The code below contains an additional transpose() described above.

perspective = Matrix44.perspective_projection(45.0, 1.0, 0.1, 1000.0)
lookat = Matrix44.look_at(
    (-85, -180, 140),
    (0.0, 0.0, 65.0),
    (0.0, 0.0, 1.0),
)

mvp = (perspective.transpose() * lookat.transpose()).transpose()
szabolcsdombi commented 7 years ago

The entire code can be found here

adamlwgriffiths commented 7 years ago

=/ There was a fix in #42 that may be relevant. Perhaps both look_at and *_projection have a similar issue.

The matrix shouldn't have to be transposed for OpenGL, since the matricies are already in that format AFAIK.

Here's code from the last time I was using OpenGL I don' think the fixes in #42 would invalidate this code, since it was matrix / quaternion conversions. https://github.com/adamlwgriffiths/OMGL/blob/master/examples/mesh.py

szabolcsdombi commented 7 years ago

These are the same

mvp = lookat * perspective
mvp = (perspective.transpose() * lookat.transpose()).transpose()

Either the multiplication or the matrices need to be changed. what do you think?

adamlwgriffiths commented 7 years ago

The multiplication just uses numpy.dot underneath, so the multiplication should be correct.

I think what's happened is the ordering in the object is logically backwards. Because matrix*.multiply is a proxy to numpy.dot, you normally would do mv = np.dot(model_view, projection). As seen in this old code in PyGLy

self._matrix = matrix44.multiply(
                    self._transform.matrix,
                    self.parent.matrix
                    )

This seems to have been forgotten and the objects are multiplying as multiply(parent, self) rather than multiply(self, parent).

<object/matrix33.py>
    @dispatch((BaseMatrix, np.ndarray, list))
    def __mul__(self, other):
        return Matrix33(matrix33.multiply(self, Matrix33(other)))

You proved this by reversing the order of your multiplications.

adamlwgriffiths commented 7 years ago

Yep, the tests are backwards =/

    def test_multiply(self):
        m1 = Matrix33(np.arange(self._size))
        m2 = Matrix33(np.arange(self._size)[::-1])
        m = m1 * m2
        self.assertTrue(np.array_equal(m, matrix33.multiply(m1, m2)))

it should be

m1 * m2 == matrix33.multiply(m2, m1)
adamlwgriffiths commented 7 years ago

Pushed fixes and released as 0.8.4. Please confirm with your code that it's working. I can only apologise for the oversight.

Again, thank you very much for pointing these issues out!

szabolcsdombi commented 7 years ago

Thank you for your help and for the release! For my examples I will use pyrr from now on.

Headless rendering works fine with:

perspective = Matrix44.perspective_projection(45.0, 1.0, 0.1, 1000.0)

lookat = Matrix44.look_at(
    (-85, -180, 140),
    (0.0, 0.0, 65.0),
    (0.0, 0.0, 1.0),
)

mvp = perspective * lookat

prog.uniforms['Mvp'].write(mvp.astype('float32').tobytes())

Here is the code

adamlwgriffiths commented 7 years ago

Glad you've found value in the library =)

francoisruty commented 5 years ago

@adamlwgriffiths Hi, first of all many thanks for this great work! I generate an OpenGL projection matrix (it's correct, I checked it by other means) and upload it to the shader via: prog['Mvp'].write(projection_matrix.astype('float32').tobytes())

I noticed that I have to transpose the projection_matrix for it to work properly with modernGL, is that normal?

thanks!

adamlwgriffiths commented 5 years ago

This question is better asked of @cprogrammer1994

szabolcsdombi commented 5 years ago

Hi @francoisruty,

the Uniform.write method was designed to act similar to uniform buffer writes, data is copied byte by byte.

https://www.khronos.org/opengl/wiki/Interface_Block_(GLSL)

in GLSL the default layout of a matrix is column_major

if you have to transpose the matrix before passing it, the matrix must have a row_major layout on client side.

francoisruty commented 5 years ago

thx for the answer, just wanted to make sure it wasn't a bug It might be a good idea to put comments regarding this matter next to "prog['Mvp'].write" occurrences in source code examples I can do it with a PR if you want

adamlwgriffiths commented 5 years ago

This sounds a bit suspicious, the np arrays should be directly copy-able to GL/SL for use without transposition.

szabolcsdombi commented 5 years ago

@adamlwgriffiths

len(Matrix44.perspective_projection(60.0, 1.0, 0.1, 1000.0).tobytes()) # 128
 len(Matrix44.perspective_projection(60.0, 1.0, 0.1, 1000.0).astype('f4').tobytes()) # 64 (floats, not doubles)

To get the proper binary format we have to add .astype('f4').tobytes() at the end..

some older numpy version exports a buffer with some padded bytes when accessing it through the BufferProtocol - this never happens with tobytes() whose size is perfectly defined :)

to get moderngl examples working safely, I have added .astype('f4').tobytes() at the end.

uniform setters also work with an sequence of float like objects (only flattened arrays work) -- but this one is way slower than using the buffer protocol.

adamlwgriffiths commented 5 years ago

Well python floats are 64-bit for the most part, so the 32-bit conversion is expected. I'm just concerned that the transpose indicates the projection matrices aren't in the right format?