Implementation of elbow model for the controller position and rotation.

Implementation of elbow model for the controller position and rotation.

The starting position for the laser is also obtained from the model.

The code was adapted from: https://github.com/googlevr/gvr-unity-sdk/blob/master/Samples/DaydreamLabsControllerPlayground/Assets/GoogleVR/Scripts/Controller/GvrArmModel.cs

BUG=644482
Review-Url: https://codereview.chromium.org/2795793002
Cr-Commit-Position: refs/heads/master@{#467440}
Committed: https://chromium.googlesource.com/chromium/src/+/b0ca33e8d54d93e0dc85e51dbd109e4c9145dfe2

[acondor_, 2017-04-03 17:40:47]

acondor@google.com changed reviewers:
+ bajones@chromium.org, cjgrant@chromium.org, ddorwin@chromium.org,
mthiesse@chromium.org

[acondor_, 2017-04-03 17:40:47]

PTAL

[bajones, 2017-04-03 17:52:19]

On 2017/04/03 17:40:47, acondor wrote:
> PTAL

Patch seems pretty good, though I haven't gone over it line-by-line. The 1.2 SDK
added a "gvr_controller_state_get_position" function, though, which the release
notes say is "reserved for future elbow model simulations." Do we know if it
actually provides anything transform 1.3 or 1.4? If so, we should probably
prefer to use the built in function. (And even if not, we should ensure that our
version of the code can be easily swapped out for the standardized version when
it is available.)

[acondor_, 2017-04-03 19:11:06]

On 2017/04/03 17:52:19, bajones wrote:
> On 2017/04/03 17:40:47, acondor wrote:
> > PTAL
> 
> Patch seems pretty good, though I haven't gone over it line-by-line. The 1.2
SDK
> added a "gvr_controller_state_get_position" function, though, which the
release
> notes say is "reserved for future elbow model simulations." Do we know if it
> actually provides anything transform 1.3 or 1.4? If so, we should probably
> prefer to use the built in function. (And even if not, we should ensure that
our
> version of the code can be easily swapped out for the standardized version
when
> it is available.)

It seems that is not released yet (for reference:
https://developers.google.com/vr/android/release-notes). But note that all the
code for the elbow model is in a separate class which can be removed when the
API function comes available. This class exposes just the same: a direction and
a position.

[cjgrant, 2017-04-03 19:25:00]

Here are some line-by-line comments.  Brandon's point about whether this is is
the right approach should be addressed first.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/elbow_model.cc (right):

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/elbow_model.cc:60: Clampf((angular_velocity -
.2f) / 45.f, 0.f, 0.1f);
0.2f here (and everywhere please).  Ie, not .2f or 2.f

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/elbow_model.cc:72: // Controller's orientation
relative to the player.
s/player/user/ everywhere?

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/elbow_model.cc:83: PointwiseVectorMul({-0.13f,
0.14f, -0.08f}, handed_multiplier_);
The vector here should be a constant as well.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/elbow_model.h (right):

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/elbow_model.h:26: gvr_controller_handedness
handedness = GVR_CONTROLLER_RIGHT_HANDED);
I think use of default arguments is generally discouraged.  Can probably skip it
here.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_math.cc (right):

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.cc:70: float Clampf(float value, float
min, float max) {
std::clamp is a thing, maybe we should use it instead.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.cc:79: return (radians * 180.f) / M_PI;
No braces needed here.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.cc:85: float dot =
Clampf(VectorDot(start, end), -1.f, 1.f);
1.0f etc?

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.cc:210: return ToDegrees(angle_radians);
Could drop the intermediate variable here.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.cc:232: NormalizeQuat(result);
With the quantity of math changes here, it might be a good time to fix our
style.  Methods that modify parameters should accept pointers, but we inherited
code that uses references instead.  It makes lines like this look misleading. 
The alternative is of course returning new values based on const inputs.

Michael, how do you feel about this?

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_math.h (right):

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.h:53: // Spherical Linear Interpolation
// Spherical linear interpolation.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.h:54: gvr::Vec3f QuatSlerp(gvr::Vec3f
start, gvr::Vec3f end, float percent);
Pass start and end by const&?

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.h:76: gvr::Vec3f VectorAdd(const
gvr::Vec3f& a, const gvr::Vec3f& b);
If you're adding comments to methods above, certainly do these as well (anything
non-trivial).

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.h:96: gvr::Quatf FromToRotation(const
gvr::Vec3f& from, const gvr::Vec3f& to);
From the name, I have no idea what this method does.  Maybe FindRotation() would
be better?  It's future explanatory comment will confirm I'm sure. :)

[mthiesse, 2017-04-11 16:34:08]

