-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Use dual wide cameras for receipt camera #35926
Conversation
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
@ishpaul777 yes you would need to be added to the provisioning profile |
I think we can wait for Janic to confirm this is ready for a review before you jump in to save your time :D |
Tested and it looks much better on ios @janicduplessis 🎉 the only issue I saw is that there is a weird "switch" as you I guess go from the normal to wide camera or vice versa. I dont think that is a blocker |
Yea I think the switch is inevitable, as the 2 cameras has kind of different field of view. |
Make sense, we can explain that! |
actually this doesn't work for me on android, it just loads infinitely. tested with 3 different models: |
just checking in, whats the status here @janicduplessis ? |
@ishpaul777 Sorry I wasn't available recently, I am back and will test android to see if I also have that issue. |
04ff25b
to
1b6cb28
Compare
@ishpaul777 Ok, looks like the When we update to vision-camera v3 we can leverage the useCameraDevice api which uses the best available camera device automatically from a list of choices, but for now I think this works well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add the linked issue to the PR description @janicduplessis ?
@ishpaul777 are you able to do a checklist on this one please? |
Yes i can do it today 🫡 |
@mountiny When you are around can you please hit a adhoc build. I couldn't priortize it today but will do tomorrow. |
b92a343
to
d35a093
Compare
Triggered the build |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
@ishpaul777 can you please test, thanks! |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
@janicduplessis @mountiny i dont feel any difference i still cant focus close subject on my device, focus works only at a certain distance from subject. RPReplay_Final1710912370.MP4Same for Android (coz we fall back to wide-angle-camera when dual-wide-camera not available) Record_2024-03-20-11-02-32.mp4 |
@mountiny gentle bump on ☝️ |
@janicduplessis curious if you could check if this worked for you |
@janicduplessis bump on linking issue, thanks! |
@ishpaul777 What phone is it on iOS? Would you be able to log the different camera devices in https://github.com/Expensify/App/pull/35926/files#diff-21ba42adca2f3eb0a82245ae6bcc4614c06e9b4b7c208446ba9d6339d73e5b83R64? Maybe the dual-wide is not available. Maybe we could also try falling back to extra-wide camera instead of wide. |
I added an extra fallback to extra-wide camera, lmk if it looks better now. So basically what we do is try using dual, then ultra-wide, then wide. |
i phone 15 plus I'll do the testing soon again. Thanks! |
@janicduplessis Can you please merge main |
so my device does have a dualWideDevice camera its not undefined, but for close subject focus i dont really feel any difference with that, what works best is the ultraWideDevice but i am not sure if thats by default we can use ultra wide: RPReplay_Final1711545246.MP4dual wide: RPReplay_Final1711545659.MP4 |
Thanks @ishpaul777, I will test again on my end to see if there is an issue with dual camera focus. If that doesn't work properly then I think using ultra-wide by default would be best. |
Linked back to this issue #34898 |
Continued to fix the issue on iOS in #39589 |
we can close this PR @mountiny @janicduplessis, this was handled in other PR #39589 |
Details
This uses dual-camera when available to switch to a camera that is better at focusing close objects. This should make taking pictures of receipts easier. Note that this dual camera is only available on iOS as far as I know, but if it does exist on Android too it will be used.
Fixed Issues
$ #34898
PROPOSAL: N/A
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop