Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eradicating memory leaks & making classes copy-safe and clonable #369

Merged
merged 154 commits into from Jun 12, 2015

Conversation

@mxgrey
Copy link
Member

@mxgrey mxgrey commented Mar 26, 2015

This is a premature pull request for review and discussion purposes, not ready to be merged yet

This pull request is ready for review

This branch and pull request are dedicated to removing memory leaks from DART as well introducing the ability to copy properties and create clones of objects. Spawning clones will be useful for making efficient multithreaded applications.

The changes being made here are pervasive, and some will require breaking the API, specifically the adoption of std::shared_ptr for the Shape and Skeleton classes.

Here is a brief list of the conceptual changes:

  • BodyNode and Joint classes will all have a Properties struct that can be safely exchanged between different instances, and this struct will be used to standardize the constructors for the different types.
  • BodyNode and Joint instances will be fully managed by their Skeleton. BodyNode and Joint instances will only be creatable by the Skeleton class (the current procedure for assembling Skeletons will be deprecated) and they will be destroyed when the Skeleton class destructs
  • Shape instances and Skeleton instances will be treated as shared resources, and they will be managed by std::shared_ptr. BodyNodes and Joints are not shared resources because they are worthless/meaningless without being part of a single specific Skeleton.
  • It will be possible to clone Worlds and Skeletons.
  • Cloning a Skeleton entails creating a new Skeleton instance that is filled with new Joint instances and new BodyNodes instances which copy the properties of the original Skeleton's Joints and BodyNodes. Shape resources will be shared between Skeleton clones to minimize memory usage and copying overhead.
  • Cloning a World entails creating a new World instance that is filled with new Skeletons which are clones of the Skeletons of the original World.

To be determined:

It would also be good to have a concept of a "State" to complement the "Properties" concept. I am still hashing out the details for how a State class would work, so suggestions are welcome. At the moment, I think the State should belong to the Skeleton, but it's not clear to me exactly how much information it should contain. It could either be just Generalized Position, Velocity, Acceleration, and Force vectors, or it could also include information like external forces.

mxgrey added 26 commits Mar 25, 2015
@mxgrey
Copy link
Member Author

@mxgrey mxgrey commented Mar 30, 2015

The SDF Parsers have undergone a major overhaul. The public API is unchanged (except for providing shared_ptrs instead of raw pointers for Skeletons; we'll be doing the same for Worlds soon).

Considering the extent of changes that have been made, if anyone has an SDF file that they like, I'd appreciate testing it with the new parser. The atlasSimbicon app appears to be working fine, but I'd like to know if there are any edge cases that clobber the changes I've made.

@mxgrey
Copy link
Member Author

@mxgrey mxgrey commented Jun 5, 2015

I recently removed the call to Skeleton::init() from World::addSkeleton(), because in principle it should no longer be necessary to initialize a Skeleton before using it. However, running some of the apps, I've noticed that this has clobbered the controllers that depended on hard-coded indexing, because the BodyNodes tend to get rearranged after Skeleton::init(~) is called.

I think it's very undesirable that adding a Skeleton to a world runs the risk of changing the indexing of the BodyNodes and generalized coordinates in the Skeleton, so I want to keep Skeleton::init() out of World::addSkeleton(). However, this could kill backwards compatibility for any code that hardcoded their indexing. It's not a big deal to fix this for the apps that are provided natively in the DART repository, but the code of end users would be at risk. But even at that, old code is unlikely to be backwards compatible with the latest major release of DART anyway, so it seems like now would be the best time to break that compatibility.

What does everyone else think?

@mxgrey
Copy link
Member Author

@mxgrey mxgrey commented Jun 7, 2015

In my latest commits, I've created the dart::dynamics::Group class (which I had originally planned on naming Disjointment, but decided that Group would be less silly) to allow arbitrarily setting the indexing of a ReferentialSkeleton.

I've also fixed the indexing issues that arose with the atlasSimbicon app and the bipedStand app.

@mxgrey
Copy link
Member Author

@mxgrey mxgrey commented Jun 10, 2015

I've noticed something that might raise some red flags: The commit af95121 has altered the behavior of the softBodies app. In the past, the cylinder ends standing on its bottom, but ever since the change to the generalized coordinate differencing, it falls on its side.

The cylinder is in an unstable state that crucial point in the simulation, so either end result is entirely plausible, and I don't know that there's an objective way of determining which result is "correct". I suspect that once you account for things like integration error and the simplifications inherent in our friction and collision handling dynamics, either result can be considered "correct". But it might be worth considering what about that particular commit could have resulted in this quantitative change, and whether the change was something good, bad, or neutral.

worldClone->getFrame(current_parent->getName());

if(parent_candidate)
worldClone->getFrame(i)->setParentFrame(parent_candidate.get());

This comment has been minimized.

@jslee02

jslee02 Jun 10, 2015
Member

Any possibility if a SimpleFrame fails to find its parent in the cloned world? I think it shouldn't be.

This comment has been minimized.

@mxgrey

mxgrey Jun 10, 2015
Author Member

If the parent of the SimpleFrame is another SimpleFrame which has not been added to the original World, then it will not be found in the new World.

The motivation behind this is you can decide which SimpleFrames are tied to their World versus which that aren't. You can have some SimpleFrames which will be copied when their World is cloned while other SimpleFrames are used across all the World copies.

This comment has been minimized.

@jslee02

jslee02 Jun 10, 2015
Member

It makes sense. So the parent frame of a SimpleFrame is able to be in a different World from the World the SimpleFrame belongs to.

Beside on this, I came up with some idea that might be useful or not. That is _master world_. Multiple worlds can have a master world like master slide of presentation tools. The difference from cloning is all the worlds share the skeletons and simple frames in the master world rather than actually creating the instances. This might be useful if we want to simulate with different parameters while having invariant things in the master world.

This comment has been minimized.

@mxgrey

mxgrey Jun 10, 2015
Author Member

So the parent frame of a SimpleFrame is able to be in a different World from the World of the SimpleFrame.

It's more like, a SimpleFrame isn't necessarily tied to a World at all. If you decide to tie a SimpleFrame to a World, then a copy of it will be made when the World is cloned. But if you don't tie a SimpleFrame to a World, then it won't get cloned automatically.

I'm not sure I'm following how the master world concept would work, although it might be related to something that I'm thinking about for the future: lazy copying. If we were to lazily copy BodyNodes and Skeletons, then we could make it so that the properties of a copied BodyNode are just references to the properties of the original BodyNode, until the user decides to change one of the properties. At that point, the copied BodyNode will instantiate whatever it needs to, instead of just referencing the original. The implementation would be somewhat challenging, but certainly doable. The big question would be whether changing a property in the original should also carry over to the existing clones of the original.

This comment has been minimized.

@jslee02

jslee02 Jun 10, 2015
Member

It's more like, a SimpleFrame isn't necessarily tied to a World at all.

I understand it more clearly now. I exclusion the case a SimpleFrame isn't tied to a World.

lazy copying sounds interesting, and I agree with that the implementation would be somewhat challenging.

The big question would be whether changing a property in the original should also carry over to the existing clones of the original.

In my view, changing a property in the original shouldn't carry over to the clones, and they should be able to have different property values. It might be able to give a flag whether the property carry over to the clones, but keeping same property values in different world might be physically infeasible under certain circumstances. However, all the entities in the master world will always carry the same properties to all the dependent worlds. One use case I can imagine now is that we put entities for static environment to the master world and run different simulations in multiple worlds sharing the mast world. So whether carrying a property or not will be the differences between lazy copying and master world.

Edit: master world would be more meaningful if we form a set of world. WorldSet would have one master world and multiple worlds as many as user want to simulate at the same time. I guess simultaneous multiple simulation would be useful when user want to test his/her controller in the same environment but different parameters at the same time.


/// Remove an Entity from this world without deleting it
void withdrawEntity(dynamics::Entity* _entity);
std::string addFrame(dynamics::SimpleFramePtr _frame);

This comment has been minimized.

@jslee02

jslee02 Jun 10, 2015
Member

How about addSimpleFrame()? addFrame() sounds like it takes Frame or any classes inherited Frame.

This comment has been minimized.

@mxgrey

mxgrey Jun 10, 2015
Author Member

Yeah, we should probably change it to that. Originally, you were able to add any kind of Frame, but then I wanted to be able to clone the Frames whenever the World is cloned, which would be a problem if someone added BodyNode frames.

@jslee02

This comment has been minimized.

Copy link
Member

@jslee02 jslee02 commented on ef20002 Jun 10, 2015

Why did you use .impl extension for BodyNodePtr, JointPtr, DegreeOfFreedomPtr files? If it is to inform that those files shouldn't be included directly and encourage to use Ptr.h instead, how about using some processors rather than introducing new extension like:

// Ptr.h
#define DART_DYNAMICS_POINTER_IMPLEMENTATION
  #include "dart/dynamics/BodyNodePtr.h"
  #include "dart/dynamics/JointPtr.h"
  #include "dart/dynamics/DegreeOfFreedomPtr.h"
#undef DART_DYNAMICS_POINTER_IMPLEMENTATION

// BodyNodePtr.h
#ifndef DART_DYNAMICS_POINTER_IMPLEMENTATION
  #warning Do not include this file directly. Please use Ptr.h instead.
#endif  // DART_DYNAMICS_POINTER_IMPLEMENTATION

This comment has been minimized.

Copy link
Collaborator

@mkoval mkoval replied Jun 10, 2015

👍 I also thought this was odd.

Also, using the .impl extension breaks the default syntax highlighting settings of many programs. This includes Github: note how the source for BodyNodePtr.impl is not being highlighted as C++.

Boost uses the naming convention -impl.h, e.g. BodyNodePtr-impl.h, to avoid this problem. I suggest we do something similar here.

This comment has been minimized.

Copy link
Member Author

@mxgrey mxgrey replied Jun 10, 2015

It's because the .impl files aren't really headers; they're the implementations for the templated classes.

I can see how having an unusual file extension would cause problems for syntax highlighters. What if we changed the .impl to .h, but tucked them away in an impl subdirectory. BodyNodePtr.impl would become dart/dynamics/impl/BodyNodePtr.h?

This comment has been minimized.

Copy link
Collaborator

@mkoval mkoval replied Jun 10, 2015

@mxgrey It doesn't look like BodyNodePtr.h is purely an implementation of a templated class. It includes both the declaration and the inline definition.

If we are splitting the declaration from the definition, then I would expect BodyNodePtr.h to contain the declaration and BodyNodePtr-impl.h (or whatever we call it) to contain the definition. If we are not, then I would expect BodyNodePtr.h to include an inline definition of the entire class.

Also, both of these conventions differs from what we do for templated member functions, e.g. in BodyNode. Should we also move these to an -impl.h (or whatever we call it) file for consistency?

This comment has been minimized.

Copy link
Member Author

@mxgrey mxgrey replied Jun 10, 2015

I consider it just the implementation, because it only declares & defines the template. It would be useless to #include it in a translation unit without Ptr.h (unless you want to create a custom version of BodyNodePtr). The actual fully-defined BodyNodePtr (in addition to WeakBodyNodePtr, SoftBodyNodePtr, etc) is defined in Ptr.h.

I actually think that moving the implementations of the templated functions in BodyNode.h into a separate file would make a lot of sense. We could have a file dart/dynamics/impl/BodyNode.h where the functions are defined.

This comment has been minimized.

Copy link
Member

@jslee02 jslee02 replied Jun 10, 2015

Moving implementations of templated functions (and inline function we might have in the future) makes sense to me as well.

It seems like we have options like:
A. Move the implementation files to the sub directory /impl/ or similar name
B. Use name conventions -impl.h or similar suffix.

I prefer A. I think the main reason of separating implementation is to minimize the exposing API by hiding implementations from the header. Also, it prefer to use /detail/ or /internal/ because /impl/ might be confused with pimpl idiom.

This comment has been minimized.

Copy link
Collaborator

@mkoval mkoval replied Jun 10, 2015

I agree with @jslee02. It makes a lot of sense to move template function definitions into a separate header.

I slightly prefer a detail subdirectory, over internal or impl, because that is what Boost uses in this situation.

This comment has been minimized.

Copy link
Member Author

@mxgrey mxgrey replied Jun 10, 2015

@mxgrey
Copy link
Member Author

@mxgrey mxgrey commented Jun 11, 2015

I decided to go with a directory named detail, because I think that name had the most support overall.

@jslee02
Copy link
Member

@jslee02 jslee02 commented Jun 11, 2015

Could we change file name of Ptr.h to more informative like Pointer[s].h or SmartPointer[s].h?

Also, the copyright information should be updated, and I would like to add comment that shows the final smart pointer class names at least one per each macro. Here is possible revised version what I imagine:

/*
 * Copyright (c) 2015, Georgia Tech Research Corporation
 * All rights reserved.
 *
 * Author(s): Michael X. Grey <mxgrey@gatech.edu>
 *
 * Georgia Tech Graphics Lab and Humanoid Robotics Lab
 *
 * Directed by Prof. C. Karen Liu and Prof. Mike Stilman
 * <karenliu@cc.gatech.edu> <mstilman@cc.gatech.edu>
 *
 * This file is provided under the following "BSD-style" License:
 *   Redistribution and use in source and binary forms, with or
 *   without modification, are permitted provided that the following
 *   conditions are met:
 *   * Redistributions of source code must retain the above copyright
 *     notice, this list of conditions and the following disclaimer.
 *   * Redistributions in binary form must reproduce the above
 *     copyright notice, this list of conditions and the following
 *     disclaimer in the documentation and/or other materials provided
 *     with the distribution.
 *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
 *   CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
 *   INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
 *   MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
 *   DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
 *   CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
 *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
 *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
 *   USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
 *   AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
 *   LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
 *   ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 *   POSSIBILITY OF SUCH DAMAGE.
 */

#ifndef DART_DYNAMICS_PTR_H_
#define DART_DYNAMICS_PTR_H_

#include "dart/dynamics/detail/BodyNodePtr.h"
#include "dart/dynamics/detail/JointPtr.h"
#include "dart/dynamics/detail/DegreeOfFreedomPtr.h"

// This file is a lightweight means of providing the smart pointers which are
// commonly used within the dart::dynamics namespace. It is 'lightweight' in the
// sense that it does not depend on any types being fully defined, making this
// header suitable for inclusion in other headers which might only want access
// to the smart pointers without needing fully defined classes.

namespace dart {
namespace dynamics {

//==============================================================================
// -- Standard shared/weak pointers --
// Define a typedef for const and non-const version of shared_ptr and weak_ptr
// for the class X
#define DART_DYNAMICS_MAKE_SHARED_WEAK( X )                 \
  typedef std::shared_ptr< X >      X ## Ptr;               \
  typedef std::shared_ptr< const X > Const ## X ## Ptr;     \
  typedef std::weak_ptr< X >        Weak ## X ## Ptr;       \
  typedef std::weak_ptr< const X >   WeakConst ## X ## Ptr;

// Skeleton smart pointers
class Skeleton;
DART_DYNAMICS_MAKE_SHARED_WEAK(Skeleton)
// These pointers will take the form of:
// std::shared_ptr<Skeleton>        --> SkeletonPtr
// std::shared_ptr<const Skeleton>  --> ConstSkeletonPtr
// std::weak_ptr<Skeleton>          --> WeakSkeletonPtr
// std::weak_ptr<const Skeleton>    --> WeakConstSkeletonPtr

// MetaSkeleton smart pointers
class MetaSkeleton;
DART_DYNAMICS_MAKE_SHARED_WEAK(MetaSkeleton)

// ReferentialSkeleton smart pointers
class ReferentialSkeleton;
DART_DYNAMICS_MAKE_SHARED_WEAK(ReferentialSkeleton)

class Group;
DART_DYNAMICS_MAKE_SHARED_WEAK(Group)

class Linkage;
DART_DYNAMICS_MAKE_SHARED_WEAK(Linkage)

class Branch;
DART_DYNAMICS_MAKE_SHARED_WEAK(Branch)

class Chain;
DART_DYNAMICS_MAKE_SHARED_WEAK(Chain)

// Shape smart pointers
class Shape;
DART_DYNAMICS_MAKE_SHARED_WEAK(Shape)


//==============================================================================
// -- Custom BodyNode smart pointers --
#define DART_DYNAMICS_MAKE_BODYNODEPTR( X )                         \
  typedef TemplateBodyNodePtr< X >          X ## Ptr;               \
  typedef TemplateBodyNodePtr< const X >     Const ## X ## Ptr;     \
  typedef TemplateWeakBodyNodePtr< X >      Weak ## X ## Ptr;       \
  typedef TemplateWeakBodyNodePtr< const X > WeakConst ## X ## Ptr;

// BodyNode smart pointers
class BodyNode;
DART_DYNAMICS_MAKE_BODYNODEPTR(BodyNode)
// These pointers will take the form of:
// TemplateBodyNodePtr<BodyNode>            --> BodyNodePtr
// TemplateBodyNodePtr<const BodyNode>      --> ConstBodyNodePtr
// TemplateWeakBodyNodePtr<BodyNode>        --> WeakBodyNodePtr
// TemplateWeakBodyNodePtr<const BodyNode>  --> WeakConstBodyNodePtr

// SoftBodyNode smart pointers
class SoftBodyNode;
DART_DYNAMICS_MAKE_BODYNODEPTR(SoftBodyNode)


//==============================================================================
// -- BodyNode dependent smart pointers --
#define DART_DYNAMICS_MAKE_BN_DEPENDENT_PTR( X )                                          \
  typedef Template ## X ## Ptr < X , BodyNode >                   X ## Ptr;               \
  typedef Template ## X ## Ptr < const X , const BodyNode >       Const ## X ## Ptr;      \
  typedef TemplateWeak ## X ## Ptr < X , BodyNode >               Weak ## X ## Ptr;       \
  typedef TemplateWeak ## X ## Ptr < const X , const BodyNode >   WeakConst ## X ## Ptr;

// Joint smart pointers
class Joint;
DART_DYNAMICS_MAKE_BN_DEPENDENT_PTR(Joint)
// These pointers will take the form of:
// TemplateJointPtr<Joint>            --> JointPtr
// TemplateJointPtr<const Joint>      --> ConstJointPtr
// TemplateWeakJointPtr<Joint>        --> WeakJointPtr
// TemplateWeakJointPtr<const Joint>  --> WeakConstJointPtr

// DegreeOfFreedom smart pointers
class DegreeOfFreedom;
DART_DYNAMICS_MAKE_BN_DEPENDENT_PTR(DegreeOfFreedom)

} // namespace dynamics
} // namespace dart

#endif // DART_DYNAMICS_PTR_H_
@jslee02
Copy link
Member

@jslee02 jslee02 commented Jun 11, 2015

Beside on Ptr.h, 👍 .

There are still few issues but let's make them as separate issues or pull requests. I think the main purpose of this PR is already fulfilled.

std::cout << skel->getBodyNode(j)->getSpatialInertia() << "\n" << std::endl;
}
}

This comment has been minimized.

@jslee02

jslee02 Jun 11, 2015
Member

Is this for debug?

This comment has been minimized.

@mxgrey

mxgrey Jun 11, 2015
Author Member

Hahah, I would guess so. Probably from when I created the Spatial Inertia class. I'll remove it.

This comment has been minimized.

@jslee02

jslee02 Jun 11, 2015
Member

Thanks. I do this a lot. :)

mxgrey and others added 2 commits Jun 11, 2015
@jslee02 jslee02 closed this Jun 12, 2015
@jslee02 jslee02 reopened this Jun 12, 2015
jslee02 added a commit that referenced this pull request Jun 12, 2015
Eradicating memory leaks & making classes copy-safe and clonable
@jslee02 jslee02 merged commit 4eb7b1c into master Jun 12, 2015
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jslee02 jslee02 deleted the memory_management branch Jun 26, 2015
@jslee02 jslee02 mentioned this pull request Jul 3, 2015
4 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.