Add methods for removing groups in NonColliding. #247
Conversation
Codecov Report
@@ Coverage Diff @@
## master #247 +/- ##
==========================================
+ Coverage 70.9% 70.92% +0.01%
==========================================
Files 173 173
Lines 5118 5118
Branches 809 809
==========================================
+ Hits 3629 3630 +1
+ Misses 1001 1000 -1
Partials 488 488
|
CHANGELOG.md
Outdated
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ### 0.2.0 (201X-XX-XX) | |||
|
|
|||
| * Constraint | |||
|
|
|||
| * Add methods for removing groups from NonColliding constraints. [#247](https://github.com/personalrobotics/aikido/pull/247) | |||
There was a problem hiding this comment.
Nit: The current convention is to use past tense for the leading verb.
| @@ -50,10 +50,21 @@ class NonColliding : public Testable | |||
| std::shared_ptr<dart::collision::CollisionGroup> _group1, | |||
| std::shared_ptr<dart::collision::CollisionGroup> _group2); | |||
|
|
|||
| /// Remove collision check between group1 and group2. | |||
| /// \param group1 First collision group. | |||
There was a problem hiding this comment.
Nit: The parameter names in this docstring are not consistent with the actual parameter names. I believe the convention is not to use a leading underscore for parameter names, but it seems this file doesn't comply. Let's fix this later as wholesale.
src/constraint/NonColliding.cpp
Outdated
| return; | ||
| } | ||
|
|
||
| // Check for reverse pair |
There was a problem hiding this comment.
I think we could remove this double checking by storing the groups in a pair (e.g., ascending or descending in the pointer addresses). For this, we should:
- change
addPairwiseCheck(...)to add group pair in ascending order something like:
if (_group1 < _group2)
groupsToPairwiseCheck.emplace_back(std::move(_group1), std::move(_group2));
else
groupsToPairwiseCheck.emplace_back(std::move(_group2), std::move(_group1));- change
removePairwiseCheck(...)to find the ascending-ordered group pair
More importantly, I think we could use std::unordered_set instead of std::vector for better performance in adding and removing because we don't need random access, and the order of pairs is not important. In case we switch to std::unordered_set we might directly use std::unordered_set::erase without finding the pair.
src/constraint/NonColliding.cpp
Outdated
| auto it | ||
| = std::find(groupsToSelfCheck.begin(), groupsToSelfCheck.end(), _group); | ||
| if (it != groupsToSelfCheck.end()) | ||
| groupsToSelfCheck.erase(it); |
There was a problem hiding this comment.
I think groupsToSelfCheck also can be std::unordered_set for the same reason with groupsToPairwiseCheck.
Before creating a pull request
make formatBefore merging a pull request
CHANGELOG.md