Const changes to support new VFP Planner API refactor. #429

sniyaz · 2018-05-18T01:34:51Z

This PR has const changes to support the upcoming VFP refactor onto the new Planner API. #426 is currently blocking on this PR. Once this PR has been merged, I will rebase #426, and we can tackle that PR.

Because the new Problem derivate classes from the new Planner API (#314) make most members const, this requires shoving a bunch of const variables through AIKIDO (we probably don't want to cast the const-ness away, since this would break a promise to the user).

These changes are necessary to make planToEndEffectorOffset possible with the new Planner API. planToEndEffectorPose may require more changes- my plan is to get planToEndEffectorOffset refactored first, then tackle planToEndEffectorPose.

Summary of changes

Const all the things.
Update old planToEndEffectorOffset with the signature it needs to have in Refactor VFP with new Planner API #426.

Before creating a pull request

Document new methods and classes
Format code with make 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

…en pushed deeper into AIKIDO.

brianhou

Looks mostly good! I don't think we need to make all statespaces and testables const in this PR, but we should at least do that within each file that we've touched. I left comments on instances that I saw where there was a mix of the two, maybe the next person to review will find more.

brianhou · 2018-05-18T02:02:32Z

include/aikido/constraint/dart/FramePairDifferentiable.hpp

@@ -57,7 +57,7 @@ class FramePairDifferentiable : public Differentiable
  std::vector<ConstraintType> getConstraintTypes() const override;

  // Documentation inherited.
-  statespace::StateSpacePtr getStateSpace() const override;
+  statespace::ConstStateSpacePtr getStateSpace() const override;

 private:
  statespace::dart::MetaSkeletonStateSpacePtr mMetaSkeletonStateSpace;


Should this be statespace::dart::ConstMetaSkeletonStateSpacePtr?

It looks like we need the return type to match for the override to work.

Sorry, this comment was about mMetaSkeletonStateSpace.

Derp derp, you're right! 😅

brianhou · 2018-05-18T02:02:41Z

include/aikido/constraint/dart/FrameDifferentiable.hpp

@@ -62,7 +62,7 @@ class FrameDifferentiable : public Differentiable
  std::vector<ConstraintType> getConstraintTypes() const override;

  // Documentation inherited.
-  statespace::StateSpacePtr getStateSpace() const override;
+  statespace::ConstStateSpacePtr getStateSpace() const override;

 private:
  statespace::dart::MetaSkeletonStateSpacePtr mMetaSkeletonStateSpace;


Should this be statespace::dart::ConstMetaSkeletonStateSpacePtr?

It looks like we need the return type to match for the override to work.

Sorry, this comment was about mMetaSkeletonStateSpace.

brianhou · 2018-05-18T02:03:21Z

include/aikido/constraint/SequentialSampleable.hpp

@@ -20,7 +20,7 @@ class SequentialSampleable : public Sampleable

  // Documentation inherited.
  // TODO (avk): const-correctness after planner API is merged.


Delete this line.

brianhou · 2018-05-18T02:03:56Z

include/aikido/constraint/SequentialSampleable.hpp

@@ -20,7 +20,7 @@ class SequentialSampleable : public Sampleable

  // Documentation inherited.
  // TODO (avk): const-correctness after planner API is merged.
-  statespace::StateSpacePtr getStateSpace() const override;
+  statespace::ConstStateSpacePtr getStateSpace() const override;


I think the private member mStateSpace should be ConstStateSpacePtr.

brianhou · 2018-05-18T02:04:50Z

include/aikido/constraint/DifferentiableIntersection.hpp

@@ -34,7 +34,7 @@ class DifferentiableIntersection : public Differentiable
  std::vector<ConstraintType> getConstraintTypes() const override;

  // Documentation inherited.
-  statespace::StateSpacePtr getStateSpace() const override;
+  statespace::ConstStateSpacePtr getStateSpace() const override;


I think the private member mStateSpace should be ConstStateSpacePtr.

brianhou · 2018-05-18T02:05:52Z

include/aikido/constraint/FiniteSampleable.hpp

@@ -36,7 +36,7 @@ class FiniteSampleable : public Sampleable
  virtual ~FiniteSampleable();

  // Documentation inherited.
-  statespace::StateSpacePtr getStateSpace() const override;
+  statespace::ConstStateSpacePtr getStateSpace() const override;


I think the private member mStateSpace should be ConstStateSpacePtr.

brianhou · 2018-05-18T02:06:24Z

include/aikido/constraint/RejectionSampleable.hpp

@@ -30,7 +30,7 @@ class RejectionSampleable : public Sampleable
      int _maxTrialPerSample);

  // Documentation inherited.
-  statespace::StateSpacePtr getStateSpace() const override;
+  statespace::ConstStateSpacePtr getStateSpace() const override;


I think the private member mStateSpace should be ConstStateSpacePtr.

brianhou · 2018-05-18T02:07:40Z

include/aikido/constraint/dart/FrameTestable.hpp

@@ -45,14 +45,14 @@ class FrameTestable : public Testable
  std::unique_ptr<TestableOutcome> createOutcome() const override;

  // Documentation inherited
-  std::shared_ptr<statespace::StateSpace> getStateSpace() const override;
+  statespace::ConstStateSpacePtr getStateSpace() const override;

 private:
  statespace::dart::MetaSkeletonStateSpacePtr mMetaSkeletonStateSpace;


I think this should be statespace::dart::ConstMetaSkeletonStateSpacePtr.

It looks like this breaks the override.

brianhou · 2018-05-18T02:08:38Z

include/aikido/constraint/dart/InverseKinematicsSampleable.hpp

@@ -41,7 +41,7 @@ class InverseKinematicsSampleable : public Sampleable
  virtual ~InverseKinematicsSampleable() = default;

  // Documentation inherited.
-  statespace::StateSpacePtr getStateSpace() const override;
+  statespace::ConstStateSpacePtr getStateSpace() const override;


I think mMetaSkeletonStateSpace should be ConstMetaSkeletonStateSpacePtr.

It looks like this breaks the override.

brianhou · 2018-05-18T02:10:45Z

include/aikido/constraint/dart/JointStateSpaceHelpers.hpp

@@ -24,7 +24,7 @@ std::unique_ptr<Differentiable> createDifferentiableBoundsFor(
 /// Differtiable is created.
 /// \param _stateSpace The StateSpace where the Differentiable will be applied
 std::unique_ptr<Differentiable> createDifferentiableBounds(
-    std::shared_ptr<statespace::dart::JointStateSpace> _stateSpace);
+    std::shared_ptr<const statespace::dart::JointStateSpace> _stateSpace);


Can we make all the statespaces in this file const?

codecov · 2018-05-18T03:52:45Z

Codecov Report

Merging #429 into master will increase coverage by 0.06%.
The diff coverage is 73.52%.

@@            Coverage Diff             @@
##           master     #429      +/-   ##
==========================================
+ Coverage   79.69%   79.76%   +0.06%     
==========================================
  Files         231      231              
  Lines        5990     5986       -4     
==========================================
+ Hits         4774     4775       +1     
+ Misses       1216     1211       -5

Impacted Files	Coverage Δ
.../aikido/constraint/CartesianProductProjectable.hpp	`100% <ø> (ø)`	⬆️
.../aikido/statespace/dart/MetaSkeletonStateSpace.hpp	`100% <ø> (ø)`	⬆️
...ude/aikido/constraint/uniform/SE2BoxConstraint.hpp	`100% <ø> (ø)`	⬆️
include/aikido/constraint/SequentialSampleable.hpp	`100% <ø> (ø)`	⬆️
...e/aikido/constraint/DifferentiableIntersection.hpp	`100% <ø> (ø)`	⬆️
include/aikido/distance/DistanceMetric.hpp	`100% <ø> (ø)`	⬆️
...do/planner/vectorfield/BodyNodePoseVectorField.hpp	`100% <ø> (ø)`	⬆️
include/aikido/distance/SO2Angular.hpp	`100% <ø> (ø)`	⬆️
...ude/aikido/constraint/CartesianProductTestable.hpp	`100% <ø> (ø)`	⬆️
src/planner/ompl/Planner.cpp	`88.76% <ø> (ø)`	⬆️
... and 75 more

…nto VFP_const

…into VFP_const

sniyaz · 2018-05-18T21:11:39Z

Responded to @brianhou's comments- @jslee02, could you take a look when you have the chance?

jslee02

Looks good to me. Thanks for the fix! 👍

[skip ci]

