Skip to content

fix: vendor and fix XRControllerModelFactory #239

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

Conversation

saitonakamura
Copy link
Collaborator

@saitonakamura saitonakamura commented Feb 4, 2023

Following the discussion I vendored XRControllerModelFactory to react-xr along with some changes inside.

The main point of this is to fix problems with connecting/disconnecting different or same controllers, turns out webxr world is crazy full of quirks (who'd have thought). It all started in pmndrs/three-stdlib#214: there I tried to fix a problem when you connect and disconnect controllers and it doesn't unsubscribe from connected event. Thing is, in three, WebXRController are only created twice and never recreated, but they spew out connected/disconnected events. Connecting and disconnecting same or different controllers before this fix would result in ever more increasing connected event listeners each trying to load and add the scene. I've added the unsubscription fix, but it shot me in the foot in the most weird way. Turns out, SteamVR is connecting controllers in a peculiar way: it first dispatches inputsourceschange event with added: [some input source data], then immediately inputsourceschange with removed: [the input source that was just added] and then inputsourceschange with added: [proper input source]. And with the fix, it just unsubscribes from connected and never has a chance to resubscribe (cause it's kind of the same controller). With all that in mind I refactored XRControllerModelFactory, XRControllerModel and ControllerModel into somewhat cohesive picture that works with all this quirks. We tested it on multiple devies including Oculus Go, Quest 1, 2 and Pro, Rift S, Valve Index, Vive Cosmos and the others and we've yet to find more problems with it, this works robust. I don't really like the way it looks now, because logic is bit scattered all over the place, but it's no worse than it was before IMO

The other thing I did in this PR is to add a few events that XRControllerModel now dispatches, namely, modelconnected/disconnected and motionconnected/disconnected. They're not currently used in react-xr code, it's only used in our own app code to do 2 things:

  1. Analyze data from MotionController to dispatch events such as button press/thumbstick move/etc. This is directly applicable to react-xr and I plan to expose it as an additional api surface to Interactive
  2. Add more 3d stuff to the controller model, we add a tutorial that shows which button does what. Don't see a clear way for react-xr to take advantage of it right now, except for examples maybe

I'm willing to move these events dispatch to a separate PR if it's any problem

@saitonakamura saitonakamura force-pushed the vendor-and-fix-xr-controller-model-factory branch from 281b027 to 4eb9392 Compare February 4, 2023 13:46
@saitonakamura
Copy link
Collaborator Author

Further plans concerning this:

  1. Added button/thumbsticks events to the Interactive component
  2. Try to port this fixes to three examples and then backport them to three-stdlib
  3. Maybe also vendor HandModel and check if it works alright, haven't tested it much for now

@CodyJasonBennett
Copy link
Member

Awesome, this has been a very fragile area for a long time. I'll merge and release this under v5.1.4 and follow-up with #238 under v5.2.0 which you can target in your app.

@CodyJasonBennett CodyJasonBennett changed the title Vendor and fix XRControllerModelFactory fix: vendor and fix XRControllerModelFactory Feb 4, 2023
@CodyJasonBennett CodyJasonBennett merged commit b1c2016 into pmndrs:master Feb 4, 2023
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.

2 participants