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 wrapper for OMPL path simplifier #164

Merged
merged 76 commits into from
Apr 26, 2017
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
76 commits
Select commit Hold shift + click to select a range
7e7172a
path simplifier
aditya-vk Apr 6, 2017
da97649
adding simplifyOMPL
aditya-vk Apr 6, 2017
49d032f
simplifyOMPL included
aditya-vk Apr 6, 2017
7ec1b60
casting issues to be fixed
aditya-vk Apr 6, 2017
d3783e1
simplify OMPL seems ready to be tested
aditya-vk Apr 7, 2017
57abcec
Merge branch 'master' into feature/simplifier
Apr 7, 2017
837caa9
modified the function signature
aditya-vk Apr 10, 2017
37e3dbb
modified the simplifyOMPL function signature
aditya-vk Apr 10, 2017
2b1a6c7
Merge branch 'master' into feature/simplifier
Apr 11, 2017
fd14e5f
removed _maxplanTime argument
aditya-vk Apr 11, 2017
f82bfed
minor fixes to satisfy Travis
aditya-vk Apr 11, 2017
779d179
termination condition of shortcutting changed
aditya-vk Apr 14, 2017
225c699
returning a pair
aditya-vk Apr 15, 2017
46a5120
merging with master. helper fns and tests remain
aditya-vk Apr 15, 2017
166759f
removed unnamed namespace
aditya-vk Apr 15, 2017
874a2dd
std::chrono for timing. memory leaks issue addressed
aditya-vk Apr 15, 2017
5ff612c
removed unnecessary header file
aditya-vk Apr 15, 2017
8d431e1
moved trajectory conversions to helper functions
aditya-vk Apr 16, 2017
c7a046b
function declarations for helper functions
aditya-vk Apr 16, 2017
d55d408
Merge branch 'master' into feature/simplifier
mkoval Apr 18, 2017
8be6466
Merge branch 'master' into feature/simplifier
aditya-vk Apr 18, 2017
ceaa06f
variable type bug in simplifyOMPL fixed
aditya-vk Apr 18, 2017
f5bb657
Nit and trigger travis build
aditya-vk Apr 18, 2017
66bd650
bug in the function declaration fixed
aditya-vk Apr 18, 2017
e08b4a1
code format
aditya-vk Apr 19, 2017
940da32
code format
aditya-vk Apr 19, 2017
7b5d658
Add simplifyOMPL tests file
aditya-vk Apr 19, 2017
f1901cf
create class for simplifierTest
aditya-vk Apr 19, 2017
6312ab1
code format: nit
aditya-vk Apr 19, 2017
8cb2e83
edit CMakeLists to include test_simplifier
aditya-vk Apr 19, 2017
5bd3daf
adding required header files for simplifier test
aditya-vk Apr 20, 2017
92b0a1e
bug fix
aditya-vk Apr 20, 2017
86a7b89
test that start and goal states remain unchanged
aditya-vk Apr 21, 2017
acb021d
added math header for exponents required for calculating distances
aditya-vk Apr 21, 2017
68d7120
test for if the path REALLY got shortened
aditya-vk Apr 21, 2017
f760795
code formatting to satisfy Travis
aditya-vk Apr 21, 2017
38e468c
build tests for trajectory conversions
aditya-vk Apr 21, 2017
31d2a22
test trajectory conversion helper functions
aditya-vk Apr 21, 2017
e5a52ec
bug fix!
aditya-vk Apr 22, 2017
0312178
nit
aditya-vk Apr 22, 2017
97eab29
take _interpolatorby constant reference
aditya-vk Apr 22, 2017
43f89c5
taking path by constant reference in traj conversion functions
aditya-vk Apr 22, 2017
73f50b7
satisfy travis with code formatting
aditya-vk Apr 22, 2017
e7599f7
Fix style
jslee02 Apr 22, 2017
10e37cb
Merge branch 'master' into feature/simplifier
jslee02 Apr 22, 2017
8d89279
Merge branch 'master' into feature/simplifier
gilwoolee Apr 23, 2017
125361c
Merge branch 'master' into feature/simplifier
mkoval Apr 23, 2017
4db3011
test for non-negativity of timeout and maxEmptySteps
aditya-vk Apr 23, 2017
e206885
Separated tests. New tests added
aditya-vk Apr 23, 2017
1e1d282
using dmetric for distance computation
aditya-vk Apr 23, 2017
210325f
merging updates
aditya-vk Apr 23, 2017
15e9ec0
distance metric needs a fix still
aditya-vk Apr 23, 2017
390e25f
dmetric for calculating distances
aditya-vk Apr 23, 2017
d528ae0
helper function to compute trajectory lengths
aditya-vk Apr 23, 2017
8b66810
cleaned tests for simplifier
aditya-vk Apr 23, 2017
33a0277
Merge branch 'master' into feature/simplifier
gilwoolee Apr 23, 2017
a0cbfab
removed unused variables
aditya-vk Apr 23, 2017
7968bb6
merge with origin
aditya-vk Apr 23, 2017
b8b9281
remove unused variable
aditya-vk Apr 23, 2017
5255dd1
seperated tests. added helper functions
aditya-vk Apr 23, 2017
753671d
removed check for maxEmptySteps
aditya-vk Apr 23, 2017
f795e46
tests for trajectory conversions updated
aditya-vk Apr 23, 2017
f37b9c0
remove stateSpace argument from helper function toInterpolatedTrajectory
aditya-vk Apr 24, 2017
05da04a
removed planning from test_OMPLSimplify. cleaned code
aditya-vk Apr 24, 2017
aa9f54a
eigen_expect_equal introduced
aditya-vk Apr 24, 2017
447b75f
cleaned trajectory conversion tests and simplifier tests
aditya-vk Apr 24, 2017
0cbd2c2
refactoring. work in progress
aditya-vk Apr 24, 2017
884ca31
adding a redundant EXPECT_TRUE() statement that uses trajDistances
aditya-vk Apr 24, 2017
039e97d
add docstring to trajectory conversion functions
aditya-vk Apr 24, 2017
6244af2
remove refactor
aditya-vk Apr 25, 2017
0e84e01
static cast replaced by dynamic cast
aditya-vk Apr 25, 2017
d66fe87
Fix style
jslee02 Apr 25, 2017
e31e1d2
evaluate replaced with getWaypoint
aditya-vk Apr 26, 2017
79329bd
merge with origin
aditya-vk Apr 26, 2017
d69aec4
assertion for equal numWaypoints added
aditya-vk Apr 26, 2017
7fe0a65
Merge branch 'master' into feature/simplifier
jslee02 Apr 26, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion include/aikido/planner/ompl/Planner.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
#include <ompl/base/ProblemDefinition.h>
#include <ompl/base/SpaceInformation.h>
#include <ompl/base/goals/GoalRegion.h>
#include <ompl/base/ScopedState.h>

