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

Feature/robot hand gestures #65

Merged
merged 17 commits into from
Mar 14, 2018
Merged

Feature/robot hand gestures #65

merged 17 commits into from
Mar 14, 2018

Conversation

johnshaughnessy
Copy link
Contributor

No description provided.

@robertlong robertlong self-requested a review March 14, 2018 00:43
Copy link
Contributor

@robertlong robertlong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good! Just a few small things.

package.json Outdated
@@ -10,6 +10,7 @@
"scripts": {
"postinstall": "node ./scripts/postinstall.js",
"start": "cross-env NODE_ENV=development webpack-dev-server",
"mobile" : "cross-env NODE_ENV=development webpack-dev-server --useLocalIp --host 0.0.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brianpeiris is about to add something similar in his PR. You can probably remove this.

this.playAnimation = this.playAnimation.bind(this);

// Get the three.js object in the scene graph that has the animation data
const root = this.el.querySelector("a-gltf-entity .RootScene").object3D.children[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked briefly about creating our own animation-mixer component that we can add to the root scene element and other components such as this one can reference. I'll need that for implementing the eye animations. I think this is fine for this PR, let's talk more about it later.

// console.log(
// `Animating ${isLeft ? "left" : "right"} hand from ${prevPose} to ${currPose} over ${duration} seconds.`
// );
var from = mixer.clipAction(prevPose, this.root.parent);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: change var to const

// the optional root in `THREE.AnimationMixer.clipAction` and use the hierarchy
// preserved under the group (but not the node). Otherwise `clipArray` will be
// `null` in `THREE.AnimationClip.findByName`.
node.parent.animations = node.animations;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think this hack is crazy. Thanks, for documenting it. Hopefully we can find a more robust way of doing this in the future. This could screw up object3D.getObjectByName calls.

@robertlong robertlong merged commit 894ac59 into master Mar 14, 2018
@johnshaughnessy johnshaughnessy deleted the feature/robot-hand-gestures branch March 31, 2018 01:57
markusTraber added a commit to farvel-space/space-client that referenced this pull request Sep 30, 2022
…y the 3d picture frame+hubsCloudSeptemberUpdate

Merge in FAR/space-client from feature/FARM-225-integrate-and-deploy-the-3d-picture-frame+hubsCloudSeptemberUpdate to main

* commit '117f37499b717c77ef03d56315a01dc78a4a8323':
  fixing scale setting issue for vertical images
  FARM-225: Fix clipboard has no farvel frame
  added check for buttonEl
  FARM-225: Added default asset. Added workaround for empty assetURL
  final stable form
  work Aug 28
  Aug 20th work
  updated Aug 13 connection with spoke and stability edits
  Aug 7 functional prototype
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.

None yet

2 participants