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

Add Factory class and collision detector factory #864

Merged
merged 23 commits into from
Apr 22, 2017
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
e2eb437
Add Factory class
jslee02 Mar 29, 2017
6b1a725
Add registerCreator() for default creator function
jslee02 Mar 29, 2017
679a953
Remove FactoryScopedRegister
jslee02 Mar 29, 2017
c202407
Fix style
jslee02 Mar 29, 2017
9b148c8
Add example code of Factory
jslee02 Mar 29, 2017
5e44e81
Add minor comment to test_Factory.cpp
jslee02 Mar 29, 2017
1c1928d
Templatize Factor for smart pointer types
jslee02 Mar 29, 2017
7db6647
Register collision detectors to the factory
jslee02 Mar 29, 2017
ad5ce8a
Drop dependency of dart-collision-bullet from dart-utils
jslee02 Mar 30, 2017
5fdd74d
Allow to create objects with arguments
jslee02 Mar 30, 2017
972c269
Minor changes
jslee02 Mar 30, 2017
31d57eb
Use unordered_map instead of map
jslee02 Mar 30, 2017
edf90c8
Merge remote-tracking branch 'origin/master' into feature/collision_d…
jslee02 Mar 30, 2017
e5568af
Merge branch 'master' into feature/collision_detector_factory
jslee02 Mar 31, 2017
1a10dbd
Merge branch 'master' into feature/collision_detector_factory
jslee02 Apr 19, 2017
19a20a2
Simplifying Factory and FactoryRegistrar
jslee02 Apr 20, 2017
35032e8
Register OdeCollisionDetector to the collision factory
jslee02 Apr 20, 2017
b19b40f
Remove unused function
jslee02 Apr 20, 2017
923442a
Fix error in SkelParser.cpp
jslee02 Apr 20, 2017
662e726
Move singleton implementation to detail
jslee02 Apr 20, 2017
52f3cf9
Resolve that std::hash doesn't support enum
jslee02 Apr 20, 2017
f030e95
Move default creator into detail
jslee02 Apr 20, 2017
e25800b
Fix test_Collision.cpp
jslee02 Apr 20, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -309,8 +309,14 @@ if(TINYXML_FOUND AND TINYXML2_FOUND AND BULLET_FOUND)
endif(DART_VERBOSE)

# Add tutorial targets
dart_add_tutorial(tutorials/tutorialBiped dart-utils-urdf dart-gui)
dart_add_tutorial(tutorials/tutorialBiped-Finished dart-utils-urdf dart-gui)
if(HAVE_BULLET_COLLISION)
dart_add_tutorial(tutorials/tutorialBiped dart-utils-urdf dart-gui dart-collision-bullet)
dart_add_tutorial(tutorials/tutorialBiped-Finished dart-utils-urdf dart-gui dart-collision-bullet)
else()
dart_add_tutorial(tutorials/tutorialBiped dart-utils-urdf dart-gui)
dart_add_tutorial(tutorials/tutorialBiped-Finished dart-utils-urdf dart-gui)
endif()

dart_add_tutorial(tutorials/tutorialCollisions dart-utils-urdf dart-gui)
dart_add_tutorial(tutorials/tutorialCollisions-Finished dart-utils-urdf dart-gui)
dart_add_tutorial(tutorials/tutorialDominoes dart-utils-urdf dart-gui)
Expand Down
6 changes: 3 additions & 3 deletions dart/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ install(FILES ${DART_CONFIG_HPP_OUT} DESTINATION include/dart)
# dart-optimizer-ipopt - {dart}, optimizer/ipopt, [ipopt]
# dart-optimizer-nlopt - {dart}, optimizer/nlopt, [nlopt]
# dart-collision-bullet - {dart}, collision/bullet, [bullet]
# dart-utils - {dart-collision-bullet}, utils, utils/sdf, [tinyxml, tinyxml2]
# dart-utils - {dart}, utils, utils/sdf, [tinyxml, tinyxml2]
# dart-utils-urdf - {dart-utils}, utils/urdf, [urdfdom]
# dart-gui - {dart}, gui, [opengl, glut]
# dart-gui-osg - {dart-gui}, gui/osg, gui/osg/render, [openscenegraph]
Expand Down Expand Up @@ -97,11 +97,11 @@ if(FLANN_FOUND)
add_subdirectory(planning)
endif()

if(TINYXML_FOUND AND TINYXML2_FOUND AND BULLET_FOUND)
if(TINYXML_FOUND AND TINYXML2_FOUND)
add_subdirectory(utils)
endif()

if(OPENGL_FOUND AND GLUT_FOUND AND BULLET_FOUND)
if(OPENGL_FOUND AND GLUT_FOUND)
add_subdirectory(gui)
endif()

Expand Down
4 changes: 4 additions & 0 deletions dart/collision/CollisionDetector.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@

#include <Eigen/Dense>

#include "dart/common/Factory.hpp"
#include "dart/collision/Contact.hpp"
#include "dart/collision/CollisionOption.hpp"
#include "dart/collision/CollisionResult.hpp"
Expand Down Expand Up @@ -256,6 +257,9 @@ class CollisionDetector::ManagerForSharableCollisionObjects final :

};

using CollisionDetectorFactory
= common::Factory<std::string, CollisionDetector, std::shared_ptr>;

} // namespace collision
} // namespace dart

Expand Down
11 changes: 11 additions & 0 deletions dart/collision/bullet/BulletCollisionDetector.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -130,4 +130,15 @@ class BulletCollisionDetector : public CollisionDetector
} // namespace collision
} // namespace dart

// Register Bullet collision detector
DART_REGISTER_CREATOR_TO_FACTORY(
std::string,
dart::collision::BulletCollisionDetector::getStaticType(),
dart::collision::CollisionDetector,
dart::collision::BulletCollisionDetector,
[]() -> std::shared_ptr<dart::collision::BulletCollisionDetector> {
return dart::collision::BulletCollisionDetector::create();
}
)

#endif // DART_COLLISION_BULLET_BULLETCOLLISIONDETECTOR_HPP_
11 changes: 11 additions & 0 deletions dart/collision/dart/DARTCollisionDetector.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,15 @@ class DARTCollisionDetector : public CollisionDetector
} // namespace collision
} // namespace dart

// Register DART collision detector
DART_REGISTER_CREATOR_TO_FACTORY(
std::string,
dart::collision::DARTCollisionDetector::getStaticType(),
dart::collision::CollisionDetector,
dart::collision::DARTCollisionDetector,
[]() -> std::shared_ptr<dart::collision::DARTCollisionDetector> {
return dart::collision::DARTCollisionDetector::create();
}
)

#endif // DART_COLLISION_DART_DARTCOLLISIONDETECTOR_HPP_
11 changes: 11 additions & 0 deletions dart/collision/fcl/FCLCollisionDetector.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -196,4 +196,15 @@ class FCLCollisionDetector : public CollisionDetector
} // namespace collision
} // namespace dart

// Register FCL collision detector
DART_REGISTER_CREATOR_TO_FACTORY(
std::string,
dart::collision::FCLCollisionDetector::getStaticType(),
dart::collision::CollisionDetector,
dart::collision::FCLCollisionDetector,
[]() -> std::shared_ptr<dart::collision::FCLCollisionDetector> {
return dart::collision::FCLCollisionDetector::create();
}
)

#endif // DART_COLLISION_FCL_FCLCOLLISIONDETECTOR_HPP_
191 changes: 191 additions & 0 deletions dart/common/Factory.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
/*
* Copyright (c) 2017, Graphics Lab, Georgia Tech Research Corporation
* Copyright (c) 2017, Personal Robotics Lab, Carnegie Mellon University
* All rights reserved.
*
* 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_COMMON_FACTORY_HPP_
#define DART_COMMON_FACTORY_HPP_

#include <functional>
#include <map>
#include <memory>

#include "dart/common/StlHelpers.hpp"

namespace dart {
namespace common {

/// Implementation of the Abstract Factory Pattern.
///
/// Factory class is a pure static class (i.e., no need to create an instance to
/// use this class).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to not make Factory a pure static class. There are many applications of the abstract factory pattern that require multiple instances of the same factory class, e.g. because their constructor requires arguments. We could still define a helper SingletonFactory<TFactory> class to provide a pure static wrapper for a Factory class.

///
/// Example:
/// \code
/// using CdFactory = Factory<std::string, CollisionDetector>;
///
/// CdFactory::registerCreator<FclCollisionDetector>("fcl");
/// auto fclCd = CdFactory::create("fcl");
/// \endcode
template <typename KeyT,
typename BaseT,
template<typename...> class SmartPointerT = std::shared_ptr>
class Factory final
{
public:
using ReturnType = SmartPointerT<BaseT>;
using Creator = std::function<ReturnType()>;
using CreatorMap = std::map<KeyT, Creator>;
using This = Factory<KeyT, BaseT, SmartPointerT>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Could we replace this with decltype(*this) to be more DRY?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wouldn't work since we cannot use this in type aliasing.

using RegisterResult = std::pair<typename CreatorMap::iterator, bool>;

/// Registers a object creator function with a key.
static RegisterResult registerCreator(const KeyT& key, Creator creator);

/// Registers the default object creator function with a key.
template <typename Derived>
static RegisterResult registerCreator(const KeyT& key);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's odd to return a mutable iterator into the private CreatorMap instance here. I suggest making this function return void or bool instead. As an added note - why does the first call to registerCreator win? I would expect a later call to win, i.e. use operator[] instead of insert() to perform the insert operation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a legacy code that was used to support ScopedFactoryRegistrar, which is removed. Will remove the return type as well.

As an added note - why does the first call to registerCreator win? I would expect a later call to win, i.e. use operator[] instead of insert() to perform the insert operation.

👍


/// Unregisters the object creator function that is registered with a key. Do
/// nothing if there is no creator function associated with the key.
static void unregisterCreator(const KeyT& key);

/// Unregisters all the object creator functions.
static void unregisterAllCreators();

/// Returns true if an object creator function is registered with the key.
/// Otherwise, returns false.
static bool canCreate(const KeyT& key);

/// Creates an object of the class that is registered with a key. Returns
/// nullptr if there is no object creator function associated with the key.
static ReturnType create(const KeyT& key);
// TODO(JS): Add create() for creating smart_pointers
// (see: https://github.com/dartsim/dart/pull/845)

protected:
/// Constructor is disabled. This class is a pure static class.
Factory() = delete;

/// Destructor is disabled. This class is a pure static class.
~Factory() = delete;

private:
/// Returns the object creator function map. A static map is created when this
/// function is firstly called.
static CreatorMap& getMap();
};

/// The default creator. Specialize this template class for smart pointer types
/// other than std::unique_ptr and std::shared_ptr.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document the fact that DefaultCreator requires: (1) T to have a no-arg constructor and (2) SmartPointer<T> to have a single argument constructor that accepts a T * argument.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DefaultCreator is now able to take arbitrary arguments.

template <typename T, template<typename...> class SmartPointerT>
struct DefaultCreator
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class also could accept SmartPointerT<T> directly as a template argument, eliminating the need for a template template argument. See above.

{
static SmartPointerT<T> run()
{
return SmartPointerT<T>(new T());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: In my opinion, new T is more idiomatic than new T().

}
};

/// Helper class to register a object creator function.
template <typename KeyT,
typename BaseT,
typename DerivedT,
template<typename...> class SmartPointerT = std::shared_ptr>
class FactoryRegistrar final
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to accept SmartPointerT<BaseT> directly and rename this to HeldT. See above.

{
public:
using FactoryType = Factory<KeyT, BaseT, SmartPointerT>;
using Creator = typename FactoryType::Creator;
using This = FactoryRegistrar<KeyT, BaseT, DerivedT, SmartPointerT>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd prefer decltype(*this) here to be more DRY. See above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this cannot be used here (see above).


/// Returns the static instance of FactoryRegistrar.
static This& getInstance(
const KeyT& key,
Creator creator = []()
-> decltype(DefaultCreator<DerivedT, SmartPointerT>::run())
{
return DefaultCreator<DerivedT, SmartPointerT>::run();
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a lot of trouble parsing the fact that this is a default argument for an argument, not the function body. 😱 I am not sure how to modify the formatting to make this more clear. Maybe add a comment? 😅


private:
/// Constructor. Interanlly, this constructor registers Derived class with
/// the key and the default creator function.
FactoryRegistrar(const KeyT& key, Creator creator);

/// Constructor. Interanlly, this constructor registers Derived class with
/// the key and the default creator function.
FactoryRegistrar(const KeyT& key);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Mark this as explicit.


/// Constructor is disabled. This class is a pure static class.
FactoryRegistrar(const This&) = delete;

/// Destructor is disabled. This class is a pure static class.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment refers to the destructor, but is above the declaration of an assignment operator.

FactoryRegistrar& operator=(const This&) = delete;
};

#define DART_CONCATENATE(x, y) x##y
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this macro to StlHelpers.hpp.


#define DART_GEN_UNIQUE_NAME_DETAIL(x, unique_key) \
DART_CONCATENATE(x, unique_key)

#define DART_GEN_UNIQUE_NAME(seed_name) \
DART_GEN_UNIQUE_NAME_DETAIL(seed_name, __LINE__)

#define DART_REGISTER_CREATOR_TO_FACTORY( \
key_type, key, base, derived, creator) \
namespace { \
const auto& DART_GEN_UNIQUE_NAME(unique_name_seed) \
= dart::common::FactoryRegistrar<key_type, base, derived> \
::getInstance(key, creator); \
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could eliminate the need for unique_name_seed using a template class. Here's my idea:

template <class FactoryRegistrarT>
struct register_creator_to_factory {
  static const auto& instance = FactoryRegistrarT::getInstance(key, creator);
}

#define DART_REGISTER_CREATOR_TO_FACTORY(key_type, key, base, derived, creator, smart_ptr_type) \
struct register_creator_to_factory<dart::common::FactoryRegistrar<key_type, base, derived, smart_ptr_type>>;

I am not sure if this will work. Maybe @mxgrey will know? 😅


#define DART_REGISTER_DEFAULT_CREATOR_TO_FACTORY( \
key_type, key, base, derived, smart_ptr_type) \
namespace { \
const auto& DART_GEN_UNIQUE_NAME(unique_name_seed) \
= dart::common::FactoryRegistrar<key_type, base, derived> \
::getInstance(key); \
}

#define DART_REGISTER_DEFAULT_UNIQUE_PTR_CREATOR_TO_FACTORY( \
key_type, key, base, derived) \
DART_REGISTER_DEFAULT_CREATOR_TO_FACTORY( \
key_type, key, base, derived, std::unique_ptr)

#define DART_REGISTER_DEFAULT_SHARED_PTR_CREATOR_TO_FACTORY( \
key_type, key, base, derived) \
DART_REGISTER_DEFAULT_CREATOR_TO_FACTORY( \
key_type, key, base, derived, std::shared_ptr)

} // namespace common
} // namespace dart

#include "dart/common/detail/Factory-impl.hpp"

#endif // DART_COMMON_FACTORY_HPP_
Loading