From b80fa1c8064c23bb3709593dafcef968cb7e1aa3 Mon Sep 17 00:00:00 2001
From: Seth Alves <seth@highfidelity.io>
Date: Thu, 17 Dec 2015 11:46:41 -0800
Subject: [PATCH] code review

---
 interface/src/avatar/AvatarActionHold.cpp | 29 +++++++++++++++++++----
 interface/src/avatar/AvatarActionHold.h   |  3 ++-
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/interface/src/avatar/AvatarActionHold.cpp b/interface/src/avatar/AvatarActionHold.cpp
index b2343c8bce..be562e2773 100644
--- a/interface/src/avatar/AvatarActionHold.cpp
+++ b/interface/src/avatar/AvatarActionHold.cpp
@@ -78,8 +78,16 @@ void AvatarActionHold::prepareForPhysicsSimulation() {
         // bullet has moved the av's rigid body but the rigid body's location has not yet been
         // copied out into the Avatar class.
         glm::quat avatarRotationInverse = glm::inverse(avatarRigidBodyRotation);
-        _palmOffsetFromRigidBody = avatarRotationInverse * (palmPosition - avatarRigidBodyPosition);
-        _palmRotationFromRigidBody = avatarRotationInverse * palmRotation;
+
+        // the offset should be in the frame of the avatar, but something about the order
+        // things are updated makes this wrong:
+        //   _palmOffsetFromRigidBody = avatarRotationInverse * (palmPosition - avatarRigidBodyPosition);
+        // I'll leave it here as a comment in case avatar handling changes.
+        _palmOffsetFromRigidBody = palmPosition - avatarRigidBodyPosition;
+
+        // rotation should also be needed, but again, the order of updates makes this unneeded.  leaving
+        // code here for future reference.
+        // _palmRotationFromRigidBody = avatarRotationInverse * palmRotation;
     });
 }
 
@@ -116,8 +124,21 @@ std::shared_ptr<Avatar> AvatarActionHold::getTarget(glm::quat& rotation, glm::ve
             // and the data in the Avatar class is stale.  This means that the result of get*PalmPosition will
             // be stale.  Instead, determine the current palm position with the current avatar's rigid body
             // location and the saved offsets.
-            palmPosition = avatarRigidBodyPosition + avatarRigidBodyRotation * _palmOffsetFromRigidBody;
-            palmRotation = avatarRigidBodyRotation * _palmRotationFromRigidBody;
+
+            // this line is more correct but breaks for the current way avatar data is updated.
+            // palmPosition = avatarRigidBodyPosition + avatarRigidBodyRotation * _palmOffsetFromRigidBody;
+            // instead, use this for now:
+            palmPosition = avatarRigidBodyPosition + _palmOffsetFromRigidBody;
+
+            // the item jitters the least by getting the rotation based on the opinion of Avatar.h rather
+            // than that of the rigid body.  leaving this next line here for future reference:
+            // palmRotation = avatarRigidBodyRotation * _palmRotationFromRigidBody;
+
+            if (isRightHand) {
+                palmRotation = holdingAvatar->getRightPalmRotation();
+            } else {
+                palmRotation = holdingAvatar->getLeftPalmRotation();
+            }
         } else {
             if (isRightHand) {
                 palmPosition = holdingAvatar->getRightPalmPosition();
diff --git a/interface/src/avatar/AvatarActionHold.h b/interface/src/avatar/AvatarActionHold.h
index fc8baf6dcc..b97ec59780 100644
--- a/interface/src/avatar/AvatarActionHold.h
+++ b/interface/src/avatar/AvatarActionHold.h
@@ -61,7 +61,8 @@ private:
     glm::vec3 _previousPositionalDelta;
 
     glm::vec3 _palmOffsetFromRigidBody;
-    glm::quat _palmRotationFromRigidBody;
+    // leaving this here for future refernece.
+    // glm::quat _palmRotationFromRigidBody;
 };
 
 #endif // hifi_AvatarActionHold_h