-
Notifications
You must be signed in to change notification settings - Fork 285
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
Add TranslationalJoint2D #1003
Add TranslationalJoint2D #1003
Conversation
Codecov Report
@@ Coverage Diff @@
## release-6.4 #1003 +/- ##
==============================================
- Coverage 56.81% 56.62% -0.2%
==============================================
Files 310 314 +4
Lines 24117 24298 +181
==============================================
+ Hits 13703 13758 +55
- Misses 10414 10540 +126
|
I should have some time tonight to give this a review. |
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.
I left a suggestion regarding TranslationalJoint2dUniqueProperties
, but nothing too significant. A few higher level thoughts below:
-
I wonder if we should consider naming the joint
TranslationalJoint2D
, since2d
might look awfully similar to Eigen's convention, where the lowercased
refers todouble
rather thanDimension
. -
This PR is making me realize just how much silly boilerplate is still required to implement a new joint type correctly. Ideally, we should just be able to define the unique properties, and then define the relative transform behavior and the Jacobian behavior. I think that could be achieved through a policy-based design scheme. But that's far outside the scope of this PR.
struct TranslationalJoint2dUniqueProperties | ||
{ | ||
/// Plane type | ||
PlaneType mPlaneType; |
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.
Since we've gone through the trouble of making the member functions, I feel that it would make sense to make these member variables private. That would ensure that this member data always remains sane. I think the mild inconvenience of not being able to set these values directly is heavily outweighed by the importance of keeping the values sane and consistent.
struct TranslationalJoint2dUniqueProperties | ||
{ | ||
/// Plane type | ||
PlaneType mPlaneType; |
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.
Since we've gone through the trouble of making all the member function, I feel like it would make sense to have these member variables be private. That way, we can rely on the member functions to keep the values sane and consistent. I think the inconvenience of not having direct access to the members is heavily outweighed by the value of guaranteeing sanity and consistency.
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.
Okay, apparently if you leave a review comment, accidentally close the webpage, and come back to continue reviewing, Github will (sometimes) pretend that you didn't leave the review comment at all....
dart/utils/SkelParser.cpp
Outdated
|
||
if (type == "xy") | ||
{ | ||
properties.mPlaneType = dynamics::PlanarJoint::PlaneType::XY; |
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.
If we do make the member variables private, we'll need to tweak the implementation of this parser to store these parameters and then pass them into the member functions of properties
.
Agreed. This is one of the reasons |
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.
Looks like a member variable of TranslationalJoint2DUniqueProperties
was left behind.
struct TranslationalJoint2DUniqueProperties | ||
{ | ||
/// First and second translational axis | ||
Eigen::Matrix<double, 3, 2> mTransAxes; |
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.
We would also want this member variable to be private.
I would also suggest changing this from a struct
to a class
since it's no longer a POD.
TranslationalJoint2D
is a 2d version ofTranslationalJoint
orPlanarJoint
without the rotational axis.Before creating a pull request
Before merging a pull request
CHANGELOG.md
SkelParser
)