Support new hand animations #2191

machenmusik · 2016-12-19T21:18:09Z

update to use new hand animations from arturitu via rawgit
TODO: update to aframe CDN URLs once published
TODO: unused poses

dmarcos · 2016-12-19T21:20:33Z

src/components/hand-controls.js

-    mesh.mixer.clipAction(animation).clampWhenFinished = true;
-    mesh.mixer.clipAction(animation).timeScale = timeScale;
+    clipAction = mesh.mixer.clipAction(animation);
+    if (!clipAction) { return; }


I prefer this to break loudly on a Q/A pass. If the animation is not present then hand-controls are not working as expected and I want it to be hard to miss. The hand model and this component will be always in sync.

great for QA, not great for people actually using it.
Maybe spew warnings / errors to console to get the required attention?
Without this, things break but there is no messaging on why (and we clearly know why in this case, so may as well say so)

The whole point of Q/A is to help users. Your change makes more likely for us to push bugs to them. I think the code we had was fine. We have just have to make sure that when we update the model this component reflects the changes.

different perspective, the change makes it more forgiving for users to take as example and replace with their own model -- instead of "why does the controller no longer move or react" it becomes "why do my animations not work" as it should be. But I do understand your perspective. Would you prefer the early-return that masks errors to be commented and disabled?

dmarcos · 2016-12-19T21:22:05Z

src/components/hand-controls.js

-var OCULUS_LEFT_HAND_MODEL_URL = 'https://cdn.aframe.io/controllers/oculus-hands/leftHand.json';
-var OCULUS_RIGHT_HAND_MODEL_URL = 'https://cdn.aframe.io/controllers/oculus-hands/rightHand.json';
+var OCULUS_LEFT_HAND_MODEL_URL = 'https://rawgit.com/arturitu/assets/e698020d6f335fb46a6169f8985b4e12297e157d/controllers/oculus-hands/leftHand.json'; // 'https://cdn.aframe.io/controllers/oculus-hands/leftHand.json';
+var OCULUS_RIGHT_HAND_MODEL_URL = 'https://rawgit.com/arturitu/assets/e698020d6f335fb46a6169f8985b4e12297e157d/controllers/oculus-hands/rightHand.json'; // 'https://cdn.aframe.io/controllers/oculus-hands/rightHand.json';


Let's point these URLs to the proper CDN so we can merge it.

I'll update the models in the CDN before merge

for backwards compatibility, I strongly suggest making a new CDN URL for the new files, to avoid the breakage issue we had earlier today.

if you wouldn't mind updating these once you make the new CDN URLs, that would be great, thx!

I don't want to confuse people with multiple versions of the same model for each tiny change we make. I don't think this model has that much use to justify keeping the old one.

but then you have to coordinate the CDN change with global replacement of code everywhere that references the old version... which seems unlikely to me. this is just like the difference between cdn.rawgit.com and rawgit.com - when you publish the URLs to production, the assumption is that you won't break things by destructive changes to required CDN assets.

to use a recent example, we changed the URL for oculus-compatible hands rather than overwriting the old ones, which would have broken hand-controls usage across all of a-frame...

The change is really exceptional. I really don't expect assets to change once published.

They definitely shouldn't (also so you don't have to deal with flushing older versions from all the caches in the Internet).

If you don't use different URLs, then you have to attempt to update the CDN assets and simultaneously the referencing code (and all uses of it, which seems unreasonable given encouragement to pull versions via npm eather than using direct URL).

You've offered to to update the CDN assets and then update the code, so if that is really what you'd like to do, feel free -- but then we are likely to face spurious issues as we did before reverting the prior hands change.

if you change the url back I can update the CDN and merge this PR simultaneously.

ok, see latest commit

ngokevin · 2016-12-19T22:09:29Z

src/components/hand-controls.js

-    'touch': 'touch',
-    'thumb': 'thumb'
+    '': 'Open',
+    'pointing': 'Point',


don't need quotes for js keys heere

well, the empty string does... is there any harm in leaving them for consistency?

The empty string is the default. An empty string as a key feels weird to me.

right, the default (no gesture) may still have a named animation associated (as it does here)

dmarcos · 2016-12-20T01:56:51Z

src/components/hand-controls.js

+    // (e.g. controllers no longer updating pose)
+    // but per https://github.com/aframevr/aframe/pull/2191#discussion_r93121878
+    // the preference is to NOT prevent the issues to catch bugs earlier in QA.
+    // if (!clipAction) { return; }


Remove line

machenmusik · 2016-12-22T07:49:01Z

may want to do #2192 first to avoid spurious conflicts, as it uses the hand animation names that are then updated as part of this one

dmarcos · 2016-12-27T23:35:12Z

needs rebase

machenmusik · 2016-12-27T23:41:04Z

rebased but not yet double-checked. still recommend new CDN URLs to avoid breaking npm builds that already pulled down old code...

machenmusik · 2016-12-28T17:20:08Z

ran this last night looked ok on Touch...

machenmusik · 2017-01-05T20:40:37Z

I am guessing this one may be waiting for point release

machenmusik · 2017-02-09T04:23:45Z

tested working with latest experimental Chromium and Oculus Touch;
using Nightly plus Vive, gamepad id has changed from OpenVR Gamepad to OpenVR Controller

machenmusik · 2017-02-09T04:35:44Z

latest version confirmed working with examples/showcase/tracked-controls in VR on:

latest Nightly 54.0a1 (2017-02-08), latest SteamVR 1485823399 , OpenVR API 1.02 DLL, Vive controllers
OpenVR Controller x2 (when in concert with #2370)

latest experimental Chromium 56.0.2910.0, Oculus 1.11.0.335615 (1.11.0.335772), Rift and Touch controllers

older experimental Chromium 56.0.2900.0, older SteamVR vrserver.exe 1481163074, Vive controllers
OpenVR Gamepad x2
(see toji/chrome-webvr-issues#135 (comment) for details)

update to CDN URLs from aframevr/assets#18

machenmusik · 2017-02-09T19:23:58Z

separating the old-and-new Vive support into
#2370

dmarcos requested changes Dec 19, 2016

View reviewed changes

ngokevin reviewed Dec 19, 2016

View reviewed changes

dmarcos requested changes Dec 20, 2016

View reviewed changes

machenmusik force-pushed the support-new-hand-animations branch 3 times, most recently from a7ad3d6 to ebbb26b Compare December 22, 2016 01:59

dmarcos added the needs rebase label Dec 27, 2016

machenmusik force-pushed the support-new-hand-animations branch from ebbb26b to 4f4a46a Compare December 27, 2016 23:39

ngokevin added this to the 0.5.0 milestone Feb 1, 2017

ngokevin added needs review and removed needs rebase labels Feb 7, 2017

ngokevin mentioned this pull request Feb 9, 2017

add oculus hand models with new poses (from @arturitu) aframevr/assets#18

Merged

ngokevin modified the milestones: 0.6.0, 0.5.0 Feb 9, 2017

machenmusik force-pushed the support-new-hand-animations branch from d29d0b1 to 341a018 Compare February 9, 2017 03:47

working remapping to new hands from @arturitu

c9ae9e8

machenmusik force-pushed the support-new-hand-animations branch from 819820c to 3f3a2d1 Compare February 9, 2017 05:49

minor changes requested per discussion on PR

7b2c6a7

update to CDN URLs from aframevr/assets#18

machenmusik force-pushed the support-new-hand-animations branch from 530f417 to 29c1364 Compare February 9, 2017 18:46

This was referenced Feb 9, 2017

SteamVR update causes Chrome VR failure toji/chrome-webvr-issues#135

Closed

accommodate both names for Vive controllers: old 'OpenVR Gamepad' and… #2370

Merged

machenmusik force-pushed the support-new-hand-animations branch from 29c1364 to 7b2c6a7 Compare February 9, 2017 19:25

ngokevin merged commit 5dfe0f9 into aframevr:master Feb 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support new hand animations #2191

Support new hand animations #2191

machenmusik commented Dec 19, 2016

dmarcos Dec 19, 2016

machenmusik Dec 20, 2016 •

edited

Loading

dmarcos Dec 20, 2016

machenmusik Dec 20, 2016

dmarcos Dec 19, 2016

dmarcos Dec 19, 2016

machenmusik Dec 20, 2016

dmarcos Dec 20, 2016

machenmusik Dec 20, 2016

machenmusik Dec 20, 2016

dmarcos Dec 20, 2016

machenmusik Dec 22, 2016

dmarcos Dec 22, 2016

machenmusik Dec 22, 2016

ngokevin Dec 19, 2016

machenmusik Dec 20, 2016

dmarcos Dec 20, 2016

machenmusik Dec 20, 2016

dmarcos Dec 20, 2016

machenmusik commented Dec 22, 2016

dmarcos commented Dec 27, 2016

machenmusik commented Dec 27, 2016

machenmusik commented Dec 28, 2016

machenmusik commented Jan 5, 2017

machenmusik commented Feb 9, 2017

machenmusik commented Feb 9, 2017 •

edited

Loading

machenmusik commented Feb 9, 2017

Support new hand animations #2191

Support new hand animations #2191

Conversation

machenmusik commented Dec 19, 2016

Choose a reason for hiding this comment

machenmusik Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machenmusik commented Dec 22, 2016

dmarcos commented Dec 27, 2016

machenmusik commented Dec 27, 2016

machenmusik commented Dec 28, 2016

machenmusik commented Jan 5, 2017

machenmusik commented Feb 9, 2017

machenmusik commented Feb 9, 2017 • edited Loading

machenmusik commented Feb 9, 2017

machenmusik Dec 20, 2016 •

edited

Loading

machenmusik commented Feb 9, 2017 •

edited

Loading