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

feat: Scanner improvements #1781

Merged
merged 11 commits into from
May 11, 2022
Merged

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented May 7, 2022

This PR contains several improvements for the scanner:

  • Move the decoding part in an Isolate
  • Improve the MLKit algorithm by limiting the types of supported barcodes
  • "Adaptive algorithm" to start the decoding (cf below for more details)
  • Ask for a camera lower resolution (high -> medium)
  • [UX] When a barcode is scanned, a small vibration notifies the user

"Adaptive algorithm" :

  • The duration of each decoding is now stored.
  • An average value of the last 10 decodings is computed.
  • The average time (what I call a window) is then multiplied by 10 (this value is arbitrary, we can change it)
  • This sum is then used as the time between two decodings

What's still missing: pause the barcode when scrolling between the carousel's cards.
-> Will be implemented in a separated PR

@codecov-commenter
Copy link

codecov-commenter commented May 7, 2022

Codecov Report

Merging #1781 (d337abb) into develop (2ea0da3) will decrease coverage by 2.24%.
The diff coverage is 2.26%.

@@            Coverage Diff             @@
##           develop   #1781      +/-   ##
==========================================
- Coverage     8.86%   6.62%   -2.25%     
==========================================
  Files          161     172      +11     
  Lines         6623    7809    +1186     
==========================================
- Hits           587     517      -70     
- Misses        6036    7292    +1256     
Impacted Files Coverage Δ
...h_app/lib/cards/category_cards/abstract_cache.dart 0.00% <0.00%> (ø)
...p/lib/cards/category_cards/asset_cache_helper.dart 0.00% <0.00%> (ø)
...p/lib/cards/category_cards/raster_async_asset.dart 0.00% <0.00%> (ø)
...oth_app/lib/cards/category_cards/raster_cache.dart 0.00% <0.00%> (ø)
..._app/lib/cards/category_cards/svg_async_asset.dart 0.00% <0.00%> (ø)
...smooth_app/lib/cards/category_cards/svg_cache.dart 0.00% <0.00%> (ø)
...th_app/lib/cards/data_cards/image_upload_card.dart 0.00% <0.00%> (ø)
...es/smooth_app/lib/cards/data_cards/score_card.dart 4.54% <0.00%> (-0.22%) ⬇️
...t_cards/knowledge_panels/knowledge_panel_card.dart 0.00% <0.00%> (ø)
...t_cards/knowledge_panels/knowledge_panel_page.dart 0.00% <0.00%> (ø)
... and 104 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3894610...d337abb. Read the comment docs.

@g123k
Copy link
Collaborator Author

g123k commented May 7, 2022

Hi @M123-dev & @monsieurtanuki, I have implemented a big part of the improvements for the scanner/camera.
Everything is not yet finished, but I would appreciate if you could send me your feedback on the current implementation.

Thanks ;)

@g123k
Copy link
Collaborator Author

g123k commented May 7, 2022

After discussing with @teolemon, we will measure if these improvements are enough.
And if necessary the start/resume detection will be implemented.

@g123k g123k marked this pull request as ready for review May 7, 2022 19:57
@g123k g123k requested a review from a team as a code owner May 7, 2022 19:57
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @g123k!
As I said before, I don't believe in Isolate. A lot of trouble, and probably not worth it.
We're not talking about heavy processing that makes the app stutter, we're talking about heavy processing that drains the battery. cf. https://www.youtube.com/watch?v=5AxWC49ZMzs
Besides, tomorrow I travel all day long. It's fair to say that you need someone else to approve (or not) this PR.

@teolemon teolemon added the 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… label May 8, 2022
@teolemon teolemon added this to the V1 milestone May 8, 2022
@g123k g123k force-pushed the scanner_isolate branch from f6f4d65 to e675f9d Compare May 10, 2022 17:04
@g123k
Copy link
Collaborator Author

g123k commented May 10, 2022

@M123-dev Do you have a few minutes to review this PR?

Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

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

Heyy @g123k I just had a look at your code, I didn't studied it and I didn't double checked everything but what I read looks good to me.

But I agree with @monsieurtanuki it doesn't solve battery problems (might even add, as we move data to a different threat) but that's not a reason for me to not merge. It assures smoothness in the smoothie main threat thats great.

Also good that you added haptic feedback, just searched for a issue with that to link to this PR but didn't found one. Wonder why I haven't opened one. I've thought about haptic feedback multiple times already.

}
}

// ignore: avoid_classes_with_only_static_members
Copy link
Member

Choose a reason for hiding this comment

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

I think adding a private empty constructor is better then doing it like that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An isolate requires a top-level or static function, that's why :/

packages/smooth_app/lib/pages/scan/ml_kit_scan_page.dart Outdated Show resolved Hide resolved
@M123-dev
Copy link
Member

Some merge conflicts @g123k

@g123k g123k force-pushed the scanner_isolate branch from 32bfbda to a433ffc Compare May 11, 2022 11:44
@g123k
Copy link
Collaborator Author

g123k commented May 11, 2022

It should now be OK :)

@M123-dev
Copy link
Member

@g123k the analyzer goes wild, I guess a file has change or something like that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion…
Development

Successfully merging this pull request may close these issues.

5 participants