Skip to content

Introduce ability to start and stop depth processing #58

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

Merged
merged 9 commits into from
Apr 17, 2025

Conversation

alcooper91
Copy link
Contributor

@alcooper91 alcooper91 commented Apr 4, 2025

Introduces the depthActive property to allow pages to request starting or stopping the depth activity.

Addresses #52


Preview | Diff

@alcooper91 alcooper91 mentioned this pull request Apr 4, 2025
@alcooper91 alcooper91 requested review from cabanier and bialpio April 4, 2025 22:15
@cabanier
Copy link
Member

cabanier commented Apr 4, 2025

This is a bit weird. I don't think we have logic in WebXR that triggers by setting a boolean.
Maybe this would be more suitable if we add it to the renderstate?

@alcooper91
Copy link
Contributor Author

We don't have a boolean, but we do have a float that functions very similarly: https://immersive-web.github.io/webxr/#dom-xrwebgllayer-fixedfoveation

I think there was some opposition to adding it to the renderState. Mainly @toji IIRC?

The original proposal was a pair of methods; but I felt that this was more elegant since calling the start/stop methods didn't necessarily guarantee anything.

@alcooper91
Copy link
Contributor Author

@cabanier I've shifted away from the attribute.

Introduces the `depthActive` property to allow pages to request
starting or stopping the depth activity.
@alcooper91 alcooper91 force-pushed the alcooper/start_stop branch from 4e00931 to d18cae6 Compare April 9, 2025 21:16
@alcooper91
Copy link
Contributor Author

/agenda discuss options to start/stop depth sensing

Since the in-comment-thread tag didn't seem to flag it.

@probot-label probot-label bot added the agenda label Apr 11, 2025
@AdaRoseCannon
Copy link
Member

Thanks, interesting that probot doesn't get the comment events from inline code discussions.

@alcooper91 alcooper91 changed the title Introduce depthActive property Introduce ability to start and stop depth processing Apr 16, 2025
@alcooper91
Copy link
Contributor Author

I did make one rename here from (start|stop)Depth -> (start|stop)DepthSensing to make the methods a little bit clearer. Otherwise, this should now address all of the feedback from yesterday's meeting to the best of my ability.

@alcooper91 alcooper91 requested a review from cabanier April 16, 2025 17:43
@alcooper91 alcooper91 requested review from cabanier and toji April 16, 2025 18:03
Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! One minor comment.

@alcooper91
Copy link
Contributor Author

@bialpio I'll merge tomorrow AM unless you provide additional feedback.

@alcooper91 alcooper91 merged commit 76ce48f into main Apr 17, 2025
2 checks passed
@alcooper91 alcooper91 removed the agenda label Apr 17, 2025
github-actions bot added a commit that referenced this pull request Apr 17, 2025
SHA: 76ce48f
Reason: push, by alcooper91

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@alcooper91 alcooper91 deleted the alcooper/start_stop branch May 1, 2025 22:27
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