personalrobotics / aikido

Artificial Intelligence for Kinematics, Dynamics, and Optimization
https://personalrobotics.github.io/aikido/
BSD 3-Clause "New" or "Revised" License
214 stars 30 forks source link

Skeleton should be locked while planning and postprocessing #447

Open aditya-vk opened 6 years ago

aditya-vk commented 6 years ago

Previously the skeleton was locked in planTo... methods before calling any of the planners to plan.

In the new API this does not happen anywhere currently. I think it should now reside in <Planner><Problem> -> plan() methods or someplace just before planning calls are made. @brianhou, can you confirm?

brianhou commented 6 years ago

Hmm, I could be wrong but I was under the impression that @sniyaz tried to preserve the locking in the lowest level planTo functions (e.g. https://github.com/personalrobotics/aikido/blob/master/src/planner/vectorfield/VectorFieldPlanner.cpp#L151).

Regardless, I think this sounds reasonable. We might also want to consider adding an argument bool lockSkeleton to Planner::plan that controls whether the planner needs to lock the skeleton. This would be useful for wrapper/delegate planners: if the wrapper has already acquired the skeleton's lock in order to do something (e.g. sample configurations from a TSR), then the delegate doesn't need to lock the skeleton.

brianhou commented 5 years ago

We should use DART's MetaSkeleton::getLockableReference() method rather than locking Skeletons (available since DART 6.5).

@jslee02's comment from #417:

Previously, AIKIDO had to use Skeleton rather than MetaSkeleton due to lack of mutex support in MetaSkeleton. In the latest release of DART (6.5.0), MetaSkeleton has this function so let's utilize it (see https://github.com/dartsim/dart/pull/1011).

sniyaz commented 4 years ago

I believe that https://github.com/personalrobotics/aikido/pull/588 handles and documents this, at least for DART planners (I'm not sure what to do with non-DART planners, considering that those don't even know they're operating on a skeleton).

@brianhou @aditya-vk Does this seem like enough to close this issue, or are there other things we want to look at?