Descartes Improvements

[marip8]

Various thoughts for improving the Descartes planner interface

• Convert the DescartesVertexEvaluator class into a descartes_light::StateEvaluator
• Remove the DescartesVertexEvaluator from the DescartesRobotSampler class
• Split the DescartesCollisionEdgeEvaluator class into two implementations, one for discrete collision checking and one for continuous
• Remove/revise the DescartesCollision class - it seems like a somewhat unnecessary wrapper for tasks that could be utility functions in a different class (specifically the robot sampler)
• Remove custom XML serialization in favor of boost::serialization
• Consolidate headers to simplify overall structure

[Levi-Armstrong]

Convert the DescartesVertexEvaluator class into a descartes_light::StateSampler

The DescartesVertexEvaluator is used for checking if a state is valid within the State Sampler. Do you mean switch this to descartes_light::StateEvaluator?

Remove/revise the DescartesCollision class - it seems like a somewhat unnecessary wrapper for tasks that could be utility functions in a different class (specifically the robot sampler)

I agree, this was carried over from the very first implementation of descartes_light.

Split the DescartesCollisionEdgeEvaluator class into two implementations, one for discrete collision checking and one for continuous

The only reason it is all in one class is to simplify the switching for the user by changing the evaluator type in tesseract_collision::CollisionCheckConfig. This is used throughout tesseract for switching how-to collision check states and transitions between states. At the tesseract motion planner side, the user only sets the tesseract_collision::CollisionCheckConfig parameters and the Descartes collision evaluator is added behind the scene. It should still be able to split these up into two different classes and keep the same workflow simplifying the API so I am on board with splitting them up.

Remove custom XML serialization in favor of boost::serialization

I agree.

Consolidate headers to simplify the overall structure

Sounds good to me.

[marip8]

Convert the DescartesVertexEvaluator class into a descartes_light::StateSampler

The DescartesVertexEvaluator is used for checking if a state is valid within the State Sampler. Do you mean switch this to descartes_light::StateEvaluator?

Yes. That was a typo

My thought on the collision checking class is that it would be more clear if they were separate. Personally I think having flags for choosing between the two is more ambiguous and requires more unnecessary information than having a class that only takes one. Ideally, both the continuous and discrete collision checker could use the same interface, but I don't think that's possible with the current API and the requirement of the continuous collision checker to have two states. Regardless, I think there are some simple things we can do to improve the DescartesCollision class

Similarly, we might consider making two robot sampler "layers", specifically one that does collision checking and one that doesn't. It seems a bit hacky to give the sampler a null pointer for collision checking and an allow_collision parameter and to do a lot of logic checking if the user doesn't intend to do collision checking

[Levi-Armstrong]

My thought on the collision checking class is that it would be more clear if they were separate. Personally I think having flags for choosing between the two is more ambiguous and requires more unnecessary information than having a class that only takes one. Ideally, both the continuous and discrete collision checker could use the same interface, but I don't think that's possible with the current API and the requirement of the continuous collision checker to have two states. Regardless, I think there are some simple things we can do to improve the DescartesCollision class

I agree with you, I just want to make sure at the default planner profile we still leverage the tesseract_collision::CollisionCheckConfig to control the defining of the collision evaluator.

Similarly, we might consider making two robot sampler "layers", specifically one that does collision checking and one that doesn't. It seems a bit hacky to give the sampler a null pointer for collision checking and an allow_collision parameter and to do a lot of logic checking if the user doesn't intend to do collision checking

Sounds good.

[Levi-Armstrong]

@marip8 Is this still valid?

[marip8]

I think these are still valid points even after the refactor of the profile. I think we should at least remove the DescartesVertexEvaluator; we may need to see if the comments about separating the collision checkers still hold

[Levi-Armstrong]

Thanks, I have added it to the punch list.