Fix some issues with RigidBodyConstraint not conforming to the style guide. #2325

sammy-tri · 2016-05-11T21:52:45Z

Fixes #2306... well, partly. I could, I think, spend days cleaning up in there. But it fixes the spacing issue identified there along with access controls and variable names. I did not get to function names, namespaces, etc...

This change is

liangfok · 2016-05-12T14:42:44Z

Looks like there are some API changes here. Need to update the CHANGELOG documenting those changes.

Previously, sammy-tri wrote…

Fix some issues with RigidBodyConstraint not conforming to the style guide.

Fixes #2306... well, partly. I could, I think, spend days cleaning up in there. But it fixes the spacing issue identified there along with access controls and variable names. I did not get to function names, namespaces, etc...

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

drake/systems/plants/constraint/RigidBodyConstraint.cpp, line 54 [r1] (raw file):

                          robot, tspan),
      m_robotnumset_(robotnumset),
      shrink_factor_(0.9),

Shouldn't these be initialized using in-class member initialization? #2310

drake/systems/plants/constraint/RigidBodyConstraint.h, line 19 [r1] (raw file):

namespace DrakeRigidBodyConstraint {
extern DRAKERIGIDBODYCONSTRAINT_EXPORT Eigen::Vector2d default_tspan;
}

Can we add // namespace DrakeRigidBodyConstraint after the above }?

drake/systems/plants/constraint/RigidBodyConstraint.h, line 22 [r1] (raw file):

