-
Notifications
You must be signed in to change notification settings - Fork 85
APP-8003 Add world state store service #695
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
base: main
Are you sure you want to change the base?
Conversation
proto/viam/service/worldobjectstore/v1/world_object_store.proto
Outdated
Show resolved
Hide resolved
Removing some folks from review while I tweak things to avoid noise. |
Was there a scope for this? Would be helpful to understand the thinking behind it |
Yes sorry, meant to add a link in the description. Here you go: https://docs.google.com/document/d/1ionvyBa7x3HZU_rwDPBvrAUrQjBrP6sJ10Z_xbBTue8/edit?tab=t.0#heading=h.tcicyojyqi6c The API design has been slightly tweaked based on testing @micheal-parks did, but for the most part things are the same. |
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.
This looks good to me from a code perspective based on what was decided on in the scope doc. I wonder though if it would be possible to put this under the app proto instead of the general service proto. I expect this API to change frequently and it seems like there is a lot more leniency towards making breaking changes to the App API as compared to resources
Fair point, I brought this up to steve and he suggested it could live here because it is a new service resource type, but we can mark it as experimental for now, and allow some level of leniency until we have published some Viam-supported visualization modules and are confident with the API. @micheal-parks has also been testing visualizations with the assumption that this is how the API will work, so we have some real-world proof of concepts to work from. I ended up writing the code to implement the I'd be happy to get some more opinions on this before moving forward, though, if you have anyone in mind. |
Note from @erh to use existing world object state |
…world-object-store-service
…viamrobotics/api into APP-8003-add-world-object-store-service
proto/viam/service/worldobjectstore/v1/world_object_store.proto
Outdated
Show resolved
Hide resolved
Updated per latest conversation:
|
Ok after reading deeper I understand more. I think initially I was just surprised by everything that was changing because that was not my read from the email for how the changes would need to be written. I think all we need to do here is add the metadata and UUID onto the GeometryInFrame object since PoseInFrame is a GeometryInFrame with a nil |
…viamrobotics/api into APP-8003-add-world-object-store-service
…world-object-store-service
…world-object-store-service
Hi, @biotinker brought up a great point which is that we've wanted the Transform.Geometry field to be a |
I am ok with that. As I was looking into implementing @biotinker's suggestions, I noticed that we would encounter some issues with managing composite geometries compared to our protos here. Should we remove the |
While I'm not sold on needing a special
Currently PCDs are sent over the wire elsewhere as |
…world-object-store-service
// StreamTransformChanges streams changes to world state transforms | ||
rpc StreamTransformChanges(StreamTransformChangesRequest) returns (stream StreamTransformChangesResponse) { | ||
option (google.api.http) = {get: "/viam/api/v1/service/worldstatestore/{name}/stream_transform_changes"}; | ||
} |
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.
Added this based on a conversation with @stevebriskin and @micheal-parks . The intention of this RPC is to allow visualization tools to open a stream and listen for change events from this service. We define a change event as one of the following:
// TransformChangeType is the type of change that has occurred for a transform.
enum TransformChangeType {
TRANSFORM_CHANGE_TYPE_UNSPECIFIED = 0;
TRANSFORM_CHANGE_TYPE_ADDED = 1;
TRANSFORM_CHANGE_TYPE_REMOVED = 2;
TRANSFORM_CHANGE_TYPE_UPDATED = 3;
}
When an implementor of this service is going to Send
a change message, they must define what type of change event is occurring in the response:
message StreamTransformChangesResponse {
TransformChangeType change_type = 1;
common.v1.Transform transform = 2;
// The field mask of the Transform that has changed, if any. For added transforms, this will be empty. For updated
// transforms, this will be the fields that have changed. For removed transforms, this will be the Transform's UUID
// path.
google.protobuf.FieldMask updated_fields = 3;
}
Note we use a FieldMask
, which will allow the implementor only to include fields that were changed during a TRANSFORM_CHANGE_TYPE_UPDATED
event, or just the change_type
and transform.uuid
fields for a TRANSFORM_CHANGE_TYPE_REMOVED
event.
The field mask will enable the implementor to reduce the amount of data sent over the wire in a stream response to only the relevant data, allowing the client to understand how to manage each event it receives quickly. This should be particularly helpful when dealing with Transform
s that have large amounts of geometry data (like point clouds) or have large quantities of geometries defined.
message Line { | ||
repeated float segments = 1; | ||
} |
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.
Do we still want to introduce the Line
type if we are allowing Transform
to have multiple Geometry
s?
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.
I'm not sure what this has to do with Transform having multiple geometries
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.
Based on this comment:
While I'm not sold on needing a special line type and think it would be nice to implement multi geometries with lines as 0-radius capsules, I acknowledge this would be a larger lift and not strictly necessary.
Introducing a new service type, the
WorldStateStoreService
.Visualization modules will utilize this service to retrieve information about the machine's world object store for rendering on the client side.
List of changes:
Points
andLine
geometry types and add them to theoneof geometry_type
on theGeometry
messageWorldStateStoreService
with three RPCs:ListUUIDs
returns all transform UUIDsGetTransform
returns a transform by UUIDDoCommand
sends/receives arbitrary commandsTransform
message (these include breaking changes):These changes are primarily aimed at aligning the usage of the
Transform
more closely with the expectations of engineers working with motion and visualization tools.A quick look at RDK and the SDKs seems to indicate that none of our code interacts with
Transform
s, and just passes them through in various APIs. Our thinking is that, despite these being breaking changes, they should have a relatively low impact and set us up with a better foundation on which to expand our motion and visualization tools.Scope doc: https://docs.google.com/document/d/1ionvyBa7x3HZU_rwDPBvrAUrQjBrP6sJ10Z_xbBTue8/edit?tab=t.0#heading=h.tcicyojyqi6c