Refactor Parabolic timer and add Parabolic smoother #206

dqyi11 · 2017-05-02T01:27:28Z

(1) Refactor Parabolic timer into
(1.a) convert an interpolated into a spline
(1.b) convert a spline into a timed spline
(2) Add Parabolic smoother
(2.a) convert a spline to a dynamic path
(2.b) convert a dynamic path to a spine
(2.c) add HauserParabolicSmoother class that handles "doShortcut", "doBlend" and "doShortcutAndBlend"

coveralls · 2017-05-02T03:32:53Z

Coverage increased (+0.6%) to 79.863% when pulling a33a50e on feature/ParabolicSmootherHelper into bb5a1b2 on master.

mkoval

This generally looks good. I have a bunch of relatively minor comments that need to be addressed, but no major changes.

Having two functions for each operation - one for a Spline and one for an Interpolated - adds a lot of boilerplate. Could we eliminate the ones that accept Intnerpolated and rely on the user to call the appropriate conversion function?

mkoval · 2017-05-02T01:29:26Z

include/aikido/planner/parabolic/ParabolicSmoother.hpp

+/// \return smoothed trajectory that satisfies acceleration constraints
+std::unique_ptr<trajectory::Spline> doShortcutAndBlend(
+    const trajectory::Interpolated& _inputTrajectory,
+    const aikido::constraint::TestablePtr _feasibilityCheck,


Nit: Don't mark arguments that are passed by value in a function declaration as const.

mkoval · 2017-05-02T01:30:46Z

include/aikido/planner/parabolic/ParabolicSmoother.hpp

+/// \param _feasibilityCheck Check whether a position is feasible
+/// \param _maxVelocity maximum velocity for each dimension
+/// \param _maxAcceleration maximum acceleration for each dimension
+/// \param _timelimit The maximum time to allow for doing shortcut


Nit: Specify the units (seconds, I think).

mkoval · 2017-05-02T01:31:00Z

include/aikido/planner/parabolic/ParabolicSmoother.hpp

+/// \param _maxAcceleration maximum acceleration for each dimension
+/// \param _timelimit The maximum time to allow for doing shortcut
+/// \param _useVelocity whether velocity is considered in shortcut
+/// \param _blendRadius the radius used in doing blend


Nit: Specify the units (seconds, I think).

mkoval · 2017-05-02T01:31:33Z

include/aikido/planner/parabolic/ParabolicSmoother.hpp

+/// \return smoothed trajectory that satisfies acceleration constraints
+std::unique_ptr<trajectory::Spline> doBlend(
+    const trajectory::Interpolated& _inputTrajectory,
+    const aikido::constraint::TestablePtr _feasibilityCheck,


Nit: It's meaningless to mark an argument that is passed by value as const in a function declaration.

mkoval · 2017-05-02T01:32:00Z

include/aikido/planner/parabolic/ParabolicSmoother.hpp

+/// \param _useVelocity whether velocity is considered in shortcut
+/// \return smoothed trajectory that satisfies acceleration constraints
+std::unique_ptr<trajectory::Spline> doShortcut(
+    const trajectory::Spline* _inputTrajectory,


Nit: Take the _inputTrajectory by const & instead of const *.

mkoval · 2017-05-02T03:25:46Z

tests/planner/parabolic/test_ParabolicSmoother.cpp

+    }
+}
+
+TEST_F(ParabolicSmootherTests, doShort)


Typo: doShortcut.

mkoval · 2017-05-02T03:30:56Z

tests/planner/parabolic/test_ParabolicSmoother.cpp

+   state.setValue(p4);
+   nonStraightLine.addWaypoint(0.75, state);
+   state.setValue(p5);
+   nonStraightLine.addWaypoint(1., state);


Nit: Could we implement this test using only three waypoints?

No.... In Hauser's code, shortcut will be executed only when the waypoint number is larger than 3....

mkoval · 2017-05-02T03:32:25Z

tests/planner/parabolic/test_ParabolicSmoother.cpp

+   smoothedTrajectory->evaluate(smoothedTrajectory->getEndTime(), state);
+   EXPECT_TRUE(p5.isApprox(state.getValue()));
+
+   EXPECT_TRUE(shortenDist <= orignDist);


Nit: Make this a strict inequality and increase the time limit until it succeeds with very high probability.

mkoval · 2017-05-02T03:32:41Z

tests/planner/parabolic/test_ParabolicSmoother.cpp

+
+   double originTime = nonStraightLine.getDuration();
+   double shortenTime = smoothedTrajectory->getDuration();
+   EXPECT_TRUE(shortenTime<=originTime);


Make this a strict inequality.

mkoval · 2017-05-02T03:33:09Z

tests/planner/parabolic/test_ParabolicSmoother.cpp

+
+   double originTime = nonStraightLine.getDuration();
+   double shortenTime = smoothedTrajectory->getDuration();
+   EXPECT_TRUE(shortenTime<=originTime);


See my comment above. Make this a strict inequality.

Also: There's a lot of duplicate code here. Can we move some of it to a helper function?

mkoval · 2017-05-02T03:35:54Z

src/planner/parabolic/ParabolicUtil.cpp

+  auto outputPath = make_unique<ParabolicRamp::DynamicPath>();
+  // Apply the adjoint limits
+  outputPath->Init(toVector(_maxVelocity), toVector(_maxAcceleration));
+  outputPath->SetMilestones(milestones);


We need another version of this helper function - or an overload of this function - that optionally passes velocities to SetMilestones. We need that for all of the functions that accept non-piecewise linear splines.

mkoval · 2017-05-02T03:36:58Z

src/planner/parabolic/ParabolicSmoother.cpp

+
+  double startTime = 0.0;
+  auto dynamicPath = convertToDynamicPath(_inputTrajectory, startTime,
+                                          _maxVelocity, _maxAcceleration);


This call to convertToDynamicPath should preserve velocities.

mkoval · 2017-05-02T03:37:07Z

src/planner/parabolic/ParabolicSmoother.cpp

+                                            _blendIterations);
+  double startTime = 0.0;
+  auto dynamicPath = convertToDynamicPath(_inputTrajectory, startTime,
+                                          _maxVelocity, _maxAcceleration);


