hand-controls component: broken features, performance.

[kylebakerio]

The hand-controls component is a bit creaky--as it mentions in its own comments, it was originally just a proof of concept. It seems incomplete; as issue #4596 points out, the events are problematic, and the 'color' property doesn't affect the mesh. It also has a lot of room for improvement in terms of not being sufficiently dry, being written in a bit of an old fashioned style, and not being written in a way that is at all conscious of the effects on garbage collection.

When writing a version of this component for NAF, I went ahead and gave the component an overhaul, and ended up with this:

https://github.com/networked-aframe/networked-aframe/blob/master/src/components/networked-hand-controls.js

Would a pull request be welcome to add these changes over to core? Any comments on improving this code? Should I just make a pull request and we work from there?

Note: lines 20-40 are NAF-specific and can be ignored.

As you'll see, I have been working on a code style that minimizes garbage collection. You can see this in the str property and the this.Y[this.Z] pattern. (Because of other quiet object re-use patterns in A-Frame, there are very tricky pitfalls that show up when just assigning properties itself).

I know it's unorthodox; I'd love feedback on a cleaner way to write this code, if you have ideas. If it's agreed that it's a good idea, though, it would be interesting to discuss ways to support this code style in A-Frame to help component authors reduce garbage collection by default, without tripping over some of the existing optimizations under the hood in A-Frame.

[dmarcos]

Thanks! Can you break the work in separate PRs to address issues independently? It makes it easier to review and understand.

[dmarcos]

Not sure what the following code does:

   this.Z = Math.random();
   this.Y[this.Z] = { contact: {}, reverse: false, isContact: false };

[dmarcos]

also helpful if you can elaborate where you're seeing performance issues.

[kylebakerio]

I feel a bit stupid, but I actually just looked into the registerComponent source code and realized how it works. I found a cleaner way to write that--the key is not dynamically assigning properties to a component instance. If more than one entity uses that component, because of the prototypal inheritance, it causes issues. I was working around that in a more clunky way, but I now realize the key is just declaring properties in advance before registerComponent() is called.

I hope you can forgive that error, that's honestly pretty embarrassing! I hadn't touched A-Frame in a few months and just papered over some puzzling behavior that threw me off by working around it instead of trying to understand what was going on underneath. I've included cleaning up that code in a pull request to NAF that also includes some other new features: networked-aframe/networked-aframe#358

Anyways, as to the other issues:

1. There's some room for improvement in regards to garbage collection in the existing component. Given that all the button touch events are constantly flickering on/off while the controllers are being held, there are easily many thousands of events being fired, and many short-lived values. I had a demo tracking every gesture calculation and it would very quickly get to several hundred within a few seconds of use--that was without multiplying it by all the individual vars present. How big of an impact are they? I can't say I've benchmarked it, I'd be curious to see hard data on this, but it doesn't hurt to avoid it wherever reasonably possible.

2. The original component doesn't actually allow setting the color of the hands, even though it claims to. I've added a way to set color on the hand meshes that works, though this is not my expertise--open to any more ideal implementation patterns for this if desired.

3. The emitted events... are arguably a bit odd in the original component? For one, not every gesture has a matching event--including some primary ones. The original events were 'grip' 'pistol' and 'pointing'... but 'pistol' corresponds to thumbUp, instead of pointThumb--and there is no proper event for the actual "pistol" (pointThumb), open, or hold gestures--and the docstring at the top seems to incorrectly describe the gestures--and they all use different suffixes (up/down vs. open/close), making them unnecessarily more complex. I get the vibe that this was modified over time but never cleaned up at the end.

I've implemented an approach that I think is cleaner and consistent, and should be pretty easy to read (and is documented correctly in the docstring). Open to feedback.

4. I made some code more "DRY", especially the button event listeners, which are a bit awkward in the original code; instead of every function name being copy/pasted 3 times, I've used a loop, etc.

5. I've added support for some controllers that were missing--most odd was to see the valve index missing, but I have also gone ahead and added several older controllers as well. (There may be a reason that wasn't done--If so, glad to hear it.)

6. Lots of other little code cleanup throughout--various redundancies simplified, getClip() replaced with a lookup dictionary, a rewrite of determineGesture() to make it easier (imho) to read, isVive() being run once and then cached, getGestureEventName() has been rewritten as a lookup dictionary, no longer adding listeners to thumbsticks that weren't being used, simplified code that doesn't separate touch and press unnecessarily, etc.

7. Use of ES6+ features throughout to produce slightly cleaner, more modern looking code.

In the most recent version I've submitted for a pull request to NAF, I've also added the ability to specify "controller" as a hand model. I don't assume you also want that feature in core, so I wouldn't expect to propose that here, but I'm happy to include that feature if that's interesting.

[dmarcos]

Thanks so much.

Can we open a PR to change the color of the hands? It looks like a bug.

We should fix the events if they're not consistent. A separate PR will also help me understand better. It's been a while I haven't used hand-controls.

Hand-controls predates Valve Index (and many other headsets. it's been around even before Oculus Touch controllers) and probably nobody cared until now :)

A-Frame code is ES5

[dmarcos]

About tons of events emitted. Variables should be reused if we're not doing that already. We can look into those. We could also throttle the events if possible.

[Elettrotecnica]

For reference, inconsistencies in the events fired by hand-controls have also been reported in #4722 and #4883