#include <ompl/geometric/PathSimplifier.h>

namespace aikido {
namespace planner {
Expand Down Expand Up @@ -241,10 +244,45 @@ trajectory::InterpolatedPtr planOMPL(const ::ompl::base::PlannerPtr &_planner,
statespace::InterpolatorPtr _interpolator,
double _maxPlanTime);


/// Take in an aikido trajectory and simplify it using OMPL methods
/// \param _statespace The StateSpace that the planner must plan within
/// \param _interpolator An Interpolator defined on the StateSpace. This is used
/// to interpolate between two points within the space.
/// \param _dmetric A valid distance metric defined on the StateSpace
/// \param _sampler A Sampleable that can sample states from the
/// StateSpace. Warning: Many OMPL planners internally assume this sampler
/// samples uniformly. Care should be taken when using a non-uniform sampler.
Copy link
Member

Choose a reason for hiding this comment

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

Minor Nit: I don't know if this is actually true. As far as I know, most randomized algorithms require that the distribution has non-zero probability of sampling all configurations (i.e. have support over all of configuration space). There is nothing special about the uniform distribution. In fact, what it means to be "uniform" depends on your choice of measure.

@siddhss5 Is what I said above true? 😅

Choose a reason for hiding this comment

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

Yes! The only requirement is that the sampler densely sample configuration space.

/// \param _validityConstraint A constraint used to test validity during
/// planning. This should include collision checking and any other constraints
/// that must be satisfied for a state to be considered valid.
/// \param _boundsConstraint A constraint used to determine whether states
/// encountered during planning fall within any bounds specified on the
/// StateSpace. In addition to the _validityConstraint, this must also be
/// satsified for a state to be considered valid.
/// \param _boundsProjector A Projectable that projects a state back within
/// valid bounds defined on the StateSpace
/// \param _maxPlanTime The maximum time to allow the planner to search for a
/// solution
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Specify units (i.e. seconds).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this parameter actually. Forgot to take it out of the comment as well. But will add a similar parameter for the timeout as suggested in your other comments.

/// \param _maxDistanceBtwValidityChecks The maximum distance (under dmetric) between
/// validity checking two successive points on a tree extension
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Is "tree extension" meaningful for a shortcutting algorithm?

/// \param _originalTraj The untimed trajectory obtained from the planner,
/// needs simplifying.
trajectory::InterpolatedPtr simplifyOMPL(statespace::StateSpacePtr _stateSpace,
statespace::InterpolatorPtr _interpolator,
distance::DistanceMetricPtr _dmetric,
constraint::SampleablePtr _sampler,
constraint::TestablePtr _validityConstraint,
constraint::TestablePtr _boundsConstraint,
constraint::ProjectablePtr _boundsProjector,
double _maxDistanceBtwValidityChecks,
trajectory::InterpolatedPtr _originalTraj);
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to return an std::unique_ptr<Interpolated> rather than a shared_ptr. It's easy to convert from a unique_ptr to a shared_ptr, but the converse is impossible.



} // namespace ompl
} // namespace planner
} // namespace aikido

#include "detail/Planner-impl.hpp"

