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

Public events from the hand-controls component are not extensive and do not all have accurate names #4883

Open
Clicky02 opened this issue Jul 9, 2021 · 3 comments

Comments

@Clicky02
Copy link
Contributor

Clicky02 commented Jul 9, 2021

Description: The hand-controls component emits events on certain animation triggers; however, the event name for the thumbUp animation is not accurate to the animation. There are also not events for each of the animation states, making it difficult to know the state of the hand.

  • A-Frame Version: 1.2.0
  • Platform / Device: Should apply to all platforms/devices, but tested in Firefox 89
  • Reproducible Code Snippet or URL:

The only truly relevant code is from the top of hand-controls.js

image

The event for starting/ending thumbUp animation (described in the comments above) is called pistol, but pistol would seem to fit the pointThumb animation much better. Additionally, there is no event at all for the pointThumb, open, or hold animations. Instead, lines 31-33 should be replaced with something like:

EVENTS[ANIMATIONS.fist] = 'grip';
EVENTS[ANIMATIONS.thumbUp] = 'thumb';
EVENTS[ANIMATIONS.point] = 'pointing';
EVENTS[ANIMATIONS.pointThumb] = 'pistol';
EVENTS[ANIMATIONS.hold] = 'hold';

And the get getGestureEventName function (hand-controls.js, lines 397-412) should be

function getGestureEventName (gesture, active) {
  var eventName;

  if (!gesture) { return; }

  eventName = EVENTS[gesture];
  if (eventName === 'grip') {
    return eventName + (active ? 'close' : 'open');
  }
  if (eventName === 'thumb') {
    return eventName + (active ? 'up' : 'down');
  }
  if (eventName === 'pointing' || eventName === 'pistol' || eventName === 'hold') {
    return eventName + (active ? 'start' : 'end');
  }
}

Or alternatively, getGestureEventName could always return the eventName with 'start' or 'end' concatenated at the end, since (with this change) switching from thumbUp to pointThumb would emit a thumbdown event despite the thumb staying up.

@dmarcos
Copy link
Member

dmarcos commented Jul 10, 2021

The events are documented in https://aframe.io/docs/1.2.0/components/hand-controls.html#events

Which ones are not correct or missing?

What headset are you using? Have you tried Chrome? Firefox is stuck in the old WebVR API. I recommend using Chrome if you can.

@Clicky02
Copy link
Contributor Author

Clicky02 commented Jul 12, 2021

The events thumbup , thumbdown, pointup, and pointdown are all missing. Additionally, pistolstart/pistolend gets emitted when thumbup/thumbdown should be called, and gripdown/gripup are replaced by gripclose/gripopen. I do not currently have the means to test with Chrome, but looking at the code, I'm pretty sure this issue would exist on all browsers.

aframe/src/components/hand-controls.js lines 14 - 27
image
For reference, this snippet of code shows the definition of all possible gestures/animations for the hand. As far as I can tell, the comments accompanying each state are correct.

aframe/src/components/hand-controls.js lines 29 - 33
image
This shows the definition of the EVENTS object, which is used to determine the event name from a given animation/gesture.

aframe/src/components/hand-controls.js lines 326 - 342
image
Whenever the gesture changes, this function gets called to emit the appropriate events. It calls the getGestureEventName function twice to get the event name for both the starting and ending gesture. If the function returns a value other than undefined, it uses that value as the name for the emitted event So, this is what actually emits the events like pointingstart and pistolend, but the names themselves come from the getGestureEventName function.

aframe/src/components/hand-controls.js lines 391 - 412
image
This is getGestureEventName function mentioned above, and it is what uses the EVENT object (also shown above). Just from this function alone, it is clear that the thumbup and thumbdown events will never be called. This is because, if the eventName variable is 'thumb', it will never reach a return statement, and the function will return undefined (meaning the event won't get emitted). Additionally, the pointup and pointdown events will not be called because the EVENT object does not have an entry with "point" as its value. Finally, pistolstart and pistolend get emitted when eventName is 'pistol'. As you can see with the EVENT object, eventName is 'pistol' when the gesture/animation is thumbUp (described as "grip active, trigger active, trackpad surface inactive" on line 25 above). This means pistolStart/pistolEnd is being emitted when thumbup/thumbdown should be. Instead, eventName should be 'pistol' when the gesture/animation is pointThumb. Finally, you can also see if eventName is 'grip' the returned value will be either 'gripclose' or 'gripiopen', rather than the 'gripdown' and 'gripup' that is listed in the documentation.

My previous suggestion to fix this would not be correct according to the documentation. For everything to work as it says in the documentation, lines 31-33 should be replaced with:

EVENTS[ANIMATIONS.fist] = 'grip';
EVENTS[ANIMATIONS.thumbUp] = 'thumb';
EVENTS[ANIMATIONS.point] = 'pointing';
EVENTS[ANIMATIONS.pointThumb] = 'pistol';
EVENTS[ANIMATIONS.hold] = 'point';

and lines 397 - 412 should be replaced with:

function getGestureEventName (gesture, active) {
  var eventName;

  if (!gesture) { return; }

  eventName = EVENTS[gesture];
  if (eventName === 'grip') {
    return eventName + (active ? 'down' : 'up');
  }
  if (eventName === 'point' || eventName === 'thumb') {
    return eventName + (active ? 'up' : 'down');
  }
  if (eventName === 'pointing' || eventName === 'pistol') {
    return eventName + (active ? 'start' : 'end');
  }
}

@Clicky02
Copy link
Contributor Author

Clicky02 commented Jul 14, 2021

I created a PR to solve this issue, but if there is anything wrong with my changes, I'd be happy to fix it. I will say though that the pointup and pointdown events seem oddly named given their description in the documentation, and there's probably a better way to describe that gesture (but I have no clue what to call it).

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

No branches or pull requests

2 participants