flyx / OpenGLAda

Thick Ada binding for OpenGL and GLFW
flyx.github.io/OpenGLAda/
MIT License
95 stars 13 forks source link

Translation Problem With Vertex Shader #59

Closed rogermc2 closed 7 years ago

rogermc2 commented 7 years ago

square_test_minimal.zip I have run into a problem involving the drawing of vertical square (plane) using a translation transformation (MV) followed by an orthographic transformation (P). (MVP = P * MV) The problem is that the translated square is rendered as a trapezoid instead of a square. When translating along the x axis, the vertical sides of the rendered object are parallel as expected but, in the horizontal direction, the vertical size decreases from left to right. This occurs when the matrix multiplication is done in the Ada program and also when it is done in the shader program. However, when done in the shader program the effect is worse. I have checked, using Mathematica, that the MVP matrix generated should transform a square to a square. The MVP matrix generated by the Ada program also seems correct has it has the scaling factors for both X and Y are equal. No problem occurs when rotating the square using a rotation transformation followed by an orthographic projection. I also checked that the scaling transformation works correctly. Also if I change the square's coordinates to the required translated coordinates then perform only an orthographic projection the square renders correctly. It is as if the MVP matrix is getting corrupted in being provided to the shader or that the shader operation is faulty. This seems unlikely as so many other operations occur correctly. A minimal version of my program is #attached. square_test_minimal.zip

rogermc2 commented 7 years ago

A further test: Perform translation in the Ada code then supply the translated array ( T V) and orthographic transformation P to the shader (actually (P I)). This generates a correct result: A correctly translated square. So (P I) ( T V) works, whereas (P T I) V fails, producing a trapezoid instead of a square. where: P is the orthographic matrix T is the transformation matrix I is the identity matrix V is the original vertex array This involved quite a lot of translation back and forth between 3 space and 4 space, despite which a correct result was obtained. The minimal code is no longer minimal and I remain baffled.

rogermc2 commented 7 years ago

I have now realized that the trapezium appears when the matrix sent to the shader comprises only the translation matrix. So, the shader seems to render T * V as a trapezium, instead of a square, clipped appropriately. In its simplest form: MVP_Matrix := Maths.Translation_Matrix ((1.0, 0.0, 0.0)); The attachment png file shows the result with the X translation set to 1. clipped_trapezium

flyx commented 7 years ago

fyi, I am currently busy planning to move houses, so I do not have time to help you.

However, I would suggest not to experiment with the visual result, but directly look at the resulting calculated matrix. Did you compare the result of (P * I) * ( T * V) with the one of (P * T * I) * V? If the former one works, but the latter doesn't, this suggests a numerical difference between those values, and that is probably where the error lies.

rogermc2 commented 7 years ago

Thanks for your suggestions. Good luck with your housemove. Response appreciated at whatever your convenience. Actually, I am doing all this debugging using vector and matrix results. I only included the graphic in case my description of the end result wasn't clear. In all cases, so far, the matrices appear correct. I have also been doing the computations in Mathematica to ensure correct results. The only thing I haven't been able to do is debug the shader as I have no means or idea of how to.

rogermc2 commented 7 years ago

Please regard the preceding as commentary only. I now believe that the problem can be investigated using only translation matrices without the need to involve projection matrices and am modifying my program accordingly. I appreciate that you have higher priority responsibilities; in particular, house moving.

rogermc2 commented 7 years ago

I have compared the result of (P I) ( T V) with the one of (P T I) V. They are the same and neither work.

I have done some tracking of:

MVP_Location           : GL.Uniforms.Uniform;
MVP_Matrix             : GL.Types.Singles.Matrix4;
GL.Uniforms.Set_Single (MVP_Location, MVP_Matrix); 

