Fixes to scale and rotation in Matrix4f

Hey eveyone,

I think I came accross problems with scale and rotation in the Matrix4f class.
I found this while working on the blender importer when sometimes I had problems with nodes transformations :wink:

Consider this piece of code:
[java]
public class Transformations extends SimpleApplication {

@Override
public void simpleInitApp() {
	Geometry geometry = new Geometry("box", new Box(0.5f, 1f , 0.5f));
	Material defaultMaterial = new Material(assetManager, "Common/MatDefs/Misc/Unshaded.j3md");
	defaultMaterial.setColor("Color", ColorRGBA.DarkGray);
	geometry.setMaterial(defaultMaterial);
	
	Matrix4f m = new Matrix4f();

            //FIRST LINE TO UNCOMMENT
	//m.setRotationQuaternion(new Quaternion(new float[] {FastMath.HALF_PI, 0, 0}));

            //SECOND LINE TO UNCOMMENT
	//m.setScale(1, 0.5f, 1);
	System.out.println(m);
	
	Transform t = new Transform();
	t.setRotation(m.toRotationQuat());
	t.setTranslation(m.toTranslationVector());
	t.setScale(m.toScaleVector());
	
	geometry.setLocalTransform(t);
	
	rootNode.attachChild(geometry);
}

public static void main(String[] args){
    new Transformations().start();
}

}
[/java]

In a result you will see a box that has the height of 2. Quite simple :slight_smile:
Its transformation matrix is:

Matrix4f
[
1.0 0.0 0.0 0.0
0.0 1.0 0.0 0.0
0.0 0.0 1.0 0.0
0.0 0.0 0.0 1.0
]

Now uncomment the first line I pointed out. The result should be a ‘lying’ box.
Its transformation matrix:
Matrix4f
[
1.0 0.0 0.0 0.0
0.0 0.0 -1.0 0.0
0.0 1.0 0.0 0.0
0.0 0.0 0.0 1.0
]

And now uncomment the second line. I would expect that the box will shrink to the size of 1x1x1.
But instead, its size did not change. The transformation matrix is still:
Matrix4f
[
1.0 0.0 0.0 0.0
0.0 0.0 -1.0 0.0
0.0 1.0 0.0 0.0
0.0 0.0 0.0 1.0
]
even though we set the scale to [1, 0.5, 1].

The problem here is with this method in Matrix4f:
[java]
public void setScale(float x, float y, float z) {
m00 *= x;
m11 *= y;
m22 *= z;
}
[/java]
It works only when there is no rotation. The scale should affect the whole forward (F), up (U) and side (S) vectors of the matrix.
Which are the first, second and third columns of the matrix).

My suggestion is to fix it with these methods:
[java]
public void setScale(float x, float y, float z) {
Vector3f v = new Vector3f(m00, m10, m20);
v.normalizeLocal().multLocal(x);
m00 = v.x;
m10 = v.y;
m20 = v.z;

	v.set(m01, m11, m21);
	v.normalizeLocal().multLocal(y);
	m01 = v.x;
	m11 = v.y;
	m21 = v.z;
	
	v.set(m02, m12, m22);
	v.normalizeLocal().multLocal(z);
	m02 = v.x;
	m12 = v.y;
	m22 = v.z;
}

public void setScale(Vector3f scale) {
	this.setScale(scale.x, scale.y, scale.z);
}

[/java]

When you do that and run the program you will notice that the box had indeed shrinked but it is not properly rotated.
Even though its transformation matrix is now fine:
Matrix4f
[
1.0 0.0 0.0 0.0
0.0 0.0 -1.0 0.0
0.0 0.5 0.0 0.0
0.0 0.0 0.0 1.0
]

The problem is with extracting the rotation quaternion from the matrix.
The F, U and S vectors should first be normalized so that the quaternion is extracted without ‘scale noise’.
The patch for that is simple. In the Quaternion class, at the begining of the method:
[java]
public Quaternion fromRotationMatrix(float m00, float m01, float m02,
float m10, float m11, float m12,
float m20, float m21, float m22);
[/java]

add the following lines:
[java]
float lengthSquared = m00 * m00 + m10 * m10 + m20 * m20;
if (lengthSquared != 1f && lengthSquared != 0f){
lengthSquared = 1.0f / FastMath.sqrt(lengthSquared);
m00 *= lengthSquared;
m10 *= lengthSquared;
m20 *= lengthSquared;
}
lengthSquared = m01 * m01 + m11 * m11 + m21 * m21;
if (lengthSquared != 1f && lengthSquared != 0f){
lengthSquared = 1.0f / FastMath.sqrt(lengthSquared);
m01 *= lengthSquared;
m11 *= lengthSquared;
m21 *= lengthSquared;
}
lengthSquared = m02 * m02 + m12 * m12 + m22 * m22;
if (lengthSquared != 1f && lengthSquared != 0f){
lengthSquared = 1.0f / FastMath.sqrt(lengthSquared);
m02 *= lengthSquared;
m12 *= lengthSquared;
m22 *= lengthSquared;
}
[/java]

The code is identical to Vector3f.normalize() method. But I used the code to avoid instantiating additional vectors since this method might be called quite often.

If you run the code now, you will notice that the box is shrinked and properly rotated.

Let me know what do you think about those changes. I will do more testing with transformations but maybe someone will notice errors here :wink:

Cheers

2 Likes

You should collect statistics whether a length of 1f happens that often.
If it’s rare, I’d avoid that test, because then you’re speeding up a rare case and slowing down the common case.

Is a length of 0f even valid?
If no, I’d avoid that test and let the division-by-zero exception happen.

Unit tests would be essential, so that this bug cannot get back into the code base, but I guess you know that already :slight_smile:

<cite>@toolforger said:</cite> You should collect statistics whether a length of 1f happens that often.

Doesn’t a length of 1 = has been normalized? Seems sorta silly to gather statistics on something so common used.

Not statistics :slight_smile:

And yes it’s silly to do that twice, but this is about performance.
And comparing numbers is roughly as costly as multiplying them.
In fact if it’s compiled to machine code, the branched variant can be slower. The CPU is speculatively executing setup activity for the next few machine instructions (those multiply-and-add stuff) which it will throw away if the branch is taken; that throwing away can take more time than simply running through a single code path.

If you REALLY want to know, you need to benchmark :slight_smile:

@toolforger said: Not statistics :-)

And yes it’s silly to do that twice, but this is about performance.
And comparing numbers is roughly as costly as multiplying them.
In fact if it’s compiled to machine code, the branched variant can be slower. The CPU is speculatively executing setup activity for the next few machine instructions (those multiply-and-add stuff) which it will throw away if the branch is taken; that throwing away can take more time than simply running through a single code path.

If you REALLY want to know, you need to benchmark :slight_smile:

Comparing numbers is WAAAAAAAAAAAAAAAAAAAY cheaper than a sqrt().

Oh right, my bad. I stared at the addition/multiplication and completely overlooked the sqrt.
And I bet that sqrt has lots of comparisons and branches, too, so CPU pipeline stalls etc. will happen inside that, too. So I take that back and agree that the 1.0f comparison can hep.

Still, there’s the question how often a 1.0f is already present. Could be 99.999% or 0.001% of cases for all I know.

Those comparisons are also in Vector3f normalization methods so it might be worht rethinking their usage there as well.

But the comparison is the minor issue here. Tell me what you think about the patch itself :wink:
I will be back after the weekend and I will push the issue forward with more testing :wink:

The patch seems fine to me. The comparison is good for calling code that was already normalizing its data… or any transform matrix that wasn’t scaled, basically.

Well, since the code goes out of its way to optimize some things, I thought that the usual word of warning about optimization would be okay :slight_smile:

I don’t see anything wrong with the code in all, but I’m considerably weaker around quaternions than around Java in general, so I can’t comment on the validity of what the code is actually doing. I have some linear algebra background so I can agree that normalization should be okay :slight_smile:

IOW all I miss is the unit test.
Something that applies a rotation matrix of vectors of length >2.0f, and checks that vertices keep their distances.

@pspeed could it be the reason of the issues we have on non uniform scales?
@Kaelthas the patch looks ok to me, I would have use a tempVar for the setScale though instead of creating a new Vector3f. Arguably it won’t matter much on desktop, but it can on android.

@nehon said: @pspeed could it be the reason of the issues we have on non uniform scales? @Kaelthas the patch looks ok to me, I would have use a tempVar for the setScale though instead of creating a new Vector3f. Arguably it won't matter much on desktop, but it can on android.

Pretty sure it’s different… since that’s because Transform keeps them separate.

Though I do wonder if that will eventually confound things for Kaelthas… I don’t know how these matrix4fs are used so I can’t be sure.

OK, I’ll do some testing this week and if everything is fine I will prepare a version of code to commit and post it here :slight_smile:

@nehon OK I will use tempVars

1 Like

I found another issue with rotation/scale.
If in the example above I set rotation first and then the scale everything works fine.
But if I switch the order and set the scale and then the rotation - the scale is completely lost.

So my fix for that would be a modification of a method in the Quaternion class:
[java]
public Matrix4f toRotationMatrix(Matrix4f result) {
Vector3f originalScale = result.toScaleVector();// *** ADDED
result.setScale(1, 1, 1);// *** ADDED

    float norm = norm();
    // we explicitly test norm against one here, saving a division
    // at the cost of a test and branch.  Is it worth it?
    float s = (norm == 1f) ? 2f : (norm > 0f) ? 2f / norm : 0;

    // compute xs/ys/zs first to save 6 multiplications, since xs/ys/zs
    // will be used 2-4 times each.
    float xs = x * s;
    float ys = y * s;
    float zs = z * s;
    float xx = x * xs;
    float xy = x * ys;
    float xz = x * zs;
    float xw = w * xs;
    float yy = y * ys;
    float yz = y * zs;
    float yw = w * ys;
    float zz = z * zs;
    float zw = w * zs;

    // using s=2/norm (instead of 1/norm) saves 9 multiplications by 2 here
    result.m00 = 1 - (yy + zz);
    result.m01 = (xy - zw);
    result.m02 = (xz + yw);
    result.m10 = (xy + zw);
    result.m11 = 1 - (xx + zz);
    result.m12 = (yz - xw);
    result.m20 = (xz - yw);
    result.m21 = (yz + xw);
    result.m22 = 1 - (xx + yy);

    result.setScale(originalScale);// *** ADDED
    return result;
}

[/java]

The thing is that there is another method with Matrix3f as a parameter.
Matrix3f has no toScaleVector() method.
Shall I add it or rather compute it in the Quaternion’s method ??

Btw. why there is neither toRotationQuaternion() and toScaleVector() method in the Matrix3f class ??
The matrix stores this data so an easy access to it would be cool :wink:

is that non-uniform scale? IIRC momoko_fan said its not a bug.

Edit: I see others have mentioned it already, I might be thinking about something else

In general the scaling is non-uniform.
You do not need to have the objects scaled equally in all directions.

So I think that it is a bug when setting the rotation erases your scale setting.

I’d expect to get different results depending on whether the nonuniform scale or the rotation is done first, but I wouldn’t expect the scale to get ignored entirely.

If I had the methods in Matrix4f named like: ‘move’, ‘rotate’ and ‘scale’ then I would expect that the proper computations would be done step by step after calling each of these methods.
So in this case I agree that the order of calling them might matter.

But I always thought that when I have methods named like: ‘setTranslation’, ‘setRotation’ and ‘setScale’ then I treat the MAtrix4f as some kind of black-box and I only take care of the final result. And my object will be positioned in the place I pointed out, it will be rotated by exactly the angles I specified and will be scaled as I set it to be.

But if you guys prefer the first idea then I am OK with it and I will discard the last suggested change :wink:

@Kaelthas said: In general the scaling is non-uniform. You do not need to have the objects scaled equally in all directions.

So I think that it is a bug when setting the rotation erases your scale setting.

Yes… which actually has nothing to do with non-uniform scaling since even uniform scaling gets wiped out.

So how about these changes ??
Shall I commit the patch to the repo or not ?? :slight_smile:

Hmmm, looks like nobody entered here for some time :slight_smile:
@pspeed, @normen, @nehon

What about the patch ? I’d like to push some topics forward but this blocks me a little and I do not want to submit anything to the core without consulting it with you :wink: