Change mouse button config to a function (e: Event) => ACTION

[jamesnakagawa]

Regarding #458,

I propose a new structure for configuring mouse buttons. Instead of a configuration object, user can pass a configuration function which receives an Event (either wheel event, click event, or contextmenu event) and which returns the intended action they want. With this, user can access event.shiftKey and all other modifier key flags and reliably set them without overly verbose configuration.

I have also updated the example at examples/mouse-drag-with-modifier-keys.html to show how it could be used.

Lastly, I wrote a custom setter for the previous configuration, mouseButtons, so that this is not a breaking change.

I'm not familiar with the coding conventions of this project, so please excuse if I didn't document something correctly. I look forward to hearing your feedback on my proposal!

[jamesnakagawa]

I was able to simplify this conditional because of this change.

( event.buttons & MOUSE_BUTTON.ALL ) > 0 looks a little weird, and technically it would exclude event.buttons === 8 || event.buttons === 16, which is not a common use case. I wanted the result of the function to match previous behaviour exactly.

[jamesnakagawa]

I do not use MOUSE_BUTTON.ALL here because having ! this._lockedPointer && changes the logic.

[jamesnakagawa]

@yomotsu Just wondering if you had a chance to see this?

[yomotsu]

Sorry for the delay, I've been busy with house moving.
And thank you for the PR.

This contains a lot change than I thought...and going to be a breaking change (no compatibility with older versions).
Let me confirm a little bit:

• How do we handle if the user presses with "A" key or others, like, rotate for the R key, translate for the T key, etc. This could be multiple keys at the same time. (Therefore, the demo code was handling additional keys without MouseEvents)

• Camera Controls accepts touch events via Pointer Events too. Can mouseEventToAction handle any pointer events?

[jamesnakagawa]

引っ越しおめでとう！

Thank you for the response.

1. If I understand correctly, using "R" for rotate, "T" for transform, means that pressing once would change the mode but it does not need to be held down?

In that case, it could be done using closure:

let mode = 'rotate';

document.addEventListener('keypress', e => {
    if (e.key === 'R' || e.key === 'r') {
        mode = 'rotate';
    } else if (e.key === 'T' || e.key === 't') {
        mode = 'translate';
    }
});

cameraControls.mouseEventToAction = event => {
    if ( ( event.buttons & MOUSE_BUTTON.LEFT ) === MOUSE_BUTTON.LEFT ) {
        if (mode === 'translate') {
            return ACTION.TRUCK;
        }
        if (mode === 'rotate') {
            return ACTION.ROTATE;
        }
        // etc
    }
};

2. For PointerEvent, it subclasses MouseEvent and properly sets event.buttons and event.shiftKey/metaKey/etc, so I see no incompatibility here.

3. Re: breaking change. Now that I think about this more, you are right, you can't set deep values using this compatibility bridge and turning mouseEventToAction back into mouseButtons is quite complex. In any case, probably not worth it.

Could this wait for the next major version of camera-controls?

Otherwise, the alternative would be to extend mouseButtons like this:

this.mouseButtons = {
	left: ACTION.ROTATE,
	middle: ACTION.DOLLY,
	right: ACTION.TRUCK,
	wheel: ACTION.DOLLY,
	'left.ctrl': ACTION.TRUCK,
	'left.shift': ACTION.TRUCK,
	'left.meta': ACTION.DOLLY,
	'left.ctrl.shift': null,
	'left.ctrl.shift.meta': ACTION.DOLLY,
};

Where a null/undefined value would indicate to inherit from the base setting. i.e 'left.ctrl.shift': null would inherit from 'left.ctrl': ACTION.TRUCK instead.

This would be much more verbose, which is why I think you didn't want this feature in the first place.

[jamesnakagawa]

@yomotsu Any thoughts on this? I'm happy to do another PR or make changes based on the direction you'd like to go.

[yomotsu]

Sorry again for the delay.
Could this be a none break change PR?
I mean,

• It's okay to add the interrupting feature as an advanced usage.
• However, I also would like to keep the simple setting feature.

Also, what do you think about the following:

Renaming MOUSE_BUTTON.ALL to MOUSE_BUTTON.ANY.
Renaming mouseEventToAction to pointerEventToAction and can handle other pointer events such as touch and pen.

[jamesnakagawa]

We could do it that way, yes. If pointerEventToAction == null or undefined, then use mouseButtons, otherwise mouseButtons will be ignored. Is that what you were thinking?