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

Improve webgl_canvas mouse event handling and fix mouse coordinates #207

Merged
merged 2 commits into from
Apr 12, 2020

Conversation

alvinhochun
Copy link
Collaborator

@alvinhochun alvinhochun commented Feb 24, 2020

A previous version of this PR narrowed the event listeners target, introduced the use of pointer events to enable pointer capturing via setPointerCapture and added some cleanup code. Following the comments, part of this PR was split into another PR #219. The current PR focuses on improving the mouse event handling without using pointer event.

@alvinhochun alvinhochun changed the title Change webgl_canvas to listen events on the canvas element Change webgl_canvas to listen events on the canvas element and add some cleanup code Feb 25, 2020
@alvinhochun
Copy link
Collaborator Author

I added a commit to also do some cleanup when the Kiss3d window is closed.

@alvinhochun alvinhochun force-pushed the html-canvas-events branch 2 times, most recently from 48c91e4 to b785bea Compare February 27, 2020 16:09
@alvinhochun alvinhochun marked this pull request as ready for review February 27, 2020 16:12
@alvinhochun
Copy link
Collaborator Author

I've updated the branch to master but since this PR uses PointerEvent instead of MouseEvent it may not interact with TouchEvent nicely. The idea of PointerEvent is that it replaces both MouseEvent and TouchEvent, so the TouchEvent handlers will need to be modified to use PointerEvent handlers instead.

@sebcrozet I would like to know how you feel about this before I move forward. Also, do the touch events actually do anything now in Kiss3d? I will need an example to test it.

@sebcrozet
Copy link
Owner

Hi! Sorry for the late answer. Thank you for this PR!

I think it is misleading to expose pointer events behind events containing Mouse in their name. So I suggest that we keep the existing mouse events, and simply add a new WindowEvent::Pointer enum variant. In addition, it appears pointer events are not supported by all platforms: according to mdn it is not supported by safari. So it is best to keep both events so we don't loose any compatibility.

Now the question is whether or not we will want to emulate the pointer events when using the GLCanvas for native apps (I believe it would make sense). But that should be the subject of another issue.

@alvinhochun
Copy link
Collaborator Author

alvinhochun commented Apr 10, 2020

@sebcrozet

I think it is misleading to expose pointer events behind events containing Mouse in their name. So I suggest that we keep the existing mouse events, and simply add a new WindowEvent::Pointer enum variant.

Here's the thing, "pointer events" are a consolidation of mouse, touch and pen events. Dispatching the pointer events of "mouse" type as mouse events is IMO quite acceptable. Pointer events of "touch" (assuming no touch event handlers are in use) and "pen" type will also, in one way or another, get translated to mouse events for compatibility if no pointer event handlers are in use, so it's not really so misleading to just map them to existing mouse events when Kiss3d did not have built-in handling for touch and pen events. Of course, with the introduction of WindowEvent::Touch, pointer events of type "touch" should be mapped to that.

(On a related note, Qt is doing the equivalent with Windows' WM_POINTER messages, mapping them to Q[Mouse|Touch|Tablet]Event.)

In addition, it appears pointer events are not supported by all platforms: according to mdn it is not supported by safari. So it is best to keep both events so we don't loose any compatibility.

The PointerEvent page on mdn and caniuse seem to indicate that the latest version of Safari does support it. There are also polyfills like https://github.com/jquery/PEP but I do understand that using polyfills are not ideal.

Now the question is whether or not we will want to emulate the pointer events when using the GLCanvas for native apps (I believe it would make sense). But that should be the subject of another issue.

I'm not sure if it's worth putting WindowEvent::Pointer in Kiss3d now, as having to deal with both pointer events and separate mouse/touch events (and pen events if Kiss3d wants to handle them) would add quite a bit of complexity to Kiss3d's event stack. (It might be better if WindowEvent::Touch does not exist and Kiss3d goes directly to WindowEvent::Pointer, but perhaps too late for this.) But yes it would be for another discussion.


To reiterate the reason of using pointer events: The problem is that adding the mouse event handlers to the canvas element would limit dragging actions to the bounds of the element, making it inconvenient for camera navigation especially when the canvas element is small in size. Using pointer events enables the use of setPointerCapture which allow the dragging actions to extend beyond not only the canvas element but also outside of the browser window.

However, I realize that part of the changes in this PR can be split into another PR that doesn't include pointer events, and since you may not agree with the change to use pointer events, here's what I plan to do:

  • Open a new PR for actually adding the event handlers to the canvas element and adding cleanup code.
  • Repurpose this PR for using pointer events and adding mouse capture.

@sebcrozet
Copy link
Owner

sebcrozet commented Apr 10, 2020

Thank you for the explanation! So I guess we have two choices regarding pointer events:

  1. We could do it like Qt and translate pointer events to mouse/touch events depending on they "type".
  2. Or replace our current Mouse and Touch events by one single unified Pointer event.

The item 2 seems to be the most practical (and closest to what you have been doing here) because it would reduce cases of duplicate event handling code for mouse and touch.

My main concern then is the platform support. Is there a way for kiss3d to provide the polyfills transparently? Without requiring the user to include them in their non-rust code?

However, I realize that part of the changes in this PR can be split into another PR that doesn't include pointer events, and since you may not agree with the change to use pointer events, here's what I plan to do [...]

Moving the mouse event handler to the canvas changes the current behavior too much (for example mouseup events outside the canvas won't be detected, which will break camera systems that rely on it to stop rotating/panning the view). So we can't merge a PR that will just change the mouse event handler target without providing a way to reproduce the old behavior.

The transition towards pointer events should be smoother. To that end, I suggest to:

  • Keep mouse and touch events handler where they are currently, on the window element.
  • Add pointer event listener to the canvas element. And add the WindowEvent::Pointer enum variant.
  • Mark the WindowEvent::Touch and WindowEvent::MouseButton enum variants as deprecated, suggesting to use WindowEvent::Pointer instead.
  • Modify kiss3d cameras so they use the WindowEvent::Pointer events instead of
    MouseButton.

What do you think about that?

@alvinhochun
Copy link
Collaborator Author

alvinhochun commented Apr 10, 2020

Thank you for the explanation! So I guess we have two choices regarding pointer events:

1. We could do it like Qt and translate pointer events to mouse/touch events depending on they "type".

2. Or replace our current `Mouse` and `Touch` events by one single unified `Pointer` event.

The item 2 seems to be the most practical (and closest to what you have been doing here) because it would reduce cases of duplicate event handling code for mouse and touch.

I don't really have a preference for either one, but implementing the second choice would mean introducing a breaking API change - while it may not be hard to convert the code replacing Mouse to Pointer, it also forces the user to think about how to handle touch and pen events at the same time, instead of pretending them to be mouse events and expecting only one pointer at a time. (However on the current Kiss3d release, WindowEvent::MouseButton is no longer sent for touch presses since the browser does not fire mouse events when touch events are being handled, and Kiss3d has not emulated mouse events for compatibility.) Perhaps there can be an option to force Kiss3d to emulate pointer events type of mouse for any touch/pen events, but that also brings a third option...

A third option may be to keep just mouse events for compatibility and ease of use, but require the use of the unified pointer events if the user want to also handle touch events. However I have not thought of how it should be handled if built-in touch handling is added to the cameras.

My main concern then is the platform support. Is there a way for kiss3d to provide the polyfills transparently? Without requiring the user to include them in their non-rust code?

I don't think you can easily include a polyfill... perhaps it can be done by appending a script tag to the document but it seems like a horrible hack, and what if it conflicts with other user code?

However, I realize that part of the changes in this PR can be split into another PR that doesn't include pointer events, and since you may not agree with the change to use pointer events, here's what I plan to do [...]

Moving the mouse event handler to the canvas changes the current behavior too much (for example mouseup events outside the canvas won't be detected, which will break camera systems that rely on it to stop rotating/panning the view). So we can't merge a PR that will just change the mouse event handler target without providing a way to reproduce the old behavior.

I understand, but in some way the current mouse handling is also broken since it doesn't detect mouse up if you drag the mouse to outside of the browser window. It also does not properly convert the offsetX/offsetY coordinates to be relative to the canvas element so it causes the camera to jump around, but this one is fixable.

If it can't use setPointerCapture right away, I might just make a smaller change so that it keeps track of whether the mouse down started within the canvas element and send or discard the other mouse events accordingly, and also fix the coordinates handling

The transition towards pointer events should be smoother. To that end, I suggest to:

* Keep mouse and touch events handler where they are currently, on the `window` element.

* Add pointer event listener to the canvas element. And add the `WindowEvent::Pointer` enum variant.

* Mark the `WindowEvent::Touch` and `WindowEvent::MouseButton` enum variants as deprecated, suggesting to use `WindowEvent::Pointer` instead.

* Modify kiss3d cameras so they use the `WindowEvent::Pointer` events instead of
  `MouseButton`.

What do you think about that?

You will also need to make other native platforms support the WindowEvent::Pointer model. Also, on the web I don't think it is the best idea to simultaneously make use of MouseEvent/TouchEvent and PointerEvent; I think it might be better to only use PointerEvent and have Kiss3d internally map them to the respective WindowEvent::MouseButton/CursorPos/Touch events such that they can be guaranteed to receive the same data as WindowEvent::Pointer.

Otherwise it sounds fine, but I can't promise to help with the implementation and testing so in the end it is still up to you to design and implement the new APIs. Perhaps you should open an issue to keep track of this and continue over there, while I try to fix up the current mouse event behaviour here.

This prevents mouse events from being handled if the mouse down event
started outside of the canvas element, while allowing them to be handled
everywhere on the page if the mouse down event started inside of the
canvas element.
@alvinhochun alvinhochun marked this pull request as ready for review April 10, 2020 12:30
@alvinhochun alvinhochun changed the title Change webgl_canvas to listen events on the canvas element and add some cleanup code Improve webgl_canvas mouse event handling and fix mouse coordinates Apr 10, 2020
@alvinhochun
Copy link
Collaborator Author

Updated the mouse event handling without using pointer event. I think it now works in a satisfactory manner.

I understand, but in some way the current mouse handling is also broken since it doesn't detect mouse up if you drag the mouse to outside of the browser window.

I guess I misremembered because this turns out to not be true. I might have confused it with alt-tabbing to another window.

@sebcrozet
Copy link
Owner

You are right that cursor events is not an easy topic and should be part of a separate issue. I've created #220.

The changes you have made here are very interesting and look like a good way of achieving mouse capture-like behavior. Thank you for your involvement!

@sebcrozet sebcrozet merged commit 32a7598 into sebcrozet:master Apr 12, 2020
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.

2 participants