DAIRLab / dairlib

MIT License
65 stars 26 forks source link

Declare all internal states as drake State or CacheEntry #311

Open Brian-Acosta opened 1 year ago

Brian-Acosta commented 1 year ago

So far we have been relatively laissez faire when it comes to storing controller state (i.e. previous OSC solutions, tracking data, etc.) as member variables instead of in some way that would expose them to the relevant context. This seems to be causing weird bugs with efforts to gymify our controllers, as some internal state variables are not being reset with the rest of the context. We should consider refactoring as these issues come up and being more strict in subsequent code review to avoid these kinds of design patterns.

Brian-Acosta commented 1 year ago

Something I learned trying to write a controller complying with these rules: to store something as an AbstractState or CacheEntry, it must be able to be stored as an AbstractValue , which requires that it be a copyable object or a cloneable pointer.

Many drake objects (like MathematicalProgram ) have explicitly deleted copy constructors and copy assignment operators, but can be stored as an abstract value using drake's copyable_unique_ptr wrapper. E.g. drake::copyable_unique_ptr<MathematicalProgram> prog(std::make_unique<MathematicalProgram>()).

Additionally, if you write a container class for your optimization which holds the MP, the decision variables, and the constraints, you can use drake::copyable_unique_ptr<MathematicalProgram> to ensure that the default copy constructor and copy assignment operators of your container class aren't implicitly deleted. This way you avoid having to write your own copy assignment operator, which can get tricky.