RSDK-7565, RSDK-7471: add new methods to vision service get_properties and capture_all_from_camera #615

bhaney · 2024-05-14T21:21:55Z

This adds two new methods the python SDK vision service

GetProperties
CaptureAllFromCamera

The mock vision service and tests were updated for these methods as well.

github-actions · 2024-05-14T21:22:08Z

Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!

component	function
base	is_moving
board	gpio_pin_by_name
camera	get_image
encoder	get_position
motor	is_moving
sensor	get_readings
servo	get_position
arm	get_end_position
gantry	get_lengths
gripper	is_moving
movement_sensor	get_linear_acceleration
input_controller	get_controls
audio	get_properties
pose_tracker	get_poses
power_sensor	get_power
motion	get_pose
vision	get_classifications_from_camera

bhaney · 2024-05-15T18:41:07Z

src/viam/services/vision/service.py

+            try:
+                fmt = result.image.mime_type.to_proto()
+                img_bytes = result.image.data
+                img = Image(source_name=request.name, format=fmt, image=img_bytes)


this Image is the viam.proto.component.camera.Image class, not a PIL image

bhaney · 2024-05-15T18:44:59Z

src/viam/services/vision/vision.py

+    CaptureAllFromCamera method. This is used most often for visualization purposes, since normally,
+    returning the image on every call to a classifier/detector/etc would be costly and unnecessary.
+    """
+    def __init__(self, image=None, classifications=None, detections=None, objects=None, extra={}):


I set the default results to None rather than the empty list to distinguish between "there was no request for classifier to return a result" vs. "the classifier was requested, but there were no results" (I've put this explanation in the doc string as well)

stuqdog

Generally looks good to me! I am unconvinced we need the CaptureAllRequest type, but open to being talked into it :)

src/viam/services/vision/client.py

stuqdog · 2024-05-16T15:29:42Z

src/viam/services/vision/vision.py

+class CaptureAllRequest:
+    """
+    CaptureAllRequest represents the collection of fields you can ask the
+    CaptureAllFromCamera method to return. If you request something that the service
+    cannot provide, a warning message will be generated, and the service will return an empty
+    version of the requested thing.
+    """
+    def __init__(self, return_image=False, return_classifications=False,
+                 return_detections=False, return_object_point_clouds=False):
+        """
+        Args:
+            return_image (bool): Return the image from the GetImage of the underlying camera.
+            Default is False.
+            return_classifications (bool): Return the classifications from GetClassifications.
+            Default is False.
+            return_detections (bool): Return the detections from GetDetections.
+            Default is False.
+            return_objet_point_clouds (bool): Return the point cloud objects from GetObjectPointClouds.
+            Default is False.
+
+        Returns:
+            None
+        """
+        self.return_image: bool = return_image
+        self.return_classifications: bool = return_classifications
+        self.return_detections: bool = return_detections
+        self.return_object_point_clouds: bool = return_object_point_clouds


I'm not convinced this is necessary or particularly helpful. This is only being used for a single call (CaptureAllFromCamera) so we could just put these params and defaults on the capture_all_from_camera method. And that way, a user doesn't have to look in multiple places to understand what their options are or why they matter.

Mostly this is for future changes to the method and to ensure backwards compatability. Given that it is Capture"All", what "All" may mean may change in the future.

Let's say we add 2D segmentation as a vision service method. Then CaptureAll should be able to return it, too. But then it would be a breaking change to the method.

Actually, saying that outloud, python is nice in that you can assign defaults to arguments. So, if a future vision method was added, then the argument list could be extended then without breaking previous versions of the modules, because by default, the extra argument would be assigned a default value

Essentially -- I was attempting to find a way to make it so there would not have to be a breaking change to the CaptureAll method if for some reason a method was added to the vision service in the future. If this is a futile task and creating a Request class doesn't prevent a breaking change from happening, I'm willing to get rid of the class

Agree, I don't think we need to have this class

njooma

Apologies for making you do this, but could you base these changes off the rc-0.21.0 branch? That branch has changes that will affect this PR, and I don't want anyone to have to do this work twice, or go through a nasty rebase

njooma · 2024-05-16T16:15:42Z

src/viam/services/vision/vision.py

+class CaptureAllRequest:
+    """
+    CaptureAllRequest represents the collection of fields you can ask the
+    CaptureAllFromCamera method to return. If you request something that the service
+    cannot provide, a warning message will be generated, and the service will return an empty
+    version of the requested thing.
+    """
+    def __init__(self, return_image=False, return_classifications=False,
+                 return_detections=False, return_object_point_clouds=False):
+        """
+        Args:
+            return_image (bool): Return the image from the GetImage of the underlying camera.
+            Default is False.
+            return_classifications (bool): Return the classifications from GetClassifications.
+            Default is False.
+            return_detections (bool): Return the detections from GetDetections.
+            Default is False.
+            return_objet_point_clouds (bool): Return the point cloud objects from GetObjectPointClouds.
+            Default is False.
+
+        Returns:
+            None
+        """
+        self.return_image: bool = return_image
+        self.return_classifications: bool = return_classifications
+        self.return_detections: bool = return_detections
+        self.return_object_point_clouds: bool = return_object_point_clouds


Agree, I don't think we need to have this class

bhaney · 2024-05-16T18:32:35Z

Closing this PR and opening the other one, based on rc-0.21.0

bhaney added 3 commits May 13, 2024 17:45

WIP for capture all method

1fb5db8

adds get properties

903272e

linting

046561b

bhaney added 8 commits May 14, 2024 17:23

linting

fcbc597

type check

abbabab

type check

4ca652d

Mapping

7c92c32

default map

319cc8e

linting

6c08064

add tests

c98cea2

refine tests

3b0cc8e

bhaney commented May 15, 2024

View reviewed changes

bhaney added 3 commits May 15, 2024 14:46

edit comment string

8fc3ae9

edit comment string

6bef84e

linting

86c7e77

bhaney marked this pull request as ready for review May 15, 2024 18:50

bhaney requested a review from a team as a code owner May 15, 2024 18:50

bhaney requested review from njooma and purplenicole730 May 15, 2024 18:50

bhaney changed the title ~~RSDK-7565: add new methods to vision service get_properties and _capture_all_from_camera~~ RSDK-7565, RSDK-7471: add new methods to vision service get_properties and _capture_all_from_camera May 15, 2024

bhaney changed the title ~~RSDK-7565, RSDK-7471: add new methods to vision service get_properties and _capture_all_from_camera~~ RSDK-7565, RSDK-7471: add new methods to vision service get_properties and capture_all_from_camera May 15, 2024

bhaney added 2 commits May 15, 2024 15:32

edit doc string

bc8e15c

doc string

598d643

stuqdog reviewed May 16, 2024

View reviewed changes

njooma requested changes May 16, 2024

View reviewed changes

purplenicole730 removed their request for review May 16, 2024 17:15

bhaney closed this May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSDK-7565, RSDK-7471: add new methods to vision service get_properties and capture_all_from_camera #615

RSDK-7565, RSDK-7471: add new methods to vision service get_properties and capture_all_from_camera #615

bhaney commented May 14, 2024 •

edited

github-actions bot commented May 14, 2024

bhaney May 15, 2024 •

edited

bhaney May 15, 2024 •

edited

stuqdog left a comment

stuqdog May 16, 2024

bhaney May 16, 2024 •

edited

bhaney May 16, 2024 •

edited

njooma May 16, 2024

njooma left a comment •

edited

njooma May 16, 2024

bhaney commented May 16, 2024

RSDK-7565, RSDK-7471: add new methods to vision service get_properties and capture_all_from_camera #615

RSDK-7565, RSDK-7471: add new methods to vision service get_properties and capture_all_from_camera #615

Conversation

bhaney commented May 14, 2024 • edited

github-actions bot commented May 14, 2024

bhaney May 15, 2024 • edited

Choose a reason for hiding this comment

bhaney May 15, 2024 • edited

Choose a reason for hiding this comment

stuqdog left a comment

Choose a reason for hiding this comment

stuqdog May 16, 2024

Choose a reason for hiding this comment

bhaney May 16, 2024 • edited

Choose a reason for hiding this comment

bhaney May 16, 2024 • edited

Choose a reason for hiding this comment

njooma May 16, 2024

Choose a reason for hiding this comment

njooma left a comment • edited

Choose a reason for hiding this comment

njooma May 16, 2024

Choose a reason for hiding this comment

bhaney commented May 16, 2024

bhaney commented May 14, 2024 •

edited

bhaney May 15, 2024 •

edited

bhaney May 15, 2024 •

edited

bhaney May 16, 2024 •

edited

bhaney May 16, 2024 •

edited

njooma left a comment •

edited