RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.28k stars 1.26k forks source link

math: Add sugar for `Class(std::initializer_list<T>)` for `RigidTransform` + `RollPitchYaw` #11033

Closed EricCousineau-TRI closed 5 years ago

EricCousineau-TRI commented 5 years ago

From: https://reviewable.io/reviews/robotlocomotion/drake/10959#-LaX1_PJ4UjFsxlYfPq8:-LapnPdt7Mziuu88TpmA:bu87cfd

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

BTW Is this achievable via initializer lists? RigidTransformd({1, 2, 3})?

Would likely require an initializer list constructor in RigidTransform -- seems very useful though, file an issue?

Same thing for RollPitchYaw as well.

jwnimmer-tri commented 5 years ago

I am not sure that RigidTransform({a, b, c}) would obviously imply to a reader that the thing being created is an xyz translation, so I'm not sure that such a sugar is a good tradeoff.

EricCousineau-TRI commented 5 years ago

At present, we have the same ambiguity, but more verbose: https://github.com/RobotLocomotion/drake/blob/4c6246197ac615a335cf7302d2d95f921303af47/math/rigid_transform.h#L112

I'd like to throw my hat in the ring of "strict improvement", but am up for being convinced otherwise. (I don't think this is ambiguous, as RPY can only be specified explicitly, which makes sense for now.)

jwnimmer-tri commented 5 years ago

To me, Vector3 more clearly implies a translation, vs a pile of 3 doubles. As always though, looking at calling code would inform the best answer.

EricCousineau-TRI commented 5 years ago

To me, Vector3 more clearly implies a translation [...]

Disagreed. Vector3 implies a 3-tuple to me, in any space. The only disambiguation is Eigen::Translation3d, but meh, b/c it can be implicitly constructed.

jwnimmer-tri commented 5 years ago

This is why I hate backlog feature-request issues like this. If you need the constructor, open a PR to implement it and demonstrate how it's useful. If you don't need it, then there is no need to clutter up the issue database with hypothetical requests.

sherm1 commented 5 years ago

My fault for encouraging this issue when it came up in a review discussion where an Isometry3({1,2,3}) turned into a RigidTransform(Vector3{1,2,3}). Maybe that Vector3 isn't such a bad thing, @EricCousineau-TRI and we should close this?

amcastro-tri commented 5 years ago

I personally hate typing RigidTransform(Vector3{1,2,3}) when we could type RigidTransform({1,2,3}) with no ambiguity. I we meant rpy, then we'd just write RigidTransform(RollPitchYaw(1,2,3)). So yes, I agree with @EricCousineau-TRI on this one.

EricCousineau-TRI commented 5 years ago

Thanks @amcastro-tri! Will open the PR and test it out - and no worries, @sherm1!

Agreed on the policy that Jeremy mentioned too. I do get a bit trigger happy on speculative issues sometimes... Will work on that.

sherm1 commented 5 years ago

I am worried that RigidTransform({.1, .2, .3}) could be mistaken for roll-pitch-yaw by people who use that form frequently, especially since we always take rotations first. Also, the initializer list could be any length, and 3, 6, and 7 all make some sense for RigidTransform. I guess we would have to insist on it being exactly 3 long for the constructor.

I don't think the minor annoyance of typing or reading Vector3 here is unreasonable to avoid ambiguity.

EricCousineau-TRI commented 5 years ago

I've typed up a prototype PR; will add ya for review!

EricCousineau-TRI commented 5 years ago

I am worried that RigidTransform({.1, .2, .3}) could be mistaken for roll-pitch-yaw [...]

If that's a problem, it's not relevant here (at least for me)? As I mentioned above, Vector3 means any space to me, so if that's the case, then that's a defect with the class itself. Vector3(1, 2, 3) vs. {1, 2, 3} mean the same thing. Translation3(1, 2, 3) has more meaningful semantics (but aren't enforceable with Eigen's permitted implicit conversions).

