More "link" to "body" refactoring. #2597

liangfok · 2016-06-20T17:42:13Z

This change is

liangfok · 2016-06-20T17:44:08Z

+@sherm1 for both levels of review since this is a minor non-functional change.

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

Comments from Reviewable

sherm1 · 2016-06-20T20:33:04Z

@liangfok, build failed because this changes the API and forces downstream changes that haven't been made. Also, let's make sure @RussTedrake is happy with the change from "link" to "body" before doing this everywhere. Russ?

liangfok · 2016-06-20T20:53:42Z

I agree.

FTR, it looks like unit test test_python_pydrake.test.testPR2IK failed.

RussTedrake · 2016-06-20T23:29:40Z

what was the motivation for the change? it will break the current "API".

Review status: 0 of 10 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

liangfok · 2016-06-21T00:47:36Z

There are two primary motivations:

Terminology simplicity and consistency. Note the software abstractions RigidBody and RigidBodyTree both contain the term "body". Intermixing "body" with "link" will only result in confusion and inconsistency.
This comment by @sherm1 indicating that the term "link" is something that's only used in the robotics community.

A secondary motivation is we are currently in a pre-release period where the API is less set-in-stone. It would be best to clean up the API as much as possible now before our first 1.0 release.

Review status: 0 of 10 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

RussTedrake · 2016-06-21T01:25:53Z

i guess i'm ok with it, but i think you have the burden for now of adding back findLink() and finkLinkId() methods which throw a static assert instructing people to update their code to FindBody.

Review status: 0 of 10 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

liangfok · 2016-06-21T01:35:37Z

Sure no problem! Thanks.

Review status: 0 of 10 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

…Body of these methods contain a static assertion.

…e/link_to_body_refactor

…ause it delays downstream users from discovering that they need to update their code.

liangfok · 2016-06-21T21:21:30Z

I attempted to add back RigidBodyTree::findLinkID() and RigidBodyTree::findLink() with static_assert lines in their method bodies but couldn't because that results in a compile-time error even if the methods are not called.

I then considered replacing the static_assert lines with thrown exceptions, but this is strictly worse than simply not providing the old methods back since it prolongs the time it takes for downstream users to realize that they need to update their code.

I thus removed these deprecated methods again.

If a downstream user compiles their code and it fails because of a missing RigidBodyTree::findLinkID() or RigidBodyTree::findLink() method, they can consult the CHANGELOG to determine the new methods that replace them.

Perhaps we can institute a more formal deprecation glide path after release 1.0?

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

Comments from Reviewable

liangfok · 2016-06-21T23:00:15Z

Forgot to mention: Another option I considered was to have the old API call the new API after printing a warning to std::cerr. However, this is also not as good as just removing the old API methods since it delays when downstream users discover the need to update to the latest API.

On another note, this PR passed https://drake-jenkins.csail.mit.edu/job/experimental/2187/ and is thus ready for another round of review / merge.

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

Comments from Reviewable

wxmerkt · 2016-06-21T23:03:01Z

Would it be possible within the remits of the style guide or similar to add a deprecation comment, use a pragma/trigger a deprecated compiler warning/output an error on runtime and internally call the new API (just saw you added a new comment to that regard). I feel like having a warning/error message shows users they need to act on it (and the deprecation comment when it's going to be removed, e.g. to be removed in 1.1) but doesn't throw them under the bus immediately.

sherm1 · 2016-06-21T23:56:40Z

Since the compilers all support deprecation as a compile-time warning (standardized in C++14), we could define a Drake macro to mark the old methods, which would call the new ones. The warnings are only issued when the old methods are used. Here is an example of a functional macro for that purpose:

/* C++14 introduces a standard way to mark deprecated declarations. Before
that we can use non-standard compiler hacks. */
#ifndef SWIG
    #if __cplusplus >= 201402L
        /* C++14 */
        #define DEPRECATED_14(MSG) [[deprecated(MSG)]]
    #elif _MSC_VER
        /* VC++ just says warning C4996 so add "DEPRECATED" to the message. */
        #define DEPRECATED_14(MSG) __declspec(deprecated("DEPRECATED: " MSG))
    #else /* gcc or clang */
        #define DEPRECATED_14(MSG) __attribute__((deprecated(MSG)))
    #endif
#else /* Swigging */
    #define DEPRECATED_14(MSG)
#endif

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

Comments from Reviewable

jwnimmer-tri · 2016-06-22T00:28:51Z

FWIW -- I am open to the idea of using deprecated markers and enabling a soft cut-over, but I am unclear in which cases a soft cut-over is required / desired. On master recently, we have already renamed dozens of APIs, shuffled responsibility between classes, hidden public fields behind getters, etc. Is there a heuristic that guides when we should be doing a soft-transition and when we can do hard? Prior @RussTedrake's upthread comment in this PR, my understanding was that only matlab APIs are sacred, and C++ APIs can be changed with abandon.

liangfok · 2016-06-22T01:58:17Z

Thanks for all of the advice!

How about let's strike a compromise: From now till the 1.0 release, we'll do everything possible to honor the existing method API alongside the new API but add deprecation compile-time and run-time warnings stating that the old APIs will be removed by the 1.0 release. Just prior to the 1.0 release, we remove all of these depreciated methods.

Caveat 1: Drake also uses numerous public variables that we should ideally make private or protected. I see no way of doing this without breaking the API, and don' t think we can keep them public until the last minute. For these, I can think of no other option but to force downstream users to use the soon-to-be-introduced accessors for them.

Caveat 2: We are considering changing entire class names prior to the 1.0 release. If a class' name is changed, I see no way to maintain both simultaneously unless the new classes are decoupled from the old ones and ideally physically located in a separate directory. This may justify developing the new classes in a separate directory alongside the existing classes (e.g., System 2.0 vs. System 1.0 and potentially Dynamics 2.0 vs. Dynamics 1.0).

Thoughts?

RussTedrake · 2016-06-22T06:51:46Z

I agree that we should be able to change the c++ code with abandon. I'll go further -- i think it's really important that we feel unconstrained in making those changes. However, findLink is a method that probably every piece of c++ code using drake is calling. Changing it somewhat arbitrarily (because we suddenly like FindBody better) feels like an extreme of abuse to any users, and i hoped that adding a deprecated warning is very easy in this case. If there is an easy solution available, then i think we should consider it.

…rake into feature/link_to_body_refactor

…Id() because it delays downstream users from discovering that they need to update their code." This reverts commit f337c61.

…ee::findLinkId.

jwnimmer-tri · 2016-06-23T20:03:20Z

Shouldn't this be merged or rebased to master, to use the existing deprecated.h instead of a different one? Both reviewable and github are showing drake/common/deprecated.h as new-code in this PR.

Comments from Reviewable

liangfok · 2016-06-23T20:04:29Z

I think that may be a Reviewable issue. I'll close and re-open this PR to fix.

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

Comments from Reviewable

…e/link_to_body_refactor

sherm1 · 2016-06-23T20:09:27Z

Looks like this includes your own deprecated.h. The master branch has drake_deprecated.h now so you should delete yours.

Reviewed 1 of 12 files at r9.
Review status: 1 of 12 files reviewed at latest revision, 1 unresolved discussion.

CHANGELOG.md, line 79 [r11] (raw file):

[#2597]: https://github.com/RobotLocomotion/drake/issues/2597
[#2602]: https://github.com/RobotLocomotion/drake/issues/2602
[#2621]: https://github.com/RobotLocomotion/drake/issues/2621

The PR # is wrong and there is no final newline.

Comments from Reviewable

liangfok · 2016-06-23T20:10:40Z

You're right! Doing that right now. Thanks.

Reviewed 1 of 12 files at r9.
Review status: 2 of 12 files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

Signed-off-by: Chien-Liang Fok <[email protected]>

sherm1 · 2016-06-23T20:16:46Z

Reviewed 2 of 12 files at r9.
Review status: 2 of 11 files reviewed at latest revision, 5 unresolved discussions.

drake/systems/plants/RigidBodyTree.h, line 10 [r11] (raw file):

#include <unordered_map>

#include "drake/common/deprecated.h"

drake_deprecated.h

drake/systems/plants/RigidBodyTree.h, line 581 [r11] (raw file):

   * instead.
   */
#ifndef SWIG

@liangfok, this shouldn't be necessary since DRAKE_DEPRECATED has its own #ifdef SWIG. What happens if you remove these now?

drake/systems/plants/RigidBodyTree.h, line 584 [r11] (raw file):

      DRAKE_DEPRECATED(
          "RigidBodyTree: findLink: WARNING: This method is "
          "deprecated. Please use the newer "

This message is wrong -- the macro generates its own boilerplate and the compiler will identify the deprecated declaration. You should just write DRAKE_DEPRECATED("Please use RigidBodyTree::FindBody instead.").

drake/systems/plants/RigidBodyTree.h, line 616 [r11] (raw file):

      DRAKE_DEPRECATED(
          "RigidBodyTree: findLinkId: ERROR: This "
          "method is deprecated. Please use the newer "

Fix message.

Comments from Reviewable

liangfok · 2016-06-23T20:20:53Z

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

CHANGELOG.md, line 79 [r11] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

The PR # is wrong and there is no final newline.

Fixed. Thanks.

drake/systems/plants/RigidBodyTree.h, line 10 [r11] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

drake_deprecated.h

Fixed.

drake/systems/plants/RigidBodyTree.h, line 581 [r11] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

@liangfok, this shouldn't be necessary since DRAKE_DEPRECATED has its own #ifdef SWIG. What happens if you remove these now?

The following error occurred yesterday:

Error: Syntax error - possibly a missing semicolon.

I tried a whole bunch of things to avoid this work around but they all failed.

I am re-testing right now using the latest version of this PR's branch.

drake/systems/plants/RigidBodyTree.h, line 584 [r11] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

This message is wrong -- the macro generates its own boilerplate and the compiler will identify the deprecated declaration. You should just write DRAKE_DEPRECATED("Please use RigidBodyTree::FindBody instead.").

Done.

drake/systems/plants/RigidBodyTree.h, line 616 [r11] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Fix message.

Done.

Comments from Reviewable

sherm1 · 2016-06-23T20:49:59Z

Reviewed 5 of 12 files at r9, 1 of 1 files at r11.
Review status: 8 of 11 files reviewed at latest revision, 4 unresolved discussions.

CHANGELOG.md, line 79 [r11] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Fixed. Thanks.

still no final newline

drake/systems/plants/RigidBodyTree.h, line 581 [r11] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

The following error occurred yesterday:
Error: Syntax error - possibly a missing semicolon.
I tried a whole bunch of things to avoid this work around but they all failed.

I am re-testing right now using the latest version of this PR's branch.

I wonder if SWIG has to be told explicitly which macros to expand? If it did expand this one it would have encountered the internal `#ifndef SWIG` that would have made it expand to nothing.

drake/systems/plants/test/rigid_body_tree_test.cc, line 195 [r13] (raw file):

//   EXPECT_NE(tree->findLink("body1", "robot1"), nullptr);
//   EXPECT_NE(tree->findLinkId("body1"), -1);
// }

why is this here?

Comments from Reviewable

liangfok · 2016-06-23T20:57:36Z

Review status: 7 of 11 files reviewed at latest revision, 4 unresolved discussions.

CHANGELOG.md, line 79 [r11] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

still no final newline

Done. Hmm, my text editor must have removed it because I'm pretty sure I had left one previously. Will look into it later.

drake/systems/plants/RigidBodyTree.h, line 581 [r11] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

I wonder if SWIG has to be told explicitly which macros to expand? If it did expand this one it would have encountered the internal #ifndef SWIG that would have made it expand to nothing.

I removed the `#ifndef SWIG` and `#endif` preprocessor directives surrounding the `DRAKE_DEPRECATED` macro and encountered the following compile-time error:

/home/liang/dev/drake2-distro/drake/../drake/systems/plants/RigidBodyTree.h:582: Error: Syntax error in input(3).

How does one tell SWIG which macros to expand?

drake/systems/plants/test/rigid_body_tree_test.cc, line 195 [r13] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

why is this here?

Oops. It was test code I used to evaluate the deprecate macro. Removed in next revision.

Comments from Reviewable

sherm1 · 2016-06-23T21:09:50Z

Reviewed 2 of 12 files at r9, 1 of 1 files at r13, 2 of 2 files at r14.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

drake/systems/plants/RigidBodyTree.h, line 581 [r11] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

I removed the #ifndef SWIG and #endif preprocessor directives surrounding the DRAKE_DEPRECATED macro and encountered the following compile-time error:
/home/liang/dev/drake2-distro/drake/../drake/systems/plants/RigidBodyTree.h:582: Error: Syntax error in input(3).
How does one tell SWIG which macros to expand?

SWIG's macro documentation [here](http://www.swig.org/Doc1.3/Preprocessor.html) makes me think it should have no trouble with this at all. I suspect that SWIG is not seeing the `drake_deprecated.h` header so just doesn't know this is a macro and instead thinks it is a broken function call.

Comments from Reviewable

liangfok · 2016-06-23T21:19:34Z

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

drake/systems/plants/RigidBodyTree.h, line 581 [r11] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

SWIG's macro documentation here makes me think it should have no trouble with this at all. I suspect that SWIG is not seeing the drake_deprecated.h header so just doesn't know this is a macro and instead thinks it is a broken function call.

I see. Makes sense. At @jwnimmer-tri's suggestion, I'm planning on writing a minimal test that replicates this problem.

Comments from Reviewable

jwnimmer-tri · 2016-06-23T21:43:38Z

This PR passed https://drake-jenkins.csail.mit.edu/job/experimental/2251/ and is thus ready for final review / merge. @jwnimmer-tri

I'm going to de-assign myself. By all means if merging ends up blocked on a flake, please bring ping me here or DM me on slack to use my override powers, but in general I think either the platform reviewer or author should push the merge button; the on-call reviewer doesn't need to be involved (and will often lack the necessary context).

liangfok · 2016-06-24T11:06:16Z

OS X CI appears to have suffered a known flake:

ld: malformed archive TOC entry for I[NON-UTF-8-BYTE-0x8B][NON-UTF-8-BYTE-0xBC]$[NON-UTF-8-BYTE-0xB8], offset 274736 is beyond end of file 12288
 file '/Users/yosemite/workspace/experimental/5403bcc7/build/lib/liblcmtypes_bot2-core.a' for architecture x86_64

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

liangfok · 2016-06-24T14:22:50Z

@drake-jenkins-bot retest this please

liangfok · 2016-06-25T02:40:45Z

This PR passed https://drake-jenkins.csail.mit.edu/job/experimental/2276/ and is thus ready for final review / merge.

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

Comments from Reviewable

sherm1 · 2016-06-25T05:12:07Z

Just one SWIG question remaining.

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

drake/systems/plants/RigidBodyTree.h, line 581 [r11] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

I see. Makes sense. At @jwnimmer-tri's suggestion, I'm planning on writing a minimal test that replicates this problem.

Liang, there is still this SWIG question open. Do you want to leave it here and deal with it elsewhere? If so please file an issue so we can keep track.

Comments from Reviewable

liangfok · 2016-06-25T11:32:40Z

I addressed all reviewer comments. PTAL. Thanks!

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

drake/systems/plants/RigidBodyTree.h, line 581 [r11] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Liang, there is still this SWIG question open. Do you want to leave it here and deal with it elsewhere? If so please file an issue so we can keep track.

Done: https://github.com//issues/2646

Comments from Reviewable

sherm1 · 2016-06-25T19:00:52Z

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

Comments from Reviewable

sherm1 · 2016-06-25T19:02:58Z

Liang, reviewable completion is held up on two of your own comments that need acknowledging.

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

Comments from Reviewable

More "link" to "body" refactoring.

1ac16e1

liangfok added type: cleanup priority: low team: software core labels Jun 20, 2016

liangfok self-assigned this Jun 20, 2016

Added CHANGELOG entry.

5a2a6ad

liangfok assigned sherm1 Jun 20, 2016

Added missing newline to CHANGELOG.

7bbd717

Chien-Liang Fok added 4 commits June 21, 2016 16:47

Updated to use new FindBody() API.

149160c

Added back RigidBodyTree::findLinkID() and RigidBodyTree::finLink(). …

8aacceb

…Body of these methods contain a static assertion.

Merge branch 'master' of github.com:RobotLocomotion/drake into featur…

aa82143

…e/link_to_body_refactor

Removed RigidBodyTree::findLink() and RigidBodyTree::findLinkId() bec…

f337c61

…ause it delays downstream users from discovering that they need to update their code.

liangfok mentioned this pull request Jun 22, 2016

Add explicit instantiations for RigidBodyTree::doKinematics. #2610

Merged

Chien-Liang Fok added 3 commits June 22, 2016 11:49

Merge branch 'feature/link_to_body_refactor' of github.com:liangfok/d…

2c499b1

…rake into feature/link_to_body_refactor

Revert "Removed RigidBodyTree::findLink() and RigidBodyTree::findLink…

0146808

…Id() because it delays downstream users from discovering that they need to update their code." This reverts commit f337c61.

Added deprecated attribute to RigidBodyTree::findLink and RigidBodyTr…

ed495f0

…ee::findLinkId.

liangfok closed this Jun 23, 2016

liangfok reopened this Jun 23, 2016

liangfok closed this Jun 23, 2016

Merge branch 'master' of github.com:RobotLocomotion/drake into featur…

a82963b

…e/link_to_body_refactor

liangfok reopened this Jun 23, 2016

Updated RigidBodyTree to use correct drake_deprecated.h.

45f43d0

Signed-off-by: Chien-Liang Fok <[email protected]>

Simplified deprecation message.

3495c54

Chien-Liang Fok added 2 commits June 23, 2016 16:56

Added newline.

638b36e

Removed dead code.

ce7f8ea

jwnimmer-tri removed their assignment Jun 23, 2016

liangfok mentioned this pull request Jun 25, 2016

Create SWIG + Deprecation Unit Example Demonstrating Syntax Error #2646

Closed

liangfok merged commit df6143f into RobotLocomotion:master Jun 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More "link" to "body" refactoring. #2597

More "link" to "body" refactoring. #2597

liangfok commented Jun 20, 2016 •

edited by david-german-tri

Loading

liangfok commented Jun 20, 2016

sherm1 commented Jun 20, 2016

liangfok commented Jun 20, 2016

RussTedrake commented Jun 20, 2016

liangfok commented Jun 21, 2016

RussTedrake commented Jun 21, 2016

liangfok commented Jun 21, 2016

liangfok commented Jun 21, 2016

liangfok commented Jun 21, 2016

wxmerkt commented Jun 21, 2016

sherm1 commented Jun 21, 2016

jwnimmer-tri commented Jun 22, 2016

liangfok commented Jun 22, 2016 •

edited

Loading

RussTedrake commented Jun 22, 2016

jwnimmer-tri commented Jun 23, 2016

liangfok commented Jun 23, 2016

sherm1 commented Jun 23, 2016

liangfok commented Jun 23, 2016

sherm1 commented Jun 23, 2016

liangfok commented Jun 23, 2016

sherm1 commented Jun 23, 2016

liangfok commented Jun 23, 2016

sherm1 commented Jun 23, 2016

liangfok commented Jun 23, 2016

jwnimmer-tri commented Jun 23, 2016

liangfok commented Jun 24, 2016

liangfok commented Jun 24, 2016

liangfok commented Jun 25, 2016

sherm1 commented Jun 25, 2016

liangfok commented Jun 25, 2016

sherm1 commented Jun 25, 2016

sherm1 commented Jun 25, 2016

More "link" to "body" refactoring. #2597

More "link" to "body" refactoring. #2597

Conversation

liangfok commented Jun 20, 2016 • edited by david-german-tri Loading

liangfok commented Jun 20, 2016

sherm1 commented Jun 20, 2016

liangfok commented Jun 20, 2016

RussTedrake commented Jun 20, 2016

liangfok commented Jun 21, 2016

RussTedrake commented Jun 21, 2016

liangfok commented Jun 21, 2016

liangfok commented Jun 21, 2016

liangfok commented Jun 21, 2016

wxmerkt commented Jun 21, 2016

sherm1 commented Jun 21, 2016

jwnimmer-tri commented Jun 22, 2016

liangfok commented Jun 22, 2016 • edited Loading

RussTedrake commented Jun 22, 2016

jwnimmer-tri commented Jun 23, 2016

liangfok commented Jun 23, 2016

sherm1 commented Jun 23, 2016

liangfok commented Jun 23, 2016

sherm1 commented Jun 23, 2016

liangfok commented Jun 23, 2016

sherm1 commented Jun 23, 2016

liangfok commented Jun 23, 2016

sherm1 commented Jun 23, 2016

liangfok commented Jun 23, 2016

jwnimmer-tri commented Jun 23, 2016

liangfok commented Jun 24, 2016

liangfok commented Jun 24, 2016

liangfok commented Jun 25, 2016

sherm1 commented Jun 25, 2016

liangfok commented Jun 25, 2016

sherm1 commented Jun 25, 2016

sherm1 commented Jun 25, 2016

liangfok commented Jun 20, 2016 •

edited by david-german-tri

Loading

liangfok commented Jun 22, 2016 •

edited

Loading