search

moveit / moveit_calibration

Hand-eye calibration tools for robot arms.
BSD 3-Clause "New" or "Revised" License
132 stars 76 forks source link

Enforce min rotation between samples #62

Open JStech opened 3 years ago

JStech commented 3 years ago

If there is too little rotation between two samples, the solver can fail. Rather than waiting until the solver is run, this prevents the sample from being captured in the first place. It also provides a helpful error message.

JStech commented 3 years ago

Thanks for the review, @felixvd. I actually implemented this because I had misdiagnosed a solver failure I was hitting (which turned out to be much simpler: #63). I have refined it some and I like your suggestions, so I'll try to incorporate them.

  1. Why is only the orientation compared? Imagine that you move the end-effector or camera horizontally in a grid above the marker, so the orientation does not change between samples, but the marker changes position in the camera view. It seems like this PR would refuse these samples, although (unless I am mistaken) they are informative and should not be discarded.

Actually, rotation is more informative than translation. See section III.C on page 354 of Tsai & Lenz, 1989 (page 354, bottom left), although I admit I haven't fully digested this paper and I don't know how much their recommendations generalize to other solvers. Besides that, measuring translation similarity isn't very straightforward. There's the problem of scale: a 1cm translation might be huge on one robot and tiny on another. There's also the difficulty of considering both the camera and effector translation--one could have a large translation and the other doesn't. (This can't happen with rotation--if the two are rigidly attached, then the magnitude of the rotation will be the same for both, although the axes will differ.)

  1. Instead of refusing to record a new sample, a dialogue box should allow the user to override the warning.

Good idea. I'll see if I can learn enough Qt to do this.

  1. Why compare with only the previous sample? Why not go through the other recorded samples? It's not like it's an expensive or frequent operation.

I just pushed this.

I would suggest:

  1. Making a function transformsAreClose(transform1, transform2) to calculate the distance between two transforms (both in translation and rotation)
  2. When a new sample is considered, loop through the samples in memory and confirm that the distance is sufficient
  3. If it is not, alert the user, display the distance to the closest orientation, and let them choose if they want to keep the sample or skip it.

I think that would be more readable (= easier to maintain) and easier to use.

Since I'm inclined to continue ignoring the translations, I think it's sufficient to calculate the rotation angle in line, as it's just one line. I could be convinced otherwise, though.

felixvd commented 3 years ago

I don't really know how translation differences affect solver performance either, but I assume there is non-zero information in two samples where the marker is in opposite corners of the camera view, even if their orientation is the same. You could use the marker scale to set the limit. Either way it is just a warning and I wouldn't insist on including it.

Since I'm inclined to continue ignoring the translations, I think it's sufficient to calculate the rotation angle in line, as it's just one line. I could be convinced otherwise, though.

Matter of taste I suppose. If you keep it rotation-only and in-line, I would make the variable name much more verbose (e.g. previous_eef_rotation).

felixvd commented 3 years ago

You can copy-paste the dialog messagebox from here, by the way.