SeanCurtis-TRI commented 5 years ago

I don't see that it's a defect in the underlying API. The underlying API uses distinct types to make it unambiguous (RPY vs V3). Introducing a type that is a least-common denominator for both of those is what introduces the ambiguity.

jwnimmer-tri commented 5 years ago

My reading of the above is that myself, Sherm, and Sean all share the belief that RigidTransform(Vector3(#,#,#)) implies a translation -- that we do not share Eric's belief that Vector3 has no semantics beyond a 3-tuple of doubles. An Eigen::Vector is not an Eigen::Array, for good reason!

EricCousineau-TRI commented 5 years ago

Er, but we have the constructor RollPitchYaw(Vector3) - what does that mean?

sherm1 commented 5 years ago

but we have the constructor RollPitchYaw(Vector3) - what does that mean?

That's a good question. There we are misusing the Vector3 as a container -- (roll,pitch,yaw) angles do not form a vector (in any space!). I think this is conventional but it should really be an Array3 -- I suspect that would not make it easier to use for most users but I could be convinced otherwise.

sherm1 commented 5 years ago

I would actually be fine with RollPitchYaw({.1, .2, .3}) - that's not ambiguous at all. But RigidTransform({.1, .2, .3}) is problematic.

jwnimmer-tri commented 5 years ago

We hate RigidTransform({.1, .2, .3}) but do we hate RigidTransform(RollPitchYaw(.1, .2, .3), {.1, .2, .3}) also? That is, do we care if the two-argument constructors gain the de-Vector sugar? The PR has some examples of that.

jwnimmer-tri commented 5 years ago

I suspect that would not make it easier to use for most users but I could be convinced otherwise.

I believe Array3 and Vector3 are implicitly convertible, so it would be more of a documentation affordance than a static-type-checking thing.

SeanCurtis-TRI commented 5 years ago

I agree with @jwnimmer-tri summation and with @sherm1's point. It is conventional to use Vector3 as both a vector and an array of size three. Arguably, if we were to restrict Vector3 to be strictly a vector, then one might argue that it should always have a space associated with it.

As for the two-argument constructors....I'm mixed. I like the compactness but I feel that the call-site loses meaning for later readers.

jwnimmer-tri commented 5 years ago

Possibly we've exhausted our "post opening bids to the issue" and should move on to "defect specific sample code in the PR".

amcastro-tri commented 5 years ago

I posted my 2 cents in #11083. Summary, I like RigidTransform({1,2,3}). I am not sure (mostly not) about RigidTransform(RollPitchYaw(.1, .2, .3), {.1, .2, .3})

amcastro-tri commented 5 years ago

leh me add that I love how python code looks with things like RigidTransform([1., 2., 3.]). It reads very neat, why would we hate it in C++, just cause it's C++ and it's supposed to be overly verbose?

jwnimmer-tri commented 5 years ago

In experimental locally, I'm pretty convinced that writing RigidTransformd(Vector3d(1,2,3)) should just be Translation3d(1,2,3). Isn't that better in every regard? Better than RigidTransformd({1,2,3}) even? I'll submit a sample PR soon.

EricCousineau-TRI commented 5 years ago

Another option is that we get rid of all single-argument rotation-only or translation-only constructors for RigidTransform, and figure out an unambiguous placeholder for identity-like values, e.g.

RigidTransform(nullopt, my_translation)
RigidTransform(my_rotation, nullopt)

This would be my preference, where we could stick to (rotation, translation) ordering always, and then Vector3 has less ambiguity.

I also disprefer excessive verbosity when I want to hack. It'd be nice if RigidTransform permitted hacking in the midst of clarity, which is what an initializer list could offer.

EricCousineau-TRI commented 5 years ago

[...] should just be Translation3d(1,2,3)

I'm not sure how you'll be able to enforce explicit conversions of Eigen::Translation<> in an API friendly way that also permits constants and such. (I haven't tried it, but I'm guessing that may complicate things...)

jwnimmer-tri commented 5 years ago

My counter-proposal solution is in #11090.

EricCousineau-TRI commented 5 years ago

There we are misusing the Vector3 as a container -- (roll,pitch,yaw) angles do not form a vector (in any space!)

To follow-up: As I understand it, this is disqualifying Vector3 in RollPitchYaw because it's not a "physically meaningful" vector space; however, we use VectorX for things like generalized positions... which may not be a "physically meaningful" vector space?

In argument of consistency, I still favor VectorN to always be considered a "tuple of things for use with linear algebra" rather than "a physically meaningful vector space". To make things be ArrayN means we have to be careful to ensure it's used in linear algebra and not array algebra - and yuck! That's like porting over issues with NumPy and operator* from Python to C++!

(If my understanding is wrong, please correct me!)

jwnimmer-tri commented 5 years ago

To follow-up: As I understand it, this is disqualifying Vector3 in RollPitchYaw because it's not a "physically meaningful" vector space ...

I almost didn't write the "An Eigen::Vector is not an Eigen::Array, for good reason!" above, because I knew the RPY constructor used it and so it might derail the discussion of RigidTransform. I wanted to add some flourish to a boring discussion, but I should have trusted my hesitation because that whole subthread is a distraction. Sorry!

The relevant debate is about RigidTransform, and it goes like this: Even if you, Eric, think that RigidTransformd({.1, .2, .3}) and RigidTransformd(Vector3d{.1, .2, .3}) are 100% interchangeable and convey absolutely no semantic difference to the reader, several of us do read Vector3d to hint that the numbers are a translation instead of a rotation, and do think that RigidTransformd({.1, .2,, .3}) is too ambiguous on its own.

All else being equal there is virtue in brevity, but if RigidTransformd({.1, .2,, .3}) doesn't sit well with some readers, then we also need to build an exhaustion argument in support of it -- that all of the other possible resolutions are worse -- before we resign ourselves to it. Perhaps that is Translation3d, or perhaps that is (nullopt, translation), or etc.

... why would we hate it in C++, just cause it's C++ and it's supposed to be overly verbose?

I don't think trolling helps us seek out and agree on improvements.

sherm1 commented 5 years ago

I don't think trolling helps us seek out and agree on improvements.

Where were you when I needed you on Internet comment threads? :)

sherm1 commented 5 years ago

(If my understanding is wrong, please correct me!) [regarding q and v as vectors]

As Jeremy pointed out, this is an aside from the RigidTransform topic, but as an FYI ... Generalized positions and generalized velocities actually are a vector space, which is why we can write M v̇ = f for Newton's 2nd law (i.e., that really is a matrix-vector multiply, not just a convenient fiction). It is a high-dimensional space, but legit. (Generalized coordinates are a bit weirder to talk about as a space in the most-general case (with quaternions and what not), but for the basics you'll often seen Newton's 2nd written M q̈ = f which isn't totally wrong.) The generalized velocity space is dual to the generalized force space so the f is a vector there also. Similarly, the constraint forces λ form a vector space (though a different one from f); we can write f = Gᵀ λ mapping from one space to the other, with similar relations between generalized velocities and constraint-space velocities.

So anyway calling q and v vectors is technically correct while r,p,y isn't. I don't think this is an important distinction to make in practice though. Any thoughts to add, @mitiguy ?

EricCousineau-TRI commented 5 years ago

Following comment in #11090, closing this out as that PR and its successor, #11198, address the underlying pain point. Even if the PR does not land (dunno why that'd happen), we can just leave this closed until it becomes an acute pain point.

Regarding the Eigen::Vectors as strictly physical vector spaces and what units they imply, I'm still a bit concerned on whether or not this will get encoded in a policy (implicitly or explicitly). However, I'll wait until this actually manifests before raising a complaint again. If anyone has concerns about this point that they'd like answered more immediately, let me know, and we can schedule an (ideally) <=15min VC to discuss.

Thank y'all!