Remove / Re-Name std::shared_ptr<RigidBodyFrame> "weld_to_frame"

[liangfok]

It should ideally be a const reference or unique_ptr. This entails modifying the APIs of RigidBodyTree and RigidBodySystem.

In addition, per discussion in #4107, consider renaming it so it does not sound like a function. Consider frame or weld_frame.

[scpeters]

Do you have any policies about changing API/ABI? I'll go look in the docs...

[scpeters]

I didn't see any mention of changing API/ABI in the docs. If drake CI is clean, is that all you're concerned with? There aren't any downstream packages that use drake that we need to worry about changing API/ABI?

[jwnimmer-tri]

ABI is not a concern (downstream always build from source, at the moment).

We have Russ's blessing to change APIs (with a CHANGELOG.md entry), presuming we have a good reason. (For example, randomly renaming things isn't super helpful, but hiding implementation details would be, or broadening the method to support more use cases, etc.) In short, there are some downstream users, but they try to track our changes, and will be happier if the API breaks are for the greater good.

I haven't thought about whether this particular change is of merit in that context (or even worth pursuing in the near term).

[jwnimmer-tri]

Also, plans to edit the RidigBodyTree should be circulated to the Dynamics team for input, so /CC @sherm1 and @amcastro-tri who can rope anyone else in.

[scpeters]

Ok, it's marked as high priority, but I'll wait to see what is the consensus.

[jwnimmer-tri]

Heh, I didn't see that. I think @liangfok was perhaps a bit generous with that label. Let's have @sherm1 and @amcastro-tri definitely weigh in on what to do here.

[liangfok]

Ha! Yeah, I don't know what I was thinking when I set the priority. Since it's not blocking anyone at the moment, I'll downgrade the priority.

[sherm1]

I believe this change (or at least some change to weld_to_frame) was suggested by @amcastro-tri so I will leave it to him to comment.

[amcastro-tri]

Getting rid of share_ptr's is a good improvement I think. This is something I started in my first huge PR #2207 when I started at TRI and I would be thrilled if @scpeters wants now to clean the RigidBodyFrame shared_ptr's.

It will also be a big PR because, as you will see when you start doing it, the shared_ptr's very easily propagate and contaminate entire code, hiding ownership and creating many times difficult to understand cross references. This change will touch many files: RigidBodySystem, RigidBodyTree, RigidBody, parsers, tests, etc. However it can only be done in one single pass.

RigidBodySystem now contains a std::vector of shared pointers to RigidBodySensor's. I would start by making those unique_ptr's (the system owns them, or should). Then replace every other shared_ptr<RigidBodyFrame> in the code base with either a reference (const where appropriate) or a raw pointer, depending on whether null semantics is needed or not.

[sherm1]

Is RigidBodyPlant also contaminated with shared_ptr leakage?

[liangfok]

Luckily, @amcastro-tri did an excellent job preventing contamination of RigidBodyPlant. It simply maintains a unique_ptr to a RigidBodyTree.

[amcastro-tri]

yes! I hate shared_ptr's!
BTW, thank you for the nice comment @liangfok!

[jwnimmer-tri]

I'm not sure this is relevant anymore, given the planned deprecation of RBT?

[liangfok]

@amcastro-tri, what do you think? Should this be merged into #5013?

[amcastro-tri]

We know we should not be using shared pointers in the way they are being used for RigidBodyFrame objects. This was already taken into account in the design of MultibodyTree's API.
MultibodyTree also will introduce (as early as this week) the concept of frames with a slight difference in semantics and API to allow the modeling of soft bodies.
Therefore I think this is not relevant anymore unless it is blocking somebody's work? @liangfok, thank you for bringing it up to my attention, should I worry about closing this? do you know of someone who needs this to be resolved?

[liangfok]

Thanks @amcastro-tri. I'll go ahead and close this issue since I don't think it's blocking anyone and MBT will have a totally different API.