borglab / GTDynamics

Full kinodynamics constraints for arbitrary robot configurations with factor graphs.
BSD 2-Clause "Simplified" License
39 stars 10 forks source link

Make factors and others type-agnostic (precursor to SphericalJoints) #286

Closed gchenfc closed 2 years ago

gchenfc commented 2 years ago

Previously, many factors and other places in the code were depending on JointTyped which I think was not appropriate. I refactored these to be agnostic to joint type as an intermediate step to adding Spherical/Ball and Universal joints. I believe the code is now much cleaner and less hacky.

For example, TwistFactor was previously using JointTyped and calling the JointTyped version of transformTwistTo. The problem with this is that then any instance of TwistFactor has to be templated / type known at compile-time. However, the type kind of has to be determined at run-time, e.g. reading from an SDF you couldn't possibly know the types before runtime. Now, TwistFactor calls the Joint version of transformTwistTo (argument const Values &q_and_q_dot).

Tradeoffs

The upside is that this makes the code much more general and easier to maintain (IMO) and will make adding Spherical/Ball/other joints much easier. The downside is a potential runtime penalty due to runtime polymorphism (not benchmarked, but I would guess negligible).

Alternatives

An alternative would be to shift all the runtime polymorphism to the graph generation (e.g. DynamicsGraph, aFactors, vFactors, etc) but I believe this makes the code much more difficult to maintain since we would have tons of RTTI, e.g.

if (auto *joint_cast = dynamic_pointer_cast<ScrewJointBase>(joint)) {
  graph.emplace_shared<TwistFactor<ScrewJointBase>>(args);
} else if ... // for every joint type, and for every factor type

Alternatively, the factor creation functions could also be moved back into the Joint classes, but I think previously Frank strongly pushed for moving the factor creation functions out since the Link and Joint classes should be purely for defining the robot configuration and not have anything to do with factor graphs / gtsam.

Notes/dependencies

https://github.com/borglab/gtsam/pull/885 (Adjoint with Jacobian) needs to be merged in first. After https://github.com/borglab/gtsam/pull/884 merges, we can also optionally clean up some code (I left comments about "Due to quirks of OptionalJacobian, this is the cleanest way (Gerry)" prior to having created that PR, but with the PR then I think it can be cleaner), but that's low priority.

API / Breaking Changes

All unit tests still passing of course, so if anyone has objections, add unit tests :)

gchenfc commented 2 years ago

Abandoning this in favor of #291

Since this PR actually had some other changes wrapped inside it, the changes that still apply are moved to the following PRs: