search

borglab / GTDynamics

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

Change `internal::*Key` to `*Key` #282

Closed varunagrawal closed 2 years ago

varunagrawal commented 2 years ago

The current pattern we use to define various keys is for example gtd.internal.PoseKey. After using this pattern extensively the past many months, it seems unnecessarily verbose, since to add the keys to factor graphs, we have to type the whole thing in, which defeats the purpose of marking it "internal". The current functional approach only works when dealing with Values and it seems like a half-baked design choice. The same thing applies when writing tests.

My suggestion would be to remove the internal namespace and allow direct access. Since this is a design choice, I want to hear @dellaert's opinion on this.

gchenfc commented 2 years ago

I know this is really old but was just looking back at old issues and saw this

I think this is actually getting at a higher level design philosophy that's related to #290, which is that the gtdynamics::internal::XXXKey design philosophy is to force users to stay within the framework we've prescribed to prevent them shooting their own foot. My goal with #290 was to continue that philosophy to the factor constructors, because as it stands, several factors require you to use the internal namespace to construct them which shouldn't be that way.

The alternative philosophy is to allow everyone to use whatever keys they want, even if it doesn't make sense. Although this allows us to hack new applications faster, personally I think it's ultimately more cumbersome and prone to bugs that are difficult to debug (correctness bugs).

Just my $0.02

varunagrawal commented 2 years ago

The issue here is that if I use a GTSAM key, that does not have an index referring to the link or the joint, and so there is a type mismatch.

A simple example for you to try: run a forward kinematics optimization using a GTSAM key and you'll see where these high level functions fail.

It's nice to have these conveniences but not at the cost of flexibility. A user should be able to use whatever key makes semantic sense for their use case.

Regarding the internal, we are using internal::key when adding factors to the factor graph so that basically defeats the purpose.

dellaert commented 2 years ago

I like both points of view, but I find myself agreeing with @varunagrawal more, so, let's remove the verbose namespace