PrePointerObservable pointerTap fired after drag when skipOnPointerObservable true

Bugs
1

After this commit, prePointer pointerTap events are fired even after dragging, if a prePointer pointerMove observer has set skipOnPointerObservable to true. (PG)

To fix this I think these lines should be moved above the _checkPrePointerObservable call.

Another issue (though this is long standing) is that prePointer pointerUp observers aren’t called on pointerTap if skipOnPointerObservable has been set to true. The same is true for pointerDoubleTap. (PG)

These lines could be refactored to allow both up and tap/doubleTap events to be observed.

Finally, perhaps not an issue, but should pointerUp be called before pointerTap/pointerDoubleTap (for both prePointer and normal pointer observables)? For mouse events, the order is:

  • mousedown
  • mouseup
  • click
  • mousedown
  • mouseup
  • click
  • dblclick

For consistency, it might be nice to call the pointerEvent equivalent observers in a similar order (pointerDown → pointerUp → pointerTap).

I can do a pr for some or all of the above if required.

2

cc @PolygonalSun would be great to have a quick look

3

Lemme see what’s going on

1 Like
4

So I’ve been looking at what you’ve got here and here are my thoughts:

100% agree with your fix for this. I was initially worried that there may be side effects but there isn’t anything that stood out for me.

It does feel like this could be refactored to not have the POINTERTAP cancel/prevent the POINTERUP pre-check from occurring. I feel like this will also need to account for skipping the specific observable calls (TAP only skips TAP, UP only skips UP, etc)

TBH, I actually really like this, but this technically changes the expected behavior so I’d kinda wanna bring this to the team to see what their thoughts are on it.

I’ll try to get all of this addressed on Monday.

1 Like
5

Here’s an update:
So as far as I know, changing the order of the PointerEventTypes to more closely resemble the order of execution for PointerEvents isn’t going to be an issue. As far as the skipOnPointerObservable calls, the logic for this need to be that TAP can cancel TAP, and DOUBLETAP can cancel DOUBLETAP but UP cancels all three. This is due in part to backwards compatibility, at least as far as I understand. In any case, I’m currently testing the changes to make sure that there aren’t any side effects or issues that I’m not accounting for. I’ll keep you posted on the status of this.

6

Thanks for the update; sounds good.

By this do you just mean that the onPointerObservables that would be cancelled (i.e., _checkPrePointerObservable would be called for UP and TAP/DOUBLETAP even if one or both set skipOnPointerObservable to true)?

7

In the code found in the current release, what happens is that the _checkPrePointerObservable is call for TAP/DOUBLETAP, then UP. The first one to set skipOnPointerObservable to true will stop all up-input processing for both UP and TAP/DOUBLETAP.

These new changes would modify this by processing UP first, skipping everything if skipOnPointerObservable is true (similar to how it’s originally done). If TAP/DOUBLETAP set that flag then UP will still execute and the respective TAP will be skipped.

Here’s the PR: InputManager: Fix Order and Execution of onPrePointerObservable by PolygonalSun · Pull Request #13295 · BabylonJS/Babylon.js (github.com)

8

So as the PR currently stands, setting skipOnPointerObservable to true for a prePointerUp event also stops any prePointerTap or prePointerDoubleTap events from firing. This is probably fine, but it is a change in behaviour, and is a bit at odds with how skipOnPointerObservable is currently documented (which currently would imply that only the actual pointer observables are skipped, not any of the pre ones).

Otherwise looks all good.

9

Coming back to this @PolygonalSun, as I do I think it makes sense for the prePointerTap and prePointerDoubleTap events to fire, even if the pointerUp observable is skipped. This would mean that all prePointer events can be relied upon to always fire.

Changing these lines to something like the following would achieve this, while preserving the order of the events:

if (!clickInfo.ignore) {
    const skipUp = this._checkPrePointerObservable(null, evt, PointerEventTypes.POINTERUP);
    if (!clickInfo.hasSwiped) {
        if (clickInfo.singleClick && scene.onPrePointerObservable.hasSpecificMask(PointerEventTypes.POINTERTAP)) {
            if (this._checkPrePointerObservable(null, evt, PointerEventTypes.POINTERTAP)) {
                this._skipPointerTap = true;
            }
        }
        if (clickInfo.doubleClick && scene.onPrePointerObservable.hasSpecificMask(PointerEventTypes.POINTERDOUBLETAP)) {
            if (this._checkPrePointerObservable(null, evt, PointerEventTypes.POINTERDOUBLETAP)) {
                this._skipPointerTap = true;
            }
        }
    }
    if (skipUp) {
        // If we're skipping the next observable, we need to reset the swipe state before returning
        if (this._swipeButtonPressed === evt.button) {
            this._isSwiping = false;
            this._swipeButtonPressed = -1;
        }
        return;
    }
}
10
11

So, after talking with the team, the thoughts are that if a specific event’s Observable is determined to never fire before reaching its pre observable then the pre observable also shouldn’t fire.

12