Drake: multibody: Make directory structure more concise

[EricCousineau-TRI]

_Edited to add overview by jwnimmer-tri:_

We will move the multibody-related source files and clean up their package names and pydrake module names.

Checklist:

• [x] Rename files and fix namespaces per https://github.com/RobotLocomotion/drake/issues/9314#issuecomment-433128655. This allows users to employ the new C++ spellings.
• [x] #10220 #10231 Bulk-patch Drake C++ to only use the new spellings
• [x] #10231 Turn on C++ deprecation warnings for the old spellings.
• [x] #10207 Update pydrake to offer the new module names. This allows users to employ the new Python spellings.
• [ ] #10207 Bulk-patch Drake Python to only use the new spellings; turn on deprecation warnings for the old spellings.
• [x] #10287 Remove the leftover implicit_stribeck namespace.
• [ ] Remove the deprecation stubs around 2019-03-01.

---

_Overview by EricCousineau-TRI:_

From slack convo:

jwnimmer-tri [May 21st at 8:49 AM]
As an aside, I will again register my distaste at 4x multibody in the pathname multibody/multibody_tree/multibody_plant/multibody_plant.h. multibody/plant/multibody_plant.h seems to me just as descriptive, still well-organized in a folder, and is much shorter.

25 replies
sean.curtis [3 months ago]
While I completely agree with what you're saying, how much of that current name is a function of the fact that MBP/T and RBP/T are both in multibody and much of the cruft arises from partitioning the two sets of logic? If true, that would still allow for the temporary:

multibody/multibody_tree/plant/multibody_plant

WIth the expectation that multibody_tree would eventually be eliminated and its contents moved up a directory.

jwnimmer-tri [3 months ago]
Personally, I see no reason for the presence of RigidBodyTree to affect the layout of MBP. Do the right thing. Its easy to ignore the old files.

jwnimmer-tri [3 months ago]
Hmm. I bet I could figure out a way to move RBP into multibody/v1/rigid_body_tree.h etc in git, but still include and install with their current paths. If that's a blocker to fixing MBP names and paths, I can do it ASAP.

alejandro [3 months ago]
Let me add more to your burden; namespaces. RBT has no namespace and that's gonna become a headache when having, say, MBP simulation + RBT IK planning (at least in the short term)

alejandro [3 months ago]
I think at least we'd like to fix that first

alejandro [3 months ago]
We have an example of that here: https://github.com/amcastro-tri/drake/blob/kuka_arm_demo/examples/kuka_iiwa_arm/controlled_kuka/controlled_kuka_demo.cc

alejandro [3 months ago]
@ sammy-tri is working out the details of how this integration will go

jwnimmer-tri [3 months ago]
I don't think we should add namespaces to RBT. I am unconvinced that there is "a headache" of meaningful magnitude.

alejandro [3 months ago]
The headache starts in the example above if I try to bring, say, RigidBody to scope.

alejandro [3 months ago]
there might be others

jwnimmer-tri [3 months ago]
Anyway, that's a separate thread.

jwnimmer-tri [3 months ago]
We can shorten the include paths without fixing RBT namespaces.

alejandro [3 months ago]
sure, I just thought it was good bringing that up to attention

Also sent to the channel
jwnimmer-tri [19 hours ago]
Resurrecting this thread. Possibly we should fix multibody_tree's include pathnames before pybind adds even more cement to our grave?

alejandro [19 hours ago]
I'm open to suggestions. @ eric.cousineau?

eric.cousineau [19 hours ago]
My first suggestion is flatten multibody_tree, e.g. merge multibody_plant into it.

Second suggestion is maybe move multibody_tree to top-level, to have drake/multibody_tree separate from drake/multibody.

That's all I can think of at a first pass; what say y'all?

jwnimmer-tri [19 hours ago]
@ alejandro What kind of suggestions are you looking for? Re: whether to fix this now, or Re: how the files should be path'd when we do fix it, or Re: should be preemptively fix in pydrake only, or ...?

