-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add image data api #165
base: main
Are you sure you want to change the base?
Add image data api #165
Conversation
My thoughts are that this shouldn't be in the dart part of the SDK, but rather in the flutter/widget part. We should make a custom widget that can display a I agree that it's super gross converting things from |
this wasn't needed
@njooma - I agree we shouldn't add any more flutter dependencies in the dart part of the SDK - I didn't realize i had pulled in one, but I was able to remove it. I think providing the imageData wrapper is still a useful way to provide the data to SDK consumers, because some will want more finite control over their state and a pre wrapped widget doesn't always handle things how you want, but I also agree I see the value of providing the widgets that handle everything for the user. One example of needing more finite control that immediately comes to mind is how could we link a button for manually refreshing the image - like we have in the web app. I am not thinking of an easy way to do this only with widgets and not using state and managing the data outside of the build method. I've added some wrapped widgets, which use the new method to get their data, but still left the new function in. Screen.Recording.2024-01-04.at.2.51.23.PM.mov |
After some conversations with @mcvella I realized it was really difficult for SDK consumers to get images from cameras on Viam and use it easily in a Flutter UI. We had already presented this code in our examples, but this PR proposes pulling it into the SDK as a convenience function so that SDK consumers may easily get image data in a format that then easily allows them to use it in a Flutter UI.
I think I added the field in the correct layer, as a function on
Camera
- this did cause some extra overrides, in the tests for example, to need to be added. I don't think adding it only in theCameraClient
is the right spot, correct me if i'm wrong though because I second guessed myself.Also, the tests don't feel great. because on paper i don't know what happens under the hood to get the new expected value, but at least we'll know if something changes down the road.
This is also a useful change for the mobile app because Eliot just requested adding live or manual, 1 second,..., 30 second video refresh frequency (like we have in the web app), so this change makes requests like that much easier. Alternative is duplicating the code in that was in the stream.dart file in the mobile app, but I preferred making a function to call in both places instead.
open to feedback here because I feel this is a weird change, could use some guidance on how we want to handle convenience wrappers in our SDKs, i know we do it in other SDKs (python?) but couldn't remember or quickly find any examples in our flutter SDK already.