Allow Decoupling of TouchEvents and Look-Contols #3012
Conversation
src/components/look-controls.js
Outdated
| this.onTouchStart = bind(this.onTouchStart, this); | ||
| this.onTouchMove = bind(this.onTouchMove, this); | ||
| this.onTouchEnd = bind(this.onTouchEnd, this); | ||
| } |
There was a problem hiding this comment.
This event binding should happen regardless of data.touchEnabled, because touch events might be enabled or disabled at a later time.
There was a problem hiding this comment.
Yes, This condition is not necessary. We should bind the methods regardless of the flag
There was a problem hiding this comment.
Addressed in commit 1d6930b
src/components/look-controls.js
Outdated
| canvasEl.removeEventListener('touchstart', this.onTouchStart); | ||
| canvasEl.removeEventListener('touchmove', this.onTouchMove); | ||
| canvasEl.removeEventListener('touchend', this.onTouchEnd); | ||
| } |
There was a problem hiding this comment.
It may be better to return early from these event listeners when touch is disabled, rather than conditionally binding or not. Otherwise, (1) setAttribute('look-controls', {touchEnabled: false}) will have no effect, and (2) calls to pause() after that will fail to remove the old touch listeners.
So instead:
onTouchStart: function () {
if (!this.data.touchEnabled) { return; }
// ...
}There was a problem hiding this comment.
Addressed in commit 1d6930b
examples/index.html
Outdated
|
|
||
| <ul class="links"> | ||
| <li><a href="boilerplate/hello-world/">Hello World</a></li> | ||
| <li><a href="boilerplate/hello-world-touch-disabled/">Hello World Touch Disabled</a></li> |
There was a problem hiding this comment.
It's not necessary to add an example for this, but could you instead add an entry in the look-controls documentation for the new option?
There was a problem hiding this comment.
Yes, It seems that just a flag does not justify a whole example.
There was a problem hiding this comment.
Addressed in commit 1d6930b
| <!DOCTYPE html> | ||
| <html> | ||
| <head> | ||
| <meta charset="utf-8"> |
There was a problem hiding this comment.
Not sure if we need an example specific for just one flag.
There was a problem hiding this comment.
Addressed in commit 1d6930b
src/components/look-controls.js
Outdated
| canvasEl.addEventListener('touchstart', this.onTouchStart); | ||
| window.addEventListener('touchmove', this.onTouchMove); | ||
| window.addEventListener('touchend', this.onTouchEnd); | ||
| if (this.data.touchEnabled) { |
There was a problem hiding this comment.
We also have to handle the dynamic case when the flag changes at runtime. We probably need a method updateTouchListeners that contains this logic and its called from here and the update method.
There was a problem hiding this comment.
When the enabled flag is updated at runtime, a grab cursor is shown or hidden accordingly. I'm not sure what would need to be updated, other than the behavior that the flag controls (updated in commit 1d6930b). Would the behavior change accordingly when the flag is set at runtime?
There was a problem hiding this comment.
I think this is taken care of, given the last set of changes, yes?
|
Thanks for the feedback @donmccurdy and @dmarcos . I will implement those suggested changes soon. |
…instead of conditionally managing event handlers.
|
Thanks! |
3 participants
Description:
While working on an AR.js + A-Frame app that allows users to draw/paint in AR, I wanted users to be able to use touch events to draw on a mobile device and use device orientation to change camera orientation. Touch events are included in
look-controlsby default, with no disable action allowed. Usingtouchmoveevents to draw resulted in drawing across the scene in a reverse pan because of the way thattouchmoveis coupled withlook-controls. In magic window mode, there could be many applications of touch events that would not want to also change device orientation.Changes proposed: