Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Event Ordering for touch and mouse events #7378

Merged
merged 13 commits into from
Dec 3, 2024

Conversation

diyaayay
Copy link
Contributor

@diyaayay diyaayay commented Nov 17, 2024

Resolves #7260 and #7195

Changes:

  • Replaced mouse events with pointer-events to collectively handle mouse and touch inputs.
  • Updated mouse tests to use pointer events

Description:
I have tested all the examples given for both the issues above on an Android device, and Windows. The outputs are same for both the touch device, and mouse.
Screenshots of the change:

PR Checklist

@diyaayay
Copy link
Contributor Author

@davepagurek @limzykenneth I've tried to maintain the original behavior that these issues aim to address. We also discussed the deprecation of touchStarted() and touchEnded(). Should these still be deprecated, leaving only mousePressed() and mouseReleased()?

@limzykenneth
Copy link
Member

If there are no special differences between touchStarted/Ended() and mousePressed/Released(), then let's remove touchStarted/Ended().

@diyaayay
Copy link
Contributor Author

diyaayay commented Nov 18, 2024

If there are no special differences between touchStarted/Ended() and mousePressed/Released(), then let's remove touchStarted/Ended().

I've removed them from mouse.js. So, does that mean that the inline documentation would change to something like "mousePressed()/Released()/Moved()` works for both the mouse and touch events, or should it only mention mouse features and no touch features at all?

@limzykenneth
Copy link
Member

It probably can mention supporting touch as well although there may not be practical differences.

@diyaayay
Copy link
Contributor Author

diyaayay commented Nov 19, 2024

It probably can mention supporting touch as well although there may not be practical differences.

Is there anything else that needs to be done in this PR? #6738 and #6739 are reverted because of changes in this PR but I don't think that breaks anything.

src/core/main.js Outdated
Comment on lines 62 to 64
mousemove: null,
mousedown: null,
mouseup: null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these and the touch ones below still need to be here?

Copy link
Contributor Author

@diyaayay diyaayay Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes! I missed these during the cleanup. But I think we need the touch ones for the touches[] array since we're handling multiple touches with touch events. I've removed the mouse events.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be possible to implement touches[] with pointer events as well, essentially caching the events as they come in, perhaps something like described here: https://developer.mozilla.org/en-US/docs/Web/API/Pointer_events/Multi-touch_interaction

Copy link
Contributor Author

@diyaayay diyaayay Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did implement touches[] with pointer events and the sketches seem to work just fine but the tests fail currently. I'll try to fix these.
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the local test sketches you tried are working as intended then it should be fine for now, you can try to fix the tests if it is easy to do but that's not a priority as I may need to rewrite them later.

Copy link
Contributor Author

@diyaayay diyaayay Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized later that if I implement touches with pointer-events then onpointerup()/down()/move() exists in both mouse.js and touch.js and mousePressed()/Released() stops working. I think this is because, for sketches, these events fire from both touch.js and mouse.js in parallel. So, maybe the choice lies between having one file pointer.js/ one file which would have pointer events for touches and mouse or letting the touches be decoupled by using touch events.

I did try a pointer.js handling touches and mouse on the basis of event.pointerType and everything seems to work. Let me know if I should go with this approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's go with just one file pointer.js

@limzykenneth
Copy link
Member

For the test we'll probably transition to use Vitest Browser Mode Interaction API but I can handle that after this is completed.

@diyaayay
Copy link
Contributor Author

diyaayay commented Nov 28, 2024

I've uploaded the build of p5.js for this PR and tested these sketches here:
orbitControl()
Mouse Events Ordering for touch #7260
Mouse Events ordering #7195
touches
mouseDragged()
mouseWheel()
@davepagurek @limzykenneth

@davepagurek
Copy link
Contributor

This is looking good @diyaayay! @limzykenneth is there anything else we want to add to this PR before merging? There are some follow-up updates to mouseButton in diyaayay#1 that branch off of this that we can start looking at after.

@limzykenneth
Copy link
Member

Just to confirm the tested existing sketches work as before without breaking changes? If so then let's go ahead and merge.

@davepagurek
Copy link
Contributor

Oh looks like this mouseDragged one is triggering the drag event on mouse move: https://editor.p5js.org/diya.solanki.31/sketches/aYfJEzpjgu

Other than that they look good from my testing!

@diyaayay
Copy link
Contributor Author

diyaayay commented Dec 2, 2024

Oh looks like this mouseDragged one is triggering the drag event on mouse move: https://editor.p5js.org/diya.solanki.31/sketches/aYfJEzpjgu

Other than that they look good from my testing!

My bad! I think it should work as intended now. I've updated the sketch too to test it.

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I'll merge this, and then we can change the base of your other PR.

@davepagurek davepagurek merged commit bc5db33 into processing:dev-2.0 Dec 3, 2024
2 checks passed
@diyaayay diyaayay mentioned this pull request Dec 3, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants