Arm widget #131
Arm widget #131
Conversation
|
I also changed the return type of |
| /// Move each joint on the arm to the corresponding position specified in [JointPositions] | ||
| Future<void> moveToJointPositions(JointPositions positions, {Map<String, dynamic>? extra}); | ||
| /// Move each joint on the arm to the corresponding position specified in [positions] | ||
| Future<void> moveToJointPositions(List<double> positions, {Map<String, dynamic>? extra}); |
There was a problem hiding this comment.
Nice, good catch here; definitely agree with preferring native types where possible.
| super.initState(); | ||
| camera = widget.cameras.firstOrNull; |
There was a problem hiding this comment.
[q] why move this after the super init state? Was it getting overwritten or something?
There was a problem hiding this comment.
We've just been doing it wrong the entire time. From the documentation:
Implementations of this method should start with a call to the inherited method, as in super.initState().
There was a problem hiding this comment.
Similarly, you might see changes to super.dispose() in other PRs (not in this one) for the inverse reason:
Implementations of this method should end with a call to the inherited method, as in super.dispose().
There was a problem hiding this comment.
Cool; makes intuitive sense too that you want to do the inherited construction/destruction before the custom class construction/destruction.
lib/widgets/resources/arm.dart
Outdated
| Pose endPosition = Pose(); | ||
| List<double> jointPositions = []; | ||
|
|
||
| Future<void> _positions() async { |
There was a problem hiding this comment.
[minor] I think in this case it is okay to prefix the method name with get. omitting get / set is more for API design, since this the internal workings of a widget and isn't a public API I think naming the method more clearly would be helpful.
| Future<void> _positions() async { | |
| Future<void> _getPositions() async { |
ref: https://dart.dev/effective-dart/design#avoid-starting-a-method-name-with-get
lib/widgets/resources/arm.dart
Outdated
| Future<void> updateEndPosition(String field, double increment) async { | ||
| final ep = endPosition; | ||
| if (field == 'x') { | ||
| ep.x += increment; | ||
| } else if (field == 'y') { | ||
| ep.y += increment; | ||
| } else if (field == 'z') { | ||
| ep.z += increment; | ||
| } else if (field == 'theta') { | ||
| ep.theta += increment; | ||
| } else if (field == 'ox') { | ||
| ep.oX += increment; | ||
| } else if (field == 'oy') { | ||
| ep.oY += increment; | ||
| } else if (field == 'oz') { | ||
| ep.oZ += increment; | ||
| } | ||
| await widget.arm.moveToPosition(ep); | ||
| await _positions(); | ||
| } |
There was a problem hiding this comment.
[suggestion] I think the string comparison here is fine, I might suggest defining an enum for the fields with the different options, and using a switch statement instead.
Not something blocking merging, just a preference on my side.
| crossAxisAlignment: CrossAxisAlignment.center, | ||
| children: [ | ||
| const Text('End Positions (mm)', style: TextStyle(fontWeight: FontWeight.bold)), | ||
| Table( |
There was a problem hiding this comment.
[minor] This Table is a BEEFY widget, can we break it out into its own widget?
lib/widgets/resources/arm.dart
Outdated
| TableRow(children: [ | ||
| const TableCell(child: Padding(padding: EdgeInsets.symmetric(horizontal: 2), child: Text('X', textAlign: TextAlign.end))), | ||
| TableCell( | ||
| child: Padding( | ||
| padding: const EdgeInsets.symmetric(horizontal: 2), | ||
| child: ViamButton(onPressed: () => updateEndPosition('x', -10), text: '--', size: ViamButtonSizeClass.small))), | ||
| TableCell( | ||
| child: Padding( | ||
| padding: const EdgeInsets.symmetric(horizontal: 2), | ||
| child: ViamButton(onPressed: () => updateEndPosition('x', -1), text: '-', size: ViamButtonSizeClass.small))), | ||
| TableCell( | ||
| child: Padding( | ||
| padding: const EdgeInsets.symmetric(horizontal: 2), | ||
| child: Text(endPosition.x.toStringAsFixed(2), textAlign: TextAlign.center))), | ||
| TableCell( | ||
| child: Padding( | ||
| padding: const EdgeInsets.symmetric(horizontal: 2), | ||
| child: ViamButton(onPressed: () => updateEndPosition('x', 1), text: '+', size: ViamButtonSizeClass.small))), | ||
| TableCell( | ||
| child: Padding( | ||
| padding: const EdgeInsets.symmetric(horizontal: 2, vertical: 1), | ||
| child: ViamButton(onPressed: () => updateEndPosition('x', 10), text: '++', size: ViamButtonSizeClass.small))), | ||
| ]), |
There was a problem hiding this comment.
These TableRows and TableCells have a lot of repeated code, can we make ArmRow and ArmCell widgets to clean this up?
| Future<void> updateEndPosition(_PoseField field, double increment) async { | ||
| final ep = endPosition; | ||
| switch (field) { | ||
| case _PoseField.x: | ||
| ep.x += increment; | ||
| case _PoseField.y: | ||
| ep.y += increment; | ||
| case _PoseField.z: | ||
| ep.z += increment; | ||
| case _PoseField.theta: | ||
| ep.theta += increment; | ||
| case _PoseField.oX: | ||
| ep.oX += increment; | ||
| case _PoseField.oY: | ||
| ep.oY += increment; | ||
| case _PoseField.oZ: | ||
| ep.oZ += increment; | ||
| } |
| await _getPositions(); | ||
| } | ||
|
|
||
| TableRow _getEndPositionRow(_PoseField field) { |
There was a problem hiding this comment.
Nice! Can we put this in it's own widget instead of a helper function? See here for a bit more of a deep dive on why. Basically it's more performant when the widget tree rebuilds.
There was a problem hiding this comment.
@njooma and I spoke offline, I was mistaken and TableRow is not a Widget (<--- WHAT?!) so I was mistaken. this helper function in this case is fine :)
| await _getPositions(); | ||
| } | ||
|
|
||
| TableRow _getEndPositionRow(_PoseField field) { |
There was a problem hiding this comment.
@njooma and I spoke offline, I was mistaken and TableRow is not a Widget (<--- WHAT?!) so I was mistaken. this helper function in this case is fine :)
New arm widget.
Also, update buttons so we can actually have small sizes.