-
Notifications
You must be signed in to change notification settings - Fork 31
[APP-9877] Implement button functionality (pose) #442
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
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.
A couple small things but generally looks good!
| _controlValues = [ | ||
| startPose.x.roundToDouble(), | ||
| startPose.y.roundToDouble(), | ||
| startPose.z.roundToDouble(), | ||
| startPose.oX.roundToDouble(), | ||
| startPose.oY.roundToDouble(), | ||
| startPose.oZ.roundToDouble(), | ||
| startPose.theta.roundToDouble(), | ||
| ]; |
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.
missed this in the last PR (sorry!), but representing the _controlValues as a list seems not great to me. Very easy to run into issues if at any point the data representation changes. Would much prefer to have some sort of struct with named fields to make sure that we're always setting the right data to the right place.
| TextEditingController theta; | ||
|
|
||
| _TextControlStruct(this.x, this.y, this.z, this.oX, this.oY, this.oZ, this.theta); | ||
| } |
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.
@stuqdog added this structure to also manage the textcontrollers, as they were only related to the pose before by their shared index
| _controlValues.oZ = value; | ||
| case 'theta': | ||
| _controlValues.theta = value; | ||
| } |
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.
only complication to using the Pose instead of the list, but I think this is pretty simple still. now we depend on a specific string passed in though so still not ideal
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.
Yeah, definitely not ideal but it seems pretty straightforward and stable at least. Do you think it would be useful to have a default case here just in case these values change out or a new one is added somehow? Or is that not a realistic concern? I defer to your judgment!
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 think if it's not one of these, we wouldn't want to change any arm position, so default is do nothing I guess?
| onValueChanged: (newValue) => _updateControlValue(6, newValue.clamp(_minTheta, _maxTheta)), | ||
| ), | ||
| ], | ||
| child: Column( |
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 is all indented one less level because I removed the empty check since it's no longer a list
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.
lgtm!
https://viam.atlassian.net/browse/APP-9879
This PR adds the backend functionality for the pose sliders in both live and execute modes. The attached video shows the pose being adjusted on the viam example app (only changes to theta result in valid poses in the fake arm), and updating accordingly on app.viam.com in both modes. On app.viam.com, the arm card doesn't live update the pose, so the page must be refreshed.
Screen.Recording.2025-10-29.at.5.12.48.PM.mov