Search energy classes

[jvarley]

@mateiciocarlie

In this Commit:
Switched the SearchEnergy class to have different subclasses with different energy() functions rather than having different methods for thinks like contact energy or potential energy. The functional setup made it difficult to create search energies that had state. For instance some of my deep learning energy functions had to load in heatmaps, something that should not happen on every step of the planner and should also not be owned by the base searchenergy class.

I have tested this on linux. It builds just fine with both independently and with other plugins using catkin_make. In my opinion, this is ready to be pulled in.

For the Future:
This should also make it easy to pull our more of the different hardcoded constants such as the mixing parameter in the contact and potential quality into member variables which can then be set in the EG planner dlg.

[mateiciocarlie]

This leaks memory as a possible previous SearchEnergy instance is not deleted

[mateiciocarlie]

This looks great overall, but has a few relatively small issues.

The whole mType business has not been integrated with the new model. The variable still exists, but is not set properly in the constructors. Some of the classes still do things differently based on internal checks on the mType variable.

Ideally, we would have a pure abstract getType() function in the interface that each energy type must overload to return the proper string.

Also, inheriting classes should no longer fork anywhere based on their own type. It looks like perhaps we need to also specialize the function that decides if a state is legal. The base class could provide the default version (which just checks if there are no collisions) leaving inheriting classes to override that as needed.

Finally, how has this been tested? Compiling is great, but we'd need to also see that the various energy types still work, in as much as possible. Some of them should be accessible thrgough the Eigengrasp planner dialog.

Overall thanks a lot for looking into this - as the comment in the original file said, it was horrible before, and looks great now!

[jvarley]

1)I fixed the memory leak, good catch.
2) I added a setType function to searchEnergy, and called it before returning the energyType in getSearchEnergy which is the only way to construct and energy. This works.
3) I tested this by using EGPlannerDLG and ran it with contact energy, potential energy, and contact and potential energy, and they all behaved as expected.

Overall, I feel this is ready, and provides the same functionality as before, in a cleaner manner, and allows the creation of stateful search energies. Completely agree that a pure abstract getType(), and an inherited function that decides if a state is legal should both be added but would rather address those in separate PRs.

[mateiciocarlie]

OK, sounds good. I had missed the setType inside the factory.

Hope that a proper getType and inherited legal state check will indeed come in a future PR :)

[mateiciocarlie]

Actually, let's make one more change before we merge. Since the factory is the only way to build one of these that also correctly sets the type flag, can we make it impossible to build one another way? Perhaps with a protected constructor or something similar.

[jvarley]

I changed
bool legal() const;
to:
virtual bool legal() const;

So it can be overridden. I also made SearchEnergy() protected so it can't be initialized outside the factory method.