RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.28k stars 1.26k forks source link

Recommend refactoring drake/systems/plants/rigidbody to drake/multibody #3481

Closed RussTedrake closed 7 years ago

RussTedrake commented 8 years ago

It is a big piece of the codebase, and seems to belong alongside automotive

amcastro-tri commented 8 years ago

I was thinking to place all the RBT and the family, which in my mind are a "dynamics engine" module of Drake, to live somewhere like in drake/MultibodyDynamics.

RussTedrake commented 8 years ago

That is consistent with my recommendation except for multibodydynamics instead of rigidbody.

I agree it is the dynamics engine. But it is not only dynamics (kinematics, visualization, etc, too) -- this is also a reason to pull it out from systems. That plus the inertia makes me prefer rigidbody.

On Sep 16, 2016, at 4:37 PM, Alejandro Castro notifications@github.com wrote:

I was thinking to place all the RBT and the family, which in my mind are a "dynamics engine" module of Drake, to live somewhere like in drake/MultibodyDynamics.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

amcastro-tri commented 8 years ago

Ah, I see, I didn't realize that you meant to create a directory rigidbody in drake. I thought you just meant "all physics engine related".

jwnimmer-tri commented 7 years ago

I would like to suggest that this is an important priority. (At least, once #3253 is finished, which should be within a week.) The code is hefty enough (in terms of number of files, build time, and maintenance) that we should give it a new home as this issue suggests, and it seems smart to do that before we ramp up even more effort on improving this piece of the toolkit. Having it outside of drake/systems would make it much easier to thoughtfully clean up, polish, and promote what remains within the Systems libraries.

What do you think about making this a near-term goal @sherm1 @amcastro-tri @edrumwri @SeanCurtis-TRI?

I can lend a hand with tools that help examine file and build dependencies (in order to figure out which modules move, and where they go) and techniques for performing the shuffle (automation and scripting to avoid merge hell), but I am hoping to not have to do this cleanup myself.

sherm1 commented 7 years ago

I like that idea. I would suggest drake/multibody as the directory name and nominate @amcastro-tri to do it, taking @jwnimmer-tri up on his offer to consult. What do you think, Alejandro?

amcastro-tri commented 7 years ago

Absolutely, I like this plan and I will do this even before the templatization of RBT on <T>. Thank you @jwnimmer-tri for offering help with this.

jwnimmer-tri commented 7 years ago

Per f2f with @amcastro-tri, I am going to list a few specific possible outcomes for where code would end up living, to help everyone agree on the end goal. Stay tuned next week.

edrumwri commented 7 years ago

I'm going to abstain from the discussion until I have a better idea of drake's bigger picture. My head is still in the context of integrating simple systems and my first opportunity to move up the stack won't be until I get to guard function evaluation.

On Fri, Oct 21, 2016 at 8:15 AM, Michael Sherman notifications@github.com wrote:

I like that idea. I would suggest drake/multibody as the directory name and nominate @amcastro-tri https://github.com/amcastro-tri to do it, taking @jwnimmer-tri https://github.com/jwnimmer-tri up on his offer to consult. What do you think, Alejandro?

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

jwnimmer-tri commented 7 years ago

So I've done a quick look at this, though should do more after some other cleanup passes merge.

Option (A) will be something like:

  1. The new rigidbody is a higher dependency layer than systems. (In other words, rigidbody will depend on systems.)
  2. The new rigidbody directory gathers together several related modules (libraries), detailed below. I doesn't have to be a single monolith; having sub-structure within it is still helpful.
  3. All of post-#3861 systems/plants moves into rigidbody, except for direct_collocation_constraint and spring_mass_system. Notably this includes rigid_body_plant, as well as collision, joints, shapes, etc. All of it seems to be closely coupled to class RigidBodyTree so should also be in the same physical module.
  4. The controllers that control a RBT probably can't still live in systems; we'll have to find a new home. Possibly not an immediate priority, if we can work around it with TODO documentation.
  5. The trajectories home is already wrong and will have to move a more basic dependency layer (closer to common or math) no matter what.
sherm1 commented 7 years ago

The revised multibody code will have large non-System classes but those will have a dependency on Context since they will be employing the System 2 caching functionality. I would like to see that code under drake/multibody. This would include code descended from RBTree and its related classes -- let's call it MBTree for discussion.

Separately, there will be a System or Systems that make use of MBTree, say MBPlant. We should take some pains to keep that separate from the non-System computational classes. I'm not sure where that should go but drake/multibody/systems makes sense to me.

("Multibody" is a more general term than "Rigid body" -- we'll be switching to that terminology soon.)

david-german-tri commented 7 years ago

@sherm1's request seems consistent with @jwnimmer-tri's proposal to me (and also reasonable).

The revised multibody code will have large non-System classes but those will have a dependency on Context since they will be employing the System 2 caching functionality.

I do wonder if we can/should avoid that dependency with a proxy API. But I'll wait for code on the table.

jwnimmer-tri commented 7 years ago

So my assignment here was to come up with the pros/cons of a few options and present them for the dynamics team to decide. As it turns out though, I think option (A) is the only good option; are you folks happy with it?

Next step is for the dynamics team and Russ to decide if the directory is called rigidbody or multibody. Maybe do that soon so we can move on?

The only other open discussions relate to what to do with controllers and trajectories, but that's a separate question from the rigidbody shuffle.

Once we have the above questions sorted, I can help steer @amcastro-tri through a large-code-shuffle PR.

RussTedrake commented 7 years ago

I'm fine with multibody. But the bike shed must be blue. :)

sherm1 commented 7 years ago

We wanted multibody so the bike shed can be soft!

jwnimmer-tri commented 7 years ago

@amcastro-tri What do you think? Ready to start on this relocation ASAP?

amcastro-tri commented 7 years ago

I am happy with this plan @jwnimmer-tri, thank you for writing down the options. I would like however to wait until my PR #3940 lands. Otherwise it'd be a lot of work for me to merge with these changes. It should hopefully be ready for today. Thank you all for your feedback.

jwnimmer-tri commented 7 years ago

Sure. But note that this issue will be resolved by writing a shell script that performs the change, which is equally applicable to any version of master, so you could start prototyping and testing this fix now, even with other PRs in flight (yours and/or others').

amcastro-tri commented 7 years ago

I am still unsure whether rigid_body_plant should be moved into drake/multibody. I feel more inclined to leaving it in drake/systems/plants as it is right now. That makes sense to me. Pros/cons?

jwnimmer-tri commented 7 years ago

It is more tightly coupled with the multibody classes than the framework classes. It should go into multibody.

sherm1 commented 7 years ago

I agree with Jeremy -- let's put it under multibody.