alejandro [19 hours ago]
I was only asking for path suggestions really

alejandro [19 hours ago]
I'd personally skip the "flattening" step and have drake/multibody (with all the MBT code in it) and drake/multibody_plant

alejandro [19 hours ago]
that might require moving RBT somewhere else... too violent of a change?

jwnimmer-tri [19 hours ago]
As I said upthread, I _think_ I could update the BUILD files so that we can move the RBT files in git, but not disturb any include paths or build labels.

jwnimmer-tri [18 hours ago]
Personally I am a fan of placing code two directories deep, always. (First folder is general topic, second folder is each major library.)

jwnimmer-tri [18 hours ago]
So I would do multibody/{math,tree,plant,parsing,visualization} and lose the multibody_treetree/{joints,implicit_stribeck} and etc subfolders. (edited)

jwnimmer-tri [18 hours ago]
We could have multibody/legacy/multibody/rigid_body*.h in git to hold the old stuff (but cut out the multibody/legacy/ part when installing, so its unchanged for users).

jwnimmer-tri [18 hours ago]
In any case, if we are going to propose to do this rename now (yay!) then we should open an issue and discuss / plan there.

Participants mapped to GitHub usernames:
@jwnimmer-tri @SeanCurtis-TRI @amcastro-tri @mpetersen94

Relates #9281.

[EricCousineau-TRI]

Followup: In writing #9322, pybind or Python is currently truncating the full-name of MultibodyPlant. This is what an error message looks like:

...
    cart_pole.AddForceElement(
AttributeError: 'pydrake.multibody.multibody_tree.multibody_plant.M' object has no attribute 'AddForceElement'

[amcastro-tri]

That is because python has a limit on the size of these paths for error messages? or... (sry if another newbie question)

[jwnimmer-tri]

I am going to attempt to do the piece where we move RBT code into a subfolder (but don't affect any projects that consume Drake). See #9343.

[EricCousineau-TRI]

That is because python has a limit on the size of these paths for error messages? or... (sry if another newbie question)

Not exactly sure, will def. see if I can fix it (esp. given that it's at 50 characeters...); however, the fact that it pushes up against this limit and only has one character from the class name seems like a good case to make the path a bit less verbose.

EDIT: Following up, it seems that Python 2.7 truncates the class name in its error message (kind of annoying?); pybind11 stores the full name. A pure Python example:

>>> class SomeReallyLoooooooooooooooooooooooooooooooooooooongClassName(object):
...     pass
... 
>>> x = SomeReallyLoooooooooooooooooooooooooooooooooooooongClassName()
>>> x.doesnt_exist()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'SomeReallyLoooooooooooooooooooooooooooooooooooooon' object has no attribute 'doesnt_exist'

[jwnimmer-tri]

@amcastro-tri The RBT files have been moved out of the way, on master. I think its probably ready for you guys to move / rename the MBT files now. There are some lingering mostly-empty directories under multibody that I will still work to remove, but hopefully they are not too onerous for a little while.

Maybe the first step is to make a proposal for the new paths and filenames?

[amcastro-tri]

Thanks @jwnimmer-tri! I'll consult with the team.
cc'ing @sherm1, @SeanCurtis-TRI, @DamrongGuoy, @mitiguy, @edrumwri

[sherm1]

A few thoughts on this:

• there are still a bunch of obsolete directories under drake/multibody using up nice names like joints and collision. I don't think we should attempt reorganizing the multibody_tree stuff until those are gone. Do they have to remain in place?
• since you are about to go on vacation I think we should shelve this topic for now so you can focus on anything substantive you want to get done before you leave.

[edrumwri]

I second Sherm's second thought.

---

Evan Drumwright
Senior Research Scientist
http://positronicslab.github.io
Toyota Research Institute
Palo Alto, CA

On Wed, Sep 12, 2018 at 8:57 AM Michael Sherman [e-mail]
wrote:

A few thoughts on this:

• there are still a bunch of obsolete directories under drake/multibody
  using up nice names like joints and collision. I don't think we should
  attempt reorganizing the multibody_tree stuff until those are gone. Do they
  have to remain in place?
• since you are about to go on vacation I think we should shelve this
  topic for now so you can focus on anything substantive you want to get done
  before you leave.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/RobotLocomotion/drake/issues/9314#issuecomment-420702131,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACHwz0rp51SB7uUU337dKwwm2CVImzigks5uaS8AgaJpZM4WP8hp
.

--

Confidential or protected information may be contained in this email
and/or attachment. Unless otherwise marked, all TRI email communications
are considered "PROTECTED" and should not be shared or distributed. Thank
you.

[jwnimmer-tri]

There are still a bunch of obsolete directories under drake/multibody using up nice names like joints and collision. I don't think we should attempt reorganizing the multibody_tree stuff until those are gone. Do they have to remain in place?

They are on the roadmap for removal, I just need to warn users first. However, nothing is stopping you from either placing new code there (we'd just change the README), or simply ignoring them if you don't need the name anymore.

To me, its important to get the multibody includes fixed quickly, because we want to be able to tell users to start using it. If that means I need to nuke the empty directories with abandon and disturb a few downstream users who compile with Bazel, I'd rather do that than wait another day to fix the new pathnames.

[sherm1]

Let's give the users a few weeks to adapt while Alejandro recharges on some tropical beach. (He should of course be a primary participant but has a long list of more-pressing things to stabilize by tomorrow.) Then I'd like to nuke the old stuff prior to reshuffling so that we have a blank page to populate.

[SeanCurtis-TRI]

@sherm1 what exactly should the users be getting used to? I don't follow. I don find @jwnimmer-tri' s point compelling. The whole point of this exercise is to change the in-code paths to the new multibody entities and doing that before we press people to use it makes more sense than doing it after.

[jwnimmer-tri]

@SeanCurtis-TRI My point was that in order to clear the deck so that, I guess, dynamics team is more comfortable to create the new folders, @sherm1 wants me to rmdir multibody/shapes so there are no more misleading stub directories. If I do that, any downstream Bazel users who have deps=[] on //multibody/shapes would fail to compile (since the alises disappeared). I'd _like_ to give those users a warning before I break them, and I think @sherm1 was saying its okay to let this sit while I do so. It's about letting them respell their deps on RBP, not asking them to port to MBP right away.

[SeanCurtis-TRI]

I had clearly missed the boat on that one. I'm still missing some common assumption. The discussion is clearly revolving around something I haven't grokked. The danger of a false positive -- mistakenly thinking I understood.

[jwnimmer-tri]

The "this bazel package name is deprecated now" warnings are in as of today (#9444).

[sherm1]

Thanks, Jeremy!

[jwnimmer-tri]

Within the next day or two, all of the cruft will have been removed from //multibody. I think we're past due to come up with a proposal for the new structure? What's the plan @sherm1 @amcastro-tri?

I think the best format for a proposal is a table that lists the mapping from old filename to new filename, for all files (possibly using globs for brevity) -- possibly with some placeholders for to-be-written subsystems.

My vote is still what I said in the original post, though I haven't fleshed it out yet:

I would do multibody/{math,tree,plant,parsing,visualization} and lose the multibody_tree/{joints,implicit_stribeck} and etc subfolders.

Does that seem promising enough that I should write it up in a fully-itemized proposal?

[sherm1]

I like your proposal Jeremy but Alejandro's approval is more important than mine. He is on vacation through next week. Also the priority on this (for us to get to it) has to be behind the stack of fires we are currently trying to put out for various users.

[jwnimmer-tri]

Hopefully, once we have quorum vs vacations, it won't be that much effort to agree on the structural goal. I assume someone on the manipulation team will be doing the work, though.

[jwnimmer-tri]

In the Project Flower meeting, I proposed that the Flower team be the ones to do the PRs, and that seemed fine with everyone. So the only Dynamics team responsibility here will be to help design and/or agree on the directory and namespace structure that we'll end up with, and then Flower will take it from there.

[jwnimmer-tri]

Specifically, here is what I would propose for the file paths. This lists _all_ of the multibody folders, even the unchanged ones [1].

New path | Old path(s) | Status
--- | --- | ---
_multibody/benchmarks/_ | _multibody/benchmarks/_ | n/a
_multibody/constraint/_ | _multibody/constraint/_ | n/a
_multibody/inverse_kinematics/_ | _multibody/inverse_kinematics/_ | n/a
__multibody/math/__ | __multibody/multibody_tree/math/__ | ~#10140~
_multibody/models/_ | _multibody/models/_ | n/a
__multibody/parsing/__ [2] | __multibody/multibody_tree/parsing/__ | ~#10045~
__multibody/plant/__ | __multibody/multibody_tree/multibody_plant/__
__multibody/multibody_tree/implicit_stribeck/__ | ~#10160~
__multibody/tree/__ [3] | __multibody/multibody_tree/__
__multibody/multibody_tree/joints/__ | ~#10142~

[1] The old paths multibody/rigid_body_plant/visualization/** will be re-located into //tools (PRs already in progress), and will no longer live under //multibody/....

[2] ... for the parsers, we'd also remove the multibody_plant_ prefix from the two basenames that redundantly begin with it. So, e.g., multibody/parsing/sdf_parser.h instead of multibody/multibody_tree/parsing/multibody_plant_sdf_parser.h.

[3] ... to be clear, this means that we no longer have a separate joints sub-folder, those few files move into the tree folder directly.

Then, for namespace, I'd propose that:

• The //multibody/benchmarks/... keep their namespaces (i.e., namespace mutibody::pendulum or etc.), but otherwise ...
• ... everything else is either
  
  
  
  • namespace multibody or
  
  
  • namespace multibody::detail or
  
  
  • namespace multibody::internal or
  
  
  • namespace multibody::test.
  
  

That is, we stop using multibody_tree and mutlibody_plant and implicit_stribeck as sub-namespaces.

Thoughts / votes / amendments?

[sherm1]

:+1: that looks great to me, Jeremy.

[mitiguy]

I like many of these changes. With regards to:
[3] ... this means that we no longer have a separate joints sub-folder, those few files move into the tree folder directly.

My preference (along with Sherm's) is to keep a separate joints sub-folder. We anticipate (experiential knowledge) that the joints sub-folder will contain enough other joints and code to make it worthwhile.

[jwnimmer-tri]

Experientially, approximately how many? There are already 35 headers in multibody_tree. It seems like even 10-15 more joint types would be on par with the scale of what's already there.

Ideally I would prefer each folder be a library. Right now, the files in multibody_tree and multibody_tree/joints are circularly dependent, so could not be separate libraries.

There are also a lot of mobilizers in the multibody_tree folder right now. Should they be partitioned out? Also into the joints folder, or into their own? Maybe each different kind MultibodyTreeElement should be in its own folder? (Joint, Body, Frame, Mobilizer, Actuator, ModelInstance).

I think that deep folders with <10 components per folder hurt usability. The more you can fit on a screen, the easier it is to grok in fullness.

It would also be easy, you'll recall, to write up a short doxygen outline that enumerates the joint types, so its easy to read about them. The filesystem doesn't have to the only way to organize information. I would rather keep the filesystem what makes sense for _physical_ concerns (packaging, installing), than _logical_ ones (for discoverability) that can be addressed in multiple cooperating presentations via documentation.

Ultimately its your call. Maybe we can make a mock-up or two and discuss live. Like, if you can supply the 10 expected future joint names or whatever, we can see what the file manifest or BUILD file or whatever looks like.

[sherm1]

Simbody has the following 20 joint types:

MobilizedBody_Ball.h
MobilizedBody_BendStretch.h
MobilizedBody_Bushing.h
MobilizedBody_Custom.h
MobilizedBody_Cylinder.h
MobilizedBody_Ellipsoid.h
MobilizedBody_Free.h
MobilizedBody_FreeLine.h
MobilizedBody_FunctionBased.h
MobilizedBody_Gimbal.h
MobilizedBody_Ground.h
MobilizedBody_LineOrientation.h
MobilizedBody_Pin.h
MobilizedBody_Planar.h
MobilizedBody_Screw.h
MobilizedBody_Slider.h
MobilizedBody_SphericalCoords.h
MobilizedBody_Translation.h
MobilizedBody_Universal.h
MobilizedBody_Weld.h

There are also 20 .cpp files to go with these. (The file structure is very flat with include/ and src/ directories and grouping by file prefix rather than directory structure -- not suggesting that for Drake.)

[mitiguy]

From SimWise (formerly Working Model 3D), there are 20+ joints listed in the UI.
The joints are: Rigid, Revolute, Sphere, Rigid on Slot, Revolute on Slot, Spherical on Slot, Spherical on Curve, Rigid on Plane, Revolute on Plane, Spherical on Plane, Parallel Joint, Linear Actuator (position, velocity, acceleration), Revolute Motor (rotation, angular velocity, angular acceleration), Belt, Spur Gear, Bevel Gear, Rod, Rope, Separator.

[jwnimmer-tri]

Alright, that's a lot of joints! Will any other item in the folder (mobilizers?) similarly scale up, that we should anticipate now?

[sherm1]

Hmm. Mobilizers (internal or tree joints) are the most common implementation for Joints in Drake, so there is roughly one per joint. (The other implementation is Constraints which are used for some joints and a dozen or so other things.) "Joint" is the higher-level concept and properly belongs with MultibodyPlant. Mobilizers and constraints are lower-level implementation objects and properly belong with MultibodyTree (which is internal now [soon]). Force elements will come in similar variety to joints and constraints.

It is not obvious to me that all these things should be in subdirectories now when they are so lightly populated. I'd say it is very likely we'll want them that way eventually.

[sherm1]

I am also somewhat sympathetic to Jeremy's desire to have the directory structure reflect the physical library structure produced by Bazel, since we mostly follow that convention elsewhere in Drake. Possibly some compromises will be necessary here but deferring those until forced to make them might be a good idea where we can do that.

[tri-ltyyu]

Per issue scrub, for @amcastro-tri to take a look and see if there's any action that can be taken here. if not, should we close it?

[jwnimmer-tri]

There is still plenty of work to do here. Blocked on dynamics team (probably Alejandro) to choose a directory structure, or else defer to my proposal above.

[amcastro-tri]

I like your proposal above @jwnimmer-tri, thank you for pushing on this!. My preference would be to go with that for now. @sherm1, @mitiguy, do you agree?

[sherm1]

If you're OK with the flat structure for now (no joints subdirectory) then I'm happy with it.

[amcastro-tri]

Yes, I am good with that. It's all yours @jwnimmer-tri. Sorry for the countless delays from my part and thank you again.

[mitiguy]

All good -- lets move ahead.

[jwnimmer-tri]

Focused on project flower now, but I'll try to poke at this within a week. I'll probably start with something simple, like moving just parsing.

[jwnimmer-tri]

The file renames are in progress, with automatically-generated forwarding headers and bazel labels.

The forwarders all have a "should we generated deprecation warnings" flag; my plan is to leave it set to "False" until _all_ of the moves and cleanups are done, and then flip it globally. That way, users only have one spike in warnings, which they can resolve all in a single go.

[jwnimmer-tri]

Presently there are only three tasks remaining: update pydrake (tracked under #10207 umbrella), tidy up the implicit_stribeck namespace (under review at #10287), and remove the stubs (in three months). None of those benefit from this tracking issue being open, so we'll declare victory.

Let's celebrate :partyparrot: having the shorter names to use!