Skip to content

Make AR module optional for the sample viewer #346

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 6 commits into from
Apr 9, 2025

Conversation

shubham7109
Copy link
Collaborator

@shubham7109 shubham7109 commented Apr 8, 2025

Description

PR to make the AR module an optional module for the sample viewer and add logic to handle the scenario if the device does not support AR.

Links and Data

Sample Epic: #5638

@shubham7109 shubham7109 self-assigned this Apr 8, 2025
@shubham7109 shubham7109 marked this pull request as ready for review April 8, 2025 19:56
@TADraeseke TADraeseke self-requested a review April 8, 2025 22:25
Copy link
Collaborator

@TADraeseke TADraeseke left a comment

Choose a reason for hiding this comment

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

Looks good to me. I was blocked (without a crash) when running an API 28 emulate device and all worked well on modern device--that it redirected me to the google play store to download ar core.

@@ -6,7 +6,9 @@

<!-- Limits app visibility in the Google Play Store to ARCore supported devices
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to consider altering this comment. The line below now doesn't limit visibility in the store, but it could be a useful flag to indicate to users where/how they do limit

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess the comment on line 28/29 is still valid

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated comment to say:

    <!-- Provide support for ARCore via Google Play Services,
         by checking to see if device is compatible and ready for AR.
         (https://developers.google.com/ar/devices). -->

@shubham7109
Copy link
Collaborator Author

@TADraeseke, it turns out the solution I originally had didn't resolve the issue on GooglePlay. I added a new commit change following Google's doc to make AR core optional: https://developers.google.com/ar/develop/java/enable-arcore#ar-optional

Test this change on the play store, and no issues were found. Requesting a re-review.

@shubham7109 shubham7109 requested a review from TADraeseke April 9, 2025 19:26
Copy link
Collaborator

@TADraeseke TADraeseke left a comment

Choose a reason for hiding this comment

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

LGTM!

@shubham7109 shubham7109 merged commit 41098ef into v.next Apr 9, 2025
1 check passed
@shubham7109 shubham7109 deleted the shubham/AR-sample-fix branch April 9, 2025 21:01
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