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

Add image data api #165

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

clintpurser
Copy link
Member

@clintpurser clintpurser commented Jan 3, 2024

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 the CameraClient 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.

@clintpurser clintpurser requested a review from njooma January 3, 2024 22:17
@clintpurser clintpurser requested a review from a team as a code owner January 3, 2024 22:17
@clintpurser clintpurser requested a review from stuqdog January 3, 2024 22:17
@njooma
Copy link
Member

njooma commented Jan 3, 2024

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 ViamImage, and also a helper function contained within an extension on ViamImage to get a ui.Image.

I agree that it's super gross converting things from bytes to img.Image to ui.Image to UInt8List. So let's take it all away -- give them a default widget they can use, and also give them functions in case they want to do their own thing.

@clintpurser
Copy link
Member Author

@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

@clintpurser clintpurser marked this pull request as draft January 31, 2024 18:08
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