This call to convertToDynamicPath should preserve velocities.

mkoval · 2017-05-02T03:37:20Z

src/planner/parabolic/ParabolicSmoother.cpp

+
+  double startTime = 0.0;
+  auto dynamicPath = convertToDynamicPath(_inputTrajectory, startTime,
+                                          _maxVelocity, _maxAcceleration);


This call to convertToDynamicPath should preserve velocities.

coveralls · 2017-05-02T22:16:32Z

Coverage increased (+0.6%) to 79.9% when pulling a5bbcf7 on feature/ParabolicSmootherHelper into bb5a1b2 on master.

dqyi11 · 2017-05-03T00:31:50Z

@mkoval All the comments are addressed.
The only concern is whether we should support the derivative at the start time of an interpolated trajectory, which defines the modifications in the interploated class.
I think we need the velocity to convert an interpolated trajectory to a spline trajectory, because a spline trajectory assumes the smoothness.
Please let me know what you think about it. Thanks!

coveralls · 2017-05-03T00:37:14Z

Coverage increased (+0.6%) to 79.868% when pulling 1ed7efb on feature/ParabolicSmootherHelper into bb5a1b2 on master.

coveralls · 2017-05-03T00:58:14Z

Coverage increased (+0.6%) to 79.868% when pulling 1ed7efb on feature/ParabolicSmootherHelper into bb5a1b2 on master.

dqyi11 · 2017-05-03T14:03:18Z

src/planner/parabolic/HauserParabolicSmootherHelpers.cpp

+       stateSpace->expMap(eigA, startState);
+       stateSpace->expMap(eigB, goalState);
+
+       aikido::util::VanDerCorput vdc{1, true, true, checkResolution_};


I checked that "ConfigFeasible" will be called for both ends of a segment before calling "SegmentFeasible". I think you are right. Great catch!
…

On Wed, May 3, 2017 at 9:40 AM, Michael Koval ***@***.***> wrote: In src/planner/parabolic/HauserParabolicSmootherHelpers.cpp <#206 (comment)> : > + } > + > + virtual bool SegmentFeasible(ParabolicRamp::Vector const &a, > + ParabolicRamp::Vector const &b) > + { > + statespace::StateSpacePtr stateSpace = testable_->getStateSpace(); > + Eigen::VectorXd eigA = toEigen(a); > + Eigen::VectorXd eigB = toEigen(b); > + > + auto testState = stateSpace->createState(); > + auto startState = stateSpace->createState(); > + auto goalState = stateSpace->createState(); > + stateSpace->expMap(eigA, startState); > + stateSpace->expMap(eigB, goalState); > + > + aikido::util::VanDerCorput vdc{1, true, true, checkResolution_}; I *think* (but am not 100% sure) that we actually want to set includeStartpoint = includeEndpoint = false. The start and end configurations are already on the input trajectory, which we assume to be collision-free. @siddhss5 <https://github.com/siddhss5> @dqyi11 <https://github.com/dqyi11> Does this change make sense to you? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/personalrobotics/aikido/pull/206/files/1ed7efb117806f07ffb34f43fca5a4c4fc56c42a#r114546183>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADah4YhFOOltHgOPeNkGQtQVucnN7Lixks5r2IPHgaJpZM4NNnRS> .

mkoval · 2017-05-03T13:09:57Z

include/aikido/planner/parabolic/ParabolicSmoother.hpp

+///
+/// while _timelimit is not out:
+/// - Randomly sample two times uniformly at random.
+/// - Find two waypoints according two times in _inputTrajectory.


This is technically incorrect. We evaluate the trajectory at whatever times we sample, which are almost surely (in the technical sense of that term) not waypoints.

@mkoval Should I then say "points" instead of "waypoints"?

mkoval · 2017-05-03T13:12:01Z

include/aikido/planner/parabolic/ParabolicSmoother.hpp

+/// - Randomly sample two times uniformly at random.
+/// - Find two waypoints according two times in _inputTrajectory.
+/// - Attempt to connect the two waypoints with a time-optimal parabolic
+/// spline.


Add a note that we then evaluate the feasibility of the spline using _feasibilityCheck.

mkoval · 2017-05-03T13:13:13Z

include/aikido/planner/parabolic/ParabolicSmoother.hpp

+    const Eigen::VectorXd& _maxAcceleration,    
+    double _timelimit = DEFAULT_TIMELIMT,
+    double _tolerance = DEFAULT_TOLERANCE,
+    aikido::util::RNG::result_type _rngSeed = std::random_device{}());


There is no need to take a seed here - we can simply pass in a util::RNG& to use for sampling.

mkoval · 2017-05-03T13:21:56Z

include/aikido/planner/parabolic/ParabolicSmoother.hpp

+///
+/// This function smooths `_inputTrajectory' by firstly applying shortcut
+/// to _inputTrajectory and then using blend to remove segments that have
+/// zero velocities.


Note that this function is equivalent to calling doShortcut , followed bydoBlend. Add a note that this function is more efficient than calling those two functions in sequence because it avoids duplicated effort.

mkoval · 2017-05-03T13:22:14Z

include/aikido/planner/parabolic/ParabolicSmoother.hpp

+    double _blendRadius = DEFAULT_BLEND_RADIUS,
+    int _blendIterations = DEFAULT_BLEND_ITERATIONS,
+    double _tolerance = DEFAULT_TOLERANCE,
+    aikido::util::RNG::result_type _rngSeed = std::random_device{}()


This should be a util::RNG&, not a seed. See my note above.

mkoval · 2017-05-03T13:58:28Z

src/planner/parabolic/ParabolicUtil.hpp

+/// \param[out] position at time \c _t
+/// \param[out] velocity at time \c _t
+void evaluateAtTime(ParabolicRamp::DynamicPath& _path, double _t,
+    Eigen::VectorXd& _position, Eigen::VectorXd& _velocity);


I think it should be safe to take _path by const & here.

mkoval · 2017-05-03T13:59:27Z

src/planner/parabolic/ParabolicUtil.hpp

+    convertToDynamicPath(const aikido::trajectory::Spline& _inputTrajectory,
+                         const Eigen::VectorXd& _maxVelocity,
+                         const Eigen::VectorXd& _maxAcceleration,
+                         bool _useVelocity = true);


I suggest removing the default value for _useVelocity because it dramatically changes the behavior of the function. Or - if it does not require duplicating too much code - split this into two separate functions.

@dqyi11 I would still like to address this.

mkoval · 2017-05-03T14:00:55Z

