This repository has been archived by the owner. It is now read-only.

Added maximum acceleration scaling factor #273

Merged
merged 1 commit into from Feb 4, 2016

Conversation

Projects
None yet
3 participants
@ghost

ghost commented Jan 12, 2016

This is a change related to ros-planning/moveit_ros#634 and adds a maximum acceleration scaling factor for optionally limiting trajectory acceleration.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Jan 13, 2016

Member

+1 lgtm

could you look into why there is a merge conflict?

Member

davetcoleman commented Jan 13, 2016

+1 lgtm

could you look into why there is a merge conflict?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 13, 2016

Yeah, I don't really know what the issue is. I am fairly new to Git and assumed someone would enlighten me....

ghost commented Jan 13, 2016

Yeah, I don't really know what the issue is. I am fairly new to Git and assumed someone would enlighten me....

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Jan 13, 2016

Member

there are many ways to fix this

make sure you have two remotes added to your .git/config file - github.com/hemes and github.com/ros-planning. then you can create a branch that tracks the upstream version

git checkout -b upstream-indigo-devel upstream/indigo-devel

switch back to your branch

git checkout indigo-devel

now merge

git merge upstream-indigo-devel

to see conflicts

git diff

commit after resolving the conflicts

Member

davetcoleman commented Jan 13, 2016

there are many ways to fix this

make sure you have two remotes added to your .git/config file - github.com/hemes and github.com/ros-planning. then you can create a branch that tracks the upstream version

git checkout -b upstream-indigo-devel upstream/indigo-devel

switch back to your branch

git checkout indigo-devel

now merge

git merge upstream-indigo-devel

to see conflicts

git diff

commit after resolving the conflicts

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 14, 2016

Nice. That took me longer than it should have but I finally got it. The issue was with your most recent whitespace changes. Should be good now; thanks for your help!

ghost commented Jan 14, 2016

Nice. That took me longer than it should have but I finally got it. The issue was with your most recent whitespace changes. Should be good now; thanks for your help!

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Jan 14, 2016

Member

FYI I'm waiting on a +1 from someone else before merging this, since it changes a ROS message in Indigo

Member

davetcoleman commented Jan 14, 2016

FYI I'm waiting on a +1 from someone else before merging this, since it changes a ROS message in Indigo

@dornhege

This comment has been minimized.

Show comment
Hide comment
@dornhege

dornhege Jan 15, 2016

Contributor

In principle this looks good to me, but I can't see a message being changed. There is only one parameter added to the public API, including a default value. Unless we'd want to guarantee ABI compatibility this should be fine.

However, the second merge commit looks odd to me as it contains a lot of (whitespace) changes that aren't visible in the "files changed" tab.

Contributor

dornhege commented Jan 15, 2016

In principle this looks good to me, but I can't see a message being changed. There is only one parameter added to the public API, including a default value. Unless we'd want to guarantee ABI compatibility this should be fine.

However, the second merge commit looks odd to me as it contains a lot of (whitespace) changes that aren't visible in the "files changed" tab.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 15, 2016

@dornhege Git newcomer here but I believe the commit strangeness is from me updating my fork to include #271, which happened after I forked it initially (and was causing a merge conflict). Maybe I merged incorrectly somehow?

ghost commented Jan 15, 2016

@dornhege Git newcomer here but I believe the commit strangeness is from me updating my fork to include #271, which happened after I forked it initially (and was causing a merge conflict). Maybe I merged incorrectly somehow?

@dornhege

This comment has been minimized.

Show comment
Hide comment
@dornhege

dornhege Jan 18, 2016

Contributor

@davetcoleman I didn't see the dependent pull request. For this one it's a +1 for me also in connection with the message.

@Hemes I believe that's from the whitespace changes, so your changes should apply cleanly. It probably only shows because it's a bit forth and back. You could try to remove the merge commit and instead git rebase upstream/indigo-devel. I would recommend to backup your repository before you do.

Contributor

dornhege commented Jan 18, 2016

@davetcoleman I didn't see the dependent pull request. For this one it's a +1 for me also in connection with the message.

@Hemes I believe that's from the whitespace changes, so your changes should apply cleanly. It probably only shows because it's a bit forth and back. You could try to remove the merge commit and instead git rebase upstream/indigo-devel. I would recommend to backup your repository before you do.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 19, 2016

@dornhege All right, I think I got it fixed up.

ghost commented Jan 19, 2016

@dornhege All right, I think I got it fixed up.

davetcoleman added a commit that referenced this pull request Feb 4, 2016

Merge pull request #273 from hemes/indigo-devel
Added maximum acceleration scaling factor

@davetcoleman davetcoleman merged commit a8a616d into ros-planning:indigo-devel Feb 4, 2016

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