Fix unwanted roll #5083 (#5092)

* add failing unit test to demonstrate #5083 * make unit tests actually fail when run as part of test suite. * fix #5083. * fix lint * fix lint * fix accidentally deleted code and improve unit test coverage * test coverage * Update src/geo/projection/camera_helper.ts * make setRollEnabled private and @internal * make setRollEnabled protected since it is accessed from Map * move setRollEnabled out of Camera * disable SLERP when startRoll != endRoll, instead of based on rollEnabled. * PR feedback: define a type for updateRotation * move quaternion calculationinto updateRotation * move comments --------- Co-authored-by: Harel M <harel.mazor@gmail.com>
maplibre · Nov 27, 2024 · 86ea129 · 86ea129
1 parent 985b657
commit 86ea129
Show file tree

Hide file tree

Showing 6 changed files with 131 additions and 34 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,7 @@
 
 - ⚠️ Change drag rotate behavior to be less abrupt around the center ([#5104](https://github.com/maplibre/maplibre-gl-js/pull/5104))
 - Fix regression in render world copies ([#5101](https://github.com/maplibre/maplibre-gl-js/pull/5101))
+- Fix unwanted roll when motion is interrupted ([#5083](https://github.com/maplibre/maplibre-gl-js/pull/5083))
 - _...Add new stuff here..._
 
 ## 5.0.0-pre.8

diff --git a/src/geo/projection/camera_helper.ts b/src/geo/projection/camera_helper.ts
@@ -4,8 +4,9 @@ import {type LngLat, type LngLatLike} from '../lng_lat';
 import {type CameraForBoundsOptions, type PointLike} from '../../ui/camera';
 import {type PaddingOptions} from '../edge_insets';
 import {type LngLatBounds} from '../lng_lat_bounds';
-import {getRollPitchBearing, type RollPitchBearing, warnOnce} from '../../util/util';
+import {getRollPitchBearing, type RollPitchBearing, rollPitchBearingToQuat, warnOnce} from '../../util/util';
 import {quat} from 'gl-matrix';
+import {interpolates} from '@maplibre/maplibre-gl-style-spec';
 
 export type MapControlsDeltas = {
     panDelta: Point;
@@ -61,6 +62,33 @@ export type FlyToHandlerResult = {
     pixelPathLength: number;
 }
 
+export type UpdateRotationArgs = {
+    /**
+     * The starting Euler angles.
+     */
+    startEulerAngles: RollPitchBearing;
+
+    /**
+     * The end Euler angles.
+     */
+    endEulerAngles: RollPitchBearing;
+
+    /**
+     * The transform to be updated
+     */
+    tr: ITransform;
+
+    /**
+     * The interpolation fraction, between 0 and 1.
+     */
+    k: number;
+
+    /**
+     * If true, use spherical linear interpolation. If false, use linear interpolation of Euler angles.
+     */
+    useSlerp: boolean;
+}
+
 /**
  * @internal
  */
@@ -97,26 +125,29 @@ export interface ICameraHelper {
 
 /**
  * @internal
- * Set a transform's rotation to a value interpolated between startRotation and endRotation
- * @param startRotation - the starting rotation (rotation when k = 0)
- * @param endRotation - the end rotation (rotation when k = 1)
- * @param endEulerAngles - the end Euler angles. This is needed in case `endRotation` has an ambiguous Euler angle representation.
- * @param tr - the transform to be updated
- * @param k - the interpolation fraction, between 0 and 1.
+ * Set a transform's rotation to a value interpolated between startEulerAngles and endEulerAngles
  */
-export function updateRotation(startRotation: quat, endRotation: quat, endEulerAngles: RollPitchBearing, tr: ITransform, k: number) {
-    // At pitch ==0, the Euler angle representation is ambiguous. In this case, set the Euler angles
-    // to the representation requested by the caller
-    if (k < 1) {
-        const rotation: quat = new Float64Array(4) as any;
-        quat.slerp(rotation, startRotation, endRotation, k);
-        const eulerAngles = getRollPitchBearing(rotation);
-        tr.setRoll(eulerAngles.roll);
-        tr.setPitch(eulerAngles.pitch);
-        tr.setBearing(eulerAngles.bearing);
+export function updateRotation(args: UpdateRotationArgs) {
+    if (args.useSlerp) {
+        // At pitch ==0, the Euler angle representation is ambiguous. In this case, set the Euler angles
+        // to the representation requested by the caller
+        if (args.k < 1) {
+            const startRotation = rollPitchBearingToQuat(args.startEulerAngles.roll, args.startEulerAngles.pitch, args.startEulerAngles.bearing);
+            const endRotation = rollPitchBearingToQuat(args.endEulerAngles.roll, args.endEulerAngles.pitch, args.endEulerAngles.bearing);
+            const rotation: quat = new Float64Array(4) as any;
+            quat.slerp(rotation, startRotation, endRotation, args.k);
+            const eulerAngles = getRollPitchBearing(rotation);
+            args.tr.setRoll(eulerAngles.roll);
+            args.tr.setPitch(eulerAngles.pitch);
+            args.tr.setBearing(eulerAngles.bearing);
+        } else {
+            args.tr.setRoll(args.endEulerAngles.roll);
+            args.tr.setPitch(args.endEulerAngles.pitch);
+            args.tr.setBearing(args.endEulerAngles.bearing);
+        }
     } else {
-        tr.setRoll(endEulerAngles.roll);
-        tr.setPitch(endEulerAngles.pitch);
-        tr.setBearing(endEulerAngles.bearing);
+        args.tr.setRoll(interpolates.number(args.startEulerAngles.roll, args.endEulerAngles.roll, args.k));
+        args.tr.setPitch(interpolates.number(args.startEulerAngles.pitch, args.endEulerAngles.pitch, args.k));
+        args.tr.setBearing(interpolates.number(args.startEulerAngles.bearing, args.endEulerAngles.bearing, args.k));
     }
 }
diff --git a/src/geo/projection/globe_camera_helper.ts b/src/geo/projection/globe_camera_helper.ts
@@ -1,12 +1,12 @@
 import Point from '@mapbox/point-geometry';
 import {type IReadonlyTransform, type ITransform} from '../transform_interface';
-import {cameraBoundsWarning, type CameraForBoxAndBearingHandlerResult, type EaseToHandlerResult, type EaseToHandlerOptions, type FlyToHandlerResult, type FlyToHandlerOptions, type ICameraHelper, type MapControlsDeltas, updateRotation} from './camera_helper';
+import {cameraBoundsWarning, type CameraForBoxAndBearingHandlerResult, type EaseToHandlerResult, type EaseToHandlerOptions, type FlyToHandlerResult, type FlyToHandlerOptions, type ICameraHelper, type MapControlsDeltas, updateRotation, type UpdateRotationArgs} from './camera_helper';
 import {type GlobeProjection} from './globe';
 import {LngLat, type LngLatLike} from '../lng_lat';
 import {MercatorCameraHelper} from './mercator_camera_helper';
 import {angularCoordinatesToSurfaceVector, computeGlobePanCenter, getGlobeRadiusPixels, getZoomAdjustment, globeDistanceOfLocationsPixels, interpolateLngLatForGlobe} from './globe_utils';
-import {clamp, createVec3f64, differenceOfAnglesDegrees, remapSaturate, rollPitchBearingToQuat, warnOnce} from '../../util/util';
-import {type mat4, quat, vec3} from 'gl-matrix';
+import {clamp, createVec3f64, differenceOfAnglesDegrees, remapSaturate, rollPitchBearingEqual, warnOnce} from '../../util/util';
+import {type mat4, vec3} from 'gl-matrix';
 import {MAX_VALID_LATITUDE, normalizeCenter, scaleZoom, zoomScale} from '../transform_helper';
 import {type CameraForBoundsOptions} from '../../ui/camera';
 import {type LngLatBounds} from '../lng_lat_bounds';
@@ -262,11 +262,11 @@ export class GlobeCameraHelper implements ICameraHelper {
 
         const startZoom = tr.zoom;
         const startCenter = tr.center;
-        const startRotation = rollPitchBearingToQuat(tr.roll, tr.pitch, tr.bearing);
+        const startEulerAngles = {roll: tr.roll, pitch: tr.pitch, bearing: tr.bearing};
         const endRoll = options.roll === undefined ? tr.roll : options.roll;
         const endPitch = options.pitch === undefined ? tr.pitch : options.pitch;
         const endBearing = options.bearing === undefined ? tr.bearing : options.bearing;
-        const endRotation = rollPitchBearingToQuat(endRoll, endPitch, endBearing);
+        const endEulerAngles = {roll: endRoll, pitch: endPitch, bearing: endBearing};
 
         const optionsZoom = typeof options.zoom !== 'undefined';
 
@@ -317,8 +317,13 @@ export class GlobeCameraHelper implements ICameraHelper {
         isZooming = (endZoomWithShift !== startZoom);
 
         const easeFunc = (k: number) => {
-            if (!quat.equals(startRotation, endRotation)) {
-                updateRotation(startRotation, endRotation, {roll: endRoll, pitch: endPitch, bearing: endBearing}, tr, k);
+            if (!rollPitchBearingEqual(startEulerAngles, endEulerAngles)) {
+                updateRotation({
+                    startEulerAngles,
+                    endEulerAngles,
+                    tr,
+                    k,
+                    useSlerp: startEulerAngles.roll != endEulerAngles.roll} as UpdateRotationArgs);
             }
 
             if (options.around) {

diff --git a/src/geo/projection/mercator_camera_helper.ts b/src/geo/projection/mercator_camera_helper.ts
@@ -1,15 +1,14 @@
 import Point from '@mapbox/point-geometry';
 import {LngLat, type LngLatLike} from '../lng_lat';
 import {type IReadonlyTransform, type ITransform} from '../transform_interface';
-import {cameraBoundsWarning, type CameraForBoxAndBearingHandlerResult, type EaseToHandlerResult, type EaseToHandlerOptions, type FlyToHandlerResult, type FlyToHandlerOptions, type ICameraHelper, type MapControlsDeltas, updateRotation} from './camera_helper';
+import {cameraBoundsWarning, type CameraForBoxAndBearingHandlerResult, type EaseToHandlerResult, type EaseToHandlerOptions, type FlyToHandlerResult, type FlyToHandlerOptions, type ICameraHelper, type MapControlsDeltas, updateRotation, type UpdateRotationArgs} from './camera_helper';
 import {type CameraForBoundsOptions} from '../../ui/camera';
 import {type PaddingOptions} from '../edge_insets';
 import {type LngLatBounds} from '../lng_lat_bounds';
 import {normalizeCenter, scaleZoom, zoomScale} from '../transform_helper';
-import {degreesToRadians, rollPitchBearingToQuat} from '../../util/util';
+import {degreesToRadians, rollPitchBearingEqual} from '../../util/util';
 import {projectToWorldCoordinates, unprojectFromWorldCoordinates} from './mercator_utils';
 import {interpolates} from '@maplibre/maplibre-gl-style-spec';
-import {quat} from 'gl-matrix';
 
 /**
  * @internal
@@ -128,11 +127,11 @@ export class MercatorCameraHelper implements ICameraHelper {
     handleEaseTo(tr: ITransform, options: EaseToHandlerOptions): EaseToHandlerResult {
         const startZoom = tr.zoom;
         const startPadding = tr.padding;
-        const startRotation = rollPitchBearingToQuat(tr.roll, tr.pitch, tr.bearing);
+        const startEulerAngles = {roll: tr.roll, pitch: tr.pitch, bearing: tr.bearing};
         const endRoll = options.roll === undefined ? tr.roll : options.roll;
         const endPitch = options.pitch === undefined ? tr.pitch : options.pitch;
         const endBearing = options.bearing === undefined ? tr.bearing : options.bearing;
-        const endRotation = rollPitchBearingToQuat(endRoll, endPitch, endBearing);
+        const endEulerAngles = {roll: endRoll, pitch: endPitch, bearing: endBearing};
 
         const optionsZoom = typeof options.zoom !== 'undefined';
 
@@ -160,8 +159,13 @@ export class MercatorCameraHelper implements ICameraHelper {
             if (isZooming) {
                 tr.setZoom(interpolates.number(startZoom, endZoom, k));
             }
-            if (!quat.equals(startRotation, endRotation)) {
-                updateRotation(startRotation, endRotation, {roll: endRoll, pitch: endPitch, bearing: endBearing}, tr, k);
+            if (!rollPitchBearingEqual(startEulerAngles, endEulerAngles)) {
+                updateRotation({
+                    startEulerAngles,
+                    endEulerAngles,
+                    tr,
+                    k,
+                    useSlerp: startEulerAngles.roll != endEulerAngles.roll} as UpdateRotationArgs);
             }
             if (doPadding) {
                 tr.interpolatePadding(startPadding, options.padding, k);

diff --git a/src/ui/camera.test.ts b/src/ui/camera.test.ts
@@ -1536,6 +1536,58 @@ describe('#flyTo', () => {
         expect(fixedLngLat(camera.getCenter())).toEqual({lng: 100, lat: 0});
     });
 
+    test('no roll when motion is interrupted', () => {
+        const stub = vi.spyOn(browser, 'now');
+
+        const camera = createCamera();
+        stub.mockImplementation(() => 0);
+        camera.easeTo({pitch: 10, bearing: 100, duration: 1000});
+        stub.mockImplementation(() => 100);
+        camera.simulateFrame();
+        camera.easeTo({elevation: 1, duration: 0});
+        expect(camera.getRoll()).toBe(0);
+    });
+
+    test('no roll when motion is interrupted: globe', () => {
+        const stub = vi.spyOn(browser, 'now');
+
+        const camera = createCameraGlobe();
+        stub.mockImplementation(() => 0);
+        camera.easeTo({pitch: 10, bearing: 100, duration: 1000});
+        stub.mockImplementation(() => 100);
+        camera.simulateFrame();
+        camera.easeTo({elevation: 1, duration: 0});
+        expect(camera.getRoll()).toBe(0);
+    });
+
+    test('angles when motion is interrupted', () => {
+        const stub = vi.spyOn(browser, 'now');
+
+        const camera = createCamera();
+        stub.mockImplementation(() => 0);
+        camera.easeTo({pitch: 10, bearing: 20, roll: 30, duration: 1000});
+        stub.mockImplementation(() => 500);
+        camera.simulateFrame();
+        camera.easeTo({elevation: 1, duration: 0});
+        expect(camera.getRoll()).toBeCloseTo(25.041890412598942);
+        expect(camera.getPitch()).toBeCloseTo(8.116189398053095);
+        expect(camera.getBearing()).toBeCloseTo(15.041890412599061);
+    });
+
+    test('angles when motion is interrupted: globe', () => {
+        const stub = vi.spyOn(browser, 'now');
+
+        const camera = createCameraGlobe();
+        stub.mockImplementation(() => 0);
+        camera.easeTo({pitch: 10, bearing: 20, roll: 30, duration: 1000});
+        stub.mockImplementation(() => 500);
+        camera.simulateFrame();
+        camera.easeTo({elevation: 1, duration: 0});
+        expect(camera.getRoll()).toBeCloseTo(25.041890412598942);
+        expect(camera.getPitch()).toBeCloseTo(8.116189398053095);
+        expect(camera.getBearing()).toBeCloseTo(15.041890412599061);
+    });
+
     test('can be called from within a moveend event handler', async () => {
         const camera = createCamera();
         const stub = vi.spyOn(browser, 'now');

diff --git a/src/util/util.ts b/src/util/util.ts
@@ -916,6 +916,10 @@ export type RollPitchBearing = {
     bearing: number;
 };
 
+export function rollPitchBearingEqual(a: RollPitchBearing, b: RollPitchBearing): boolean {
+    return a.roll == b.roll && a.pitch == b.pitch && a.bearing == b.bearing;
+}
+
 /**
  * This method converts a rotation quaternion to roll, pitch, and bearing angles in degrees.
  * @param rotation - The rotation quaternion