src/trajectory/Interpolated.cpp

+      mInterpolator->getDerivative(mWaypoints[0].state, mWaypoints[1].state,
+              _derivative, alpha, _tangentVector);
+      _tangentVector /= segmentTime;
+      return;


I see - that makes sense if we have to handle this case. I am inclined to remove handling of this case for now, since an Interpolated trajectory with a GeodesicInterpolator has undefined derivatives at its waypoints.

mkoval · 2017-05-03T14:01:15Z

src/trajectory/Interpolated.cpp

+{
+  if (_index < mWaypoints.size())
+    //return getStartTime() + mWaypoints[_index].t;
+    return mWaypoints[_index].t;


I see - that seems to be correct. 👍

mkoval · 2017-05-03T14:02:09Z

src/trajectory/Spline.cpp

+
+//=============================================================================
+double Spline::getWaypointTime(size_t _index) const
+{


Check that _index < mSegments.size().

This is resolved.

coveralls · 2017-05-04T01:01:55Z

Coverage increased (+0.5%) to 79.805% when pulling 3f97e2c on feature/ParabolicSmootherHelper into bb5a1b2 on master.

dqyi11 · 2017-05-04T01:03:24Z

I have addressed all the comments.

The only thing left is whether we shall keep "useVelocity" in converting an interpolated to a spline.
This can be useful in the ParabolicSmoother.

In that, I convert a spline trajectory to a dynamic path without providing velocity.
Then dynamic path will re-evaluate the velocity.
If velocities are given, dynamic path will not re-evaluate the velocities.
Thus no new timing information will be added.
Does this make sense?

coveralls · 2017-05-04T01:29:27Z

Coverage increased (+0.5%) to 79.827% when pulling 3f97e2c on feature/ParabolicSmootherHelper into bb5a1b2 on master.

coveralls · 2017-05-04T05:02:25Z

Coverage increased (+0.5%) to 79.827% when pulling 5a6b908 on feature/ParabolicSmootherHelper into bb5a1b2 on master.

jslee02 · 2017-05-04T15:00:10Z

include/aikido/planner/parabolic/ParabolicTimer.hpp


 /// Convert an interpolated trajectory to a spline trajectory
-/// This function requires the \c _inputTrajectory to use a \c GeodesicInterpolator.
+/// This function requires the \c _inputTrajectory to use a \c
+/// GeodesicInterpolator.


Nit: Could you fix the wrapping? ClangFormat isn't good at it. 😞

Nit: I thought "So the conversion ..." was the continuing sentence without line breaking.

jslee02 · 2017-05-04T15:01:36Z

src/planner/parabolic/HauserParabolicSmootherHelpers.cpp

+  {
+  }
+
+  virtual bool ConfigFeasible(ParabolicRamp::Vector const& x)


Nit: Please use const X& pattern rather than X const&.

Also, please begin with a lower letter for member functions.

jslee02 · 2017-05-04T15:02:48Z

src/planner/parabolic/HauserParabolicSmootherHelpers.cpp

+    , statespace_(testable_->getStateSpace())
+    , interpolator_(statespace_)
+  {
+  }


Nit: Please add // Do nothing comment to indicate that this function is intentionally empty (but initializing the member variables).

jslee02 · 2017-05-04T15:03:03Z

src/planner/parabolic/HauserParabolicSmootherHelpers.cpp

+  }
+
+  virtual bool SegmentFeasible(
+      ParabolicRamp::Vector const& a, ParabolicRamp::Vector const& b)


Please rename to segmentFeasible(...).

jslee02 · 2017-05-04T15:04:00Z

src/planner/parabolic/HauserParabolicSmootherHelpers.cpp

-    double                                   checkResolution_;
-    aikido::statespace::StateSpacePtr        statespace_;
-    aikido::statespace::GeodesicInterpolator interpolator_;
+  aikido::constraint::TestablePtr testable_;


Nit: Could you rename these private member variables using the pattern of mX? I believe we don't use the trailing underscore for member variables (even for private member variables).

jslee02 · 2017-05-04T15:05:47Z

src/planner/parabolic/HauserParabolicSmootherHelpers.cpp

+  // that two waypoints are closer than _blendRadius together - which means
+  // that waypoint indicies can change between iterations of the algorithm.
+  size_t const numRamps = dynamicPath.ramps.size();
+  double const tMax = dynamicPath.GetTotalTime();


Nit: const double

jslee02 · 2017-05-04T15:06:16Z

src/planner/parabolic/HauserParabolicSmootherHelpers.cpp

-                rampNd.blendAttempts++;
-            }
-        }
+      double const t1 = std::max(t - dtShortcut, -ParabolicRamp::EpsilonT);


