8000 matrix decompose(): fix bug by devernay · Pull Request #1046 · g-truc/glm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

matrix decompose(): fix bug #1046

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

matrix decompose(): fix bug #1046

wants to merge 1 commit into from

Conversation

devernay
Copy link
@devernay devernay commented Dec 2, 2020

Here is a test case (in Python, sorry) that proves that glm.decompose() has a bug.
It builds a simple transformation that contains a rotation built from a known quaternion, and prints the output quaternion: it's wrong.
If I use quat_cast instead, I get the right result.

# Here is a proof that glm's decompose has a bug.
import numpy as np
import glm
rotation = np.random.random((4,))
rotation /= np.linalg.norm(rotation)
rotation = glm.quat(rotation)
print(f'rotation               ={rotation} len={np.linalg.norm(rotation)}') # glm bug: outputs a non-normalized rotation
rotmat = glm.mat3_cast(rotation)
transformation = np.eye(4)
transformation[:3,:3] = np.array(rotmat)
transformation = glm.mat4(transformation)
rotation_out = glm.quat()
glm.decompose(transformation, scale, rotation_out, translation, skew, perspective)
print(f'rotation_from_decompose={rotation_out} len={np.linalg.norm(rotation_out)}') # glm bug: outputs a non-normalized rotation
print(f'rotation_from_quat_cast={glm.quat_cast(rotmat)} len={np.linalg.norm(glm.quat_cast(rotmat))}') # glm bug: outputs a non-normalized rotation

sample output:

rotation               =quat(     0.433091,     0.745813,    0.0454693,     0.504111 ) len=1.0
rotation_from_decompose=quat(     0.433091,    0.0454692,     0.504111,            0 ) len=0.6661555171012878
rotation_from_quat_cast=quat(     0.433091,     0.745813,    0.0454692,     0.504111 ) len=1.0

Looking at the source code, it seems to take inspiration from several sources:

Except that when the code was copied: the following trick was ommited:

Real* apfQuat[3] = { &m_afTuple[1], &m_afTuple[2], &m_afTuple[3] };

so that index 0 in apfQuat is actually index 1 in m_afTuple. In the code, i j and k refer to some ordering of x,y and z, which have indices 1 2 and 3 in the quaternion.

A simple fix is to add 1 to i, j and k:

			Orientation[i + 1] = static_cast<T>(0.5) * root;
			root = static_cast<T>(0.5) / root;
			Orientation[j + 1] = root * (Row[i][j] + Row[j][i]);
			Orientation[k + 1] = root * (Row[i][k] + Row[k][i]);

A better fix would be to replace this duplicated functionality with a call to quad_cast which does the job without a bug.

@thekend
Copy link 845B
thekend commented Mar 6, 2021

It's related to #1012

@stephomi stephomi mentioned this pull request Jun 1, 2021
Merged
@devernay devernay closed this Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@devernay @thekend
0