We're going to need this patch for short-term releases at the very least, if not
long-term releases. We should get this landed.

[acondor_, 2017-04-19 18:44:27]

The suggestions where handled. But now I need to do a rebase.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/elbow_model.cc (right):

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/elbow_model.cc:60: Clampf((angular_velocity -
.2f) / 45.f, 0.f, 0.1f);
On 2017/04/03 19:24:59, cjgrant wrote:
> 0.2f here (and everywhere please).  Ie, not .2f or 2.f 

Done.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/elbow_model.cc:72: // Controller's orientation
relative to the player.
On 2017/04/03 19:24:59, cjgrant wrote:
> s/player/user/ everywhere?

Done.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/elbow_model.cc:83: PointwiseVectorMul({-0.13f,
0.14f, -0.08f}, handed_multiplier_);
On 2017/04/03 19:24:59, cjgrant wrote:
> The vector here should be a constant as well.

Done.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/elbow_model.h (right):

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/elbow_model.h:26: gvr_controller_handedness
handedness = GVR_CONTROLLER_RIGHT_HANDED);
On 2017/04/03 19:24:59, cjgrant wrote:
> I think use of default arguments is generally discouraged.  Can probably skip
it
> here.

Done.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_math.cc (right):

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.cc:70: float Clampf(float value, float
min, float max) {
On 2017/04/03 19:25:00, cjgrant wrote:
> std::clamp is a thing, maybe we should use it instead.

I checked, and it is a C++17 feature.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.cc:79: return (radians * 180.f) / M_PI;
On 2017/04/03 19:24:59, cjgrant wrote:
> No braces needed here.

Done.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.cc:85: float dot =
Clampf(VectorDot(start, end), -1.f, 1.f);
On 2017/04/03 19:25:00, cjgrant wrote:
> 1.0f etc?

Done.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.cc:210: return ToDegrees(angle_radians);
On 2017/04/03 19:24:59, cjgrant wrote:
> Could drop the intermediate variable here.

Done.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.cc:232: NormalizeQuat(result);
On 2017/04/03 19:24:59, cjgrant wrote:
> With the quantity of math changes here, it might be a good time to fix our
> style.  Methods that modify parameters should accept pointers, but we
inherited
> code that uses references instead.  It makes lines like this look misleading. 
> The alternative is of course returning new values based on const inputs.
> 
> Michael, how do you feel about this?

The change was already done. I just need to rebase.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_math.h (right):

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.h:53: // Spherical Linear Interpolation
On 2017/04/03 19:25:00, cjgrant wrote:
> // Spherical linear interpolation.

Done.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.h:54: gvr::Vec3f QuatSlerp(gvr::Vec3f
start, gvr::Vec3f end, float percent);
On 2017/04/03 19:25:00, cjgrant wrote:
> Pass start and end by const&?

Done.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.h:76: gvr::Vec3f VectorAdd(const
gvr::Vec3f& a, const gvr::Vec3f& b);
On 2017/04/03 19:25:00, cjgrant wrote:
> If you're adding comments to methods above, certainly do these as well
(anything
> non-trivial).

Done.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_math.h:96: gvr::Quatf FromToRotation(const
gvr::Vec3f& from, const gvr::Vec3f& to);
On 2017/04/03 19:25:00, cjgrant wrote:
> From the name, I have no idea what this method does.  Maybe FindRotation()
would
> be better?  It's future explanatory comment will confirm I'm sure. :)

Done.

[asimjour1, 2017-04-19 18:58:11]

asimjour@chromium.org changed reviewers:
+ asimjour@chromium.org

[asimjour1, 2017-04-19 18:58:12]

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/elbow_model.cc (right):

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/elbow_model.cc:104: lerp_suppresion * (0.4f +
0.6f * extension_ratio * kExtensionWeight);
please move the magic numbers from here and define them as const with proper
name.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/elbow_model.cc:121: void
ElbowModel::UpdateTransparency(const UpdateData& update) {
IIUC alpha_value/opacity can be obtained from the position, so it can be
calculated inside of the renderer. I don't see any reason to manage the opacity
inside of the elbow model.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_controller.cc (right):

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_controller.cc:203: float delta_time_seconds =
this conversion is repeated a few times, could you please use a function for
this.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_controller.h (right):

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_controller.h:29: static constexpr float
kErgoAngleOffset = 0.26f;
Why does it need to be static and outside of controller?

[acondor_, 2017-04-21 21:04:32]

Rebase performed and suggestions addressed.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/elbow_model.cc (right):

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/elbow_model.cc:104: lerp_suppresion * (0.4f +
0.6f * extension_ratio * kExtensionWeight);
On 2017/04/19 18:58:12, asimjour1 wrote:
> please move the magic numbers from here and define them as const with proper
> name.

Done.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/elbow_model.cc:121: void
ElbowModel::UpdateTransparency(const UpdateData& update) {
On 2017/04/19 18:58:12, asimjour1 wrote:
> IIUC alpha_value/opacity can be obtained from the position, so it can be
> calculated inside of the renderer. I don't see any reason to manage the
opacity
> inside of the elbow model.

Actually, this is an animation (so it depends on time, not just distance). The
renderer is not supposed to know about time, unless we change that.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_controller.cc (right):

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_controller.cc:203: float delta_time_seconds =
On 2017/04/19 18:58:12, asimjour1 wrote:
> this conversion is repeated a few times, could you please use a function for
> this.

Done.

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_controller.h (right):

https://codereview.chromium.org/2795793002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_controller.h:29: static constexpr float
kErgoAngleOffset = 0.26f;
On 2017/04/19 18:58:12, asimjour1 wrote:
> Why does it need to be static and outside of controller?

Removed the static, but it's also used in VrShellGl to determine laser direction
and in vr_controller to displace the starting point of the beam.

[acondor_, 2017-04-26 17:12:01]

acondor@google.com changed reviewers:
- ddorwin@chromium.org

[acondor_, 2017-04-26 17:12:02]

Friendly ping

[bajones, 2017-04-26 17:37:35]

On 2017/04/26 17:12:02, acondor wrote:
> Friendly ping

LGTM

[mthiesse, 2017-04-26 17:46:48]

lgtm

(I didn't review the math in detail, I'm trusting it's correct)

https://codereview.chromium.org/2795793002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller.cc (right):

https://codereview.chromium.org/2795793002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller.cc:443: float
VrController::DeltaTimeSeconds() {
nit: Take last_timestamp_nanos_ in as a parameter for clarity (that this
function isn't also updating the last_timestamp_nanos_.

https://codereview.chromium.org/2795793002/diff/40001/device/vr/vr_math.h
File device/vr/vr_math.h (right):

https://codereview.chromium.org/2795793002/diff/40001/device/vr/vr_math.h#new...
device/vr/vr_math.h:44: vr::Quatf DEVICE_VR_EXPORT InvertedQuat(const vr::Quatf&
quat);
nit: InvertQuat

[acondor_, 2017-04-26 18:42:26]

https://codereview.chromium.org/2795793002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller.cc (right):

https://codereview.chromium.org/2795793002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller.cc:443: float
VrController::DeltaTimeSeconds() {
On 2017/04/26 17:46:48, mthiesse wrote:
> nit: Take last_timestamp_nanos_ in as a parameter for clarity (that this
> function isn't also updating the last_timestamp_nanos_.

Done.

https://codereview.chromium.org/2795793002/diff/40001/device/vr/vr_math.h
File device/vr/vr_math.h (right):

https://codereview.chromium.org/2795793002/diff/40001/device/vr/vr_math.h#new...
device/vr/vr_math.h:44: vr::Quatf DEVICE_VR_EXPORT InvertedQuat(const vr::Quatf&
quat);
On 2017/04/26 17:46:48, mthiesse wrote:
> nit: InvertQuat

Done.

[mthiesse, 2017-04-26 18:55:23]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-04-26 18:55:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2795793002/#ps60001
(title: "nit")

[commit-bot: I haz the power, 2017-04-26 18:56:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 20:35:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493232923408930,
"parent_rev": "70f501b9dc3cd0ed78e99ec63514700c31186666", "commit_rev":
"b0ca33e8d54d93e0dc85e51dbd109e4c9145dfe2"}

[commit-bot: I haz the power, 2017-04-26 20:36:01]

Description was changed from

==========
Implementation of elbow model for the controller position and rotation.

The starting position for the laser is also obtained from the model.

The code was adapted from:
https://github.com/googlevr/gvr-unity-sdk/blob/master/Samples/DaydreamLabsCon...

BUG=644482
==========

to

==========
Implementation of elbow model for the controller position and rotation.

The starting position for the laser is also obtained from the model.

The code was adapted from:
https://github.com/googlevr/gvr-unity-sdk/blob/master/Samples/DaydreamLabsCon...

BUG=644482

Review-Url: https://codereview.chromium.org/2795793002
Cr-Commit-Position: refs/heads/master@{#467440}
Committed:
https://chromium.googlesource.com/chromium/src/+/b0ca33e8d54d93e0dc85e51dbd10...
==========

[commit-bot: I haz the power, 2017-04-26 20:36:02]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b0ca33e8d54d93e0dc85e51dbd10...