Nit: const double

jslee02 · 2017-05-04T15:06:21Z

src/planner/parabolic/HauserParabolicSmootherHelpers.cpp

-            }
-        }
+      double const t1 = std::max(t - dtShortcut, -ParabolicRamp::EpsilonT);
+      double const t2


Nit: const double

jslee02 · 2017-05-04T15:06:29Z

src/planner/parabolic/HauserParabolicSmootherHelpers.cpp

+      double const t2
+          = std::min(t + dtShortcut, tMax + ParabolicRamp::EpsilonT);
+
+      bool const success = dynamicPath.TryShortcut(t1, t2, feasibilityChecker);


Nit: const bool

jslee02 · 2017-05-04T15:09:46Z

src/planner/parabolic/HauserParabolicSmootherHelpers.cpp

+  for (int attempt = 0; attempt < blendIterations; ++attempt)
+  {
+    while (tryBlend(dynamicPath, feasibilityChecker, attempt, dtShortcut))
+      ;


😱 What's happening here?

Nit: At first, I thought this was a mistake, but now I got it. I would like to suggest make this code easier to read something like:

bool success; do { success = tryBlend(...); } while (success);

Also, I'm not sure which one is logically correct between while(success) and while(!success). 😕

The current logic is correct. The core issue is that blending can completely remove waypoints from the trajectory, which makes bookkeeping via the time parameter extraordinarily difficult. We dodge that issue by allowing tryBlend to always start at the beginning of the trajectory. We continue to call tryBlend in a loop until it finds nothing left to blend.

That makes sense. It would be nice to add this as a comment at least to tryBlend().

I still prefer to change the code to be self-documenting. For example:

bool needMoreBlending; do { needMoreBlending = tryBlend(...); } while (needMoreBlending);

…personalrobotics/aikido into feature/ParabolicSmootherHelper

mkoval · 2017-05-10T03:19:35Z

include/aikido/planner/parabolic/ParabolicSmoother.hpp

+///
+/// This function smooths `_inputTrajectory' by iteratively sampling
+/// two waypoints in a trajectory and trying to find a shortcut using
+/// the following algorithm:


Nit: "waypoints" -> "points".

mkoval · 2017-05-11T02:04:07Z

include/aikido/planner/parabolic/ParabolicSmoother.hpp

+    aikido::constraint::TestablePtr _feasibilityCheck,
+    const Eigen::VectorXd& _maxVelocity,
+    const Eigen::VectorXd& _maxAcceleration,
+    aikido::util::RNG& _rngSeed,


Nit: This is the RNG, not a seed for the RNG. 😉

mkoval · 2017-05-11T02:15:03Z

src/planner/parabolic/HauserParabolicSmootherHelpers.cpp

+    mStateSpace->expMap(eigA, startState);
+    mStateSpace->expMap(eigB, goalState);
+
+    aikido::util::VanDerCorput vdc{1, false, false, mCheckResolution};


Nit: Add a comment explaining why includeStartpoint and includeEndpoint can be false.

mkoval · 2017-05-11T02:19:11Z

src/planner/parabolic/HauserParabolicSmootherHelpers.cpp

+    aikido::util::RNG& rng)
+{
+  if (timelimit < 0.0)
+    throw std::runtime_error("Timelimit should be non-negative");


Nits:

Add a bounds check for checkResolution.

Use std::invalid_argument instead of std::runtime_error for these exceptions.

mkoval · 2017-05-11T02:20:09Z

src/planner/parabolic/HauserParabolicSmootherHelpers.cpp

+  SmootherFeasibilityCheckerBase base(testable, checkResolution);
+  ParabolicRamp::RampFeasibilityChecker feasibilityChecker(&base, tolerance);
+
+  std::chrono::time_point<std::chrono::system_clock> startTime, currentTime;


Nits:

currentTime is only used inside the loop, so move its definition to where it is first used.

Initialize startTime on the same line where the variable it is defined.

mkoval · 2017-05-11T02:50:19Z

tests/planner/parabolic/test_ParabolicSmoother.cpp

+protected:
+  void SetUp() override
+  {
+    mRng = aikido::util::RNGWrapper<std::mt19937>( std::random_device{}() );


Please use a deterministic seed (e.g. 0) in unit tests so their result is not stochastic.

mkoval · 2017-05-11T02:51:51Z

tests/planner/parabolic/test_ParabolicSmoother.cpp

+    state.setValue(Vector2d(3., 4.));
+    mStraightLine->addWaypoint(1., state);
+
+    mNonStraightLineLength = initNonStraightLine();


It's odd to have one output (mNonStraightLine) in a member variable and a second mNonStraightLineLength as a return value. Please stick to one convention or the other for this function.

mkoval · 2017-05-11T02:53:13Z

tests/planner/parabolic/test_ParabolicSmoother.cpp

+    auto currState = mStateSpace->createState();
+    auto nextState = mStateSpace->createState();
+    Eigen::VectorXd currVec, nextVec;
+    for(size_t i=0; i<spline->getNumWaypoints()-1;++i)


Nit: This will overflow if getNumWaypoints() == 0, which could cause the test to SEGFAULT instead of fail properly. Please refactor the loop so that is not an issue, e.g. by starting with i = 1 and replacing i with i - 1 in the body.

mkoval · 2017-05-11T02:53:43Z

tests/planner/parabolic/test_ParabolicSmoother.cpp

+
+TEST_F(ParabolicSmootherTests, convertInterpolatedToSpline)
+{
+    auto spline = convertToSpline(*mStraightLine.get());


Nit: *mStraightLine.get() is equivalent to *mStraightLine.

mkoval · 2017-05-11T02:55:15Z

tests/planner/parabolic/test_ParabolicSmoother.cpp

+        mStraightLine->evaluateDerivative(t, 1, interpolatedTangent);
+        EXPECT_EIGEN_EQUAL(splineTangent, interpolatedTangent, mTolerance);
+    }
+}


Add tests for:

More than one segment, e.g. the non-striaghtline trajectory.

A trajectory with a non-zero start time.

codecov-io · 2017-05-11T17:53:00Z

Codecov Report

❗ No coverage uploaded for pull request base (master@55ba523). Click here to learn what that means.
The diff coverage is 81%.

@@            Coverage Diff            @@
##             master     #206   +/-   ##
=========================================
  Coverage          ?   70.79%           
=========================================
  Files             ?      168           
  Lines             ?     4897           
  Branches          ?      774           
=========================================
  Hits              ?     3467           
  Misses            ?      954           
  Partials          ?      476

Impacted Files	Coverage Δ
include/aikido/trajectory/Interpolated.hpp	`100% <ø> (ø)`
include/aikido/trajectory/Spline.hpp	`100% <ø> (ø)`
src/planner/parabolic/ParabolicSmoother.cpp	`100% <100%> (ø)`
src/trajectory/Interpolated.cpp	`73.41% <50%> (ø)`
src/trajectory/Spline.cpp	`82.4% <68.42%> (ø)`
...anner/parabolic/HauserParabolicSmootherHelpers.cpp	`74.11% <74.11%> (ø)`
src/planner/parabolic/ParabolicUtil.cpp	`82.9% <82.9%> (ø)`
src/planner/parabolic/ParabolicTimer.cpp	`70.58% <89.28%> (ø)`

mkoval

Looks good to me. Great work @dqyi11!

mkoval · 2017-05-14T22:37:24Z

src/planner/parabolic/HauserParabolicSmootherHelpers.cpp

+
+  // tryBlend always starts at the beginning of the trajectory.
+  // Without this bookkeeping, tryBlend would could any blend that fails
+  // multiple times.


Nit: Typo "would could." I think this should say "tryBlend would retry any blend that fails multiple times."

mkoval · 2017-05-14T22:38:10Z

src/planner/parabolic/HauserParabolicSmootherHelpers.cpp

+    {
+      noMoreBlending
+          = tryBlend(dynamicPath, feasibilityChecker, attempt, dtShortcut);
+    } while (noMoreBlending);


Nit: The name noMoreBlending implies the opposite of what it actually is. I suggest renaming the flag to needsMoreBlending.

mkoval · 2017-05-14T22:41:35Z

src/planner/parabolic/ParabolicUtil.cpp

+  else if (dynamic_cast<const R<6>*>(_stateSpace) != nullptr)
+  {
+    return true;
+  }


mkoval · 2017-05-14T22:42:37Z

src/planner/parabolic/ParabolicUtil.cpp

+    milestones.emplace_back(toVector(currVec));
+
+    _inputTrajectory.getWaypointDerivative(iwaypoint, 1, tangentVector);
+    velocities.emplace_back(toVector(tangentVector));


Resolved by splitting the Spline and Interpolated cases into separate overloads. 👍

dqyi11 and others added 5 commits May 1, 2017 01:45

add parabolic smoother and refactor parabolic timer

bb6a6f7

fix travis build errors

c0685e8

correct path conversion and test cases in timing

c960c10

refactor ownership

57c221f

fix bugs and add test cases

903b12e

dqyi11 requested a review from mkoval May 2, 2017 01:27

fix building errors

a33a50e

mkoval requested changes May 2, 2017

View reviewed changes

address Mike's comments

a5bbcf7

Daqing Yi added 2 commits May 2, 2017 20:07

address Mike's comments

5eae8bd

address Mike's comment

1ed7efb

Merge branch 'master' into feature/ParabolicSmootherHelper

47fb916

dqyi11 reviewed May 3, 2017
View reviewed changes

mkoval requested changes May 3, 2017

View reviewed changes

Daqing Yi added 2 commits May 3, 2017 20:31

address Mike's comments

82beb43

increase the timelimit for success rate

3f97e2c

remove dead comment

5a6b908

jslee02 added this to the Aikido 0.1.0 milestone May 4, 2017

code formatting

d61c63e

jslee02 reviewed May 4, 2017

View reviewed changes

Daqing Yi and others added 20 commits May 9, 2017 16:32

correct path conversion and test cases in timing

52a35a6

refactor ownership

d6791e9

fix bugs and add test cases

2965a42

fix building errors

3420e25

address Mike's comments

443d2ab

address Mike's comments

e1e593f

address Mike's comment

14b571a

Use custom CMake modules for interactive_markers and tf (#198)

b28720a

address Mike's comments

37ee669

increase the timelimit for success rate

188687e

remove dead comment

856e540

Code format: perception (#168)

1461f56

Code format: statespace (#170)

b55ea3b

code formatting

1729626

Code format: rviz (#171)

28d3d19

address JS's comments

ecf22fc

Code format: control (#191)

4c2d515

Merge branch 'feature/ParabolicSmootherHelper' of https://github.com/…

b18e2ea

…personalrobotics/aikido into feature/ParabolicSmootherHelper

format

507df85

clean code

2e71c30

mkoval requested changes May 11, 2017

View reviewed changes

Merge branch 'master' into feature/ParabolicSmootherHelper

bed163d

Daqing Yi added 4 commits May 12, 2017 10:21

address Mike's comments

be0e014

merge

806117a

Merge branch 'master' into feature/ParabolicSmootherHelper

2ca1bf1

merge from master and format

36ca91d

mkoval approved these changes May 14, 2017

View reviewed changes

mkoval merged commit 00b4faa into master May 14, 2017

mkoval deleted the feature/ParabolicSmootherHelper branch May 14, 2017 22:47

Refactor Parabolic timer and add Parabolic smoother #206

Refactor Parabolic timer and add Parabolic smoother #206

Conversation

dqyi11 commented May 2, 2017

coveralls commented May 2, 2017

mkoval left a 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

coveralls commented May 2, 2017

dqyi11 commented May 3, 2017

coveralls commented May 3, 2017

coveralls commented May 3, 2017

dqyi11 May 3, 2017 via email

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

coveralls commented May 4, 2017

dqyi11 commented May 4, 2017

coveralls commented May 4, 2017

coveralls commented May 4, 2017

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

jslee02 May 4, 2017 • 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

codecov-io commented May 11, 2017 • edited Loading

Codecov Report

mkoval left a 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

jslee02 May 4, 2017 •

edited

Loading

codecov-io commented May 11, 2017 •

edited

Loading