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

WebXRManager: Fix hand joints not being initialized when hand connected. #24827

Merged
merged 3 commits into from
Nov 5, 2022

Conversation

DevPika
Copy link
Contributor

@DevPika DevPika commented Oct 21, 2022

Fixed #24826

Description

WebXRController did not initialize the hand joints Object when hand first starts getting tracked ("connected" event). This led hand.joints being {} when accessed in the connected Event Listener. This MR fixes it by adding the initialization code to controller connect event and calling it from WebXRManager.

@mrdoob
Copy link
Owner

mrdoob commented Oct 21, 2022

Lovely fix!

Shouldn't it be _getHandJoint though? Or do you think it should be a public method?

@mrdoob mrdoob added this to the r146 milestone Oct 21, 2022
@DevPika
Copy link
Contributor Author

DevPika commented Oct 21, 2022

Thank you for your comment! Yes, I think this method is quite specific to the class and need not be a public method, so _getHandJoint would be ideal. I will make a commit for these changes soon.

Longer note:
I was a bit confused by the conventions while comparing WebXRManager and WebXRController. WebXRManager has a public API with a few functions detailed in the docs. But some of the other functions that were not present in the docs (so they can be considered private?) also follow the same convention and should be accessible as a public method, for example:
this.getBaseLayer or this.getFrame
WebXRManager also declares all of its member methods inside the contructor which is different from the WebXRController

The WebXRController class doesn't have a doc page yet, so would that mean all of its methods are considered internal, and only need to be accessed through WebXRManager?

Sorry, I might be confusing missing documentation and intention of some methods to be available publicly.

@DevPika
Copy link
Contributor Author

DevPika commented Oct 21, 2022

I have made the method private and updated the built module file in the Glitch example too.

@mrdoob mrdoob modified the milestones: r146, r147 Oct 27, 2022
@Mugen87 Mugen87 changed the title [WebXR] Fix hand joints not being initialized when hand connected WebXRManager: Fix hand joints not being initialized when hand connected. Nov 4, 2022
@DevPika
Copy link
Contributor Author

DevPika commented Nov 5, 2022

I am unsure if I need to mark this as ready to review on my end as this is my first MR to the repo. I don't expect to make any other changes in this MR, so I guess this means it is ready for a review? I made some changes as per suggestions from @mrdoob above, thanks for the quick feedback!

@Mugen87 Mugen87 merged commit 4896155 into mrdoob:dev Nov 5, 2022
@DevPika
Copy link
Contributor Author

DevPika commented Nov 5, 2022

Thank you @Mugen87

@0b5vr 0b5vr mentioned this pull request Dec 7, 2022
12 tasks
0b5vr added a commit to three-types/three-ts-types that referenced this pull request Dec 19, 2022
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.

[WebXR] WebXRController doesn't initiate joints when controller connected
3 participants