-
Notifications
You must be signed in to change notification settings - Fork 111
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
DATA-780 replace commonpb.Pose in favor of spatialmath.Pose in arm interface #1597
Conversation
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.
Looks really good. Thanks for doing this.
A couple of nits, then LGTM.
The main thing is making proto structs just to make Poses. which isn't really the "good" way to do it, but since it's mostly in tests I'm ok to keep them if you like it.
There's one instance of doing so in "real" code, which should definitely be removed.
|
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 LGTM.
In the future, when a PR for one thing also does other stuff (cleaning up consts, removing old dead functions, etc) please call that out in the PR description just to mention that those also exist.
Currently the Arm interface receives and returns
commonpb.Pose
objects rather than the internally usedspatialmath.Pose
. The client/server code is then responsible for translating to/from proto which cuts down on the number of places that are dependent on proto, and makes our code internally more consistent and readableIn addition to the above the PR also cleans up places in the code where
commonpb.Pose
is used unnecessarily, replacing it withspatialmath.Pose
.Finally, the process of going through the above revealed to me sections of code where constants, functions are no longer in use. These have been removed.