-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make factors and others type-agnostic (precursor to SphericalJoints) #286
Conversation
PoseFactor(gtsam::Key wTp_key, gtsam::Key wTc_key, gtsam::Key q_key, | ||
const gtsam::noiseModel::Base* cost_model, | ||
const gtdynamics::Joint* joint); | ||
PoseFactor(const gtsam::noiseModel::Base *cost_model, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just add this constructor in addition? No reason to remove the previous constructor.
gtsam::Key qVel_key, | ||
const gtsam::noiseModel::Base* cost_model, | ||
const gtdynamics::Joint* joint); | ||
TwistFactor(const gtsam::noiseModel::Base *cost_model, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same deal
gtsam::Key q_key, gtsam::Key qVel_key, gtsam::Key qAccel_key, | ||
const gtsam::noiseModel::Base* cost_model, | ||
const gtdynamics::JointTyped* joint); | ||
TwistAccelFactor(const gtsam::noiseModel::Base *cost_model, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same deal
JointCoordinateType limit_threshold) | ||
JointLimitDoubleFactor(gtsam::Key q_key, | ||
const gtsam::noiseModel::Base::shared_ptr &cost_model, | ||
double lower_limit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do this and not keep it generic via JointCoordinateType?
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:
|
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 usingJointTyped
and calling theJointTyped
version oftransformTwistTo
. The problem with this is that then any instance ofTwistFactor
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 theJoint
version oftransformTwistTo
(argumentconst 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.
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
borglab/gtsam#885 (Adjoint with Jacobian) needs to be merged in first.
After borglab/gtsam#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
JointTyped
arguments to justJoint
arguments which lets us remove all the uglyboost::static_pointer_cast<const JointTyped>
castsJointLimitFactor
toJointLimitDoubleFactor
since it really is specific to just double-type joints and we will have to be adding new JointLimitFactors for other Joint types soonAdjointMapJacobianQ
in favor ofpose.Adjoint(twist, H_pose, H_twist)
combined withpose = joint->poseOf(link, pose_H_q)
andH_q = H_pose * pose_H_q
.transformWrenchToTorqueVector
so thatTorqueFactor
could be made type-agnostic. This now mirrors all the other functions e.g.transformTwistTo
transformTwistAccelTo
etc.boost::optional<Matrix &>
toOptionalJacobian<-1, -1>
All unit tests still passing of course, so if anyone has objections, add unit tests :)