Add Factory class and collision detector factory #864
Conversation
jslee02
changed the title
There was a problem hiding this comment.
This generally looks good. I am very positive on the idea of having a Factory for instantiating optional parts of DART, like collision detectors. Moving forward, I would love to see this expand to the other parts of DART that have optional dependencies (e.g. FCL, ODE, Skeleton loaders).
I am a bit surprised at how complicated the implementation of Factory is. I don't have a better suggestion - other than the few minor comments I posted on this pull request - so I can't really complain too much. 😬
Great work! 🎉
dart/common/Factory.hpp
Outdated
| }; | ||
|
|
||
| /// The default creator. Specialize this template class for smart pointer types | ||
| /// other than std::unique_ptr and std::shared_ptr. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
DefaultCreator is now able to take arbitrary arguments.
dart/common/Factory.hpp
Outdated
| { | ||
| static SmartPointerT<T> run() | ||
| { | ||
| return SmartPointerT<T>(new T()); |
There was a problem hiding this comment.
Nit: In my opinion, new T is more idiomatic than new T().
dart/common/Factory.hpp
Outdated
| public: | ||
| using FactoryType = Factory<KeyT, BaseT, SmartPointerT>; | ||
| using Creator = typename FactoryType::Creator; | ||
| using This = FactoryRegistrar<KeyT, BaseT, DerivedT, SmartPointerT>; |
There was a problem hiding this comment.
Nit: I'd prefer decltype(*this) here to be more DRY. See above.
There was a problem hiding this comment.
I believe this cannot be used here (see above).
dart/common/Factory.hpp
Outdated
| -> decltype(DefaultCreator<DerivedT, SmartPointerT>::run()) | ||
| { | ||
| return DefaultCreator<DerivedT, SmartPointerT>::run(); | ||
| }); |
There was a problem hiding this comment.
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? 😅
dart/common/Factory.hpp
Outdated
|
|
||
| /// Constructor. Interanlly, this constructor registers Derived class with | ||
| /// the key and the default creator function. | ||
| FactoryRegistrar(const KeyT& key); |
There was a problem hiding this comment.
Nit: Mark this as explicit.
dart/common/detail/Factory-impl.hpp
Outdated
| // TODO(JS): Print the key if the << operator is defined. | ||
| } | ||
|
|
||
| return result; |
There was a problem hiding this comment.
Returning result, which contains a mutable iterator into getMap() is iffy here.
dart/common/detail/Factory-impl.hpp
Outdated
| Factory<KeyT, BaseT, SmartPointerT, Args...>::registerCreator( | ||
| const KeyT& key, Creator creator) | ||
| { | ||
| const auto result = getMap().insert(std::make_pair(key, creator)); |
There was a problem hiding this comment.
Nits:
- Take
keyby value andstd::moveit into theunordered_maphere. - Take
creatorby value andstd::moveit into theunordered_maphere. - Use
emplaceinstead ofinsertto avoid a copy. This may require the addition of astd::piecewise_constructtype tag argument.
dart/common/detail/Factory-impl.hpp
Outdated
| typename Factory<KeyT, BaseT, SmartPointerT, Args...>::CreatorMap& | ||
| Factory<KeyT, BaseT, SmartPointerT, Args...>::getMap() | ||
| { | ||
| static CreatorMap map; |
There was a problem hiding this comment.
What's the motivation behind making this a static variable inside a function instead of a static member variable? Does this have something to do with the static initialization order fiasco?
dart/utils/SkelParser.cpp
Outdated
| } | ||
|
|
||
| if (!collision_detector) | ||
| { | ||
| collision_detector = collision::FCLCollisionDetector::create(); | ||
| collision_detector = collision::CollisionDetector::Factory::create("fcl"); | ||
| auto cd = std::static_pointer_cast<collision::FCLCollisionDetector>( | ||
| collision_detector); | ||
| cd->setPrimitiveShapeType(collision::FCLCollisionDetector::MESH); |
There was a problem hiding this comment.
Can we put the code to create the fcl_mesh collision detector into a helper function? I am worried that the two versions of this code (i.e. the one in the `cdType == "fcl_mesh" block and in the default block) will become out of sync.
unittests/unit/test_Factory.cpp
Outdated
| { | ||
| return "Seattle"; | ||
| } | ||
| }; |
There was a problem hiding this comment.
I like your choice of city names. 😉
|
@mkoval I believe all your comments are addressed. Could you do second pass? |
This PR adds
Factory, which is an implementation of the Abstract Factory Pattern, and applies it to creating collision detectors from the key ofstd::stringtype.By doing so, it decouples the dependencies of bullet collision detector from
SkelParser. Previously, the bullet collision detector type was hard-coded inSkelParserso thatdart-collision-bullethad to be one of the dependencies ofdart-utils. Now the optional collision detector (only bullet collision detector for now) can be created through the factory whendart-collision-bulletis linked.