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

Apply #659 to device-orientation-controls, integrating permission dialog into library #661

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

nickw1
Copy link
Collaborator

@nickw1 nickw1 commented Mar 23, 2025

⚠️ All PRs have to be done versus 'dev' branch, so be aware of that, or we'll close your issue ⚠️

What kind of change does this PR introduce?

Introduces the permissions dialog introduced in PR #659 (providing a gesture to initiate device orientation permission request on iOS) into the DeviceOrientationControls class within AR.js itself, meaning that users do not need to implement this code themselves.

Can it be referenced to an Issue? If so what is the issue # ?
Related to PR #659

How can we test it?

Test either the three.js location-based example or the basic-js A-Frame example. Will work as usual on Android, and should display the permissions dialog on iOS. To test the dialog on Android, change the if statement on line 311 in device-orientation-controls.js to if(true) - it works as expected. Untested on iOS at present.

Summary

See "what kind of change does the PR introduce?" - see above

Does this PR introduce a breaking change?
No

Please TEST your PR before proposing it. Specify here what device you have used for tests, version of OS and version of Browser
Tested on Android, Chrome

Other information
@kalwalt do you want to check this when you're available? Thanks.

@nickw1
Copy link
Collaborator Author

nickw1 commented Mar 23, 2025

@ma2yama do you want to test this? It's your fix applied to the main library. Thanks.

@nickw1
Copy link
Collaborator Author

nickw1 commented Mar 23, 2025

Also note that this PR includes the fixes from #657 (sorry, not ideal but thought it was the least messy solution overall) as it's designed to follow on from #657.

So #657 should be checked and merged into dev first.

@ma2yama
Copy link
Contributor

ma2yama commented Mar 24, 2025

@nickw1 Thanks for the fix.

I have verified that the modal appears and the cube appears depending on the orientation of the device after pressing the button to allow it.

While reviewing the code, I noticed a couple of things:

  1. It would be great if we could update the logic to properly detect iPads running iOS 13 and later.

const isIOS = navigator.userAgent.match(/iPhone|iPad|iPod/i);

  1. Also, I saw a console log being output — I think it can be removed if it's no longer needed.

console.log(currentEuler.x, currentEuler.y, currentEuler.z);

@nickw1
Copy link
Collaborator Author

nickw1 commented Mar 24, 2025

@ma2yama Have applied your suggested modifications, many thanks!

Just waiting for @kalwalt to check #657 and then this PR, he's away at the moment.

Also will need to resolve the conflicts below, will need to merge the latest changes in dev into this branch.

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.

2 participants