aframevr / aframe

:a: Web framework for building virtual reality experiences.
https://aframe.io/
MIT License
16.61k stars 3.94k forks source link

Only one steamvr controller visible #4042

Open InfiniteLee opened 5 years ago

InfiniteLee commented 5 years ago

Description: I'm not sure exactly what changed but it seems that now when using steamvr, one controller is always hidden. I've traced this to https://github.com/aframevr/aframe/blob/65952d6133ff23f542e43beb3ae587d66a971c21/src/components/tracked-controls-webvr.js#L107 which relies on a controller being found in https://github.com/aframevr/aframe/blob/bc577c968cf94e78f0706cae99148d254d370eba/src/utils/tracked-controls.js#L115

Because 1 is passed in for filterControllerIndex when looking for the right controller, this causes the condition here: https://github.com/aframevr/aframe/blob/bc577c968cf94e78f0706cae99148d254d370eba/src/utils/tracked-controls.js#L145 to never be true.

The simplest fix I can think of is removing https://github.com/aframevr/aframe/blob/65952d6133ff23f542e43beb3ae587d66a971c21/src/components/tracked-controls-webvr.js#L100 as it seems extraneous (and wrong), though I'm not entirely sure that is the correct fix or not. My suspicion is that the bug where the controller is not found has been around for a while but was made only "visible" because of this commit: https://github.com/aframevr/aframe/commit/7754b2c478369aa4dbe1d8730788d8335ecd2ab8#diff-efa22db2dae70644518618ea4dbfde3e

dmarcos commented 5 years ago

This was fixed by https://github.com/aframevr/aframe/pull/4024

Does master work? https://cdn.jsdelivr.net/gh/aframevr/aframe@f241c3a909e89b248361afb45c77256eb490ccc0/dist/aframe-master.min.js

InfiniteLee commented 5 years ago

I didn't see that, yea that should fix it for some cases. However in the case that someone actually does specify controller to be non-default (non-zero) in tracked-controls, I believe this bug will still occur. Given the stated purpose of that property ("Index of controller in array returned by Gamepad API"), it doesn't seem unreasonable that would happen. I also still think that it is still invalid to use that value for filterControllerIndex when calling findMatchingControllerWebVR. With #4024, the issue is just being avoided by virtue of that value being kept to the default value of 0. As such, I still think that it should be removed. I would also point out this: https://github.com/aframevr/aframe/blob/65952d6133ff23f542e43beb3ae587d66a971c21/src/components/tracked-controls-webvr.js#L55 which makes it sound like this.targetControllerNumber is what is supposed to be passed into findMatchingControllerWebVR since that property doesn't seem to get used anywhere else, however for the reasons above I believe that initializing this value to this.data.controller is incorrect. Perhaps that value should be exposed as a new property in tracked-controls?

dmarcos commented 5 years ago

@InfiniteLee Are you using tracked-controls directly? vendor specific controls (vive-controls, oculus-touch-controls...) are the ones that should be normally used.

Do you have any example where things are not working as expected after #4024?

dmarcos commented 5 years ago

@InfiniteLee The rational of #4024 is that as opposed to Oculus Touch where each controller has a fixed handennes, for Vive wands is determined at runtime and might not even present when system cannot decide. We use the Gamepads list index instead. That had been the behavior since 0.7.0 or so but changed by mistake with WebXR work.

InfiniteLee commented 5 years ago

Ah ok, I think I understand now. I misread what #4024 was doing (I thought it was removing controller not hand), and now I see how this is all supposed to work. I'll close this now because I believe that including #4204, the bug is now on our side. Thanks!

InfiniteLee commented 5 years ago

@dmarcos Actually this is still broken, but in a different way. Since hand is no longer being checked for, only index is being checked, and index is inverted in vive-controls. https://github.com/aframevr/aframe/blob/bc577c968cf94e78f0706cae99148d254d370eba/src/components/vive-controls.js#L112 This appears to be flipped, as when steamvr has two controllers connected, the left hand is index 0 and right is index 1. This causes the hands to be assigned inverse to what steam believes the hands to be.

dmarcos commented 5 years ago

Any link to reproduce?

InfiniteLee commented 5 years ago

Yep, check the showcase/tracked-controls example in aframe master. You'll see the hands are inverted from what steamvr shows when the steam overlay is up.

dmarcos commented 5 years ago

@InfiniteLee Is it always consistent or sometimes correct? We could probably just change the indices. Using the handennes field was not reliable at least on Firefox. I saw that sometimes was not even populated.

InfiniteLee commented 5 years ago

It seems pretty consistent. I've opened #4047 to fix it

InfiniteLee commented 5 years ago

I should say, it "seems" pretty consistent, but I honestly don't know enough about the gamepad API to know for sure. I suspect if you had a 3rd (or more) gamepad in the mix it could become inconsistent. I think only checking index for steamvr devices seems like its going to be wrong at some point. I think my original suggestion of removing data.controller being passed as the filterControllerIndex argument and keeping handedness in vive-controls might be better still.

dmarcos commented 5 years ago

@InfiniteLee Happy with either solution. Just make sure to verify in both Firefox and Chrome with Vive / WebVR combo.

ngokevin commented 5 years ago

The solution works temporarily, but yeah, confirmed by @kfarr that when you introduce a third gamepad such as Vive Trackers, things go wrong. So we should look to make it more robust.