#endif
#endif
62 changes: 62 additions & 0 deletions src/planner/ompl/Planner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,68 @@ trajectory::InterpolatedPtr planCRRTConnect(
std::move(_interpolator), _maxPlanTime);
}

//=============================================================================
trajectory::InterpolatedPtr simplifyOMPL(statespace::StateSpacePtr _stateSpace,
statespace::InterpolatorPtr _interpolator,
distance::DistanceMetricPtr _dmetric,
constraint::SampleablePtr _sampler,
constraint::TestablePtr _validityConstraint,
constraint::TestablePtr _boundsConstraint,
constraint::ProjectablePtr _boundsProjector,
double _maxDistanceBtwValidityChecks,
trajectory::InterpolatedPtr _originalTraj)
{
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

  • Check that _maxEmptyChecks is non-negative.
  • Check that _timeout is non-negative.

Copy link
Member

Choose a reason for hiding this comment

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

Still missing a check that _maxEmptySteps >= 0.

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, I see now that _maxEmptySteps is unsigned.


// Step 1: Generate the space information of OMPL type
auto si = getSpaceInformation(
_stateSpace, _interpolator, std::move(_dmetric), std::move(_sampler),
std::move(_validityConstraint), std::move(_boundsConstraint),
std::move(_boundsProjector), _maxDistanceBtwValidityChecks);

// Get the state space
auto sspace = ompl_static_pointer_cast<GeometricStateSpace>(
si->getStateSpace());


// Step 2: Convert the AIKIDO trajectory to Geometric Path
::ompl::geometric::PathGeometric *path = new ::ompl::geometric::PathGeometric(si);

for (size_t idx = 0; idx < _originalTraj->getNumWaypoints(); ++idx)
{
auto ompl_state = sspace->allocState(_originalTraj->getWaypoint(idx));
path->append(ompl_state);
sspace->freeState(ompl_state);
}
Copy link
Member

Choose a reason for hiding this comment

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

This code is duplicated from planOMPL. Please move it to a helper function.


// Step 3: Use the OMPL methods to simplify the path
::ompl::geometric::PathSimplifierPtr simplifier;
simplifier.reset(new ::ompl::geometric::PathSimplifier(si));
Copy link
Member

Choose a reason for hiding this comment

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

It is supfluous to use a shared_ptr here. Just declare a local variable with type PathSimplifier.

bool const shortened = simplifier->shortcutPath(*path, 1, 1, 1.0, 0.005);
Copy link
Member

Choose a reason for hiding this comment

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

Make maxSteps, maxEmptySteps, rangeRatio, and snapToVertex parameters of this function.

Also, note that the current hard-coded settings are (in my opinion) nonsensical. Setting maxSteps = 1 means that it will only try to shortcut two randomly selected points before returning. The default values in OMPL are a more reasonable choice, possibly with the modification that rangeRatio = 1.0 and snapToVertex = 0.0.

Another option is to leave maxSteps = 1, but call shortcutPath in a loop until a time limit (or other termination condition) expires. This is the approach I used for OMPLSimplifier in or_ompl.


// Step 4: Convert the simplified geomteric path to AIKIDO untimed trajectory
if(shortened)
{
auto returnTraj = std::make_shared<trajectory::Interpolated>(
std::move(_stateSpace), std::move(_interpolator));

for (size_t idx = 0; idx < path->getStateCount(); ++idx)
{
const auto *st =
static_cast<GeometricStateSpace::StateType *>(
path->getState(idx));
// Arbitrary timing
returnTraj->addWaypoint(idx, st->mState);
}
Copy link
Member

@mkoval mkoval Apr 13, 2017

Choose a reason for hiding this comment

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

This code should also be moved to a helper function. See my comment above about what it could be named and where it could live.

Copy link
Member

Choose a reason for hiding this comment

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

This comment has been addressed.

return returnTraj;
}
else
{
return _originalTraj;
Copy link
Member

Choose a reason for hiding this comment

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

Returning _originalPath here is dangerous because it aliases the input argument. This could very easily cause a bug because the user expects _originalTraj to be unmodified.

This is a bit of a moot point because:

  1. It's not possible to return _originalTraj once we change the return value to an unique_ptr.
  2. It's safe to return returnTraj even if shortcutPath returned false.

}

}

//=============================================================================
} // ns aikido
} // ns planner
} // ns ompl
Copy link
Member

Choose a reason for hiding this comment

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

Add a unit tests for simplifyOMPL. The obvious tests are: (1) each error condition, (2) shortcut a two-waypoint trajectory for a large number of iterations and verify that it doesn't change, and (3) shortcut a three waypoint trajectory for a large number of iterations and verify that the trajectory gets shorter.

It would also be nice to add tests for the GeometricPath conversion functions, but I feel too guilty to ask about that since I am the one who asked you to implement them. 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's fine! I shall work on it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests for trajectory conversion functions. Converted an interpolated trajectory back and forth and ensured it had not changed. That should suffice, yes?

Copy link
Member

Choose a reason for hiding this comment

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

Nit: The order of namespace is inverse. It should be:

} // namespace ompl
} // namespace planner
} // namespace aikido