Pulled out the changes to rigid body, rigid body system, and rigid body tree from PR #2253. #2303
Conversation
|
Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion. drake/systems/plants/RigidBody.cpp, line 33 [r1] (raw file):
(Sorry to jump into a random review...) It looks like "world" is used frequently as a magic constant in RigidBody_. Would it be safer to define this as a const char_ kConstant somewhere so that the compiler can check for potential misspellings or other errors? (as this pattern was not introduced in this PR, I have no objection to merging it without fixing this) Comments from Reviewable |
|
Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion. drake/systems/plants/RigidBody.cpp, line 33 [r1] (raw file):
|
|
How about @amcastro-tri for feature review since he is familiar with this code?
|
|
Good idea. +@amcastro-tri can you be a feature reviewer?
|
|
Reviewed 5 of 5 files at r1. drake/systems/plants/RigidBody.cpp, line 36 [r1] (raw file):
Multi-line conditional statement requires braces per cppguide. drake/systems/plants/RigidBody.cpp, line 39 [r1] (raw file):
Multi-line conditional statement requires braces per cppguide. drake/systems/plants/RigidBody.h, line 22 [r1] (raw file):
Missing doxygen. drake/systems/plants/RigidBody.h, line 22 [r1] (raw file):
Missing drake/systems/plants/RigidBody.h, line 24 [r1] (raw file):
Missing doxygen. drake/systems/plants/RigidBodySystem.cpp, line 592 [r1] (raw file):
Isn't there an invariant that drake/systems/plants/RigidBodySystem.cpp, line 596 [r1] (raw file):
Isn't there an invariant that drake/systems/plants/RigidBodySystem.cpp, line 831 [r1] (raw file):
Missing newline at end of file. drake/systems/plants/RigidBodySystem.h, line 198 [r1] (raw file):
This API doc mostly just repeats the content from the System concept that it is implementing. Citing the concept API would be a better approach (using doxygen block comments is one approach). Repeating the documentation of an "overridden" method is brittle. The only clarification this particular comment adds is that RBS's state is both position and velocity values, which would be better documented a class overview (for example), and cited here with a drake/systems/plants/RigidBodySystem.h, line 203 [r1] (raw file):
This API doc directly repeats the content from the System concept that it is implementing. Citing the concept API would be a better approach (using doxygen block comments is one approach). Repeating the documentation of an "overridden" method is brittle. drake/systems/plants/RigidBodySystem.h, line 209 [r1] (raw file):
This API doc mostly just repeats the content from the System concept that it is implementing. Citing the concept API would be a better approach (using doxygen block comments is one approach). Repeating the documentation of an "overridden" method is brittle. The only clarification this particular comment adds is that RBS's output also includes sensor values, which would be better documented a class overview (for example), and cited here with a drake/systems/plants/RigidBodySystem.h, line 256 [r1] (raw file):
This return type exposes non-const drake/systems/plants/RigidBodySystem.h, line 484 [r1] (raw file):
Missing doxygen (for all of these methods). Perhaps the implementation comments on their meaning just need to be relocated here? Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed. drake/systems/plants/RigidBodySystem.cpp, line 527 [r1] (raw file):
I am not super-well spun up on RBS, but does this method have good enough unit test coverage? (It relates to the new getters that this PR adds.) drake/systems/plants/RigidBodySystem.cpp, line 592 [r1] (raw file):
I am not super-well spun up on RBS, but does this method have good enough unit test coverage? drake/systems/plants/RigidBodySystem.cpp, line 596 [r1] (raw file):
I am not super-well spun up on RBS, but does this method have good enough unit test coverage? drake/systems/plants/RigidBodySystem.cpp, line 658 [r1] (raw file):
I am not super-well spun up on RBS, but does this method have good enough unit test coverage? Comments from Reviewable |
|
I've addressed all comments from previous round of review. PTAL.
|
|
Updated review of diffs is posted below. Still have to do some research on the testing discussions; will follow up a bit later.
|
|
Review status: all files reviewed at latest revision, 12 unresolved discussions. drake/systems/plants/RigidBodySystem.cpp, line 527 [r1] (raw file):
|
|
Review status: all files reviewed at latest revision, 35 unresolved discussions. drake/examples/Cars/car_sim_lcm.cpp, line 51 [r3] (raw file):
I personally think that something like kWorldLinkName should live in RigidBodyTree, not RigidBody. RigidBody should not even know if it belongs to a tree or not (now it sort of does but that will change). drake/systems/plants/RigidBody.cpp, line 33 [r1] (raw file):
|
|
Review status: all files reviewed at latest revision, 43 unresolved discussions. drake/systems/plants/RigidBodySystem.cpp, line 650 [r3] (raw file):
memory leak? who deallocates attr? drake/systems/plants/RigidBodySystem.cpp, line 652 [r3] (raw file):
std::string drake/systems/plants/RigidBodySystem.cpp, line 656 [r3] (raw file):
std::runtime_error drake/systems/plants/RigidBodySystem.cpp, line 818 [r3] (raw file):
Replace "force" --> "wrench". A force is a 3D vector. drake/systems/plants/RigidBodySystem.cpp, line 819 [r3] (raw file):
@sherm1, how should we call these? time to introduce your notation. I don't like it is inverted. I'd prefere drake/systems/plants/RigidBodySystem.cpp, line 828 [r3] (raw file):
Remove commented out code used for debugging. drake/systems/plants/RigidBodySystem.h, line 278 [r3] (raw file):
Why not to follow the TODO and move it now to drake/systems/plants/RigidBodySystem.h, line 303 [r3] (raw file):
We need proper Doxygen documentation here. My try: Comments from Reviewable |
|
I finished with my first pass. I hope it is useful.
|
| @@ -18,6 +20,10 @@ RigidBody::RigidBody() | |||
| I << Matrix<double, TWIST_SIZE, TWIST_SIZE>::Zero(); | |||
| } | |||
|
|
|||
| const std::string& RigidBody::GetName() const { return linkname; } | |||
There was a problem hiding this comment.
Very minor. There is a style inconsistency between linkname and model_name.
|
@nkoenig FYI, Drake recently switched from the built-in GitHub UI to reviewable.io for code reviews. Among other nice features, Reviewable will batch your comments for you, decreasing email volume. |
| @@ -641,7 +699,7 @@ void parseSDFLink(RigidBodySystem& sys, string model_name, XMLElement* node, | |||
| transform_link_to_model); | |||
| } | |||
|
|
|||
| if (type.compare("ray") == 0) { | |||
| if (type == "ray") { | |||
There was a problem hiding this comment.
Do you want to add an else clause that outputs some warning or error about not handling sensors than than 'ray'?
|
Review status: all files reviewed at latest revision, 47 unresolved discussions, some commit checks failed. drake/systems/plants/RigidBody.cpp, line 23 [r3] (raw file):
|
|
Review status: all files reviewed at latest revision, 47 unresolved discussions, some commit checks failed. drake/systems/plants/RigidBody.cpp, line 35 [r3] (raw file):
|
|
Review status: all files reviewed at latest revision, 46 unresolved discussions, some commit checks failed. drake/systems/plants/RigidBody.cpp, line 34 [r1] (raw file):
|
|
Review status: 66 of 71 files reviewed at latest revision, 17 unresolved discussions. drake/systems/plants/test/lidarTest.cpp, line 124 [r6] (raw file):
|
|
Reviewed 3 of 3 files at r7. Comments from Reviewable |
|
Review status: 69 of 71 files reviewed at latest revision, 9 unresolved discussions. drake/systems/plants/test/lidarTest.cpp, line 16 [r6] (raw file):
|
|
Review status: 68 of 71 files reviewed at latest revision, 9 unresolved discussions. drake/systems/plants/test/lidarTest.cpp, line 16 [r6] (raw file):
|
…drake into feature/ros_tf_publisher_split
|
This PR passed CI Build 1396. Thus, I believe it is ready for platform review. @jwnimmer-tri will you serve as platform reviewer?
|
|
Review status: 57 of 71 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed. drake/systems/plants/RigidBodyTree.h, line 666 [r8] (raw file):
I believe find/replace here changed something you did not want to change? It doesn't break anything in this case but for sure not needed. Comments from Reviewable |
|
Oops. I forgot to update the CHANGELOG. Will update that soon.
|
|
Reviewed 2 of 12 files at r6, 12 of 12 files at r8. drake/systems/controllers/QPCommon.h, line 4 [r8] (raw file):
This include is out of order now. drake/systems/plants/test/urdfManipulatorDynamicsTest.cpp, line 3 [r8] (raw file):
This include is in the wrong order now, but was correct previously. Comments from Reviewable |
# Conflicts: # drake/systems/plants/constraint/RigidBodyConstraint.cpp
|
I resolved the most recent round of reviewer comments, updated the CHANGELOG, and resolved the conflicts from #2325. Now waiting to verify that the latest revision passes CI.
|
|
👍 from me on all the code. Just a few changelog comments. If you want, you can wait for CI to finish and then I can still override-merge any changelog fixes even without waiting for CI again; just need one clean run first.
|
|
Wow, the #2325 merge is really messing with this PR. Sorry! If I had realized that would be the case, I would definitely have let this PR make it to master first. |
|
No worries. I do not mind resolving the conflicts in this PR. Oops. CI Build 1401 is all red b/c I forgot to commit some code. Oh well, I'm about to push some changes that should trigger another round of CI.
|
|
Awesome job @liangfok! you went through the revision process as a real gent
|
|
|
Thanks @amcastro-tri. This PR passed CI build 1402. It is thus ready for another round of review or merge.
|
|
|
I looked through the 5 reviewable discussions that were unresolved, and am making a judgement call that their concerns have been met, so I've overridden them all to be satisfied.
|
all files reviewed at latest revision, all discussions resolved, all commit checks successful.
|
Using override-merge due to "out of date with master". |
This PR is a subset of PR #2253, which only contains the changes to
RigidBody,RigidBodyTree, andRigidBodySystem.Closes #2315
@jwnimmer-tri: can you serve as both feature and platform reviewer?
This change is
Multiple assignees: 
amcastro-tri, 
jwnimmer-tri