ggould-tri
commented
4 years ago After some cleanup, https://gist.github.com/ggould-tri/b686cf978754bc2bffe55ee99daff8a5 is the (now much simplified) form the code ended up in.
Bad things that could be resolved but only by adding yet more code:
- Lack of typecheck of the joint in the ctor (what should by rights be a compile failure is detected not even at ctor time but at runtime!)
- Taking an MBT at the ctor (ie, caller has to know about multibody::internal)
Bad things that I think cannot be avoided on current architecture:
- caller cannot directly call ctor.
- retention of the MBT pointer (lifetime hazard: aliases passed to derived objects) as nothing but a database on which to resolve
joint_id_ - boilerplatey Clone methods
- Frankly, every single use of
templateorT, since the caller does not in fact need this class for anything but double
Bad things resulting from bad API docs:
- unnecessary energy/power computations
jwnimmer-tri
(This is a metabug; subsidiary issues should be opened for its contributing factors if they are judged sufficiently individually buggy)
A real-world system I am modeling has a hinge with complex dynamics (static, dynamic, and viscous friction, a catch, and a spring). I decided to model this by creating a
ForceElement, and wrote the necessary four functions (force, energy, non-conservative power, and conservative power) ondoubleas well as a class that took a Joint to apply this to.The resulting odyssey is chronicled on slack:
The major problems that I encountered:
MultibodyPlant::AddForceElementwill not accept a simple concrete object but rather demands fowarded arguments to a class template ctor (ie, you have to include all of the scalar clone boilerplate).ThrowIfAlgebraicLoopExistswill not accept a template class that throws on clone (ie, you actually have to implement all of the scalartypes). (see also #12253)Jointin the ctor makes the class un-cloneable, as you cannot resolve ajoint_idat clone time (the joints in the cloned MBT having not yet been constructed).joint_idwhen you receive it -- the obvious "dereference the ID and dynamic_cast it" check asserts.Of course each of the above also suffers from its own incomprehensibility problem:
multibody_tree.ccto understand.Which of these problems are or aren't worth solving -- and if so how -- will doubtless be a matter of some controversy. However I think there is a meta defect here that we have made a straightforward matter (a couple of dozen lines of math) into a journey through the heart of darkness of forwarding constructors, templates of templates, and the objects-versus-IDs mess. An object which could conceptually be two symbolic expressions (energy, nonconservative power) and a correspondence to joint {q,v} (or even just one function of double, given that energy and power are unused) is instead somewhat north of our maximum code-review length even without tests.