RH camera fixes (#16691)

This fixes an issue we have with cameras when in right-handed system
where setting `rotation` or `rotationQuaterion` on a camera in
right-handed system actually sets the camera 180 flipped from what is
represented in the view matrix.

You can see this problem using this PG:
https://playground.babylonjs.com/#BDQ72M#1
On line 14, we are setting the `rotation` to `0,0,0`, but the rotation
from the world matrix of the camera is `0,PI,0`.

This change fixes the problem, but it is a breaking change for setting
the rotation properties since it will behave 180 flipped from before.
The default orientation of camera will still work as before (i.e.,
always point to +Z for both LH and RH) to maintain backwards
compatibility as much as possible.

The fix involves changing the reference point of camera. The code
mistakenly uses the same reference point of `0,0,1` for both LH and RH
view matrix. Babylon target camera's LH and RH view matrices are flipped
by Z. Thus, the reference point should also flip by Z for the math to
work.

There is a secondary problem in `setTarget` where it is hard-coded to
use LH math. This has been fixed to consider RH.

Author: bghgary

packages/dev/core/src/Cameras/arcRotateCamera.ts

@@ -1394,7 +1394,7 @@ export class ArcRotateCamera extends TargetCamera {
             this._viewMatrix.addAtIndex(12, this.targetScreenOffset.x);
             this._viewMatrix.addAtIndex(13, this.targetScreenOffset.y);
         }
-        this._currentTarget = target;
+        this._currentTarget.copyFrom(target);
         return this._viewMatrix;
     }
 

packages/dev/core/src/Cameras/targetCamera.ts

@@ -12,6 +12,10 @@ Node.AddNodeConstructor("TargetCamera", (name, scene) => {
     return () => new TargetCamera(name, Vector3.Zero(), scene);
 });
 
+// Temporary cache variables to avoid allocations.
+const TmpMatrix = Matrix.Zero();
+const TmpQuaternion = Quaternion.Identity();
+
 /**
  * A target camera takes a mesh or position as a target and continues to look at it while it moves.
  * This is the base of the follow, arc rotate cameras and Free camera
@@ -36,13 +40,12 @@ export class TargetCamera extends Camera {
      */
     @serialize()
     public updateUpVectorFromRotation = false;
-    private _tmpQuaternion = new Quaternion();
 
     /**
      * Define the current rotation of the camera
      */
     @serializeAsVector3()
-    public rotation = new Vector3(0, 0, 0);
+    public rotation: Vector3;
 
     /**
      * Define the current rotation of the camera as a quaternion to prevent Gimbal lock
@@ -79,35 +82,27 @@ export class TargetCamera extends Camera {
     @serializeAsMeshReference("lockedTargetId")
     public lockedTarget: any = null;
 
-    /** @internal */
-    public _currentTarget = Vector3.Zero();
-    /** @internal */
-    public _initialFocalDistance = 1;
-    /** @internal */
-    public _viewMatrix = Matrix.Zero();
-    /** @internal */
-    public _camMatrix = Matrix.Zero();
-    /** @internal */
-    public _cameraTransformMatrix = Matrix.Zero();
-    /** @internal */
-    public _cameraRotationMatrix = Matrix.Zero();
+    protected readonly _currentTarget = Vector3.Zero();
+    protected _initialFocalDistance = 1;
+    protected readonly _viewMatrix = Matrix.Zero();
 
     /** @internal */
-    public _referencePoint = new Vector3(0, 0, 1);
+    public readonly _cameraTransformMatrix = Matrix.Zero();
     /** @internal */
-    public _transformedReferencePoint = Vector3.Zero();
+    public readonly _cameraRotationMatrix = Matrix.Zero();
+
+    protected readonly _referencePoint: Vector3;
+    protected readonly _transformedReferencePoint = Vector3.Zero();
 
-    protected _deferredPositionUpdate = new Vector3();
-    protected _deferredRotationQuaternionUpdate = new Quaternion();
-    protected _deferredRotationUpdate = new Vector3();
+    protected readonly _deferredPositionUpdate = new Vector3();
+    protected readonly _deferredRotationQuaternionUpdate = new Quaternion();
+    protected readonly _deferredRotationUpdate = new Vector3();
     protected _deferredUpdated = false;
     protected _deferOnly: boolean = false;
 
     /** @internal */
     public _reset: () => void;
 
-    private _defaultUp = Vector3.Up();
-
     /**
      * Instantiates a target camera that takes a mesh or position as a target and continues to look at it while it moves.
      * This is the base of the follow, arc rotate cameras and Free camera
@@ -119,6 +114,11 @@ export class TargetCamera extends Camera {
      */
     constructor(name: string, position: Vector3, scene?: Scene, setActiveOnSceneIfNoneActive = true) {
         super(name, position, scene, setActiveOnSceneIfNoneActive);
+
+        this._referencePoint = Vector3.Forward(this.getScene().useRightHandedSystem);
+
+        // Set the y component of the rotation to Math.PI in right-handed system for backwards compatibility.
+        this.rotation = new Vector3(0, this.getScene().useRightHandedSystem ? Math.PI : 0, 0);
     }
 
     /**
@@ -265,36 +265,17 @@ export class TargetCamera extends Camera {
 
         this._referencePoint.normalize().scaleInPlace(this._initialFocalDistance);
 
-        Matrix.LookAtLHToRef(this.position, target, this._defaultUp, this._camMatrix);
-        this._camMatrix.invert();
-
-        this.rotation.x = Math.atan(this._camMatrix.m[6] / this._camMatrix.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;
+        if (this.getScene().useRightHandedSystem) {
+            Matrix.LookAtRHToRef(this.position, target, Vector3.UpReadOnly, TmpMatrix);
         } else {
-            this.rotation.y = -Math.atan(vDir.z / vDir.x) - Math.PI / 2.0;
+            Matrix.LookAtLHToRef(this.position, target, Vector3.UpReadOnly, TmpMatrix);
         }
+        TmpMatrix.invert();
 
-        this.rotation.z = 0;
+        const rotationQuaternion = this.rotationQuaternion || TmpQuaternion;
+        Quaternion.FromRotationMatrixToRef(TmpMatrix, rotationQuaternion);
 
-        if (isNaN(this.rotation.x)) {
-            this.rotation.x = 0;
-        }
-
-        if (isNaN(this.rotation.y)) {
-            this.rotation.y = 0;
-        }
-
-        if (isNaN(this.rotation.z)) {
-            this.rotation.z = 0;
-        }
-
-        if (this.rotationQuaternion) {
-            Quaternion.RotationYawPitchRollToRef(this.rotation.y, this.rotation.x, this.rotation.z, this.rotationQuaternion);
-        }
+        rotationQuaternion.toEulerAnglesToRef(this.rotation);
     }
 
     /**
@@ -450,7 +431,7 @@ export class TargetCamera extends Camera {
      * @returns the current camera
      */
     private _rotateUpVectorWithCameraRotationMatrix(): TargetCamera {
-        Vector3.TransformNormalToRef(this._defaultUp, this._cameraRotationMatrix, this.upVector);
+        Vector3.TransformNormalToRef(Vector3.UpReadOnly, this._cameraRotationMatrix, this.upVector);
         return this;
     }
 
@@ -482,8 +463,8 @@ export class TargetCamera extends Camera {
             if (this.rotationQuaternion) {
                 Axis.Y.rotateByQuaternionToRef(this.rotationQuaternion, this.upVector);
             } else {
-                Quaternion.FromEulerVectorToRef(this.rotation, this._tmpQuaternion);
-                Axis.Y.rotateByQuaternionToRef(this._tmpQuaternion, this.upVector);
+                Quaternion.FromEulerVectorToRef(this.rotation, TmpQuaternion);
+                Axis.Y.rotateByQuaternionToRef(TmpQuaternion, this.upVector);
             }
         }
         this._computeViewMatrix(this.position, this._currentTarget, this.upVector);

packages/dev/loaders/src/glTF/2.0/glTFLoader.ts

@@ -1569,8 +1569,8 @@ export class GLTFLoader implements IGLTFLoader {
         this._babylonScene._blockEntityCollection = false;
         camera._babylonCamera = babylonCamera;
 
-        // Rotation by 180 as glTF has a different convention than Babylon.
-        babylonCamera.rotation.set(0, Math.PI, 0);
+        // glTF cameras look towards the local -Z axis.
+        babylonCamera.setTarget(new Vector3(0, 0, -1));
 
         switch (camera.type) {
             case CameraType.PERSPECTIVE: {

packages/tools/tests/test/visualization/config.json

@@ -1055,25 +1055,25 @@
         },
         {
             "title": "GLTF Serializer Camera, left-handed",
-            "playgroundId": "#O0M0J9#8",
+            "playgroundId": "#O0M0J9#20",
             "replace": "//options//, roundtripCount = 1; useRightHandedSystem = false;",
             "referenceImage": "glTFSerializerCameraLeftHand.png"
         },
         {
             "title": "GLTF Serializer Camera, right-handed",
-            "playgroundId": "#O0M0J9#8",
+            "playgroundId": "#O0M0J9#20",
             "replace": "//options//, roundtripCount = 1; useRightHandedSystem = true;",
             "referenceImage": "glTFSerializerCameraRightHand.png"
         },
         {
             "title": "GLTF Serializer Camera, left-handed, round trip twice",
-            "playgroundId": "#O0M0J9#8",
+            "playgroundId": "#O0M0J9#20",
             "replace": "//options//, roundtripCount = 2; useRightHandedSystem = false;",
             "referenceImage": "glTFSerializerCameraLeftHand.png"
         },
         {
             "title": "GLTF Serializer Camera, right-handed, round trip twice",
-            "playgroundId": "#O0M0J9#8",
+            "playgroundId": "#O0M0J9#20",
             "replace": "//options//, roundtripCount = 2; useRightHandedSystem = true;",
             "referenceImage": "glTFSerializerCameraRightHand.png"
         },