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

chore(WebXRManager): add missing methods, update getCamera signature #169

Merged
merged 15 commits into from
May 25, 2022
Merged

chore(WebXRManager): add missing methods, update getCamera signature #169

merged 15 commits into from
May 25, 2022

Conversation

CodyJasonBennett
Copy link
Contributor

@CodyJasonBennett CodyJasonBennett commented Jan 29, 2022

Why

Some changes to WebXRManager's signature weren't tracked with mrdoob/three.js#21886 and various other changes with the introduction of layers and controllers.

What

This PR updates the getCamera signature to return an XRCamera and introduces low-level getBaseLayer, getBinding, getFrame, updateCamera, and setReferenceSpace methods. I've vendored Babylon's WebXR types and updated WebXRManager and WebXRController.

Checklist

  • Checked the target branch (current goes master, next goes dev)
  • Added myself to contributors table
  • Ready to be merged

Copy link

@tongtwist tongtwist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed these types updates too.
Everything sounds good to me.
Btw, with these changes, types better reflects the r140 documentation of WebXRManager.getCamera()

@CodyJasonBennett
Copy link
Contributor Author

CodyJasonBennett commented May 9, 2022

I went ahead and fixed the webxr types and completed three's controller types.

Notably, this drops WebGLRenderingContext#makeXRCompatible and swaps XRGamepad for the native Gamepad.

@joshuaellis
Copy link
Member

@0b5vr if you approve I will merge this.

@joshuaellis
Copy link
Member

#218 merged added @types/webxr i'm wondering if this is still necessary? If so, the merge conflicts will need to be resolved.

@CodyJasonBennett
Copy link
Contributor Author

CodyJasonBennett commented May 20, 2022

The changes in my PR were more correct than @types/webxr and still Babylon. I'd need to find time to review.

@0b5vr
Copy link
Contributor

0b5vr commented May 22, 2022

@0b5vr if you approve I will merge this.

I didn't noticed to this! Let me see this real quick.

@CodyJasonBennett
Copy link
Contributor Author

CodyJasonBennett commented May 25, 2022

The updated @types/webxr look good to me. I'll revert related changes in my PR, but #218 didn't touch the three interfaces I originally corrected here.

Am I correct in assuming that these types are ambient? I've removed imports to the WebXR definitions accordingly.

Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @CodyJasonBennett see above comment

types/three/src/renderers/webxr/WebXRController.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all your work @CodyJasonBennett & thanks for the reviewing @0b5vr.

@joshuaellis joshuaellis merged commit f675677 into three-types:master May 25, 2022
@CodyJasonBennett CodyJasonBennett deleted the fix-webxrmanager-methods branch May 25, 2022 15:52
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.

4 participants