Samsung / GearVRf

The GearVR framework(GearVRf) is an Open Source VR rendering library for application development on VR-supported Android devices.
http://www.gearvrf.org
Apache License 2.0
407 stars 217 forks source link

Possible error when setting model matrix #1956

Closed crbozz closed 6 years ago

crbozz commented 6 years ago

https://github.com/Samsung/GearVRf/blob/79c0f3e023d64f939ac4648be80934ffbabd6b17/GVRf/Framework/framework/src/main/jni/objects/components/transform.cpp#L151

I'm facing a problem when trying to set model matrix to a scene object and it seems the statement starting at the line above is the reason. When setting model matrix for a scene object (using setModelMatrix(Matrix4f mat)) position and scale are correct but rotation is not. I tried to calculate position, scale and rotation at Java side (using JOML) and then apply them (using Java transform API methods) and it worked fine. Then I tried to change the above statement as below and it also worked:

glm::mat3 rotation_mat(matrix[0][0] / new_scale.x, matrix[0][1] / new_scale.x, matrix[0][2] / new_scale.x, matrix[1][0] / new_scale.y, matrix[1][1] / new_scale.y, matrix[1][2] / new_scale.y, matrix[2][0] / new_scale.z, matrix[2][1] / new_scale.z, matrix[2][2] / new_scale.z);

Is this really and issue or maybe there is something else to consider when setting model matrix?

liaxim commented 6 years ago

Can you share code showing the problem? Could it be a row vs column major confusion somewhere in the code?

crbozz commented 6 years ago

The problem was detected at the new A.R. demo we are working on: https://github.com/sidia-dev-team/GearVRf-Demos/tree/feature_arpet

More specifically in the method starting here: https://github.com/sidia-dev-team/GearVRf-Demos/blob/83a0821c57f57e9e53df07532f65dc67a3b77c25/gvr-arpet/app/src/main/java/org/gearvrf/arpet/PlaneHandler.java#L58

I replaced the call to setModelMatrix by some other operations that I supposed should have same results.

I will make some changes to this demo so that you can easily visualize the problem.

crbozz commented 6 years ago

I just added some "visual debug" code to the demo. Just uncomment the lines starting here:

https://github.com/sidia-dev-team/GearVRf-Demos/blob/73303d58f89212681f3f1778cb0c496cda3d7de5/gvr-arpet/app/src/main/java/org/gearvrf/arpet/PlaneHandler.java#L74

It will make a box appear on the detected plane. Then you can switch to way this box transform is updated uncommenting the two lines starting here:

https://github.com/sidia-dev-team/GearVRf-Demos/blob/73303d58f89212681f3f1778cb0c496cda3d7de5/gvr-arpet/app/src/main/java/org/gearvrf/arpet/PlaneHandler.java#L107

and then commenting the lines starting here:

https://github.com/sidia-dev-team/GearVRf-Demos/blob/73303d58f89212681f3f1778cb0c496cda3d7de5/gvr-arpet/app/src/main/java/org/gearvrf/arpet/PlaneHandler.java#L111

crbozz commented 6 years ago

I also created a simpler application to demonstrate the problem:

https://github.com/crbozz/TransformTest

liaxim commented 6 years ago

It does look to be row-vector vs column-vector issue. Please check https://github.com/Samsung/GearVRf/pull/1969.

crbozz commented 6 years ago

I tested your fix and it worked fine!

liaxim commented 6 years ago

Fix has been merged