Fix/chorded button event ordering#3420
Fix/chorded button event ordering#3420sudomakeinstall wants to merge 4 commits intoKitware:masterfrom
Conversation
|
@finetjul the code looks reasonable but it would be great if you could test it. Thx |
finetjul
left a comment
There was a problem hiding this comment.
It works fine ! It seems to also coexist well with touch/gesture. (did not thoroughly tested though).
I think you could factorize by having a unique function called by handlePointerUp, handlePointerCancel and handlePointerMove
e.g.
handleChordedButtons(previous, current){
// Process Release events before Press events ????
if (previous ^ current & previous & 1) {
leftButtonReleaseEvent();
}
if (previous ^ current & previous & 2) {
rightButtonReleaseEvent();
}
...
if (previous ^ current & current & 1) {
leftButtonPressEvent();
}
...
If handleMouseUp() must really be kept in handlePointerUp(), you could do:
handlePointerUp () {
...
const ignoreButton = event.button == 0 ? ~1 : event.button == 1 ? ~4 : ~2;
handleChordedButtons(previousMouseButtons & ignoreButton, event.buttons & ignoreButton);
handleMouseUp();
previousMouseButtons = event.buttons;
}
| if (!isMiddle && wasMiddle && event.button !== 1) | ||
| publicAPI.middleButtonReleaseEvent(callData); | ||
| if (!isRight && wasRight && event.button !== 2) | ||
| publicAPI.rightButtonReleaseEvent(callData); |
There was a problem hiding this comment.
what if a button was pressed in the meantime ?
| const isRight = (currentButtons & 2) !== 0; // eslint-disable-line no-bitwise | ||
| const isMiddle = (currentButtons & 4) !== 0; // eslint-disable-line no-bitwise | ||
| // Only fire for chorded buttons; the primary button (event.button) | ||
| // is handled by handleMouseUp below. |
There was a problem hiding this comment.
any reason to keep handleMouseUp call ?
|
@finetjul Thanks so much for the review and feedback! I've refactored into a helper function and tried to simplify the logic as you suggested--on my machine the test I added is passing and the example still seems to work as I expect it to. CI is running now. Let me know what you think! |
Context
In native VTK, mouse button events are registered independently of each other; i.e., if you press the left button, then press the right, then release the left, then release the right, you would get:
LeftButtonPressRightButtonPressLeftButtonReleaseRightButtonReleaseHowever, currently in
vtk.js, if you went through the same sequence, you would only register the events which occur when no other button is pressed:LeftButtonPressRightButtonReleaseThis is undesirable because:
LeftButtonReleasewas never registered). Although this PR provides an example which shows the detected mouse events, note that you can currently demonstrate this behavior in any example with LeftDown, RightDown, LeftUp, RightUp, which will leave you with no buttons depressed but the object still rotating as you move the mouse.The underlying reason for this seems to be the web specification for Chorded Button Interactions:
I believe that this is the underlying cause of an issue I previously opened in trame (here: Kitware/trame#803) before I determined the source.
Results
This PR adds new logic to
RenderWindowInteractorwhich inspectsevent.buttonsto detect and correctly fire chorded events. As far as I can tell, the behavior now matches native VTK.Changes
New example:
Examples/Rendering/MouseEvents/index.jsNew test:
Sources/Rendering/Core/RenderWindowInteractor/test/testChordedButtons.jsModified:
Sources/Rendering/Core/RenderWindowInteractor/index.jsDocumentation and TypeScript definitions were not updated, since in my mind this fix brings behavior in line with expectation, though happy to add additional information to the docs if requested.
PR and Code Checklist
npm run reformatto have correctly formatted codeTesting