Skip to content
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

rationale behind jump threshold? #773

Closed
rhaschke opened this issue Feb 16, 2018 · 14 comments
Closed

rationale behind jump threshold? #773

rhaschke opened this issue Feb 16, 2018 · 14 comments
Assignees

Comments

@rhaschke
Copy link
Collaborator

Can somebody explain me the rationale behind RobotState::testJointSpaceJump()?
Obviously the goal is to avoid joint-state jumps. However, I don't get why it computes the mean joint-state change over the whole trajectory and uses jump_threshold as a scaling factor, such that:
[single joint state change] < jump_threshold * [mean joint state change]

Wouldn't it be more reasonable to specify an absolute joint-space jump threshold?
In this case, one could even try to sample the Cartesian trajectory more densely to maintain the threshold...

I stumbled over this, because my generated joint-space trajectory is non-uniform (both in joint and Cartesian space). Thus the averaging process tries to find the mean of a multi-modal distribution...

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Feb 16, 2018

I think we discussed this before, and iirc, no one really knows (any more).

Same sort of confusion about the additional point(s) that get added to the trajectory.

@rhaschke
Copy link
Collaborator Author

Shouldn't we change it then?

@gavanderhoorn
Copy link
Member

I believe that would make sense, yes. computeCartesianPath(..) has always been a bit of an afterthought and hasn't gotten much attention. If we can find a sane(r) way to get similar functionality then we should improve it.

(I tried to find the discussion(s) I seem to remember, but I can't. Maybe they only happened in my mind ..)

@davetcoleman
Copy link
Member

@mlautman

@jrgnicho
Copy link
Contributor

Perhaps using a max jump threshold that is a function of the joint's max speed might be more appropriate.

@mlautman
Copy link
Member

+1 for absolute joint-space jump threshold. I implemented an equivalent computeCartesianPath function that uses Descartes under the hood and found that an absolute joint-space jump threshold was very helpful to ensure that the robot didn't flip joints mid trajectory.

@v4hn
Copy link
Member

v4hn commented Feb 20, 2018

👎 to a fixed absolute joint-space jump threshold as a drop-in replacement for the current mechanism.
But the current approach is inherently flawed, I fully agree.

The factor was most likely added to account for different robot kinematics.
The absolute joint difference can be quite different between a 3dof and a 7dof arm.

If we want to have an absolute factor, we should imo also add an interpolation mechanism that attempts to fill in steps that are too big.

@mlautman
Copy link
Member

mlautman commented Feb 20, 2018

I agree that an absolute joint space jump threshold is an imperfect solution but it has certain properties that are nice: it's easy for users to understand and it's easy to implement although I'm not sure it's strictly better than the existing solution. I'm not sure what an interpolation mechanism that fills in steps would look like. @v4hn Can you elaborate?

@mlautman
Copy link
Member

Upon re-reading through the implementation, jump_threshold is a terrible variable name because it implies absolute joint space jump but it acts as more of a scaling factor. Perhaps a variable name change to jump_threshold_factor would help reduce confusion.

@rhaschke
Copy link
Collaborator Author

If we want to have an absolute factor, we should imo also add an interpolation mechanism that attempts to fill in steps that are too big.

That's what I proposed. I will take care of this after all the deadlines.

Perhaps a variable name change to jump_threshold_factor would help reduce confusion.

Good suggestion. However, is this variable also exposed to any external (ROS) interface? In this case, it get's tricky. Such a variable name change should be scheduled only for Lunar or later.

@rhaschke rhaschke assigned rhaschke and unassigned davetcoleman and v4hn Feb 20, 2018
@davetcoleman
Copy link
Member

The factor was most likely added to account for different robot kinematics.

I agree with @v4hn, @isucan was very keen on making as many parameters as possible auto-tuning to avoid magic numbers. Hence the flexibility of OMPL+MoveIt!. Have an alternative jump threshold approach is fine with me, but I like the current approach that is flexible for different robot kinematic types

we should imo also add an interpolation mechanism that attempts to fill in steps that are too big.

This might be a good idea but I think it would be best to interpolate by doing finer IK calls rather than just simple joint interpolation.

Such a variable name change should be scheduled only for Lunar or later.

Changing variable names in header files does not break ABI/API as far as I know, it gets obfuscated into a symbol anyway.

@rhaschke
Copy link
Collaborator Author

we should imo also add an interpolation mechanism that attempts to fill in steps that are too big.

This might be a good idea but I think it would be best to interpolate by doing finer IK calls rather than just simple joint interpolation.

For sure! This was my intention.

Such a variable name change should be scheduled only for Lunar or later.

Changing variable names in header files does not break ABI/API as far as I know, it gets obfuscated into a symbol anyway.

This is true. However, I was asking for ROS interfaces, i.e. ROS messages, services, etc.

@v4hn
Copy link
Member

v4hn commented Feb 21, 2018

That's what I proposed. I will take care of this after all the deadlines.

👍 👍 👍

I think it would be best to interpolate by doing finer IK calls rather than just simple joint interpolation.

Joint space interpolation is not sufficient and I meant work space interpolation before IK.
In addition though, I believe it might make sense to add JS interpolation as a "last attempt" fallback (of course verifying interpolated points in work space).
If you plan right through a singularity, workspace interpolation will not help at all, but js interpolation can do the trick to "flip" redundant joints.

@mlautman
Copy link
Member

mlautman commented Mar 1, 2018

Such a variable name change should be scheduled only for Lunar or later.

Changing variable names in header files does not break ABI/API as far as I know, it gets obfuscated into a symbol anyway.

This is true. However, I was asking for ROS interfaces, i.e. ROS messages, services, etc.

GetCartesianPath has a jump_threshold variable which is used by the computeCartesianPath capability.

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

No branches or pull requests

6 participants