Breaking change in target camera rotations when using right-handed system

Breaking Changes
1

Starting with version 8.10.1, any code that modifies the rotation or rotationQuaternion property of a target camera when the scene is in right-handed system (i.e., scene.useRightHandedSystem = true) will behave exactly 180° flipped on the Y axis than before. This is due to a bug with the target camera code that fails to take the handedness of the scene into account. See RH camera fixes by bghgary · Pull Request #16691 · BabylonJS/Babylon.js for details.

For additional back story, we ran into this problem because we were trying to fix a camera issue with the glTF exporter. After a lot of investigation from both @alexchuber and myself, we discovered this problem in the code.

Please shout if this causes trouble for your scenarios.

10 Likes
2

thanks for the note @bghgary

2 Likes
4

Hello!

I may have found a slight vulnerability in this change; sadly, I cannot reproduce it in any playground, but in the previous version, this.rotation.z was forced to 0, which was a good thing because in the new version, rotationQuaternion.toEulerAnglesToRef(this.rotation); might not return a perfect 0 (which is what is happening in my project), triggering an update to the upVector in the getViewMatrix function as expected.

This means that every time I update the target — i.e. every time I move — the upVector gets updated.

The easiest fix is to add these lines at the end of setTarget:

this.rotation.z = 0;
if (this.rotationQuaternion) {
  Quaternion.FromEulerVectorToRef(this.rotation, this.rotationQuaternion);
}

For now, the workaround is to add these lines to my project:

const originalSetTarget = TargetCamera.prototype.setTarget;
// FIXME: Temporary override to fix roll being edited during setTarget...
TargetCamera.prototype.setTarget = function (target: Vector3): void {
  originalSetTarget.call(this, target);
  this.rotation.z = 0;
  if (this.rotationQuaternion) Quaternion.FromEulerVectorToRef(this.rotation, this.rotationQuaternion);
};

:innocent:

Edit: You could also replace the above line to always use a TmpQuaternion.
From line 275, it would look like something like this:

Quaternion.FromRotationMatrixToRef(TmpMatrix, TmpQuaternion);
TmpQuaternion.toEulerAnglesToRef(this.rotation);

this.rotation.z = 0;
if (this.rotationQuaternion) {
  Quaternion.FromEulerVectorToRef(this.rotation, this.rotationQuaternion);
}

or even crazier:

Quaternion.FromRotationMatrixToRef(TmpMatrix, TmpQuaternion).toEulerAnglesToRef(this.rotation);

this.rotation.z = 0; // ensure the roll remains unchanged
if (this.rotationQuaternion) {
  Quaternion.FromEulerVectorToRef(this.rotation, this.rotationQuaternion);
}

On a side note, I absolutely love all these high-level functions! <3

3 Likes
5

Yup, this is already fixed. See Camera is getting a Z rotation? - Questions - Babylon.js.

1 Like
6

Wow nice!
But I see in the fix that you are not editing this.rotationQuaternion after setting this.rotation.z = 0.

From what I read in _getViewMatrix, you might add:

if (this._rotationQuaternion) this._rotationQuaternion.z = 0;

To avoid having the update of the upVector when using the rotationQuaternion.

This solution is better than the one I proposed earlier, which again uses Quaternion.FromEulerVectorToRef, and both guarantee that rotation and rotationQuaternion remain “parallel”.

7

Hmm, I don’t think we can do this since this will change the rotation in many cases. TBH, I have no idea what the code is doing in _getViewMatrix checking just z for the quaternion. I’m not sure what that even means.

Also, my fix is just a patch to preserve the original behavior. There is most likely another issue with how the up vector is being computed. Before I set rotation.z to zero, it is already very close to zero. This should not cause the camera rotation to go crazy, but because of that check in _getViewMatrix, z has to be exactly 0 for the code to behave the same as before.

8

Yeah you’re right… the code in getViewMatrix might be incorrect, as z in a quaternion is not z in Euler (I think…)

In the previous version, the quaternion was set after the rotation.z being set 0.
So, this:
if (this.rotationQuaternion) {
Quaternion.FromEulerVectorToRef(this.rotation, this.rotationQuaternion);
}

Truly ensure the same behavior as before… not that it would make much difference…

I’ll update my project to 8.12.1 asap, I guess I can remove my workaround now!!!

Thank you :slight_smile:

1 Like
9

Yeah, I saw that from your original message. It shouldn’t make a difference because floating point is going to be imprecise anyways. Having a z value close to zero vs zero should result in a similar quaternion. My change computes the quaternion first, so I can’t just set the Euler z component to zero first like before.

1 Like
10

Hey!!!
It’s me again with a new problem. Please tell me it hasn’t been fixed in version 8.16.0, as I’m using version 8.15.1… or please tell me it has :sweat_smile:.

For context, I use my own camera, as an extend of the Universal Camera. It behaves like the Google Maps camera. To achieve this, I move it by editing the target and position. That’s why I had issues in the first place, because I use setTarget a lot.

In a certain context, I position the camera at the perfect vertical angle. From this position, I cannot tilt the camera any more, meaning that I cannot edit the rotation.x using setTarget.

After some research, I found that this is caused by:
rotationQuaternion.toEulerAnglesToRef(this.rotation);. Somehow, my angle triggers this:

 const zAxisY = rotationQuaternion._y * rotationQuaternion._z - rotationQuaternion._x * rotationQuaternion._w;
const limit = 0.4999999;
if (zAxisY < -limit) {

We fall into the first if condition, zAxisY < -limit, and it locks x at Math.PI / 2. However, this may be a side effect of Quaternion.FromRotationMatrixToRef(TmpMatrix, rotationQuaternion). My maths knowledge is a little rusty, but I don’t think so.

For now, the only workaround I have found is to override setTarget and replace this part with the old one, so it looks something like this:

  public override setTarget(target: Vector3): void {
    this.upVector.normalize();
    this._initialFocalDistance = target.subtract(this.position).length();
    if (this.position.z === target.z) {
      this.position.z += Epsilon;
    }

    this._referencePoint.normalize().scaleInPlace(this._initialFocalDistance);

    if (this.getScene().useRightHandedSystem) {
      Matrix.LookAtRHToRef(this.position, target, Vector3.UpReadOnly, TmpMatrix);
    } else {
      Matrix.LookAtLHToRef(this.position, target, Vector3.UpReadOnly, TmpMatrix);
    }
    TmpMatrix.invert();

    this.rotation.x = Math.atan(TmpMatrix.m[6] / TmpMatrix.m[10]);

    const vDir = target.subtract(this.position);

    if (vDir.x >= 0.0) {
      this.rotation.y = -Math.atan(vDir.z / vDir.x) + Math.PI / 2.0;
    } else {
      this.rotation.y = -Math.atan(vDir.z / vDir.x) - Math.PI / 2.0;
    }

    this.rotation.z = 0;

    if (isNaN(this.rotation.x)) {
      this.rotation.x = 0;
    }

    if (isNaN(this.rotation.y)) {
      this.rotation.y = 0;
    }

    if (this.rotationQuaternion) {
      Quaternion.RotationYawPitchRollToRef(this.rotation.y, this.rotation.x, this.rotation.z, this.rotationQuaternion);
    }
  }

The first part remains the same; I just replaced the rotation computation with the old one. I’m not sure if this breaks the fix for the RH problem.
However, having read your first message in the Git conversation, and given that I am still using the Matrix.LookAtRHToRef function, the fix might still be there.

I am yet to reproduce it in a playground…

1 Like
11

We need this in a playground, or it will be hard to diagnose the issue.

2 Likes