From 34547caebf46b86fb27b181640aabdadde2faa00 Mon Sep 17 00:00:00 2001 From: Dave Solares <50599569+PolygonalSun@users.noreply.github.com> Date: Thu, 2 Mar 2023 15:00:13 -0800 Subject: [PATCH 1/3] Modified ExclusiveDoubleClickMode fix --- .../dev/core/src/Inputs/scene.inputManager.ts | 13 ++++++++++--- .../DeviceInput/babylon.inputManager.test.ts | 19 +++++++++++++++++-- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/packages/dev/core/src/Inputs/scene.inputManager.ts b/packages/dev/core/src/Inputs/scene.inputManager.ts index 8dc3795a650..5aeee740d33 100644 --- a/packages/dev/core/src/Inputs/scene.inputManager.ts +++ b/packages/dev/core/src/Inputs/scene.inputManager.ts @@ -60,7 +60,11 @@ export class InputManager { public static LongPressDelay = 500; // in milliseconds /** Time in milliseconds with two consecutive clicks will be considered as a double click */ public static DoubleClickDelay = 300; // in milliseconds - /** If you need to check double click without raising a single click at first click, enable this flag */ + /** + * This flag will modify the behavior so that, when true, a click will happen if and only if + * another click DOES NOT happen within the DoubleClickDelay time frame. If another click does + * happen within that time frame, the first click will not fire an event and and a double click will occur. + */ public static ExclusiveDoubleClickMode = false; /** This is a defensive check to not allow control attachment prior to an already active one. If already attached, previous control is unattached before attaching the new one. */ @@ -518,7 +522,7 @@ export class InputManager { if (!clickInfo.hasSwiped && !this._skipPointerTap && !this._isMultiTouchGesture) { let type = 0; - if (clickInfo.singleClick && !InputManager.ExclusiveDoubleClickMode) { + if (clickInfo.singleClick) { type = PointerEventTypes.POINTERTAP; } else if (clickInfo.doubleClick) { type = PointerEventTypes.POINTERDOUBLETAP; @@ -706,7 +710,10 @@ export class InputManager { } } - if (!needToIgnoreNext) { + // The follow callback should only be executed when needToIgnoreNext is false + // and when ExclusiveDoubleClickMode is false. This is because the + // callback is already executed in the double click case when true + if (!needToIgnoreNext && !InputManager.ExclusiveDoubleClickMode) { cb(clickInfo, this._currentPickResult); } }; diff --git a/packages/dev/core/test/unit/DeviceInput/babylon.inputManager.test.ts b/packages/dev/core/test/unit/DeviceInput/babylon.inputManager.test.ts index 05cbe50dabf..8d51af241c0 100644 --- a/packages/dev/core/test/unit/DeviceInput/babylon.inputManager.test.ts +++ b/packages/dev/core/test/unit/DeviceInput/babylon.inputManager.test.ts @@ -527,7 +527,7 @@ describe("InputManager", () => { expect(tapCt).toBe(1); }); - it("Doesn't fire onPointerOberservable for POINTERTAP when ExclusiveDoubleClickMode is enabled", () => { + it("Doesn't fire onPointerOberservable for POINTERTAP when ExclusiveDoubleClickMode is enabled", async () => { let tapCt = 0; let dblTapCt = 0; @@ -552,11 +552,26 @@ describe("InputManager", () => { deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 0); deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 1); deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 0); + + // Because the input manager uses the system clock, we need to use real timers + // and wait for the double click delay to pass so that we can work with a clean slate + jest.useRealTimers(); + await new Promise(resolve => setTimeout(resolve, InputManager.DoubleClickDelay + 1)); + + // Expect a single tap only + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 1); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 0); + + // Wait for the double click delay to pass again + await new Promise(resolve => setTimeout(resolve, InputManager.DoubleClickDelay + 1)); + + // Reset to fake timers + jest.useFakeTimers(); } // Since this is static, we should reset it to false for other tests InputManager.ExclusiveDoubleClickMode = false; - expect(tapCt).toBe(1); + expect(tapCt).toBe(2); expect(dblTapCt).toBe(2); }); From f1a53e72e98ff844b41da887ffbfe7c6e920173c Mon Sep 17 00:00:00 2001 From: Dave Solares <50599569+PolygonalSun@users.noreply.github.com> Date: Wed, 8 Mar 2023 15:04:22 -0800 Subject: [PATCH 2/3] New update --- .../dev/core/src/Inputs/scene.inputManager.ts | 85 +++++++++++++++---- .../DeviceInput/babylon.inputManager.test.ts | 51 +++++++++-- 2 files changed, 113 insertions(+), 23 deletions(-) diff --git a/packages/dev/core/src/Inputs/scene.inputManager.ts b/packages/dev/core/src/Inputs/scene.inputManager.ts index 5aeee740d33..c72165179c1 100644 --- a/packages/dev/core/src/Inputs/scene.inputManager.ts +++ b/packages/dev/core/src/Inputs/scene.inputManager.ts @@ -50,6 +50,13 @@ class _ClickInfo { } } +/** @internal */ +class _ClickEvent { + public clickInfo: _ClickInfo; + public evt: IPointerEvent; + public timeoutId: number; +} + /** * Class used to manage all inputs for the scene. */ @@ -84,8 +91,6 @@ export class InputManager { ) => void; private _initActionManager: (act: Nullable, clickInfo: _ClickInfo) => Nullable; private _delayedSimpleClick: (btn: number, clickInfo: _ClickInfo, cb: (clickInfo: _ClickInfo, pickResult: Nullable) => void) => void; - private _delayedSimpleClickTimeout: number; - private _previousDelayedSimpleClickTimeout: number; private _meshPickProceed = false; private _previousButtonPressed: number; @@ -115,6 +120,7 @@ export class InputManager { private _meshUnderPointerId: { [pointerId: number]: Nullable } = {}; private _movePointerInfo: Nullable = null; private _cameraObserverCount = 0; + private _delayedClicks: Array> = [null, null, null, null, null]; // Keyboard private _onKeyDown: (evt: IKeyboardEvent) => void; @@ -593,7 +599,19 @@ export class InputManager { this._doubleClickOccured = false; clickInfo.singleClick = true; clickInfo.ignore = false; - cb(clickInfo, this._currentPickResult); + + // If we have a delayed click, we need to resolve the TAP event + if (this._delayedClicks[btn]) { + const evt = this._delayedClicks[btn]!.evt; + const type = PointerEventTypes.POINTERTAP; + const pi = new PointerInfo(type, evt, this._currentPickResult); + if (scene.onPointerObservable.hasObservers() && scene.onPointerObservable.hasSpecificMask(type)) { + scene.onPointerObservable.notifyObservers(pi, type); + } + + // Clear the delayed click + this._delayedClicks[btn] = null; + } } }; @@ -651,9 +669,12 @@ export class InputManager { } // at least one double click is required to be check and exclusive double click is enabled else { - // wait that no double click has been raised during the double click delay - this._previousDelayedSimpleClickTimeout = this._delayedSimpleClickTimeout; - this._delayedSimpleClickTimeout = window.setTimeout(this._delayedSimpleClick.bind(this, btn, clickInfo, cb), InputManager.DoubleClickDelay); + // Queue up a delayed click, just in case this isn't a double click + const delayedClick = new _ClickEvent(); + delayedClick.evt = evt; + delayedClick.clickInfo = clickInfo; + delayedClick.timeoutId = window.setTimeout(this._delayedSimpleClick.bind(this, btn, clickInfo, cb), InputManager.DoubleClickDelay); + this._delayedClicks[btn] = delayedClick; } let checkDoubleClick = obs1.hasSpecificMask(PointerEventTypes.POINTERDOUBLETAP) || obs2.hasSpecificMask(PointerEventTypes.POINTERDOUBLETAP); @@ -672,10 +693,12 @@ export class InputManager { this._doubleClickOccured = true; clickInfo.doubleClick = true; clickInfo.ignore = false; - if (InputManager.ExclusiveDoubleClickMode && this._previousDelayedSimpleClickTimeout) { - clearTimeout(this._previousDelayedSimpleClickTimeout); + // If we have a pending click, we need to cancel it + if (InputManager.ExclusiveDoubleClickMode && this._delayedClicks[btn]) { + clearTimeout(this._delayedClicks[btn]?.timeoutId); + this._delayedClicks[btn] = null; } - this._previousDelayedSimpleClickTimeout = this._delayedSimpleClickTimeout; + cb(clickInfo, this._currentPickResult); } // if the two successive clicks are too far, it's just two simple clicks @@ -686,11 +709,11 @@ export class InputManager { this._previousStartingPointerPosition.y = this._startingPointerPosition.y; this._previousButtonPressed = btn; if (InputManager.ExclusiveDoubleClickMode) { - if (this._previousDelayedSimpleClickTimeout) { - clearTimeout(this._previousDelayedSimpleClickTimeout); + // If we have a delayed click, we need to cancel it + if (this._delayedClicks[btn]) { + clearTimeout(this._delayedClicks[btn]?.timeoutId); + this._delayedClicks[btn] = null; } - this._previousDelayedSimpleClickTimeout = this._delayedSimpleClickTimeout; - cb(clickInfo, this._previousPickResult); } else { cb(clickInfo, this._currentPickResult); @@ -710,10 +733,9 @@ export class InputManager { } } - // The follow callback should only be executed when needToIgnoreNext is false - // and when ExclusiveDoubleClickMode is false. This is because the - // callback is already executed in the double click case when true - if (!needToIgnoreNext && !InputManager.ExclusiveDoubleClickMode) { + // Even if ExclusiveDoubleClickMode is true, we need to always handle + // up events at time of execution, unless we're explicitly ignoring them. + if (!needToIgnoreNext) { cb(clickInfo, this._currentPickResult); } }; @@ -777,6 +799,35 @@ export class InputManager { (evt as any).pointerId = 0; } + // If ExclusiveDoubleClickMode is true, we need to resolve any pending delayed clicks + if (InputManager.ExclusiveDoubleClickMode) { + for (let i = 0; i < this._delayedClicks.length; i++) { + if (this._delayedClicks[i]) { + // If the button that was pressed is the same as the one that was released, + // just clear the timer. This will be resolved in the up event. + if (evt.button === i) { + clearTimeout(this._delayedClicks[i]?.timeoutId); + } else { + // Otherwise, we need to resolve the click + const clickInfo = this._delayedClicks[i]!.clickInfo; + this._doubleClickOccured = false; + clickInfo.singleClick = true; + clickInfo.ignore = false; + + const prevEvt = this._delayedClicks[i]!.evt; + const type = PointerEventTypes.POINTERTAP; + const pi = new PointerInfo(type, prevEvt, this._currentPickResult); + if (scene.onPointerObservable.hasObservers() && scene.onPointerObservable.hasSpecificMask(type)) { + scene.onPointerObservable.notifyObservers(pi, type); + } + + // Clear the delayed click + this._delayedClicks[i] = null; + } + } + } + } + this._updatePointerPosition(evt); if (this._swipeButtonPressed === -1) { diff --git a/packages/dev/core/test/unit/DeviceInput/babylon.inputManager.test.ts b/packages/dev/core/test/unit/DeviceInput/babylon.inputManager.test.ts index 8d51af241c0..3277d1d8059 100644 --- a/packages/dev/core/test/unit/DeviceInput/babylon.inputManager.test.ts +++ b/packages/dev/core/test/unit/DeviceInput/babylon.inputManager.test.ts @@ -429,7 +429,7 @@ describe("InputManager", () => { it("Does not fire POINTERTAP events during multi-touch gesture", () => { let tapCt = 0; - scene?.onPointerObservable.add((eventData) => { + scene?.onPointerObservable.add(() => { tapCt++; }, PointerEventTypes.POINTERTAP); @@ -556,23 +556,62 @@ describe("InputManager", () => { // Because the input manager uses the system clock, we need to use real timers // and wait for the double click delay to pass so that we can work with a clean slate jest.useRealTimers(); - await new Promise(resolve => setTimeout(resolve, InputManager.DoubleClickDelay + 1)); + await new Promise((resolve) => setTimeout(resolve, InputManager.DoubleClickDelay + 1)); // Expect a single tap only deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 1); deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 0); // Wait for the double click delay to pass again - await new Promise(resolve => setTimeout(resolve, InputManager.DoubleClickDelay + 1)); - + await new Promise((resolve) => setTimeout(resolve, InputManager.DoubleClickDelay + 1)); + + // Expect two single taps + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 1); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 0); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.RightClick, 1); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.RightClick, 0); + + await new Promise((resolve) => setTimeout(resolve, InputManager.DoubleClickDelay + 1)); + + // Double click, immediately followed by a single click, should still fire a double click and a single click + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 1); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 0); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 1); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 0); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.RightClick, 1); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.RightClick, 0); + + await new Promise((resolve) => setTimeout(resolve, InputManager.DoubleClickDelay + 1)); + + // Single click, immediately followed by a double click, should still fire a single click and a double click + // With no additional clicks + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.RightClick, 1); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.RightClick, 0); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 1); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 0); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 1); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 0); + + await new Promise((resolve) => setTimeout(resolve, InputManager.DoubleClickDelay + 1)); + + // Three single clicks alternating between left and right + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 1); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 0); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.RightClick, 1); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.RightClick, 0); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 1); + deviceInputSystem.changeInput(DeviceType.Mouse, 0, PointerInput.LeftClick, 0); + + await new Promise((resolve) => setTimeout(resolve, InputManager.DoubleClickDelay + 1)); + // Reset to fake timers jest.useFakeTimers(); } // Since this is static, we should reset it to false for other tests InputManager.ExclusiveDoubleClickMode = false; - expect(tapCt).toBe(2); - expect(dblTapCt).toBe(2); + expect(tapCt).toBe(9); + expect(dblTapCt).toBe(4); }); it("can fire onViewMatrixObservable on camera.update", () => { From 5dfe7b24f65f10e38eee640b4cfe63a45c2ba0d6 Mon Sep 17 00:00:00 2001 From: Dave Solares <50599569+PolygonalSun@users.noreply.github.com> Date: Thu, 9 Mar 2023 14:37:22 -0800 Subject: [PATCH 3/3] PR feedback --- .../dev/core/src/Inputs/scene.inputManager.ts | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/packages/dev/core/src/Inputs/scene.inputManager.ts b/packages/dev/core/src/Inputs/scene.inputManager.ts index c72165179c1..2587365ba35 100644 --- a/packages/dev/core/src/Inputs/scene.inputManager.ts +++ b/packages/dev/core/src/Inputs/scene.inputManager.ts @@ -51,10 +51,10 @@ class _ClickInfo { } /** @internal */ -class _ClickEvent { - public clickInfo: _ClickInfo; - public evt: IPointerEvent; - public timeoutId: number; +interface _IClickEvent { + clickInfo: _ClickInfo; + evt: IPointerEvent; + timeoutId: number; } /** @@ -120,7 +120,7 @@ export class InputManager { private _meshUnderPointerId: { [pointerId: number]: Nullable } = {}; private _movePointerInfo: Nullable = null; private _cameraObserverCount = 0; - private _delayedClicks: Array> = [null, null, null, null, null]; + private _delayedClicks: Array> = [null, null, null, null, null]; // Keyboard private _onKeyDown: (evt: IKeyboardEvent) => void; @@ -670,10 +670,15 @@ export class InputManager { // at least one double click is required to be check and exclusive double click is enabled else { // Queue up a delayed click, just in case this isn't a double click - const delayedClick = new _ClickEvent(); - delayedClick.evt = evt; - delayedClick.clickInfo = clickInfo; - delayedClick.timeoutId = window.setTimeout(this._delayedSimpleClick.bind(this, btn, clickInfo, cb), InputManager.DoubleClickDelay); + // It should be noted that while this delayed event happens + // because of user input, it shouldn't be considered as a direct, + // timing-dependent result of that input. It's meant to just fire the TAP event + const delayedClick = { + evt: evt, + clickInfo: clickInfo, + timeoutId: window.setTimeout(this._delayedSimpleClick.bind(this, btn, clickInfo, cb), InputManager.DoubleClickDelay), + }; + this._delayedClicks[btn] = delayedClick; }