GL.Uniforms.Set_Single  (Location : Uniform; Value : Singles.Matrix4 calls 
     API.Singles.Uniform_Matrix4 (Location, 1, Low_Level.False, (1 => Value)); 

The problem is solved by setting (assuming it doesn't cause other problems)

API.Singles.Uniform_Matrix4 (Location, 1, Low_Level.TRUE, (1 => Value));

This is because the GLSL shader language declares its matrices as the transpose of the OpenGL matrix structure.

From https://www.khronos.org/opengl/wiki/Data_Type_(GLSL)#Matrix_constructors:

mat4(
  vec4,           //first column
  vec4,           //second column
  vec4,           //third column
  vec4);          //fourth column

whereas in all the OpenGL documentation that I have come across, the corresponding declaration would be:

mat4(
  vec4,           //first row
  vec4,           //second row
  vec4,           //third row
  vec4);          //fourth row

For example : https://open.gl/transformations I tried coding the translation matrix in the vertex shader which is how I first came across this difference in matrix construction.

In the meantime, I can solve the problem by using use GL.Types.Singles.Transpose is my matrix calls to GL.Uniforms.Set_Single.

rogermc2 commented 7 years ago

Using use GL.Types.Singles.Transpose has solved other problems in this and other examples so it looks like it is the right fix.

flyx commented 7 years ago

Good catch! I think the correct fix for this should be to change OpenGLAda's handling of matrices to match that of GLSL, i.e. regarding them as column-major array rather than as row-major array (as it is implemented currently). So, A(1, 3) should mean the value in column 1, row 3 rather than the value in row 1, column 3.

Things that need to be fixed are mainly the vector multiplication. I think most of the other operations are orthogonal.

rogermc2 commented 7 years ago

That makes sense to me. As this would be internal to OpenGLAda, I imagine that the fact that most texts use row-major array shouldn't bother the end user? I presume that the end user will multiply matrices generated by OpenGLAda without knowledge of their actual structure.

flyx commented 7 years ago

Please report back if my fix actually fixes the problem.

rogermc2 commented 7 years ago

Spinning_Cubes etc. still need Transpose. I've updated my master branch with yours. Do I need to wait for the quaternions PR to be completed before testing?

flyx commented 7 years ago

No, the quaternions PR is orthogonal to this.

Can you check whether the results from the matrix / vector multiplication differ now compared to before? How do you generate the matrices? Perhaps the generation code needs to be fixed now?

flyx commented 7 years ago

You need to fix the matrix generation functions in Maths package, they all have the wrong index order now.

rogermc2 commented 7 years ago

Would it be OK to fix by transposing in the matrix generation functions in Maths package? This would then keep the code consistent with documentation.

flyx commented 7 years ago

I would rather have the examples show the „correct“ way of indexing a matrix, since this is something that will lead to programming errors. What documentation are you referring to?

(Reopening issue until fixed in examples)

rogermc2 commented 7 years ago

I'm talking about the OpenGL textbooks that I use and, from memory, OpenGL documentation on the web. OK I'll change it.

flyx commented 7 years ago

Well, from an Ada perspective, the matrix type is now defined as being column-major. If you start filling it using row-major semantics, you change semantics and should therefore use a different type. It is very bad style to use the same type with different semantics (even if you „correct“ them afterwards with Transpose).

I think if we provide the matrix generation functions currently in Maths as stock functionality, the user will seldom access the matrix manually and thus it won't be a problem. On the long run, OpenGLAda having the same semantics as GLSL may lessen the confusion. Or it may not. Since consequences are hard to predict, I want to go with being consistent with GLSL rather than being consistent with textbooks targeting C – since the connection to GLSL is tighter than that to C.

rogermc2 commented 7 years ago

OK. I'm making the changes now.

rogermc2 commented 7 years ago

I'm having trouble with rotations about X and Y axes. I've gone back to just transposing the maths.matrices to check it out. I plan to change to the column major version once I find out what's wrong with the rotations.

rogermc2 commented 7 years ago

The product of the transpose of two matrices A and B is given by T(A)T(B) = T(BA). So, I have to multiply in reverse order: T(B)T(A) to get T(AB). I hope this won't be confusing for users?

rogermc2 commented 7 years ago

Implemented, tested and pushed. Hopefully can close this now.

flyx commented 7 years ago

Closing this since it seems to be fixed with recent PR.