jMonkeyEngine / jmonkeyengine

A complete 3-D game development suite written in Java.
http://jmonkeyengine.org
BSD 3-Clause "New" or "Revised" License
3.84k stars 1.13k forks source link

scale factors assumed positive in com.jme3.math #2089

Closed stephengold closed 11 months ago

stephengold commented 1 year ago

com.jme3.math breaks in many places if scale factors are allowed to be negative. For instance, Matrix4f.toScaleVector(Vector3f) assigns a (positive) square root to each component:

https://github.com/jMonkeyEngine/jmonkeyengine/blob/a94c37aa732b387c153d6c113e37155c0f40f320/jme3-core/src/main/java/com/jme3/math/Matrix4f.java#L1861-L1873

As a consequence of this bug (or undocumented assumption) the zero-argument Matrix4f.oScaleVector() method is also broken, as is Transform.fromTransformMatrix(Matrix4f).

Here is a code fragment that demonstrates those breakages:

        Vector3f transA = new Vector3f(1f, 2f, 3f);
        Quaternion rotA = new Quaternion(0.8f, 0.4f, 0.2f, 0.4f);
        Vector3f scaleA = new Vector3f(4f, -5f, -6f);
        Transform a = new Transform(transA, rotA, scaleA);
        Matrix4f matA = a.toTransformMatrix();

        Transform b = new Transform();
        b.fromTransformMatrix(matA);
        System.out.println("a: " + a);
        System.out.println("b: " + b);
        System.out.println();

        Vector3f scaleB = matA.toScaleVector();
        System.out.println("scaleA: " + scaleA);
        System.out.println("scaleB: " + scaleB);
        System.out.println();

If this issue were solved, the transforms a and b would be approximately equal, as would the vectors scaleA and scaleB. Instead, we get:

a: Transform[ 1.0, 2.0, 3.0]
[ 0.8, 0.4, 0.2, 0.4]
[ 4.0 , -5.0, -6.0]
b: Transform[ 1.0, 2.0, 3.0]
[ -0.40000004, -0.20000003, 0.40000004, 0.8]
[ 4.0 , 5.0, 5.9999995]

scaleA: (4.0, -5.0, -6.0)
scaleB: (4.0, 5.0, 5.9999995)

There's a similar assumption of positive scale factors in Quaternion.fromRotationMatrix(), which rejects scaling but only if all the scale factors are positive.

Directly or indirectly, JMonkeyEngine uses these methods in many places, notably Transform.invert(), which is similarly broken, as demonstrated by the following code fragment:

        Vector3f transA = new Vector3f(1f, 2f, 3f);
        Quaternion rotA = new Quaternion(0.8f, 0.4f, 0.2f, 0.4f);
        Vector3f scaleA = new Vector3f(4f, -5f, -6f);
        Transform a = new Transform(transA, rotA, scaleA);

        Transform aInverse = a.invert();
        Transform c = aInverse.invert();
        System.out.println("a: " + a);
        System.out.println("c: " + c);
        System.out.println();

If this issue were solved, the transforms a and c would be approximately equal. Instead, we get:

a: Transform[ 1.0, 2.0, 3.0]
[ 0.8, 0.4, 0.2, 0.4]
[ 4.0 , -5.0, -6.0]
c: Transform[ 1.74527, 2.0037012, 2.7823966]
[ -0.3705214, -0.15285265, 0.4035354, 0.8135669]
[ 4.5550847 , 5.043545, 4.949222]

If a matrix contains an odd number of negative scale factors, the fact can be detected using determinants. However, I don't know an easy way to detect an even number of negative scale factors.

pspeed42 commented 1 year ago

Given how limited the support for scaling is in JME, I suggest just marking this as a limitation.

I know the source of your discovery comes from interpreting scales from loaded models but we already don't support a variety of scaling. (For example, because JME doesn't really support non-uniform scaling anywhere except at the leaf Geometry.) So even if we were to somehow fix this issue in Matrix, it's not really going to solve the model interpretation issue which is effectively impossible.

stephengold commented 1 year ago

An even number of -1 scaling factors is equivalent to a rotation.

Using that insight, here's a workaround for Transform.fromTransformMatrix(Matrix4f):

        Transform result = new Transform();
        float determinant = matrix4f.determinant();
        if (determinant < 0f) {
            matrix4f.m00 *= -1f;
            matrix4f.m10 *= -1f;
            matrix4f.m20 *= -1f;
        }
        result.fromTransformMatrix(matrix4f);
        if (determinant < 0f) {
            result.getScale().x *= -1f;
        }
zzuegg commented 1 year ago

An even number of -1 scaling factors is equivalent to a rotation.

Math is not my strong point, but would a scaling factor of -1 not only rotate the model, but turn it inside out?

stephengold commented 1 year ago

An even number of -1 scaling factors is equivalent to a rotation.

Math is not my strong point, but would a scaling factor of -1 not only rotate the model, but turn it inside out?

You're right. It would turn a model inside-out. That would be something for the caller to address, if such a transform were ever applied to a mesh or model.

However, com.jme3.math doesn't deal with meshes or models, so Transform and Matrix4f don't deal with the concept of inside-out.