Skip to content
This repository has been archived by the owner on Dec 1, 2020. It is now read-only.

Feature: lock start end setpoint #100

Merged
merged 12 commits into from
Jun 25, 2020

Conversation

Roelemans
Copy link
Contributor

Closes PM-480

Description

It is now possible to lock the end and startpoint of a subgait to ensure good transition to imported side subgaits. Any change to the side subgait buttons/checkboxes can be undone.

Damn that undo functionality took so much more time than the rest...

Changes

  • Added classes SideSubgaitController and SideSubgaitView to handle the view and storage of the side subgaits. This will allow easy extension to more side subgaits if this ever becomes applicable.
  • Added logic to lock the start and endpoint of the subgait currently being edited.
  • Added logic to allow undo/redo of locking start/end points.
  • Wrote tests for new functionality.

@Roelemans Roelemans requested review from a team and RutgerVanBeek and removed request for a team June 22, 2020 18:13
@codecov
Copy link

codecov bot commented Jun 22, 2020

Codecov Report

Merging #100 into develop will increase coverage by 3.85%.
The diff coverage is 88.99%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #100      +/-   ##
===========================================
+ Coverage    69.09%   72.95%   +3.85%     
===========================================
  Files           15       17       +2     
  Lines         1430     1671     +241     
  Branches       151      186      +35     
===========================================
+ Hits           988     1219     +231     
  Misses         404      404              
- Partials        38       48      +10     
Flag Coverage Δ
#production 55.47% <82.92%> (+6.29%) ⬆️
#test 99.25% <100.00%> (+0.12%) ⬆️
Impacted Files Coverage Δ
...rc/march_rqt_gait_generator/gait_generator_view.py 27.14% <25.00%> (-0.07%) ⬇️
.../src/march_rqt_gait_generator/side_subgait_view.py 28.57% <28.57%> (ø)
...ch_rqt_gait_generator/gait_generator_controller.py 83.95% <83.15%> (+5.83%) ⬆️
...arch_rqt_gait_generator/side_subgait_controller.py 94.00% <94.00%> (ø)
...ait_generator/model/modifiable_joint_trajectory.py 92.06% <100.00%> (+2.70%) ⬆️
...r/test/nosetests/gait_generator_controller_test.py 100.00% <100.00%> (ø)
...test/nosetests/modifiable_joint_trajectory_test.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8dff5d9...fa0b49e. Read the comment docs.

Copy link
Contributor

@Olavhaasie Olavhaasie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It immediately crashes on startup 😢

@Roelemans
Copy link
Contributor Author

Sorry 'bout that 😢. Should be fixed now

# Conflicts:
#	march_rqt_gait_generator/src/march_rqt_gait_generator/gait_generator_controller.py
Copy link
Contributor

@RutgerVanBeek RutgerVanBeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment about the user interaction:

When you 'lock' a point without first specifying how, the point is not locked but the checkbox is checked. I think you should automatically check standing as well in that case as that is kind of your default right now.

@Roelemans
Copy link
Contributor Author

Roelemans commented Jun 24, 2020

When you 'lock' a point without first specifying how, the point is not locked but the checkbox is checked. I think you should automatically check standing as well in that case as that is kind of your default right now.

Hmm this was a deliberate choice since I felt this way was the most intuitive. I could also simply lock it to the current position, that makes more sense than defaulting to standing I think. @marnixbrands @gaiavdh what do you think?

@RutgerVanBeek
Copy link
Contributor

Hmm this was a deliberate choice since I felt this way was the most intuitive. I could also simply lock it to the current position, that makes more sense than defaulting to standing I think. @marnixbrands @gaiavdh what do you think?

I think locking into the current position might be nice also. I just think that checking the lock checkbox and then nothing happening is not so intuitive.

Copy link
Contributor

@Olavhaasie Olavhaasie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was playing around a bit with this new feature and found two things:

  1. When I imported the sit down gait as previous subgait and played the viz the joints wouldn't move, only when I added a new setpoint.
  2. When I then inverted the gait and imported the standing subgait as next subgait it crashed and I got this error:
Traceback (most recent call last):
  File "/home/olav/march_ws/install/march_rqt_gait_generator/lib/python2.7/dist-packages/march_rqt_gait_generator/gait_generator_controller.py", line 53, in <lambda>
    lambda: self.import_side_subgait('next'))
  File "/home/olav/march_ws/install/march_rqt_gait_generator/lib/python2.7/dist-packages/march_rqt_gait_generator/gait_generator_controller.py", line 251, in import_side_subgait
    self.next_subgait = subgait
  File "/home/olav/march_ws/install/march_rqt_gait_generator/lib/python2.7/dist-packages/march_rqt_gait_generator/gait_generator_controller.py", line 422, in next_subgait
    self.handle_sidepoint_lock('next')
  File "/home/olav/march_ws/install/march_rqt_gait_generator/lib/python2.7/dist-packages/march_rqt_gait_generator/gait_generator_controller.py", line 395, in handle_sidepoint_lock
    self.view.update_joint_widgets(self.subgait.joints)
  File "/home/olav/march_ws/install/march_rqt_gait_generator/lib/python2.7/dist-packages/march_rqt_gait_generator/gait_generator_view.py", line 173, in update_joint_widgets
    self.update_joint_widget(joint)
  File "/home/olav/march_ws/install/march_rqt_gait_generator/lib/python2.7/dist-packages/march_rqt_gait_generator/gait_generator_view.py", line 179, in update_joint_widget
    show_effort_plot=self.effort_plot_check_box.isChecked())
  File "/home/olav/march_ws/install/march_rqt_gait_generator/lib/python2.7/dist-packages/march_rqt_gait_generator/joint_plot.py", line 128, in update_setpoints
    [indices, position_data, velocity_data] = joint.interpolate_setpoints()
  File "/home/olav/march_ws/install/march_shared_classes/lib/python2.7/dist-packages/march_shared_classes/gait/joint_trajectory.py", line 93, in interpolate_setpoints
    position = BPoly.from_derivatives(time, yi)
  File "/usr/lib/python2.7/dist-packages/scipy/interpolate/interpolate.py", line 1641, in from_derivatives
    raise ValueError("x coordinates are not in increasing order")
ValueError: x coordinates are not in increasing order

Furthermore, It's kinda fun because now you can just import any two subgaits and create a transition between them 🙂

@Olavhaasie Olavhaasie changed the title Feature/pm 480 lock start end setpoint Feature: lock start end setpoint Jun 25, 2020
Copy link
Contributor

@Olavhaasie Olavhaasie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lookin' fine boi 🔥

@Olavhaasie Olavhaasie merged commit d624379 into develop Jun 25, 2020
@Olavhaasie Olavhaasie deleted the feature/PM-480-lock-start-end-setpoint branch June 25, 2020 10:13
@marnixbrands
Copy link

I think the way it is implemented now is really nice! Locking the current position when no previous/next subgait is selected is a good choice I guess, as you can use the "standing" checkbox to go to the default standing position if you want

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants