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 World::hasSkeleton() #1050

Merged
merged 3 commits into from
Apr 6, 2018
Merged

Add World::hasSkeleton() #1050

merged 3 commits into from
Apr 6, 2018

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented Apr 5, 2018

World::hasSkeleton takes Skeleton as a shared pointer since Skeleton is enforced to be created as a shared pointer by calling Skeleton::create().


Before creating a pull request

  • Document new methods and classes
  • Format new code files using clang-format

Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md
  • Add unit test(s) for this change

@jslee02 jslee02 added this to the DART 6.5.0 milestone Apr 5, 2018
@jslee02 jslee02 requested a review from mxgrey April 5, 2018 22:36
Copy link
Member

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

Just a missing const

@@ -149,6 +149,9 @@ class World : public virtual common::Subject
/// pointers to them, in case you want to recycle them
std::set<dynamics::SkeletonPtr> removeAllSkeletons();

/// Returns wether this World contains a Skeleton.
bool hasSkeleton(const dynamics::SkeletonPtr& skeleton);
Copy link
Member

Choose a reason for hiding this comment

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

This member function should be const-qualified.

Copy link
Member

@mxgrey mxgrey Apr 5, 2018

Choose a reason for hiding this comment

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

Also, if possible, it would be nice to accept a const dynamics::ConstSkeletonPtr&, but I know that sometimes the STL doesn't like to perform comparisons between std::shared_ptr<const T> and std::shared_ptr<T> objects, so maybe it would be more trouble than it's worth.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, would it be better to take just a const raw pointer as a workaround? Or having both for SkeletonPtr and ConstSkeletonPtr?

Copy link
Member

Choose a reason for hiding this comment

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

The compiler can implicitly cast a SkeletonPtr to a ConstSkeletonPtr, no problem. I'm just not sure off the top of my head if

return std::find(mSkeletons.begin(), mSkeletons.end(), skeleton)
      != mSkeletons.end();

will compile when skeleton is a ConstSkeletonPtr, since the compiler might want to cast skeleton from ConstSkeletonPtr to SkeletonPtr, which is not allowed.

I think if the compiler gives us any trouble with the ConstSkeletonPtr argument, we could use std::find_if and pass in our own unary predicate that casts each entry to a ConstSkeletonPtr before comparing it to skeleton.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems ConstSkeletonPtr works without any troubles in the simple test. Let's merge this as it is and revisit if it turns out this is problematic. 😁

@codecov
Copy link

codecov bot commented Apr 6, 2018

Codecov Report

Merging #1050 into release-6.5 will increase coverage by <.01%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##           release-6.5    #1050      +/-   ##
===============================================
+ Coverage        56.58%   56.59%   +<.01%     
===============================================
  Files              314      314              
  Lines            24312    24315       +3     
===============================================
+ Hits             13758    13761       +3     
  Misses           10554    10554
Impacted Files Coverage Δ
dart/simulation/World.cpp 72.1% <100%> (+0.3%) ⬆️

Copy link
Member

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

As long as it's compiling, there won't be any issues. I just wasn't sure how the compiler would handle the casting.

@jslee02
Copy link
Member Author

jslee02 commented Apr 6, 2018

Great! Thanks!

@jslee02 jslee02 merged commit a93d7e0 into release-6.5 Apr 6, 2018
@jslee02 jslee02 deleted the enhance/has_skeleton branch April 6, 2018 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants