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

Fix not copying constraints in setting new constraint solver #1260

Merged
merged 5 commits into from
Mar 13, 2019

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented Mar 11, 2019

Motivation

World::setConstraint() doesn't copy constraints.

Solution

Introduce ConstraintSolver::setFromOtherConstraintSolver() to properly copy skeletons and contraints from existing constraint solver. This is to let ConstraintSolver handle the copy instead of the caller (e.g., World).


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.8.0 milestone Mar 11, 2019
@jslee02 jslee02 requested a review from mxgrey March 11, 2019 10:49
@jslee02 jslee02 marked this pull request as ready for review March 11, 2019 10:50
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.

I have one comment about return values, but also:

Do we have any strategy for copying constraints over when cloning a world? It looks like we're not handling that currently. Since this PR is targeted at fixing a specific downstream issue for Gazebo, we can punt on the cloning concern, but it may be worth opening an issue about it if we're not currently cloning constraints.

@@ -89,6 +89,12 @@ class ConstraintSolver
/// Add a constraint
void addConstraint(const ConstraintBasePtr& constraint);

/// Returns all the constraints added to this ConstraintSolver.
const std::vector<constraint::ConstraintBasePtr>& getConstraints();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm not thinking about this the right way, but shouldn't the non-const overload return a non-const reference to the vector while the const overload returns a const reference to the vector (instead of a copy of the vector)? Creating a copy should only be done if:

  1. The underlying implementation does not contain a vector object that can be directly referenced
  2. The API does not want to expose the implementation detail that the constraint pointers are held in a vector, in case we want to change this implementation in the future

Condition (1) doesn't apply here, and if we want to satisfy condition (2) then both the const and non-const overloads should both return a copy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I think we should go with (2) since letting the caller to modify the list of constraints would be risky. Instead, we should enforce them to use the constraint modifier functions (add/removeConstraint).

The motivation this non-const overload returns const reference was to provide a way to iteratate the constraints or get the number of them. For this, it'd be good to provide getNumConstraint() and getConstraint(index).

@jslee02
Copy link
Member Author

jslee02 commented Mar 12, 2019

Do we have any strategy for copying constraints over when cloning a world? It looks like we're not handling that currently. Since this PR is targeted at fixing a specific downstream issue for Gazebo, we can punt on the cloning concern, but it may be worth opening an issue about it if we're not currently cloning constraints.

Good point! Let me fix that in a separate PR.

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