multibody: Implement PlanarJoint #13747
Conversation
|
@RussTedrake or @amcastro-tri for feature review. Either wait for #13711 to merge or only review the second commit (the first is a copy of #13711 and will be rebased once that merges). |
There was a problem hiding this comment.
+@RussTedrake for feature review.
i've reviewed the planar_joint files (not planar_mobilizer, which I assume are identical to #13711)
+@SeanCurtis-TRI for monday platform review.
Reviewed 4 of 7 files at r1.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
multibody/tree/planar_joint.h, line 24 at r1 (raw file):
/// y-axes of frame F and to rotate with repect to frame F about the z-axis of /// frame F. The translations along the x- and y-axes of F, the rotation about /// the z-axis of F and their rates, specify the state of the joint.
those are the configurations, not the states.
|
multibody/tree/planar_joint.h, line 24 at r1 (raw file): Previously, RussTedrake (Russ Tedrake) wrote…
Doesn't adding the rates make it state? |
|
multibody/tree/planar_joint.h, line 24 at r1 (raw file): Previously, mpetersen94 (Mark Petersen) wrote…
oops. my bad. i withdraw my comment. |
There was a problem hiding this comment.
Excellent!
Reviewed 3 of 7 files at r1.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
multibody/tree/planar_joint.h, line 84 at r1 (raw file):
/// Returns `this` joint's damping constant in N⋅s/m for the translational /// degrees and N⋅m⋅s for the roational degrees. The damping force (in N)
nit degrees -> degree
multibody/tree/planar_joint.h, line 87 at r1 (raw file):
/// is modeled as `fᵢ = -dampingᵢ⋅vᵢ, i = 1, 2` i.e. opposing motion, /// with vᵢ the translation rates along the i-th axis for `this` joint (see /// get_translation_rates()) and fᵢ the force on child body B along the same
I'd add; ...is the force on child body B at Mo and expressed in F. That is, f_BMo_F = (f₁, f₂).
multibody/tree/planar_joint.h, line 90 at r1 (raw file):
/// i-th axis. The damping torque (in N⋅m) is modeled as `τ = -damping₃⋅ω` /// i.e. opposing motion, with ω the angular rate for `this` joint (see /// get_angular_rate()) and τ the torque on child body B about the same axis.
Not sure "about the same axis." parses well here since you are not talking of any other axis?
In monogram you'd denote the z axis of the basis of, say frame F, with Fz. Then you could say that the torque on body B expresed in frame F is t_B_F = τ⋅Fz_F.
multibody/tree/planar_joint.h, line 114 at r1 (raw file):
/// @returns a constant reference to `this` joint. const PlanarJoint<T>& set_translations(Context<T>* context, const Vector2<T>& translations) const {
nit, I personally prefer monogram notation. In Drake that notation is widely accepted so it is expected a Drake user is familiar with it. It has the nice advantage of being unambiguous and I believe a user going through these docs would have exactly the same learning curve reading once symbol or the other. Acutally no, if you know monogram, you won't even need to read the docs, the monogram says it all. Ok, enough of me writing. All I want to say is that I'd write p_FoMo_F. Your nice Doxygen docs then complement with "human readable" text.
multibody/tree/planar_joint.h, line 147 at r1 (raw file):
/// @returns The rates of change of `this` joint's translations as stored in /// the `context`. Vector2<T> get_translation_rates(const systems::Context<T>& context) const {
nit, actually you do get a translational velocity vector (even if 2D). What do you think of get_translational_velocity()? and document that it returns v_FoMo_F? ( with the nice accompanying human readable text you provided as well)
multibody/tree/planar_joint.h, line 159 at r1 (raw file):
/// @returns a constant reference to `this` joint. const PlanarJoint<T>& set_translation_rates( systems::Context<T>* context, const Vector2<T>& translations_dot) const {
ditto here. I'd use monogram whenever is applicable since a Drake user will be familiar with it (if not they must learn it at some point anyway but worth the price once they know it because then they know everything they'd need just from the monogram symbols).
multibody/tree/planar_joint.h, line 192 at r1 (raw file):
/// general `Joint::default_positions()`. /// @returns The default position of `this` stored in `default_positions_` Vector3<double> get_default_position() const {
nit "position" in my mind sounds like "position vector", while "positions" (plural) seems more appropriate when we mean "vector of generalized positions".
Maybe refactor to the plural form? ditto below.
There was a problem hiding this comment.
I glanced at the build file and found the test to be commented out. Is this properly running through CI? Also, despite the lgtm it seems @amcastro-tri has created a number of open issues (some potentially including discussion).
So, I'm going to defer platform review to wait for the following:
- [] multibody: Implement PlanarMobilizer #13711 merges (and this gets rebased).
- [] Build file gets updated so unit test runs.
- [] resolution of open comments.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
a discussion (no related file):
@mpetersen94 Just so you know, when you do a PR like this, there's a trick to using Reviewable to help your reviewers along. You've got two commits and you want the reviewers to focus primarily on a single commit. Do the following:
- Submit a PR. By default, it will show a single revision. In the upper right hand corner, where Reviewable indicates the number of commits, a pull down will list: "Combine commits for Review".
- Change the pull down to the "Review each commit separately". This will give you two revisions, one per commit. At that point it becomes easy for reviewers to ignore the first commit entirely in Reviewable.
- Go down to any of the files under review and click on it like you were going to make comment. The dialog pops open for you to type. Dismiss it by hitting the trashcan icon.
- Go back up to where you set "Review each commit separately" and switch it back to "Combine commits for Review". It will warn you that your first two revisions have already been baked (due to the action you took of clicking on a file). Approve the change.
The switch and switch back has the effect of breaking out the commits so your reviewers can easily distinguish but allows all subsequent changes to simply be a single revision. It's a very slick trick. In the case of this PR, the need for this trick isn't as strong as the two commits only really collide in the build file. But it's a good trick to have in your toolbelt.
multibody/tree/BUILD.bazel, line 514 at r1 (raw file):
) # drake_cc_googletest(
nit: Pretty sure this shouldn't be commented out.
mpetersen94
force-pushed
the
planar_joint
branch
from
07b9a57 to
1190a04
Compare
There was a problem hiding this comment.
Don't know how I missed the commented out test in the build file.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
@mpetersen94 Just so you know, when you do a PR like this, there's a trick to using Reviewable to help your reviewers along. You've got two commits and you want the reviewers to focus primarily on a single commit. Do the following:
- Submit a PR. By default, it will show a single revision. In the upper right hand corner, where Reviewable indicates the number of commits, a pull down will list: "Combine commits for Review".
- Change the pull down to the "Review each commit separately". This will give you two revisions, one per commit. At that point it becomes easy for reviewers to ignore the first commit entirely in Reviewable.
- Go down to any of the files under review and click on it like you were going to make comment. The dialog pops open for you to type. Dismiss it by hitting the trashcan icon.
- Go back up to where you set "Review each commit separately" and switch it back to "Combine commits for Review". It will warn you that your first two revisions have already been baked (due to the action you took of clicking on a file). Approve the change.
The switch and switch back has the effect of breaking out the commits so your reviewers can easily distinguish but allows all subsequent changes to simply be a single revision. It's a very slick trick. In the case of this PR, the need for this trick isn't as strong as the two commits only really collide in the build file. But it's a good trick to have in your toolbelt.
Thanks for teaching that. I wondered how that was accomplished!
multibody/tree/BUILD.bazel, line 514 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Pretty sure this shouldn't be commented out.
Done.
multibody/tree/planar_joint.h, line 84 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
nit degrees -> degree
Done.
multibody/tree/planar_joint.h, line 87 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
I'd add; ...is the force on child body B at Mo and expressed in F. That is, f_BMo_F = (f₁, f₂).
Done.
multibody/tree/planar_joint.h, line 90 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
Not sure "about the same axis." parses well here since you are not talking of any other axis?
In monogram you'd denote the z axis of the basis of, say frame F, with Fz. Then you could say that the torque on body B expresed in frame F ist_B_F = τ⋅Fz_F.
Done.
multibody/tree/planar_joint.h, line 114 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
nit, I personally prefer monogram notation. In Drake that notation is widely accepted so it is expected a Drake user is familiar with it. It has the nice advantage of being unambiguous and I believe a user going through these docs would have exactly the same learning curve reading once symbol or the other. Acutally no, if you know monogram, you won't even need to read the docs, the monogram says it all. Ok, enough of me writing. All I want to say is that I'd write
p_FoMo_F. Your nice Doxygen docs then complement with "human readable" text.
Done.
multibody/tree/planar_joint.h, line 147 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
nit, actually you do get a translational velocity vector (even if 2D). What do you think of
get_translational_velocity()? and document that it returnsv_FoMo_F? ( with the nice accompanying human readable text you provided as well)
Done.
multibody/tree/planar_joint.h, line 159 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
ditto here. I'd use monogram whenever is applicable since a Drake user will be familiar with it (if not they must learn it at some point anyway but worth the price once they know it because then they know everything they'd need just from the monogram symbols).
Done.
multibody/tree/planar_joint.h, line 192 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
nit "position" in my mind sounds like "position vector", while "positions" (plural) seems more appropriate when we mean "vector of generalized positions".
Maybe refactor to the plural form? ditto below.
Done for the getter but changing it for the setter caused a collision with the super-class function name (i.e. set_default_positions calling this->set_default_positions() was causing segfaults). Thoughts?
There was a problem hiding this comment.
@mpetersen94 -- i think it's ready for you to rebase master and push.
Reviewed 2 of 6 files at r2.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
multibody/tree/planar_joint.h, line 192 at r1 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
Done for the getter but changing it for the setter caused a collision with the super-class function name (i.e.
set_default_positionscallingthis->set_default_positions()was causing segfaults). Thoughts?
well, lets see how a user would actually call this. Do you think a user would store (x, y, theta) in a 3D vector? or rather a user would have a 2D vector and a scalar angle?
Because in that case you could have the couple const PlanarJoint& set_position(const Vector2&) and const PlanarJoint& set_angle(const T&) (notice I return a reference to this joint so that you can chain them).
For the getters you could have Vector2 get_position() and T get_angle().
Or some other option? Otherwise it is kind of confusing that this class will end up with very similar looking methods, one from this class and one form the parent.
Summarizing, I am trying to imagine the call site. Also an advantage of specializing a joint is that you can add more semantics to it (like in set position and set angle which of course the parent joint could not have).
multibody/tree/planar_joint.h, line 91 at r2 (raw file):
/// is modeled as `τ = -damping₃⋅ω` i.e. opposing motion, with ω the angular /// rate for `this` joint (see get_angular_rate()) and τ the torque on child /// body B expresed in frame F as τ_B_F = τ⋅Fz_F.
nit, sry my bad. I actually purposely wrote t_B_F (with a Roman "t", not a Greek "tau"). In Drake we follow the notation convention of using "tau" for generalized forces and "t" for 3D vector torques. Sometimes they coincide, but generally they don't.
multibody/tree/planar_joint.h, line 110 at r2 (raw file):
/// the translations of `this` joint equals `p_FoMo_F`. /// @param[in] context The context of the model this joint belongs to. /// @param[in] p_FoMo_F The desired translations in meters to be stored in
nit, I know Drake users are meant the read the monogram docs, but probably spelling this as: "The position vector from Fo to Mo, expressed in F" or as "The position of Mo measured in Fo, expressed in F" might be a nice refresher for those still learning.
multibody/tree/planar_joint.h, line 145 at r2 (raw file):
nit,
rates of change of
thisjoint's translations
That to me sounds like the definition of derivative in the F frame. Why not to say: "The velocity v_FoMo_F of Mo measured and expressed in frame F".
multibody/tree/planar_joint.h, line 156 at r2 (raw file):
nit,
The desired rates of change
ditto regarding the definition as a time derivative.
To tell you how I think about it; if I can give someone an invariant physical quantity (aka tensor) I usually do, because those are frame independent and have a clear mental picture no matter what your level of math is. IMO it provides more physical intuition.
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
multibody/tree/planar_joint.h, line 192 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
well, lets see how a user would actually call this. Do you think a user would store (x, y, theta) in a 3D vector? or rather a user would have a 2D vector and a scalar angle?
Because in that case you could have the coupleconst PlanarJoint& set_position(const Vector2&)andconst PlanarJoint& set_angle(const T&)(notice I return a reference tothisjoint so that you can chain them).
For the getters you could haveVector2 get_position()andT get_angle().Or some other option? Otherwise it is kind of confusing that this class will end up with very similar looking methods, one from this class and one form the parent.
Summarizing, I am trying to imagine the call site. Also an advantage of specializing a joint is that you can add more semantics to it (like in set position and set angle which of course the parent joint could not have).
sry, I realize I meant set_default_position() and set_default_angle() with similar spelling for getters. What do you think?
There was a problem hiding this comment.
Everyone's favorite, an unrequested drive-by review! Mostly just me pushing back against @amcastro-tri's suggestions to phrase documentation in terms of physical quantities. I think that's good guidance in general, but not in this case, as I've noted inline. @mpetersen94 feel free to disregard my comments if you prefer. I'm very excited for this to land.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
multibody/tree/planar_joint.h, line 192 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
sry, I realize I meant
set_default_position()andset_default_angle()with similar spelling for getters. What do you think?
Speaking as a user who is eagerly awaiting this feature, I will store (x,y,theta) in a 3D vector.
multibody/tree/planar_joint.h, line 110 at r2 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
nit, I know Drake users are meant the read the monogram docs, but probably spelling this as: "The position vector from Fo to Mo, expressed in F" or as "The position of Mo measured in Fo, expressed in F" might be a nice refresher for those still learning.
nit: This is not a 3D position vector. It is a 2D vector of generalized positions. I think this should remain as is.
multibody/tree/planar_joint.h, line 145 at r2 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
nit,
rates of change of
thisjoint's translationsThat to me sounds like the definition of derivative in the F frame. Why not to say: "The velocity v_FoMo_F of Mo measured and expressed in frame F".
nit: This isn't a 3D velocity though. It's the rate of changed of generalized positions. I don't think that putting things in monogram notation is appropriate here.
multibody/tree/planar_joint.h, line 156 at r2 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
nit,
The desired rates of change
ditto regarding the definition as a time derivative.
To tell you how I think about it; if I can give someone an invariant physical quantity (aka tensor) I usually do, because those are frame independent and have a clear mental picture no matter what your level of math is. IMO it provides more physical intuition.
nit: Same comment as above.
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
a discussion (no related file):
Previously, mpetersen94 (Mark Petersen) wrote…
Thanks for teaching that. I wondered how that was accomplished!
No problem. Someone taught it to me, so it's important to share the wealth. And it would require a fair amount of experimentation to arrive at the end destination of one's own accord.
The one thing I failed to explicitly mention (that I'll add here for completeness) is to add a comment instructing the reviewer as to the semantics of the revisions. :) I don't add that in the main PR description, but usually do it in the very next comment prefaced with a "Note to reviewers" heading.
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
a discussion (no related file):
Is there a plan in place for parsing this joint?
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @avalenzu, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
multibody/tree/planar_joint.h, line 110 at r2 (raw file):
Previously, avalenzu (Andres Valenzuela) wrote…
nit: This is not a 3D position vector. It is a 2D vector of generalized positions. I think this should remain as is.
Another drive-by: Monogram notation works just fine in 2d. Nothing about it requires the position vector to be 3d. So I agree with Alejandro here -- make use of the notation.
Also a misplaced side comment: it is misleading to put x,y,theta into a Vec3 -- that is definitely NOT a vector. Storing it that way might be convenient, but in that case the Vec3 type is being misused as an arbitrary container, like an std::array<double, 3>. I don't really object to that convenience and would be tempted to do it myself, but I don't think the API should encourage it. I think we should have either set_pose(Vec2 xy, double theta) or set_position(Vec2) and set_angle(double).
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @avalenzu, @mpetersen94, @RussTedrake, @SeanCurtis-TRI, and @sherm1)
multibody/tree/planar_joint.h, line 192 at r1 (raw file):
Previously, avalenzu (Andres Valenzuela) wrote…
Speaking as a user who is eagerly awaiting this feature, I will store (x,y,theta) in a 3D vector.
@sherm1 put it nicely above, I also think we should discourage that to show up in the API even if as an external user you prefer to use Vector3 as a tuple.
multibody/tree/planar_joint.h, line 110 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Another drive-by: Monogram notation works just fine in 2d. Nothing about it requires the position vector to be 3d. So I agree with Alejandro here -- make use of the notation.
Also a misplaced side comment: it is misleading to put x,y,theta into a Vec3 -- that is definitely NOT a vector. Storing it that way might be convenient, but in that case the Vec3 type is being misused as an arbitrary container, like an
std::array<double, 3>. I don't really object to that convenience and would be tempted to do it myself, but I don't think the API should encourage it. I think we should have eitherset_pose(Vec2 xy, double theta)orset_position(Vec2)andset_angle(double).
I see your point @avalenzu. However I absolutely share @sherm1's view, sorry if I didn't spell it as nice.
Along those lines, to me set_translations() reads more like std::array<double, 3>, which we don't want.
I'd go with set_position(), no plural.
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @avalenzu, @mpetersen94, @RussTedrake, @SeanCurtis-TRI, and @sherm1)
multibody/tree/planar_joint.h, line 145 at r2 (raw file):
Previously, avalenzu (Andres Valenzuela) wrote…
nit: This isn't a 3D velocity though. It's the rate of changed of generalized positions. I don't think that putting things in monogram notation is appropriate here.
Au contraire: the derivative of a 2d position vector is a 2d velocity vector. So monogram notation is ideal for use here.
It is also true that you could view the planar joint's coordinates q[3] and speeds v[3] just as generalized quantities. But when the generalized coordinates have been chosen to correspond to meaningful physical quantities it is very useful to present them that way in the joint API. We also provide base-class generic methods for treating everything as generalized.
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
multibody/tree/planar_joint.h, line 192 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
@sherm1 put it nicely above, I also think we should discourage that to show up in the API even if as an external user you prefer to use Vector3 as a tuple.
I wouldn't be using Vector3 as a tuple. I would be using it as an element of a three dimensional vector space of joint coordinates (e.g. in an optimization). I don't want to cause more noise on this review, please feel free to ping me on Slack for any further discussion.
multibody/tree/planar_joint.h, line 110 at r2 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
I see your point @avalenzu. However I absolutely share @sherm1's view, sorry if I didn't spell it as nice.
Along those lines, to me
set_translations()reads more likestd::array<double, 3>, which we don't want.I'd go with
set_position(), no plural.
I didn't mean to imply that monogram notation only works in 3d. I just prefer to keep my physical position vectors and my generalized position vectors distinct. I'm not going to argue about that here though.
In terms of naming, the corresponding methods in PrismaticJoint are set_translation(), and set_translation_rate() so I think set_translations() and set_translation_rates() would be best here for consistency.
All that being said, I'm super excited for this to land, so I'm going to get out of the way and let @mpetersen94 and his reviewers do whatever they need to do!
multibody/tree/planar_joint.h, line 145 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Au contraire: the derivative of a 2d position vector is a 2d velocity vector. So monogram notation is ideal for use here.
It is also true that you could view the planar joint's coordinates q[3] and speeds v[3] just as generalized quantities. But when the generalized coordinates have been chosen to correspond to meaningful physical quantities it is very useful to present them that way in the joint API. We also provide base-class generic methods for treating everything as generalized.
For me it's more often useful to see any joint's coordinates as generalized quantities. I'm not seeing the base-class methods in joint.h, but I don't want to create more noise on this review. Could you point me towards them on Slack?
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
a discussion (no related file):
@avalenzu, I think your point is well taken! I'll open a non-blocking discussion here so that we don't need to slow down the review.
I just prefer to keep my physical position vectors and my generalized position vectors distinct.
100% agree. Joint already provides APIs for generalized position vectors and this PR adds sugar for specific physical quantities. So for users like yourself we do have the API you need? or do you think there is an API missing?
I'm not seeing the base-class methods in joint.h
For generalized positions we have position_start() start etc. But I agree we don't have GetPositions(context). It could be added if it'd proved useful.
multibody/tree/planar_joint.h, line 192 at r1 (raw file):
Previously, avalenzu (Andres Valenzuela) wrote…
I wouldn't be using Vector3 as a tuple. I would be using it as an element of a three dimensional vector space of joint coordinates (e.g. in an optimization). I don't want to cause more noise on this review, please feel free to ping me on Slack for any further discussion.
I guess the implicit convention that is not mentioned is: Vector2 and Vector3 within drake::multibody:: semantically mean "physical vectors". Outside, e.g. within some optimization, I won't argue if Vector3 is used in some other way.
Notice that even Eigen makes this distinction, with Eigen::Array.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
a discussion (no related file):
Previously, amcastro-tri (Alejandro Castro) wrote…
@avalenzu, I think your point is well taken! I'll open a non-blocking discussion here so that we don't need to slow down the review.
I just prefer to keep my physical position vectors and my generalized position vectors distinct.
100% agree.
Jointalready provides APIs for generalized position vectors and this PR adds sugar for specific physical quantities. So for users like yourself we do have the API you need? or do you think there is an API missing?I'm not seeing the base-class methods in joint.h
For generalized positions we have
position_start()start etc. But I agree we don't haveGetPositions(context). It could be added if it'd proved useful.
Thanks, @amcastro-tri! I usually end up dealing with generalized position vectors for entire model instances, which is well supported by the current API. If I ever run into a case where a Joint::GetPositions(context) method would be useful, I'll open an issue for it.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
multibody/tree/planar_joint.h, line 192 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
I guess the implicit convention that is not mentioned is:
Vector2andVector3withindrake::multibody::semantically mean "physical vectors". Outside, e.g. within some optimization, I won't argue if Vector3 is used in some other way.
Notice that even Eigen makes this distinction, withEigen::Array.
We have a minor breakdown. If I use VectorX, it doesn't imply physical quantity, even if it has size 3. We pass VectorX around all the time for generalized state. So, if I happen to know that the size of VectorX is 3 at compile time, it doesn't seem unreasonable to then use Vector3 - that should be a performance choice and not a semantics choice.
I think this points to the ambiguity of the type in Drake and don't see it as cut and dried. Certainly, for someone coding using physical quantities almost exclusively, the association seems inviolate. But I don't see that as universal, even in the context of Drake.
There was a problem hiding this comment.
@mpetersen94, this is almost ready, it'd need a rebase. I like @sherm1's proposal:
- set_pose(Vec2 xy, double theta) or set_position(Vec2) and set_angle(double).
With similarly named getters and setters for random distributions.
Reviewed 1 of 6 files at r2.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
multibody/tree/planar_joint.h, line 114 at r2 (raw file):
/// for details. /// @returns a constant reference to `this` joint. const PlanarJoint<T>& set_translations(Context<T>* context,
nit, per discussion, would it make more sense to call this set_position()?
multibody/tree/planar_joint.h, line 171 at r2 (raw file):
/// @returns The rate of change of `this` joint's angle θ as stored in the /// `context`. const T& get_angular_velocity(const systems::Context<T>& context) const {
nit, feel free to ignore but I'll give you the choice. set_angular_rate() as you had it before seemed to work better for this scalar quantity, since in Drake we typically say "angular velocity" for a vector quantity. But I do see the appeal.
multibody/tree/planar_joint.h, line 194 at r2 (raw file):
/// general `Joint::default_positions()`. /// @returns The default position of `this` stored in `default_positions_` Vector3<double> get_default_positions() const {
See bug fix in #13757.
Once that's fixed, this essentially adds a method that pretty much has the same name. That's why we proposed set_default_position() and set_default_angle() instead (or set_default_pose() if you prefer the signature with both as Sherm suggested.
multibody/tree/planar_joint.h, line 211 at r2 (raw file):
/// definition of the position. void set_random_positions_distribution( const Vector3<symbolic::Expression>& position) {
similar dissonance here. I think usually useful to pack as vectors three distributions that are the same (though they might not be). I imagine usually the distribution for rotation will be different than that for the position (which often will be the same on x and y?).
Therefore, it'd seem here you'd also want set_random_position_distribution() and set_random_angle_distribution()?
There was a problem hiding this comment.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
multibody/tree/planar_joint.h, line 192 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
We have a minor breakdown. If I use
VectorX, it doesn't imply physical quantity, even if it has size 3. We passVectorXaround all the time for generalized state. So, if I happen to know that the size ofVectorXis 3 at compile time, it doesn't seem unreasonable to then useVector3- that should be a performance choice and not a semantics choice.I think this points to the ambiguity of the type in Drake and don't see it as cut and dried. Certainly, for someone coding using physical quantities almost exclusively, the association seems inviolate. But I don't see that as universal, even in the context of Drake.
Agreed there is plenty of ambiguity and lots of ways to say the same thing. To reduce confusion, in MBP we at least try to follow this discipline consistently: use VectorX for generalized coordinates, but limit Vector2 and Vector3 for use as 2d and 3d vector quantities.
If we added methods for accessing a Joint's generalized coordinates directly we'd likely use VectorUpTo7 for returning q's and VectorUpTo6 for returning v's. Usually once you are down to a specific joint type it is much more convenient to interpret its generalized coordinates as representing physical quantities instead. (E.g. the generalized coordinate for a rotational joint is an angle.)
There was a problem hiding this comment.
One more vote for getting this landed (and following up with the parser). I'm hoping to use it!
Reviewable status: 7 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
There was a problem hiding this comment.
Another vote here. @mpetersen94, if you need help feel free to make me a collaborator of your fork and I can push the last set of changes.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
|
I've added most of the changes and am rewriting the tests to match the updated API as we speak! Should have it back for review by lunch. |
mpetersen94
force-pushed
the
planar_joint
branch
from
1190a04 to
8cb59e0
Compare
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @amcastro-tri, @RussTedrake, and @SeanCurtis-TRI)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Is there a plan in place for parsing this joint?
I don't really know the parsing code and this joint isn't supported by sdf. I was planning on opening another pr with bindings for this joint. If there is a consensus on the right way to add parsing, I'd be happy to throw it into that pr.
multibody/tree/planar_joint.h, line 192 at r1 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Agreed there is plenty of ambiguity and lots of ways to say the same thing. To reduce confusion, in MBP we at least try to follow this discipline consistently: use VectorX for generalized coordinates, but limit Vector2 and Vector3 for use as 2d and 3d vector quantities.
If we added methods for accessing a Joint's generalized coordinates directly we'd likely use VectorUpTo7 for returning q's and VectorUpTo6 for returning v's. Usually once you are down to a specific joint type it is much more convenient to interpret its generalized coordinates as representing physical quantities instead. (E.g. the generalized coordinate for a rotational joint is an angle.)
Done.
multibody/tree/planar_joint.h, line 91 at r2 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
nit, sry my bad. I actually purposely wrote
t_B_F(with a Roman "t", not a Greek "tau"). In Drake we follow the notation convention of using "tau" for generalized forces and "t" for 3D vector torques. Sometimes they coincide, but generally they don't.
Done.
multibody/tree/planar_joint.h, line 110 at r2 (raw file):
Previously, avalenzu (Andres Valenzuela) wrote…
I didn't mean to imply that monogram notation only works in 3d. I just prefer to keep my physical position vectors and my generalized position vectors distinct. I'm not going to argue about that here though.
In terms of naming, the corresponding methods in
PrismaticJointareset_translation(), andset_translation_rate()so I thinkset_translations()andset_translation_rates()would be best here for consistency.All that being said, I'm super excited for this to land, so I'm going to get out of the way and let @mpetersen94 and his reviewers do whatever they need to do!
Done.
multibody/tree/planar_joint.h, line 114 at r2 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
nit, per discussion, would it make more sense to call this
set_position()?
Done.
multibody/tree/planar_joint.h, line 145 at r2 (raw file):
Previously, avalenzu (Andres Valenzuela) wrote…
For me it's more often useful to see any joint's coordinates as generalized quantities. I'm not seeing the base-class methods in
joint.h, but I don't want to create more noise on this review. Could you point me towards them on Slack?
Done?
multibody/tree/planar_joint.h, line 156 at r2 (raw file):
Previously, avalenzu (Andres Valenzuela) wrote…
nit: Same comment as above.
Done.
multibody/tree/planar_joint.h, line 194 at r2 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
See bug fix in #13757.
Once that's fixed, this essentially adds a method that pretty much has the same name. That's why we proposed
set_default_position()andset_default_angle()instead (orset_default_pose()if you prefer the signature with both as Sherm suggested.
Done.
multibody/tree/planar_joint.h, line 211 at r2 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
similar dissonance here. I think usually useful to pack as vectors three distributions that are the same (though they might not be). I imagine usually the distribution for rotation will be different than that for the position (which often will be the same on x and y?).
Therefore, it'd seem here you'd also wantset_random_position_distribution()andset_random_angle_distribution()?
I'm not sure how to set part of the mobilizer's random distribution like I did for the default position/angle. Is this something that Drake is missing?
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
multibody/tree/planar_joint.h, line 21 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit s/resepct/respect
(Aretha Franklin called....)
Done. 😂
multibody/tree/planar_joint.h, line 22 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW "...about the z-axis of frame F." Once I finished reading this paragraph, this sentence felt dissatisfying. Frames F and M are guaranteed to have z-axes that point in the same direction. The emphasis on Frame F here seems to be suggestive in the wrong direction.
That said, it gets cleared up by the end. So, this particularly mental hiccup may not be easily resolved without trashing more text. I merely mention the brief confusion and defer to you if you feel there's a reasonable path forward to craft rhetoric that doesn't invite the confusion.
Would removing the "with respect to frame F" the two times it appears in that sentence help?
multibody/tree/planar_joint.h, line 24 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: No comma before "specify".
Done.
multibody/tree/planar_joint.h, line 39 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Of the six joint types, this "It resolves to "foo" comment only appears in three (the most recent three). Perhaps it would be simpler simply to:
static constexpr char kTypeName[] = "planar";And kill the additional comment and external definition?
I like this change. Never understood why it was done this way. Done.
multibody/tree/planar_joint.h, line 44 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Arguably the phrase "...as described in the class's documentation" already implies the phrase "see class documentation for details" rendering the follow-up sentence moot.
Done.
multibody/tree/planar_joint.h, line 83 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit s/roational/rotational
Done.
multibody/tree/planar_joint.h, line 90 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: s/expresed/expressed
Done.
multibody/tree/planar_joint.h, line 96 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: The setter is superior to the getter in two ways:
- The
setterfor this makes specific reference to "generalized coordinates".- The setter uses monogram notation so that the quantity under discussion is unambiguous.
I'd recommend changing
@returnsto@retval p_FoMo_F The generalized coordinates corresponding to the position of `this` joint.(The
@retvaltag allows you to name the otherwise unnamed return value.)Finally, make sure the
get|set_angleand velocities match the rhetorical style.
Done, I think.
multibody/tree/planar_joint.h, line 142 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: "...those corresponding to the angle..." Singular/plural disagreement.
Perhaps:
Sets the generalized coordinates in the
contextso that the position ofthisjoint isp_FoMo_Fand its orientation isangle.
Done.
multibody/tree/planar_joint.h, line 163 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Another case which is an ideal candidate for:
@retval v_FoMo_F The velocity of `this` joint ....
Done.
multibody/tree/planar_joint.h, line 208 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: When this gets renamed
get_default_translation(), the "Wrapper for the more...." comment should likewise disappear.
Done.
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: I'm really uneasy with the fact that
default_positions()andget_default_position()rely on the single character ("s") and a mountain of mental context to distinguish. This seems like a trap ready to happen.I note that the
PrismaticJointusesget_default_translation()and, generally,get|set_translation.Given the generic meaning of "position" in the context of joints, and the fact that the only previously-existing joint with translational degrees of freedom avoided the word "position", we should probably avoid it in this instance. We should call it "translation" and not rely on minor orthographic subtleties to distinguish two very different quantities. (It also works better with "translational_velocity".)
So at one point this was named something like get_defualt_translation() but was renamed at request of @amcastro-tri. I agree that this method is dangerously close to the super class method. What do we want to name this function and the related getter/setter?
multibody/tree/planar_joint.h, line 266 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit s/positve/positive
Done.
multibody/tree/planar_joint.h, line 270 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW The class is declared
final, but there are 14 instances of theoverridekeyword (instead of morefinal). Furthermore, you've got theseprotectedmethods which can only be accessed by derived classes (which are precluded by thefinalkeyword). You might as well as make this consistent in itself and replaceoverridewithfinal, and move these toprivate(with the concomitant change in comment formatting).
Every other joint has this same inconsistency. Should I follow up with a pr to do the same change to the other joints?
multibody/tree/planar_joint.h, line 286 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
With a tweak on this API (and the underlyilng mobilizer, this whole function body becomes
Eigen::VectorBlock<Eigen::Ref<VectorX<T>>> tau = get_mobilizer()->get_mutable_generalized_forces_from_array( &forces->mutable_generalized_forces()); tau -= damping.cwiseProduct(velocities(context));(Presuming that
velocitieshere means strictly the generalized coordinate velocities.)
Working.
multibody/tree/planar_joint.cc, line 58 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: copypasta "univeral_mobilizer".
Done.
multibody/tree/test/planar_joint_test.cc, line 32 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit; Body attached to what?
Done.
multibody/tree/test/planar_joint_test.cc, line 41 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: "bodies"? There looks like a single body here.
Done.
multibody/tree/test/planar_joint_test.cc, line 83 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: It seems that this test is practically a tautology but it doesn't tell me what the name is. It could be "zjjdijlkjf" for all I know. In this current revision you've documented that it should be "planar", you should have a test that supports that documentation. And, generally, putting a test on the actual value will cause future developers to pause and consider if they want to change the name.
Done.
multibody/tree/test/planar_joint_test.cc, line 121 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW These names area all "some_value" which implies they are interchangable. Perhaps they'd be better as "translation1", "angle1", etc.?
Done.
multibody/tree/test/planar_joint_test.cc, line 141 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Could you put in a comment as to why the test is structured the way it is? This doesn't seem to me to shed any light on the implementation of the
PlanarJoint, but I may be missing something.
Took a stab. This work?
multibody/tree/test/planar_joint_test.cc, line 189 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Although
PlanarJointdoesn't do anything with setting/getting limits, you've got overly exhaustive tests on all of these. Any particular reason? As it is, all you're really doing is redundantly testingJointcode.
Every other joint subclass has the same tests. I was just mimicking that. I can delete these tests if they feel redundant. Done.
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @amcastro-tri, @mpetersen94, @RussTedrake, and @SeanCurtis-TRI)
a discussion (no related file):
Previously, mpetersen94 (Mark Petersen) wrote…
Will create one follow up PR for bindings (because it is easy) and one for parsing (because it is very useful).
Excellent! thank you!
multibody/tree/planar_joint.h, line 22 at r6 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
Would removing the "with respect to frame F" the two times it appears in that sentence help?
I see Sean's point but I'm ok as is. It'd seem you say "with respect to frame F" twice, so maybe something like: "...this joint allows frame M to translate within the x-y plane of frame F and to rotate about the z-axis, with M's z-axis Mz and F's z-axis Fz coincident at all times."
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
So at one point this was named something like
get_defualt_translation()but was renamed at request of @amcastro-tri. I agree that this method is dangerously close to the super class method. What do we want to name this function and the related getter/setter?
humm... good point. I lost track of a long discussion and I do remember now I spawned that discussion because I had exactly @SeanCurtis-TRI's concern, which then finally led to the bug fix in #13757. My brain got fried that day, sry.
The thing is, we do use the word "position" everywhere to denote position vectors like p_FoMo_F here. We also use the term "generalized positions" or just "positions" to denote configuration q.
IMO the very different signature taking a Vector2 is enough. Thoughts?
multibody/tree/test/planar_joint_test.cc, line 285 at r3 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
Found an example of testing a similar method in
prismatic_joint_test. Done.
nice! I missed that, TIL
There was a problem hiding this comment.
I'm doubling down on my "position" -> "translation"...er...position. :)
Reviewed 3 of 3 files at r7.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @mpetersen94)
multibody/tree/planar_joint.h, line 22 at r6 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
I see Sean's point but I'm ok as is. It'd seem you say "with respect to frame F" twice, so maybe something like: "...this joint allows frame M to translate within the x-y plane of frame F and to rotate about the z-axis, with M's z-axis Mz and F's z-axis Fz coincident at all times."
I like @amcastro-tri's proposal.
multibody/tree/planar_joint.h, line 39 at r6 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
I like this change. Never understood why it was done this way. Done.
You missed one part that's causing you your CI problems. const --> constexpr.
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
humm... good point. I lost track of a long discussion and I do remember now I spawned that discussion because I had exactly @SeanCurtis-TRI's concern, which then finally led to the bug fix in #13757. My brain got fried that day, sry.
The thing is, we do use the word "position" everywhere to denote position vectors likep_FoMo_Fhere. We also use the term "generalized positions" or just "positions" to denote configuration q.
IMO the very different signature taking aVector2is enough. Thoughts?
Again, for "position" to mean both a 2D position vector and a 3D vector of generalized coordinates in the context of Joint is not a good thing. Since the prismatic joint calls this quantity "translation", unity should win. For Joints, "positions" should refer to its q, and if there is meaningful translation or rotational interpretations to those qs, that should be called out separately.
multibody/tree/planar_joint.h, line 270 at r6 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
Every other joint has this same inconsistency. Should I follow up with a pr to do the same change to the other joints?
I absolutely buy the "follow up" argument. And it's worth noting, if I prefix anything with "BTW", you are in no way obliged to make any changes. It's merely a passing thought. If it appeals to you, great. If it doesn't, great.
multibody/tree/planar_joint.h, line 286 at r6 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
Working.
Sorry -- that should've been prefixed with a BTW. You are not obliged to take any action in this regard. Another passing thought.
multibody/tree/planar_joint.h, line 99 at r7 (raw file):
/// /// @param[in] context The context of the model this joint belongs to. /// @retval p_FoMo_F The position of `this` joint stored in the `context` ordered
nit: line length
multibody/tree/test/planar_joint_test.cc, line 141 at r6 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
Took a stab. This work?
I'll buy that.
multibody/tree/test/planar_joint_test.cc, line 189 at r6 (raw file):
The alternative is to put a basic test in confirming they haven't been broken by the sub-class. It means you just need to set and get, but don't need to test the error conditions. Put in some comment along the lines of:
PlanarJoint inherits the Joint limits functionality directly. So, we put some token tests in to confirm that it basically works.
Or some such ilk.
mpetersen94
force-pushed
the
planar_joint
branch
from
77199df to
e3817d7
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @mpetersen94 and @SeanCurtis-TRI)
multibody/tree/planar_joint.h, line 22 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I like @amcastro-tri's proposal.
Done.
multibody/tree/planar_joint.h, line 39 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
You missed one part that's causing you your CI problems.
const-->constexpr.
Done.
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Again, for "position" to mean both a 2D position vector and a 3D vector of generalized coordinates in the context of
Jointis not a good thing. Since the prismatic joint calls this quantity "translation", unity should win. For Joints, "positions" should refer to itsq, and if there is meaningful translation or rotational interpretations to those qs, that should be called out separately.
I'm good with translation for the same rationale as Sean. If we are all ok with this, I'll make the switch.
multibody/tree/planar_joint.h, line 270 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I absolutely buy the "follow up" argument. And it's worth noting, if I prefix anything with "BTW", you are in no way obliged to make any changes. It's merely a passing thought. If it appeals to you, great. If it doesn't, great.
Done.
multibody/tree/planar_joint.h, line 286 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Sorry -- that should've been prefixed with a BTW. You are not obliged to take any action in this regard. Another passing thought.
I may circle back to this later but for now, I'm going to leave it as is. This PR is big enough.
multibody/tree/planar_joint.h, line 99 at r7 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: line length
Done.
multibody/tree/test/planar_joint_test.cc, line 189 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
The alternative is to put a basic test in confirming they haven't been broken by the sub-class. It means you just need to set and get, but don't need to test the error conditions. Put in some comment along the lines of:
PlanarJoint inherits the Joint limits functionality directly. So, we put some token tests in to confirm that it basically works.
Or some such ilk.
The setter and getter for limits are already tested elsewhere in this file. I'm good with just cutting the test.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @mpetersen94 and @SeanCurtis-TRI)
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
I'm good with translation for the same rationale as Sean. If we are all ok with this, I'll make the switch.
Fair. We also have SpatialVector::transaltional()/rotational() and similar methods for RigidTransform.
Thefore probably a good idea to have within this scope:
- set_translation() and,
- set_rotation()
since this effectively also sets a rigid pose, but a 2D one. The change would affect default and context content access. Still maintaining monogram where it applies. What do you think @SeanCurtis-TRI?
There was a problem hiding this comment.
Reviewed 1 of 1 files at r8.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @mpetersen94)
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
Fair. We also have SpatialVector::transaltional()/rotational() and similar methods for RigidTransform.
Thefore probably a good idea to have within this scope:
- set_translation() and,
- set_rotation()
since this effectively also sets a rigid pose, but a 2D one. The change would affect default and context content access. Still maintaining monogram where it applies. What do you think @SeanCurtis-TRI?
re: set_rotation()
The revolute and ball joints use "angles" instead of "rotation". So, I'd only use rotation here if you're prepared to deprecate the angles in the other Joint implementations (in a follow up PR). If we're not willing to make that change across all the joints, then I'd leave it as angle here. I don't care either way.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @mpetersen94)
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
re:
set_rotation()The revolute and ball joints use "angles" instead of "rotation". So, I'd only use
rotationhere if you're prepared to deprecate theanglesin the otherJointimplementations (in a follow up PR). If we're not willing to make that change across all the joints, then I'd leave it asanglehere. I don't care either way.
I proposed it because this joint's state actually stores a 2D "pose", while a revolute joint's state does not.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @mpetersen94)
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
I proposed it because this joint's state actually stores a 2D "pose", while a revolute joint's state does not.
I don't understand. I thought we'd agreed that the 2d pose would be "translation". I was talking about changing the name from "angle" to "rotation". So, the 2D "pose" doesn't have anything to do with what I was saying.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @mpetersen94)
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I don't understand. I thought we'd agreed that the 2d pose would be "translation". I was talking about changing the name from "angle" to "rotation". So, the 2D "pose" doesn't have anything to do with what I was saying.
I was going to say the same thing as Alejandro. If you think of a planar joint as the 2d equivalent of a free-floating joint in 3d, then it is just a representation of generic pose in a 2d world. Then just like the 3d pose it provides generic translation and rotation. Those would be fine terms and not inconsistent with revolute joint's angle() since that models a common physical device for which measuring an angle makes sense. It is harder to think of a physical device equivalent to a planar joint (not impossible, just uncommon).
OTOH angle() would not be an awful term here but I don't think consistency with revolute should be a consideration.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @mpetersen94)
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
I was going to say the same thing as Alejandro. If you think of a planar joint as the 2d equivalent of a free-floating joint in 3d, then it is just a representation of generic pose in a 2d world. Then just like the 3d pose it provides generic translation and rotation. Those would be fine terms and not inconsistent with revolute joint's
angle()since that models a common physical device for which measuring an angle makes sense. It is harder to think of a physical device equivalent to a planar joint (not impossible, just uncommon).OTOH
angle()would not be an awful term here but I don't think consistency with revolute should be a consideration.
Awesome. Then I will go with set_translation() and set_rotation() (and matching functions). Are we still good with set_translational_velocity() and set_rotational_velocity()?
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @mpetersen94)
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
Awesome. Then I will go with set_translation() and set_rotation() (and matching functions). Are we still good with set_translational_velocity() and set_rotational_velocity()?
thanks for the much better explanation @sherm1, you read my mind.
For the record, I only proposed "rotation()". But I am also ok with "angle()"
With this now clear exposition lets vote? @mpetersen94 , @SeanCurtis-TRI, @sherm1 , what do you prefer?
- translation()/rotation() or,
- translation()/angle()?
My vote since @SeanCurtis-TRI already requested a refactor: translation()/rotation()
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @mpetersen94)
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
thanks for the much better explanation @sherm1, you read my mind.
For the record, I only proposed "rotation()". But I am also ok with "angle()"
With this now clear exposition lets vote? @mpetersen94 , @SeanCurtis-TRI, @sherm1 , what do you prefer?
- translation()/rotation() or,
- translation()/angle()?
My vote since @SeanCurtis-TRI already requested a refactor: translation()/rotation()
I like the argument for translation()/rotation().
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @mpetersen94)
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
I like the argument for translation()/rotation().
It's worth noting that RevoluteJoint, BallJoint, and RevoluteJoint all use "angle" to refer to the rotational component. I'm just making a request for consistency across the Joint nomenclature.
My only point is that if you go for rotation(), you should make the Joint ecosystem consistent and plan on a follow up PR deprecating the old API and introducing the new so that they all use rotation as well. I personally prefer "rotation" over "angle", but I don't think we can do it here without considering the larger context.
mpetersen94
force-pushed
the
planar_joint
branch
from
e3817d7 to
79ba928
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @amcastro-tri, @SeanCurtis-TRI, and @sherm1)
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
It's worth noting that
RevoluteJoint,BallJoint, andRevoluteJointall use "angle" to refer to the rotational component. I'm just making a request for consistency across theJointnomenclature.My only point is that if you go for
rotation(), you should make theJointecosystem consistent and plan on a follow up PR deprecating the old API and introducing the new so that they all userotationas well. I personally prefer "rotation" over "angle", but I don't think we can do it here without considering the larger context.
Done. This look good to everyone?
There was a problem hiding this comment.
Reviewed 2 of 2 files at r9.
Reviewable status:complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),RussTedrake(platform)
There was a problem hiding this comment.
Thanks for your infinite patience @mpetersen94, we are the worst. But I like the color of this bikeshed now :-)
Reviewed 1 of 3 files at r7, 2 of 2 files at r9.
Reviewable status: 3 unresolved discussions (waiting on @mpetersen94)
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
Done. This look good to everyone?
I like it @mpetersen94 , thanks.
@SeanCurtis-TRI lets defer the "consistency across joints" to an issue. IMO we would not need any refactoring. But happy to say more in an issue.
multibody/tree/planar_joint.h, line 96 at r9 (raw file):
/// @{ /// Gets the generalized coordinates corresponding to the translation of
nit, remember the idea was that at this high API level, we do return positions, p_FoMo. If someone wants the "generalized positions" semantics, they can go to Joint::.
multibody/tree/planar_joint.h, line 120 at r9 (raw file):
} /// Gets the generalized coordinate corresponding to the rotation θ of `this`
nit, ditto. Angle or rotation should work here.
multibody/tree/planar_joint.h, line 143 at r9 (raw file):
} /// Sets the generalized coordinates in the `context` so that the translation
nit, "Sets the context so that it stores the position p_FoMo_F and angle theta for this joint"
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions (waiting on @mpetersen94)
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
I like it @mpetersen94 , thanks.
@SeanCurtis-TRI lets defer the "consistency across joints" to an issue. IMO we would not need any refactoring. But happy to say more in an issue.
Clearly, using the phrase "follow up PR" to communicate that I wasn't intending changes to other joints in this PR wasn't sufficient. I'd appreciate some help in figuring out what magical phrase I should use to make it clear.
mpetersen94
force-pushed
the
planar_joint
branch
from
79ba928 to
11a0a37
Compare
There was a problem hiding this comment.
Reviewable status:
multibody/tree/planar_joint.h, line 96 at r9 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
nit, remember the idea was that at this high API level, we do return positions,
p_FoMo. If someone wants the "generalized positions" semantics, they can go to Joint::.
Done.
multibody/tree/planar_joint.h, line 120 at r9 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
nit, ditto. Angle or rotation should work here.
Done.
multibody/tree/planar_joint.h, line 143 at r9 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
nit, "Sets the
contextso that it stores the positionp_FoMo_Fand anglethetaforthisjoint"
Done.
There was a problem hiding this comment.
Reviewable status:
multibody/tree/planar_joint.h, line 211 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Clearly, using the phrase "follow up PR" to communicate that I wasn't intending changes to other joints in this PR wasn't sufficient. I'd appreciate some help in figuring out what magical phrase I should use to make it clear.
The now-current names all look great to me -- thanks!
There was a problem hiding this comment.
Reviewed 1 of 1 files at r10.
Reviewable status:
Adds a 3 dof planar joint that allows motion in the xy plane and rotation about the z-axis of the parent frame.
Towards #12404
Relates #12401
This change is