Skip to content
This repository has been archived by the owner on Nov 13, 2017. It is now read-only.

Adding acceleration scaling factor #634

Merged
merged 3 commits into from
Feb 4, 2016
Merged

Adding acceleration scaling factor #634

merged 3 commits into from
Feb 4, 2016

Conversation

ghost
Copy link

@ghost ghost commented Jan 12, 2016

Extending changes in #547 to add acceleration scaling factor.

This adds support for setting a maximum acceleration scaling factor in the MotionPlanRequest msg, which can be used to scale the acceleration bounds in add_time_parametrization.

Includes necessary changes to
move_group
move_group_interface
Spinbox for motion planning rviz plugin

Requires other pulls requests in moveit_msgs and moveit_core which are stated below.

@ghost
Copy link
Author

ghost commented Jan 13, 2016

I noticed I had a few naming issues in motion_planning_rviz_plugin_frame.ui that would generate warnings during the build process. These were corrected in commit 5a18d6e

<item>
<widget class="QLabel" name="label_20">
<property name="text">
<string>Acceleration Scaling:</string>
Copy link
Member

Choose a reason for hiding this comment

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

Does this long name widen the UI too much? Screenshot?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I just saw your screenshot.

If you change this to "Accel Scaling" does it reduce the required screen width? The MoveIt widget is already too wide IMHO so I'd prefer not to do something to make it even wider

Copy link
Author

Choose a reason for hiding this comment

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

I agree with you and thing the whole widget is a bit big. As for the addition, I can do a side by side comparison but I am pretty sure that the widget width went unchanged.

Copy link
Author

Choose a reason for hiding this comment

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

Before and after. No change in width but the widget did get one line taller (expected).

screenshot from 2016-01-14 11 29 47

After

@davetcoleman
Copy link
Member

lgtm +1

@awesomebytes
Copy link
Contributor

Just a tiny comment: This is awesome. Looking forward to merge it (and maybe release?).

@davetcoleman
Copy link
Member

@Hemes could you fix the merge conflicts? sorry about that.

@ghost
Copy link
Author

ghost commented Jan 27, 2016

@davetcoleman ok, I got the conflicts fixed. In the process I had the same issue as last time where all of the commits that my fork was behind were added as commits to my fork. So I deleted the commit and got it working the second time but in the process seem to have messed up the commit history for the pull request. The final result is correct now.

BTW is there a good reference or best practice for keeping up to date with upstream repositories? I set the original repo as the upstream remote but am unclear on the proper commands to update and get changes that have happened between the pull request and now (i.e., when to pull, merge, rebase, etc.).

@davetcoleman
Copy link
Member

re:BTW - I maintain my indigo-devel to mirror the upstream version exactly, I never commit new stuff to that branch. Then I can just do git pull upstream indigo-devel and then merge that updated branch to whatever feature branches I have. Note that by upstream I mean ros-planning

davetcoleman added a commit that referenced this pull request Feb 4, 2016
Adding acceleration scaling factor
@davetcoleman davetcoleman merged commit 750bce6 into moveit:indigo-devel Feb 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants