Fix WorldInteractiveMarkerViewer bug when removing Skeletons #252
Conversation
name. Use the actual Skeleton as the key in WorldInteractiveMarkerViewer, rather than the Skeleton's name.
Codecov Report
@@ Coverage Diff @@
## master #252 +/- ##
=======================================
Coverage 70.43% 70.43%
=======================================
Files 181 181
Lines 5391 5391
Branches 847 847
=======================================
Hits 3797 3797
Misses 1074 1074
Partials 520 520
|
include/aikido/planner/World.hpp
Outdated
| @@ -39,6 +39,11 @@ class World | |||
| /// \param name Name of desired Skeleton | |||
| dart::dynamics::SkeletonPtr getSkeleton(const std::string& name) const; | |||
|
|
|||
| /// Find a Skeleton by Skeleton, returning null if it is not in the World | |||
| /// \param skel Desired Skeleton | |||
| dart::dynamics::SkeletonPtr getSkeleton( | |||
There was a problem hiding this comment.
This function sounds like checking whether the skeleton exists in (registered to) this world. I would like to suggest changing the function name containsSkeleton(...) or existsSkeleton(...) (or skeletonExists) (we conventionally suffix s to the leading verb for an inquiry function) like NewtonsMethodProjectable::contains and CatkinResourceRetriever::exists(...) and returning bool.
I personally prefer contains over exists because contains would sound more natural when we use the function like:
if (world.containsSkeleton(someSkeleton))
...
if (world.existsSkeleton(someSkeleton)) // or if(world.skeletonExists(...))
...
include/aikido/planner/World.hpp
Outdated
| /// Find a Skeleton by Skeleton, returning null if it is not in the World | ||
| /// \param skel Desired Skeleton | ||
| dart::dynamics::SkeletonPtr getSkeleton( | ||
| const dart::dynamics::SkeletonPtr skel) const; |
There was a problem hiding this comment.
Nit: Let's use const dart::dynamics::SkeletonPtr& to avoid unnecessary increase of the reference count by copying std::shared_ptr<...>.
There was a problem hiding this comment.
I don't think we can use std::weak_ptr here because it is not comparable or hashable. This makes sense because an unlocked weak_ptr can spontaneously degrade to nullptr if all owning shared_ptrs are destructed.
We could using a const Skeleton* as the key. But we'd need to be careful to not access the pointer after the Skeleton is destructed, e.g. by lock()ing a std::weak_ptr that we store inside the marker instance.
| /// Mapping of Skeleton names to SkeletonMarkers | ||
| std::map<std::string, SkeletonMarkerPtr> mSkeletonMarkers; | ||
| /// Mapping of Skeletons to SkeletonMarkers | ||
| std::map<dart::dynamics::SkeletonPtr, SkeletonMarkerPtr> mSkeletonMarkers; |
There was a problem hiding this comment.
Although this is a stopgap solution (over using callbacks), we might want to use std::weak_pointer<...> here because this class doesn't need to have the ownership as it just uses the skeletons only when they still exist in mWorld.
|
Good catch! I made some suggestions. |
include/aikido/planner/World.hpp
Outdated
| /// Find a Skeleton by Skeleton, returning null if it is not in the World | ||
| /// \param skel Desired Skeleton | ||
| dart::dynamics::SkeletonPtr getSkeleton( | ||
| const dart::dynamics::SkeletonPtr skel) const; |
There was a problem hiding this comment.
I don't think we can use std::weak_ptr here because it is not comparable or hashable. This makes sense because an unlocked weak_ptr can spontaneously degrade to nullptr if all owning shared_ptrs are destructed.
We could using a const Skeleton* as the key. But we'd need to be careful to not access the pointer after the Skeleton is destructed, e.g. by lock()ing a std::weak_ptr that we store inside the marker instance.
|
I changed the code according to the suggestions. @brianhou Could you take a look at the change for sure? |
* Fix bug from removing Skeletons, then adding new Skeletons with the same name. Use the actual Skeleton as the key in WorldInteractiveMarkerViewer, rather than the Skeleton's name. * Rename getSkeleton(skeleton) to hasSkeleton(skeleton) * Update changelog * Use hasSkeleton() instead of getSkeleton()
If a
Skeletonis removed and a newSkeletonwith the same name is added quickly enough (before the viewer refreshes and removes the originalSkeleton), the newSkeletonis not visualized. This PR fixes this bug by using the actualSkeletonPtras the key inWorldInteractiveMarkerViewer, rather than its name.At some point, we could fix this by registering callbacks to
Worldthat are called when adding or removingSkeletons.Before creating a pull request
make formatBefore merging a pull request
CHANGELOG.md