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.

Sign up for GitHub

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

Jump to bottom

XRButton: allow specifying optional and required features #27030

Merged
merged 4 commits into from
Oct 27, 2023
Merged

XRButton: allow specifying optional and required features #27030

merged 4 commits into from
Oct 27, 2023

Conversation

snowzurfer
Copy link
Contributor

@snowzurfer snowzurfer commented Oct 22, 2023 •
edited
Loading

Update XRButton to let user pass optional and required WebXR features

Update XRButton so that it can be passed optional and required features to be enabled on the XRSession. This is useful for enabling e.g. light estimation:

const sessionInit = {
	optionalFeatures: [ 'light-estimation' ],
};
document.body.appendChild( XRButton.createButton( renderer, sessionInit ) );
@snowzurfer
Update WebXRManager and XRButton to offer light estimation
2c51843 
Update the XRButton so that it can be passed optional and required
features to be enabled on the XRSession. This is useful for enabling
light estimation.

Update the WebXRManager interface to offer light estimation.

Also update the English documentation to document the new light
estimation method.
@github-actions
Copy link

github-actions bot commented Oct 22, 2023 •
edited
Loading

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
652.4 kB (161.7 kB) 652.8 kB (161.8 kB) +397 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
445 kB (107.7 kB) 445.4 kB (107.8 kB) +397 B
@snowzurfer snowzurfer changed the title Update WebXRManager and XRButton to offer light estimation Oct 23, 2023
@snowzurfer
Copy link
Contributor Author

not sure if this is the right forum, but @snagy would you know if you folks at Meta have a timeline for implementing this in the Quest Browser?
@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 23, 2023

Related example: https://threejs.org/examples/webxr_ar_lighting

Question: Do we need a new method like getLightProbe() or can XREstimatedLight from the addons be used for the same use cases?
@snowzurfer
Copy link
Contributor Author

snowzurfer commented Oct 23, 2023 •
edited
Loading

Neat, that looks like it already does what I wanted to do, and shows how to!

At this point that makes this PR kind of irrelevant.
@snowzurfer
Copy link
Contributor Author

One thing we could still use this PR for is to support passing requested WebXR features through the XRButton.
@snowzurfer snowzurfer changed the title WebXRManager: offer light estimation Oct 24, 2023
@snowzurfer
Copy link
Contributor Author

Updated the PR and the description.
@snowzurfer snowzurfer mentioned this pull request Oct 24, 2023
Merged
@Mugen87 Mugen87 added this to the r158 milestone Oct 24, 2023
@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 24, 2023

https://rawcdn.githack.com/snowzurfer/three.js/albtaiuti/webxr-light-probe-on-webxr-manager/examples/index.html
Mugen87
Mugen87 reviewed Oct 24, 2023
View reviewed changes
examples/jsm/webxr/XRButton.js
@@ -1,6 +1,6 @@
class XRButton {

static createButton( renderer ) {
static createButton( renderer, sessionInit = {} ) {
Copy link
Collaborator

@Mugen87 Mugen87 Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I can't merge this PR until @mrdoob approves since I'm not sure why sessionInit wasn't added in the first place (similar to ARButton). Looking at the code, the module request multiple optional features at once. Maybe this was done in order to keep the interface simple. Adding sessionInit means devs have to know how the options of requestSession() is structured.
@mrdoob
Copy link
Owner

mrdoob commented Oct 27, 2023

I'll have to think about this one...
@mrdoob mrdoob modified the milestones: r158, r159 Oct 27, 2023
@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 27, 2023 •
edited
Loading

The merged PR #27041 relies on this one since it assumes you can pass light-estimation to XRButton.

If we don't merge this PR or a similar solution, we should revert #27041 before releasing r158. Otherwise it will confuse devs.
@mrdoob
Copy link
Owner

mrdoob commented Oct 27, 2023

Oh true... Then I'll merge it.
All this is still very much WIP anyway...
@mrdoob mrdoob modified the milestones: r159, r158 Oct 27, 2023
@mrdoob mrdoob merged commit b1ff959 into mrdoob:dev Oct 27, 2023
19 checks passed
This was referenced Oct 27, 2023
Merged
Closed
@snowzurfer snowzurfer mentioned this pull request Oct 29, 2023
Closed
@Mugen87 Mugen87 mentioned this pull request Feb 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants
@snowzurfer @Mugen87 @mrdoob