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

Added support for windows mixed reality controllers #3013

Merged
merged 11 commits into from
Sep 8, 2017

Conversation

leweaver
Copy link
Contributor

Description:
Added support for windows motion controllers, with tests, which fixed one or two bugs in tracked-controls class.

Also consolidated the logic used in tracked-controls utils and component for isControllerPresent to a single function.

This PR does not yet have the final controller GLB asset files, - I do plan on uploading those to the a-frame CDN however I am still waiting for the OK internally here.

Changes proposed:

  • Added error callback on glTF-model component
  • Windows Motion Controller support
  • Consolidation of tracked-controls duplicated logic to a single utility function

var trackedControls;
el.setAttribute('tracked-controls', '');
trackedControls = el.components['tracked-controls'];
trackedControls.controller = {id: 'OpenVR Vive', connected: true};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is OpenVR Vive really how the controllers are appearing these days?

Copy link
Member

@dmarcos dmarcos Aug 31, 2017

Choose a reason for hiding this comment

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

In Firefox is OpenVR Gamepad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to use a real value, I'll update to that.

@machenmusik
Copy link
Contributor

Nice - I had actually done a version relying on tracked-controls changes from #2583 which are similar to those you made here, but I didn't have actual controller models. Wonder if it makes sense to merge them

@@ -26,6 +26,8 @@ module.exports.Component = registerComponent('gltf-model', {
self.model.animations = gltfModel.animations;
el.setObject3D('mesh', self.model);
el.emit('model-loaded', {format: 'gltf', model: self.model});
}, undefined /* onProgress */, function gltfFailed () {
el.emit('model-error', {format: 'gltf', src: src});
Copy link
Member

@dmarcos dmarcos Aug 31, 2017

Choose a reason for hiding this comment

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

Should we throw an error instead of an event? A similar scenario can be found in the text component: https://github.com/aframevr/aframe/blob/master/src/components/text.js#L391

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could do both? I listen to the error message to try loading a different (fallback) asset file, I think it could be useful in other contexts for the caller to know that the file load failed.

Copy link
Member

Choose a reason for hiding this comment

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

yes, we could have both

Copy link
Member

Choose a reason for hiding this comment

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

+1 for both. The errors should be helpful for developers debugging, who may not know to listen for the event.

But my inclination would be to log an error or warning, through the debug util, rather than throwing something that can't be caught. See obj-model.

Copy link
Member

Choose a reason for hiding this comment

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

Cool! Let's add both then 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do both, thanks!

hideDisconnected: {default: true}
},

_mapping: {
Copy link
Member

Choose a reason for hiding this comment

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

We don't use underscore to indicate private variables. Can we keep just mapping?

this.bindMethods();

// Cache for submeshes that we have looked up by name.
this._loadedMeshInfo = {
Copy link
Member

Choose a reason for hiding this comment

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

no underscore on variable name

var warn = utils.debug('components:windows-motion-controls:warn');

// TODO: Point to final glb assets once they are hosted. For now, place them in this hard coded directory.
var MODEL_BASE_URL = '/examples/assets/models/controllers/wmr/';
Copy link
Member

Choose a reason for hiding this comment

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

As you pointed out we should host the models in the CDN https://github.com/aframevr/assets/tree/gh-pages/controllers

Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity, to get the models in the CDN, I believe one does a pull request against aframevr/assets with the appropriate changes.

Copy link
Member

Choose a reason for hiding this comment

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

Correct! Just a PR against the gh-pages branch on https://github.com/aframevr/assets/tree/gh-pages/controllers

Copy link
Member

Choose a reason for hiding this comment

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

gh-pages so they get deployed on the CDN

Copy link
Contributor

Choose a reason for hiding this comment

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

specific example for reference, this was vive-tracker: aframevr/assets#21

}

// Hand
var filename;
Copy link
Member

Choose a reason for hiding this comment

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

define variables at the top


// Hand
var filename;
if (hand === 'left') filename = MODEL_LEFT_FILENAME;
Copy link
Member

@dmarcos dmarcos Aug 31, 2017

Choose a reason for hiding this comment

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

curly braces around if clause

if (hand === 'left') { filename = MODEL_LEFT_FILENAME; }


// TODO: Point to final glb assets once they are hosted. For now, place them in this hard coded directory.
var MODEL_BASE_URL = '/examples/assets/models/controllers/wmr/';
var MODEL_LEFT_FILENAME = 'left.glb';
Copy link
Member

@dmarcos dmarcos Aug 31, 2017

Choose a reason for hiding this comment

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

it might be better to consolidate names in an object:

var MODEL_FILENAMES = { left: 'left.glb`, right: 'right.glb', default: 'universal.glb' };


// Hand
var filename;
if (hand === 'left') filename = MODEL_LEFT_FILENAME;
Copy link
Member

@dmarcos dmarcos Aug 31, 2017

Choose a reason for hiding this comment

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

if we consolidate the filenames in an object as described above we can compress this if statement into:

filename = MODEL_FILES[hand] || MODEL_FILES.default;


onModelError: function (evt) {
var defaultUrl = this.createControllerModelUrl(true);
if (evt.detail.src !== defaultUrl) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a default model when we already have hand and left ones? If another component like hand-controls decides to use a custom model (hence the model property === false) is responsibility of that higher level component to handle model errors. I might be misunderstanding something though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic only applies in the case where the model property === true, and is a separate concept to the default hand... our device ID's have a suffix which represents a particular hardware form factor (as exposed by the underlying Windows Mixed Reality API).
Spatial Controller (Spatial Interaction Source) 045E-065A

While there is currently a single device (the Motion Controller, 045E-065A), the underlying windows API supports addition of new hardware form factors. As such, it is possible that new hardware exists (with a new ID suffix) before we upload a corresponding asset - which is why we fallback to a 'default' GLB file.

Hopefully I understand your comment correctly?

Copy link
Member

Choose a reason for hiding this comment

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

I see. I have not seen the default model. How does it look like?

Copy link
Contributor

Choose a reason for hiding this comment

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

in other cases we have used idPrefix e.g. if they will all be "Spatial Controller " but then all bets are off. I think the version I did uses that strategy, let me find where I put that code and put it somewhere for reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As an alternative, what if there is an additional http request prior to model download that retrieves a manifest file with mappings of suffix -> filename, with an entry for default?

My main motivator here is that we want to avoid making changes to the core library if we need to add new, radically different, models - since people don't update library versions after releasing their apps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on experience, I prefer each controller component to map to one and just one physical device (with its corresponding model).

Tactically speaking, maybe it could be as simple as a generic component with the basic functionality (I had called mine spatial-controls along prior naming convention lines) matching on idPrefix, and then specific components that matched more specific gamepad id strings with model mappings / manipulations?

Copy link
Member

Choose a reason for hiding this comment

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

Let's merge this first since it's higher priority than tracker support for 0.7.0

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd agree, this is more important, except we can't merge without the models, can we?
I think #2583 is ready to merge now.
If we have a little time, I'd rebase a fork off #2583 and PR back. I expect the delta is not large

Copy link
Member

Choose a reason for hiding this comment

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

I've been working on a separare PR for tracker support to implement a spectator camera. I prefer to keep #2583 on hold and also not slow this one down.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK - would be happy to have this contribution from Microsoft - but if you will be doing tracker support, you may need to revisit #3013 (comment) -- the current changes in this PR make it impossible to match no-handedness

if (rootNode) {
// Button Meshes
for (i = 0; i < this._mapping.buttons.length; i++) {
var buttonMeshName = this._mapping.buttonMeshNames[this._mapping.buttons[i]];
Copy link
Member

Choose a reason for hiding this comment

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

variable defined at the top

continue;
}

var buttonMesh = rootNode.getObjectByName(buttonMeshName);
Copy link
Member

Choose a reason for hiding this comment

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

variable defined at the top

continue;
}

var buttonMeshInfo = {
Copy link
Member

Choose a reason for hiding this comment

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

variable defined at the top


// Axis Meshes
for (i = 0; i < this._mapping.axisMeshNames.length; i++) {
var axisMeshName = this._mapping.axisMeshNames[i];
Copy link
Member

Choose a reason for hiding this comment

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

variable at the top

continue;
}

var axisMesh = rootNode.getObjectByName(axisMeshName);
Copy link
Member

Choose a reason for hiding this comment

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

variable at the top

continue;
}

var axisMeshInfo = {
Copy link
Member

Choose a reason for hiding this comment

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

variable at the top


// Convert from gamepad value range (-1 to +1) to lerp range (0 to 1)
var lerpValue = axisValue * 0.5 + 0.5;
target.setRotationFromQuaternion(min.quaternion.clone().slerp(max.quaternion, lerpValue));
Copy link
Member

@dmarcos dmarcos Aug 31, 2017

Choose a reason for hiding this comment

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

To appease the garbage collector we should avoid allocating new variables and reuse instead as much as we can. We usually use a closure pattern for temporary variables that don't hold state (https://github.com/aframevr/aframe/blob/master/src/components/cursor.js#L168). In this case I think we could do:

lerpAxisTransform: (function () {
 var quaternion = new THREE.Quaternion();
 return function (axis, axisValue) {
   ...
      target.setRotationFromQuaternion(quaternion.copy(min.quaternion).slerp(max.quaternion, lerpValue));
   ...
 }
});

Copy link
Contributor

Choose a reason for hiding this comment

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

my personal preference is to have them as unpublicized properties on the component instance, so that other functions can reuse them, but yes I think the vast majority of code uses closure form

Copy link
Member

@dmarcos dmarcos Aug 31, 2017

Choose a reason for hiding this comment

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

Three things:

  1. If it's just a temporary value is better to use a closure. By having the variable on the closure you share one single variable across all the object instances.
  2. In the this. it pollutes the object and it's tempting to use it to store state that will be destroyed if multiple methods use the variable for partial calculations.
  3. If those variables are not longer useful they will be hanging from the this. it's easy to forget and leave them there orphan.

Copy link
Contributor

Choose a reason for hiding this comment

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

Completely agree with 2 and 3.
On 1, I thought we ran into past issues with multiple instances / invocations there, but may be misremembering.

var max = buttonMeshInfo.pressed;
var target = buttonMeshInfo.value;

target.setRotationFromQuaternion(min.quaternion.clone().slerp(max.quaternion, buttonValue));
Copy link
Member

Choose a reason for hiding this comment

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

Same as above. Try to avoid allocations and reuse

for (i = 0; i < controllers.length; i++) {
controller = controllers[i];
// Determine if the controller ID matches our criteria
if (filterIdPrefix && controller.id.indexOf(filterIdPrefix) === -1) continue;
Copy link
Member

Choose a reason for hiding this comment

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

curly brace around if statement:

if (filterIdPrefix && controller.id.indexOf(filterIdPrefix) === -1) { continue; }
if (!filterIdPrefix && controller.id !== filterIdExact) { continue; }

}

return isPresent;
return null;
Copy link
Member

Choose a reason for hiding this comment

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

probably better return undefined for consistency with other functions across the code base

if (filterIdPrefix && controller.id.indexOf(filterIdPrefix) === -1) continue;
if (!filterIdPrefix && controller.id !== filterIdExact) continue;

if (filterHand) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe simpler like this? And maybe more accurate if there are several controllers with the same handedness. I think it cannot happen today with the available hardware but who knows if one day 2 players can play in the same machine with two pairs of controllers

// If the hand filter and controller handedness are defined we compare them.
if (filterHand && controller.hand && filterHand !== controller.hand) { continue; }

Copy link
Member

Choose a reason for hiding this comment

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

Maybe also more consistent with the negative statements above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point re: multiple pairs, will make appropriate changes and add tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

from a little experience - undefined controller hand means none, but undefined filterHand means don't care, which is a completely different thing. See #2583

Copy link
Contributor

Choose a reason for hiding this comment

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

you want to be able to specify a filter for no-handedness (vive-tracker), as well as for specific handedness (various handed controllers), as well as don't care (don't care whether the 3DOF controller is left/right/none, just give me the pose)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the utils function change is https://github.com/aframevr/aframe/pull/2583/files#diff-24b039fab259237004f42c5bf3856cdfR69 -- use DEFAULT_HANDEDNESS to replace controller hand of undefined with an actual string to match

Copy link
Contributor

Choose a reason for hiding this comment

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

but actually I think DEFAULT_HANDEDNESS was supposed to be none there

Copy link
Member

Choose a reason for hiding this comment

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

We can revisit this later for tracker support.

Copy link
Contributor

@machenmusik machenmusik Sep 1, 2017

Choose a reason for hiding this comment

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

generally though, something like:

// Only try to match filterHand if a filter value is specified.
if (filterHand) {
  // If undefined controller hand, for clarity, use string 'none' to compare against filter value
  if ((controller.hand ? controller.hand : 'none') !== filterHand) { continue; }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

If the filter does not reject based on hand value, I believe we want to fall through against controller index check (if any) so you can match against the second set of left and right hand controllers, or the second or third trackers.

- Added concept of controller pairs, with the addition of a 'pair' property on the windows-motion-controls class. This allows you to connect more than 2 controllers at a time.
- Added closures to prevent surplus allocations
- Minor formatting tweaks
@machenmusik
Copy link
Contributor

FYI, working implementation on top of #2583 is here:
https://github.com/chenzlabs/aframe/tree/support-spatial-controller

if (filterHand && controller.hand && filterHand !== controller.hand) { continue; }

// If we have detected an unhanded controller and the component was asking for a particular hand,
// we need to treat the controllers in the array as pairs of controllers. This effectively means that we
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure this is needed or desirable; originally there were issues with browsers not uniformly exposing handedness, but that is gone now, so if the filter is requesting a hand, we should require one.

Still need coverage for the case where we want to match no-handedness, my suggestion is to use the string none such as the following:

if (filterHand && (filterHand !== (controller.hand ? controller.hand : 'none'))) { continue; }

Copy link
Member

@dmarcos dmarcos Sep 1, 2017

Choose a reason for hiding this comment

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

Why do we need none? In the absence of a filterHand the condition is a pass through.

Copy link
Contributor

@machenmusik machenmusik Sep 1, 2017

Choose a reason for hiding this comment

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

For example - trackers do not have handedness. Also, if you want to use a third vive controller as spectator camera, my understanding is that the first two get hands, then the rest are no-handed (hand undefined) -- maybe that has changed. But if it has not, you would like to be able to specify idPrefix OpenVR and hand none so whether it's a third controller or a tracker, it doesn't matter

Copy link
Contributor

Choose a reason for hiding this comment

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

(This bit is inside tracked-controls utils that gets used for everything with a pose, not just within the windows-motion-controls component)

Copy link
Member

@dmarcos dmarcos Sep 1, 2017

Choose a reason for hiding this comment

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

Let's deal with the problem at hand and we will accommodate those no handedness / 3rd controler edge cases when we integrate the trackers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I am starting to get confused too!

If, as you say, all browsers are now exposing the hand property correctly - it is very tempting to update the vive controls class to use that. We do still need to consider the 'unhanded case' though as
the underlying windows API that drives the Motion Controllers support in Edge's WebVR does support unhanded controllers, even though there aren't any on the market at this time.
(The current Motion Controllers are, as you observed, handed). If our platform exposes an unhanded controller, the gamepad 'hand' property will be an empty string: ''.

The reason I have the (yes, cryptic) snippet is that the underlying platform also supports more than 2 unhanded controls connected at once. So in theory the platform could tell Edge that the user has 4 or more unhanded controllers connected. The question is - do we want these to work with the hand-controls component? If so, we need them to work with a component that has requested a particular hand.

If so, we need to keep the logic here that puts unhanded controllers into pairs of left and right, based on their index.

Regarding the number of connected handed controllers, it is easy to connect 4 handed controllers today using two physical, and two simulated via the windows mixed reality simulator.

Copy link
Member

Choose a reason for hiding this comment

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

@machenmusik Do you think the code we have here is reasonable even tough there might be an edge case or two we're missing? Can we go with this and refine when integrating the Vive trackers and test on different browsers if our assumptions about handeness still apply?

Copy link
Contributor

Choose a reason for hiding this comment

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

@leweaver When doing #2583 I did in fact switch over vive-controls to use handedness, no more weird index stuff.

@dmarcos When doing #2583, I had tested against all supported controllers as well as vive-trackers, which is why I am personally more comfortable with the changes to tracked-controls utils there. (Although it has been so long that other changes may have crept in that break it now, I suppose.)

On unhanded controllers - it sounds as though there will be WinMR equivalents of vive-trackers, in which case the main changes in #2583 may prove useful as well, namely handling no-handedness (although it sounds as though we now have empty string to contend with as well as undefined) and also adding full rotation offset to tracked-controls, not just Z (because the native orientation of vive-tracker is somewhat different from canonical model).

I am happy to welcome @leweaver to join the A-Frame project and thus take over WinMR controller support, just trying to share learnings from doing the last few. Feel free to take whatever is worthwhile from #2583 and the spatial-controls branch I had done, if it is helpful.

As for whether unhandled controllers should be allowed to match handed requests, that's a bit of a philosophical question. The dynamic Vive wand case is simpler, as the controllers will in all likelihood gain handedness eventually. In the WinMR and Vive tracker case, it's less clear, although I would expect unhandled WinMR controllers to have different id strings and thus be handled by a related but separate component.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's keep this part of the PR as is and improve it later in follow up PRs

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, we need to keep the logic here that puts unhanded controllers into pairs of left and right, based on their index.

The problem I see with this is that all unhandled controllers may not be created equal. Let's assume that unhandled controllers include totems that can be used in either hand (with no dynamic hand assignment), and also other things like gun controllers as are popular with PSVR. The user has limited or no control over enumerated order. What if the enumerated order is totem, gun, totem? Then you have two left-handed totems and a gun for a right hand, which may not be the intended outcome...

@dmarcos
Copy link
Member

dmarcos commented Sep 1, 2017

I missed one important thing. We should incorporate the windows mixed reality controlers into laser-controls too which are the A-Frame cross device universal controls (3DOF and 6DOF). It should be pretty straight forward. Just add the controls to the list:

https://github.com/aframevr/aframe/blob/master/src/components/laser-controls.js#L18

And provide the appropriate event mapping and offset for the raycaster (so it's casted from the proper position of the controller):

https://github.com/aframevr/aframe/blob/master/src/components/laser-controls.js#L46

@dmarcos
Copy link
Member

dmarcos commented Sep 1, 2017

Thanks for the patience. This is a big patch with a lot to discuss.

@dmarcos
Copy link
Member

dmarcos commented Sep 1, 2017

We're pretty close though

@leweaver
Copy link
Contributor Author

leweaver commented Sep 2, 2017

I've updated the PR to add laser-controls support. I would have preferred to do it without emitting the new 'controllerdisplayready' event; however I couldn't get the line-component to update the start/end positions once they have been set once. Have you encountered that?

Note - that the offsets and rotations default to zero/forward; but can be overridden by the data from the glTF model file (we store the offsets on specifically named nodes in the tree, similar to how we do the button transforms).

@@ -1,5 +1,7 @@
var registerComponent = require('../core/component').registerComponent;
var THREE = require('../lib/three');
var utils = require('../utils/');
var warn = utils.debug('components:windows-motion-controls:warn');
Copy link
Member

@donmccurdy donmccurdy Sep 2, 2017

Choose a reason for hiding this comment

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

This should be:

var warn = utils.debug('components:gltf-model:warn');

...since the warning will appear for any glTF model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, copy-paste error. Will fix!

@@ -26,6 +28,9 @@ module.exports.Component = registerComponent('gltf-model', {
self.model.animations = gltfModel.animations;
el.setObject3D('mesh', self.model);
el.emit('model-loaded', {format: 'gltf', model: self.model});
}, undefined /* onProgress */, function gltfFailed () {
warn('Failed to glTF-model: ' + src);
Copy link
Member

Choose a reason for hiding this comment

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

The gltfFailed callback will receive an argument, error — IMO the warning and event should use that message, as it's probably more informative than the URL. Suggested:

var message = error ? error.message : 'Failed to load glTF model';
warn(message);
el.emit('model-error', {format: 'gltf', message: message});

@machenmusik
Copy link
Contributor

Hi folks, where are we on this one? Going back to the original description of changes, it seems as though the priority order is this...

Changes proposed:

Windows Motion Controller support

This we definitely want ASAP, for the currently publicized handed controllers at least. I think that only a small subset of this PR is needed for that.

Consolidation of tracked-controls duplicated logic to a single utility function

I think much of this PR is for this consolidation actually. Do we need it? We have been talking about refactoring, this could be first step.

Added error callback on glTF-model component

Nice to have.

| trackpadup | Trackpad released. |
| trackpadchanged | Trackpad button changed. |
| trackpadmoved | Trackpad axis moved. |
| controllerdisplayready | The model file is loaded and completed parsing. |
Copy link
Member

Choose a reason for hiding this comment

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

Maybe controllermodelready is a better name?


if (evt.detail.name === 'windows-motion-controls') {
var motionControls = el.components['windows-motion-controls'];
raycasterConfig = utils.extend(raycasterConfig, motionControls.rayOrigin);
Copy link
Member

Choose a reason for hiding this comment

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

Can we pass the rayOrigin in the evt.detail object? This way we don't have to check for windows-motion-controls We should maybe move all the controllers to use a controllermodelready event for consistency. Maybe better in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the pattern of passing the ray details in the event, will modify to follow that pattern. Happy to also modify the others, just let me know if you would prefer that in a separate PR once this is completed.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! We can do it in a separate PR so we move things along.

@dmarcos
Copy link
Member

dmarcos commented Sep 5, 2017

This is pretty close! I think some rethinking refactoring is due to the controllers API after 0.7.0 release. The remaining changes If I have not missed anything are:
https://github.com/aframevr/aframe/pull/3013/files#r137131772
https://github.com/aframevr/aframe/pull/3013/files#r136705108
https://github.com/aframevr/aframe/pull/3013/files#r136705073
https://github.com/aframevr/aframe/pull/3013/files#r137129564

@leweaver
Copy link
Contributor Author

leweaver commented Sep 6, 2017

Thanks all for the reviewing! I've pushed another revision that should address those 4 changes.

// The controllermodelready event contains a rayOrigin that takes into account
// offsets specific to the loaded model.
if (evt.detail.rayOrigin) {
raycasterConfig = utils.extend(raycasterConfig, evt.detail.rayOrigin);
Copy link
Member

@dmarcos dmarcos Sep 6, 2017

Choose a reason for hiding this comment

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

is it clear to do?

raycasterConfig.rayOrigin = evt.detail.rayOrigin;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you'd need to do something like..

raycasterConfig.origin = evt.detail.rayOrigin.origin
raycasterConfig.direction = evt.detail.rayOrigin.direction

but we expand that out if preferred?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, probably better to make it explicit to avoid surprises. nothing hidden 🙂

@leweaver
Copy link
Contributor Author

leweaver commented Sep 6, 2017

@dmarcos - Something I have noticed is that the controllers are quite dark in some scenes. It would be nice to have a high-intensity directional light (position: 0, 1, 0) that only lights the controller, not the rest of the scene. I can't see an obvious way to do this, have any tricks?

@dmarcos
Copy link
Member

dmarcos commented Sep 6, 2017

@leweaver Is there anything that can be done at the model / materials level? Adding a light to the controller is a possibility but maybe too prescriptive and confusing when authors want to have control of the visual appearance of the scene. If nothing can be done at the material level we maybe consider add a light property to the windows-motion-controls component that it's false by default. This way devs that want to turn the lights on can opt into it.

@dmarcos
Copy link
Member

dmarcos commented Sep 7, 2017

I believe this is the last pending change? https://github.com/aframevr/aframe/pull/3013/files#r137319556

@leweaver
Copy link
Contributor Author

leweaver commented Sep 7, 2017

That (which I'll get done now) - and also getting controller assets uploaded to the aframevr/assets repo so that we can modify the URL at the top of the file: windows-motion-controls.js#L12
- I put a note this morning on aframevr/assets#1 related to this.

Lewis Weaver added 2 commits September 7, 2017 16:00
…nected/reconnected.

Updated MODEL_BASE_URL to point to aframe CDN.
@leweaver
Copy link
Contributor Author

leweaver commented Sep 8, 2017

I believe I am finished with my changes to this PR. Do I also need to also include the built files under dist/?

@dmarcos
Copy link
Member

dmarcos commented Sep 8, 2017

Thanks! 🥇 No need to include the built files.

@dmarcos
Copy link
Member

dmarcos commented Sep 8, 2017

@leweaver Will you open a separate PR to point the models URL whenever we merge them on the assets repo?

@dmarcos
Copy link
Member

dmarcos commented Sep 8, 2017

Thank you!

@leweaver
Copy link
Contributor Author

leweaver commented Sep 8, 2017

Sorry forgot to hit enter on this comment box! As you probably noticed, right now this PR version is pointing to: https://cdn.aframe.io/controllers/microsoft/ - I have just submitted aframevr/assets#29 so the URL should be valid soon.

Thanks very much for merging in!

@leweaver leweaver deleted the msft/wmr-motion-controls branch September 8, 2017 02:15
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.

4 participants