WebXR Controls Improvements

[BlaiseRideout]

Motivation and Context

Existing WebXR has limited unintuitive controls and this diff seeks to improve them.

How Has This Been Tested

Tested locally with a Rift S on Chrome 88.0.4324.104

Types of changes

• Docs change / refactoring / dependency upgrade
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have completed my CLA (see CONTRIBUTING)
• I have added tests to cover my changes.
• All new and existing tests passed.

[Skylion007]

@mosra You are probably the best person to review this part. Do we really not have any better bindings exposed via Magnum?

[mosra]

¯\_(ツ)_/¯

Magnum has no JS bindings on its own. Everything has been happening organically here so far, on an as-needed basis.

[eundersander]

FYI, I've reached out to Blaise about making some refactors here. The JS code uses Eigen types (vec4f, etc.), perhaps for legacy reasons.

[Skylion007]

@morsa does Magnum::Quaternion really not have a constructor for Vec4f though? Or at least a util for conversion?

[Skylion007]

Ah nvm, @eundersander posted the answer below.

[mosra]

There's a conversion from/to Eigen's Quaternion class, and possibly Eigen itself could have some way to convert to a 4-component vector on its own.

I'm not providing a conversion to a 4-component vector because it's not clear whether it should be xyzw or wxyz -- each math library uses something else, so it's better to be explicit. Which means doing a conversion using data()[i] also might not be 100% safe -- I don't know what order is the vec4f expected to be in and if it matches the order inside Magnum's Quaternion.

[Skylion007]

So for consistency, rotation is a part of AgentState on the Python side Is it not the on C++ side, @bigbike ?

[Skylion007]

Javascript looks pretty good. I just want to make sure we are leveraging as much C++ code as possible. I don't think those Quaternion utility functions are necessary since I bet they exist inside of Magnum and can be easily accessed. @mosra knows more.

Also on the Python side, rotation is a part of AgentState, so we should make that consistent here as well. @vauduong and @bigbike are currently looking into the C++ agent code

[eundersander]

Can we avoid introducing this new binding? How about implementing in JS:

  setAgentRotation(rotAngle) {
    let state = this.sim.getAgentState();
    state.rotation = WebAssembly.Module.eulerToQuaternion([0, rotAngle, 0])
    this.sim.setAgentState(state);
  }

[BlaiseRideout]

I used that approach originally, but switched to this one because I didn't need to set the whole agent state, so fetching the whole thing and then setting the whole thing when I only need to update one parameter seemed overkill and potentially less performant.

[Skylion007]

Yeah, but that former approach is consistent with our Python AgentState APIs. I'd be surprised if it was a performance bottleneck, do you have benchmarks?

[eundersander]

I agree we need a quaternion multiply. But can we avoid introducing Magnum::Quaternion in the bindings interface? So far, for legacy reasons, we use the eigen vec4f. So I'm suggesting:

vec4f quaternionMultiply(const vec4f& q1, const vec4f& q2) {
  auto product = Magnum::Quaternion(quatf(q1)) * Magnum::Quaternion(quatf(q2));
  return vec4f(product.data()[0], product.data()[1], product.data()[2],
               product.data()[3]);
}

[Skylion007]

This should be defined as custom embind factory constructor, not a static method.

[eundersander]

I'm going to push for another direction here. A nice, easy philosophy to apply everywhere here is: the Javascript class should have the same interface as the C++ class. Or a subset that grows as necessary (but remains a subset). My thinking is that @mosra already put some thought into this class's interface so why second-guess. It also avoids confusion for users learning to use both the Javascript and C++ versions of the class.

This means making fromVec4f a free function, called quaternionFromVec4f or similar.

mul is a special case because Javascript doesn't support operator overloading. I'm okay making that a member, since q1.mul(q2) would closely resemble q1 * q2.

[Skylion007]

I agree with all that except the fact that it should be a static function like above.

[eundersander]

it should be a static function like above.

Can you clarify? which function are we talking about (fromVec4f or mul)? And by static do you mean a static method on the javascript type?

I would argue that Module.Quaternion.fromVec4f (static method on Javascript Quaternion class) is confusing because Magnum::Quaternion::fromVec4f doesn't exist in C++.

If you're advocating that mul should be a static method (not a regular/non-static method), sure, I'm fine with that.

[mosra]

Vaguely (un)related and just FYI -- from what I know, it should be technically possible to create a WebXR application using just C++, without having to replicate a lot of code in JS and plain WebGL (as i saw in #984).

Of course depends on what the target audience is here -- If the WebXR app is meant to be developed, used & extended primarily by JS devs, then having it written in C++ would only add unnecessary roadblocks :)

[eundersander]

setLocalTransform takes a vec4f but you're passing a Quaternion. And the scary thing is that this builds and runs without error. I realize now that Emscripten/Embind/Javascript is not doing full type-checking. You can pass any object to setLocalTransform that happens to get storage as a 4-tuple of floats and you get no runtime error/warning.

[eundersander]

Cleaning this up a bit at a new PR: #1139