-
Notifications
You must be signed in to change notification settings - Fork 426
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
Partially refactor JS utilities to support future webapps in separate folder structure #1364
Partially refactor JS utilities to support future webapps in separate folder structure #1364
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss this PR more in person. I'm not sure about the best way forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make the requested small fix but otherwise LGTM! Oops, actually I'd like to understand the possible code redundancy before we merge this.
@@ -0,0 +1,80 @@ | |||
// Copyright (c) Facebook, Inc. and its affiliates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any code redundancy between this file and the existing vr_demo.js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, see here: #1364 (comment)
The existing vr_demo.js will be deleted in #1366, so this redundancy will be resolved as soon as that PR gets merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Motivation and Context
vr_utils.js
has been added tosrc/esp/bindings_js/modules
. This is a new file that contains functions that help with setting up the eye sensors and updating the sensor locations based on the user's pose.Changes were also made to
utils.js
so that it no longer needs to importinfoSemanticFileName
fromdefaults.js
. This allows it to be independently included as a module in webapps.How Has This Been Tested
The WebXR Hand Demo port to the new folder structure (#1366) which uses these utils functions was launched and ran successfully.
Types of changes
Checklist