* Make types play nice by changing old VFP code, but problem has now been pushed deeper into AIKIDO. * Propograte const-ness to more of AIKIDO, but not done yet. * Even more const-ing. Builds more, but still not all the way. * Add const-ness to some OMPL stuff. Bumped issues back to VFP code. * Make VFP Offset changes build. Needed to muck around with Robot class. * Yet more const-ing to make tests build. * Tests now build as well. * Clean up createDistanceMetricFor_impl code. * Add file left out from last commit * Clean up JointStateSpaceHelpers code. * Run `make format`. * Respond to Brian's comments. * Update CHANGELOG.md.

evil-sherdil added 10 commits May 16, 2018 20:49

Make types play nice by changing old VFP code, but problem has now be…

664c298

…en pushed deeper into AIKIDO.

Propograte const-ness to more of AIKIDO, but not done yet.

13f4dc2

Even more const-ing. Builds more, but still not all the way.

4556e63

Add const-ness to some OMPL stuff. Bumped issues back to VFP code.

7a0c7ce

Make VFP Offset changes build. Needed to muck around with Robot class.

86143b7

Yet more const-ing to make tests build.

2974f1b

Tests now build as well.

6fe9609

Clean up createDistanceMetricFor_impl code.

cd91a9f

Add file left out from last commit

bb78dfb

Clean up JointStateSpaceHelpers code.

dabe72d

sniyaz requested review from brianhou, jslee02 and dqyi11 May 18, 2018 01:35

evil-sherdil added 2 commits May 17, 2018 21:35

Run make format.

eceb9ba

Merge branch 'master' into VFP_const

b5dcb55

sniyaz mentioned this pull request May 18, 2018

Refactor VFP with new Planner API #426

Merged

5 tasks

brianhou suggested changes May 18, 2018

View reviewed changes

evil-sherdil added 3 commits May 18, 2018 17:06

Respond to Brian's comments.

955fd90

Merge branch 'master' of https://github.com/personalrobotics/aikido i…

8039650

…nto VFP_const

Merge branch 'VFP_const' of https://github.com/personalrobotics/aikido …

8c901e8

…into VFP_const

jslee02 previously approved these changes May 18, 2018

View reviewed changes

brianhou previously approved these changes May 18, 2018

View reviewed changes

dqyi11 previously approved these changes May 18, 2018

View reviewed changes

brianhou added this to the Aikido 0.3.0 milestone May 18, 2018

Update CHANGELOG.md.

e7f3e94

[skip ci]

brianhou dismissed stale reviews from dqyi11, jslee02, and themself via e7f3e94 May 18, 2018 22:37

brianhou merged commit e7b3758 into master May 18, 2018

brianhou deleted the VFP_const branch May 18, 2018 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Const changes to support new VFP Planner API refactor. #429

Const changes to support new VFP Planner API refactor. #429

sniyaz commented May 18, 2018 •

edited by brianhou

Loading

brianhou left a comment •

edited

Loading

brianhou May 18, 2018

sniyaz May 18, 2018

brianhou May 18, 2018

sniyaz May 18, 2018

brianhou May 18, 2018

sniyaz May 18, 2018

brianhou May 18, 2018

brianhou May 18, 2018

brianhou May 18, 2018

brianhou May 18, 2018

brianhou May 18, 2018

brianhou May 18, 2018

brianhou May 18, 2018

sniyaz May 18, 2018

brianhou May 18, 2018

sniyaz May 18, 2018

brianhou May 18, 2018

codecov bot commented May 18, 2018 •

edited

Loading

sniyaz commented May 18, 2018

jslee02 left a comment

		@@ -20,7 +20,7 @@ class SequentialSampleable : public Sampleable

		// Documentation inherited.
		// TODO (avk): const-correctness after planner API is merged.

Const changes to support new VFP Planner API refactor. #429

Const changes to support new VFP Planner API refactor. #429

Conversation

sniyaz commented May 18, 2018 • edited by brianhou Loading

brianhou left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 18, 2018 • edited Loading

Codecov Report

sniyaz commented May 18, 2018

jslee02 left a comment

Choose a reason for hiding this comment

sniyaz commented May 18, 2018 •

edited by brianhou

Loading

brianhou left a comment •

edited

Loading

codecov bot commented May 18, 2018 •

edited

Loading