/**
 * @class RigidBodyConstraint       The abstract base class. All the constraints

"All the" -> "All"

drake/systems/plants/constraint/RigidBodyConstraint.h, line 25 [r1] (raw file):

 * used in the inverse kinematics problem are inherited from
 * RigidBodyConstraint. There are 6 main categories of the RigidBodyConstraint,
 * each category has its own interface

Can you add a paragraph describing each of the six categories? Any by "categories" do you mean "collections of subclasses"?

drake/systems/plants/constraint/RigidBodyConstraint.h, line 67 [r1] (raw file):

      int category, RigidBodyTree* robot,
      const Eigen::Vector2d& tspan = DrakeRigidBodyConstraint::default_tspan);
  int getType() const { return type_; }

Should this be type()? @amcastro-tri?

drake/systems/plants/constraint/RigidBodyConstraint.h, line 68 [r1] (raw file):

      const Eigen::Vector2d& tspan = DrakeRigidBodyConstraint::default_tspan);
  int getType() const { return type_; }
  int getCategory() const { return category_; }

Should this be category()? @amcastro-tri?

drake/systems/plants/constraint/RigidBodyConstraint.h, line 69 [r1] (raw file):

  int getType() const { return type_; }
  int getCategory() const { return category_; }
  RigidBodyTree* getRobotPointer() const { return robot_; }

@amcastro-tri, do you approve of this method name? Would robot_pointer() or robot() be better and more consistent with the latest code styling customs we're trying to foster?

drake/systems/plants/constraint/RigidBodyConstraint.h, line 73 [r1] (raw file):

 protected:
  std::string getTimeString(const double* t) const;

Should this be time_string(const double* t)

Comments from Reviewable

jwnimmer-tri · 2016-05-12T14:46:47Z

Review status: all files reviewed at latest revision, 8 unresolved discussions.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 19 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Can we add // namespace DrakeRigidBodyConstraint after the above }?

Not required when the namespace is less than 10 lines. Here, it would reduce clarity, not add it.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 67 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Should this be type()? @amcastro-tri?

This PR is only a partial style fix. This is just the prior spelling.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 68 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Should this be category()? @amcastro-tri?

This PR is only a partial style fix. This is just the prior spelling.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 69 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

@amcastro-tri, do you approve of this method name? Would robot_pointer() or robot() be better and more consistent with the latest code styling customs we're trying to foster?

This PR is only a partial style fix. This is just the prior spelling.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 73 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Should this be time_string(const double* t)

This PR is only a partial style fix. This is just the prior spelling. This line isn't even changed in the diff!

Comments from Reviewable

liangfok · 2016-05-12T15:05:56Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 73 [r1] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This PR is only a partial style fix. This is just the prior spelling. This line isn't even changed in the diff!

Alas, the ever-present inherent conflict between pace of fixing defects and trying to conform to the small PR-size-objective.

Comments from Reviewable

jwnimmer-tri · 2016-05-12T15:13:21Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 73 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Alas, the ever-present inherent conflict between pace of fixing defects and trying to conform to the small PR-size-objective.

Not really. A PR chain of "batch 1 of N style fixes", evaluated on "is this monotonically better" is actually faster paced than any other mechanism available under the umbrella of pre-commit review.

Comments from Reviewable

liangfok · 2016-05-12T15:53:47Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 73 [r1] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Not really. A PR chain of "batch 1 of N style fixes", evaluated on "is this monotonically better" is actually faster paced than any other mechanism available under the umbrella of pre-commit review.

I believe the ability to create a "batch of 1 of N style fixes" where each element in the batch is "small" is in conflict with the fact that it's difficult, if not impossible, to undergo a rational process when creating a new feature. Perhaps this gets back to my other post about needing to first charge forward developing a new feature, then go back and "fake" a batch of 1-N incremental and rational changes that eventually lead up to the new feature being integrated into master.

Comments from Reviewable

jwnimmer-tri · 2016-05-12T15:55:23Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 73 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

I believe the ability to create a "batch of 1 of N style fixes" where each element in the batch is "small" is in conflict with the fact that it's difficult, if not impossible, to undergo a rational process when creating a new feature. Perhaps this gets back to my other post about needing to first charge forward developing a new feature, then go back and "fake" a batch of 1-N incremental and rational changes that eventually lead up to the new feature being integrated into master.

But this PR isn't a feature. It's only a style fix, for which there is a well-known checklist to traverse (the list of rules), and a clear intermediate pass criteria (CI passes and every edit is a strict improvement).

Comments from Reviewable

liangfok · 2016-05-12T15:58:56Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 73 [r1] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

But this PR isn't a feature. It's only a style fix, for which there is a well-known checklist to traverse (the list of rules), and a clear intermediate pass criteria (CI passes and every edit is a strict improvement).

Ah. Good point. I was clearly coming from a different context. Thanks!

Comments from Reviewable

sammy-tri · 2016-05-12T16:54:52Z

I did not make any deliberate changes to the external API for RigidBodyConstraint or its derived classes. Could you be more specific? I may want to revert.

Previously, liangfok (Chien-Liang Fok) wrote…

Looks like there are some API changes here. Need to update the CHANGELOG documenting those changes.

Review status: 1 of 2 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

drake/systems/plants/constraint/RigidBodyConstraint.cpp, line 54 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Shouldn't these be initialized using in-class member initialization? #2310

Added {} to the declarations in the header file. From discussion with @jwnimmer-tri, I believe #2310 is intended only to make sure all POD types have an in-class initializer (even if it's the default), not to ban use of initializers at the constructor.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 22 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"All the" -> "All"

Done.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 25 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Can you add a paragraph describing each of the six categories? Any by "categories" do you mean "collections of subclasses"?

Honestly? Not with any significant amount of clarity. All the categories really mean to me are their APIs.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 67 [r1] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This PR is only a partial style fix. This is just the prior spelling.

== @jwnimmer-tri

drake/systems/plants/constraint/RigidBodyConstraint.h, line 68 [r1] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This PR is only a partial style fix. This is just the prior spelling.

== @jwnimmer-tri

drake/systems/plants/constraint/RigidBodyConstraint.h, line 69 [r1] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This PR is only a partial style fix. This is just the prior spelling.

== @jwnimmer-tri

Comments from Reviewable

liangfok · 2016-05-12T17:01:23Z

Previously, sammy-tri wrote…

I did not make any deliberate changes to the external API for RigidBodyConstraint or its derived classes. Could you be more specific? I may want to revert.

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 25 [r1] (raw file):

Previously, sammy-tri wrote…

Honestly? Not with any significant amount of clarity. All the categories really mean to me are their APIs.

I guess the names of each static const int is sufficiently self explanatory.

Comments from Reviewable

sammy-tri · 2016-05-12T17:16:16Z

+@jwnimmer-tri for platform review. Thanks @liangfok!

Previously, liangfok (Chien-Liang Fok) wrote…

:lgtm:

Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

amcastro-tri · 2016-05-12T19:04:47Z

wow! I wouldn't've had the patience myself to make all these style changes. Thank you @sammy-tri!

Previously, sammy-tri wrote…

+@jwnimmer-tri for platform review. Thanks @liangfok!

Review status: all files reviewed at latest revision, 4 unresolved discussions.

drake/systems/plants/constraint/RigidBodyConstraint.cpp, line 166 [r2] (raw file):

  for (int i = 0; i < num_new_bodies; i++) {
    bool findDuplicateBody = false;
    if (new_body_pts[i].rows() != 3) {

Is this check needed? the argument is a Matrix3Xd so it has to have 3 rows or it wont compile.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 67 [r1] (raw file):

Previously, sammy-tri wrote…

== @jwnimmer-tri

I agree. It'd be nice if it was just type() but I wouldn't block this PR because of this

drake/systems/plants/constraint/RigidBodyConstraint.h, line 68 [r1] (raw file):

Previously, sammy-tri wrote…

== @jwnimmer-tri

same as above

drake/systems/plants/constraint/RigidBodyConstraint.h, line 69 [r1] (raw file):

Previously, sammy-tri wrote…

== @jwnimmer-tri

That method is part of the previous API. It'd be nice to fix it as you suggest but I won't block this PR for this. I would block it if getRobotPointer was a new method. I am ok if @sammy-tri wants to fix this now though :-)

Comments from Reviewable

amcastro-tri · 2016-05-12T19:11:30Z

Review status: all files reviewed at latest revision, 5 unresolved discussions.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 17 [r1] (raw file):

class RigidBodyTree;

namespace DrakeRigidBodyConstraint {

Sorry I don't understand but what happened here to com_pts and WorldCoMDefaultRobotNum. Where did they go?

Comments from Reviewable

jwnimmer-tri · 2016-05-12T19:42:15Z

I reviewed the commits, not the all-in diffs, and it looks good. There is still so much more to do, but this is a great start.

Previously, amcastro-tri (Alejandro Castro) wrote…

wow! I wouldn't've had the patience myself to make all these style changes. Thank you @sammy-tri!

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

sammy-tri · 2016-05-12T19:44:40Z

Review status: 1 of 2 files reviewed at latest revision, 5 unresolved discussions.

drake/systems/plants/constraint/RigidBodyConstraint.cpp, line 166 [r2] (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

Is this check needed? the argument is a Matrix3Xd so it has to have 3 rows or it wont compile.

Done.

drake/systems/plants/constraint/RigidBodyConstraint.h, line 17 [r1] (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

Sorry I don't understand but what happened here to com_pts and WorldCoMDefaultRobotNum. Where did they go?

com_pts was a Vector3d::Zero which was only used in one place in the cpp file, so I changed it to a Vector3d::zero in that location. WorldComDefaultRobotNum moved into the anonymous namesapce in the cpp file. I didn't see how these values formed part of the external API (and they weren't used externally), so I moved them out of the header.

Comments from Reviewable

amcastro-tri · 2016-05-12T21:30:49Z

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I reviewed the commits, not the all-in diffs, and it looks good. There is still so much more to do, but this is a great start.

:lgtm:

Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

jwnimmer-tri · 2016-05-12T21:39:35Z

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

jwnimmer-tri · 2016-05-12T21:39:52Z

Just need @liangfok to OK his discussions, and we should be able to merge.

liangfok · 2016-05-12T21:42:48Z

Done. PTAL and then merge!

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Just need @liangfok to OK his discussions, and we should be able to merge.

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

jwnimmer-tri · 2016-05-12T21:45:32Z

Using override-merge from out-of-date-with-master.

sammy-tri force-pushed the rigid_body_constraint_style branch from 4b202bb to bfefa4f Compare May 11, 2016 21:57

sammy-tri mentioned this pull request May 12, 2016

RigidBodyConstraint does not conform to style guide #2306

Closed

sammy-tri added 6 commits May 12, 2016 12:42

First rount of clang-format (and some other manual) cleanups

ed4288a

Get rid of copy constructors

ce8a59c

Fix field names and access controls

8e9bea1

Additional clang-format pass

e069e55

Fix cpplint issue

5d47ae0

Add _{}; initializer goop.

0786e4f

sammy-tri force-pushed the rigid_body_constraint_style branch from 25de475 to 0786e4f Compare May 12, 2016 16:54

sammy-tri assigned jwnimmer-tri May 12, 2016

Remove a runtime check which is guaranteed at compile time.

9d0ff06

jwnimmer-tri merged commit cf5225a into RobotLocomotion:master May 12, 2016

liangfok mentioned this pull request May 13, 2016

Pulled out the changes to rigid body, rigid body system, and rigid body tree from PR #2253. #2303

Merged

sammy-tri deleted the rigid_body_constraint_style branch May 16, 2016 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some issues with RigidBodyConstraint not conforming to the style guide. #2325

Fix some issues with RigidBodyConstraint not conforming to the style guide. #2325

sammy-tri commented May 11, 2016 •

edited by david-german-tri

Loading

liangfok commented May 12, 2016

Fix some issues with RigidBodyConstraint not conforming to the style guide.

jwnimmer-tri commented May 12, 2016

liangfok commented May 12, 2016

jwnimmer-tri commented May 12, 2016

liangfok commented May 12, 2016

jwnimmer-tri commented May 12, 2016

liangfok commented May 12, 2016

sammy-tri commented May 12, 2016

liangfok commented May 12, 2016

sammy-tri commented May 12, 2016

amcastro-tri commented May 12, 2016

amcastro-tri commented May 12, 2016

jwnimmer-tri commented May 12, 2016

sammy-tri commented May 12, 2016

amcastro-tri commented May 12, 2016

jwnimmer-tri commented May 12, 2016

jwnimmer-tri commented May 12, 2016

liangfok commented May 12, 2016 •

edited

Loading

jwnimmer-tri commented May 12, 2016

Fix some issues with RigidBodyConstraint not conforming to the style guide. #2325

Fix some issues with RigidBodyConstraint not conforming to the style guide. #2325

Conversation

sammy-tri commented May 11, 2016 • edited by david-german-tri Loading

liangfok commented May 12, 2016

Fix some issues with RigidBodyConstraint not conforming to the style guide.

jwnimmer-tri commented May 12, 2016

liangfok commented May 12, 2016

jwnimmer-tri commented May 12, 2016

liangfok commented May 12, 2016

jwnimmer-tri commented May 12, 2016

liangfok commented May 12, 2016

sammy-tri commented May 12, 2016

liangfok commented May 12, 2016

sammy-tri commented May 12, 2016

amcastro-tri commented May 12, 2016

amcastro-tri commented May 12, 2016

jwnimmer-tri commented May 12, 2016

sammy-tri commented May 12, 2016

amcastro-tri commented May 12, 2016

jwnimmer-tri commented May 12, 2016

jwnimmer-tri commented May 12, 2016

liangfok commented May 12, 2016 • edited Loading

jwnimmer-tri commented May 12, 2016

sammy-tri commented May 11, 2016 •

edited by david-german-tri

Loading

liangfok commented May 12, 2016 •

edited

Loading