-
-
Notifications
You must be signed in to change notification settings - Fork 155
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
refactor(controllers): Refactored how Controllers interact w/ three #294
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4103966:
|
saitonakamura
changed the title
wip
refactor(controllers): Refactored how Controllers interact w/ three
Jul 23, 2023
@CodyJasonBennett I'd really appreciate any feedback on this |
I'm happy with the premise. Will give this a deeper review later today. |
CodyJasonBennett
requested changes
Jul 25, 2023
CodyJasonBennett
approved these changes
Jul 25, 2023
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This a big refactor of controller and how they interact with three counterparts. The flow of events was very confusing, to say the least. This PR makes this flow much direct and does some other notable changes.
ControllerModel abstraction is removed
It's responsibilities are moved to
Controllers
, mainly to https://github.com/pmndrs/react-xr/pull/294/files#diff-f17a5c6c1d801f88dd02bbbcaacc32ee99c5630535d1db5f3d5527ddd84b89eaR85-R103And
Controllers
now directly renderXRControllerModel
So the flow now is
XR
creates a pair ofXRController
and subscribes toconnected
/disconnected
events from three.jsXRController
callxr.getControler
and others in constructor therefore creating aWebXRController
instances in three.js. BothWebXRController
and it'sXRController
counterparts will now be present for the lifetime of an appWebXRManager
react toinputsourceschange
and populates it's state and dispatchesconnected
eventconnected
event is handled byXR
and it populatesstate.controller
with correspondingXRController
handleControllerModel
react toref
and callsmodelFactory.initializeControllerModel
which does all the 3d/profiles heavy lifting as previousPreviously, the 5th step was actually another multi-step flow which involved sending fake events and such
XRControllerModel moved to a separate file from XRControllersModelFactory
No notable changes inside it
InputSource on XRController can now be null
Now inputSource will become null if controller is disconnected, no real reason to hold on to outdated inputSource. This resulted in a bunch of null checks here and there, but nothing substantial
Whole bunch of unit tests added
We're still far from good coverage, but this is a big step forward