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.

Sign up for GitHub

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

Jump to bottom

Merging changes from the Summer 2019 ADA Demo #542

Closed
wants to merge 11 commits into from
Closed

Merging changes from the Summer 2019 ADA Demo #542

wants to merge 11 commits into from

Conversation

egordon
Copy link
Contributor

@egordon egordon commented Aug 20, 2019 •
edited
Loading

Remove GCC7 / Ubuntu 18.04 compiler warnings, fixed NominalConfigurationRanker for ADA.

Tested on ADA directly, plus simulator tested via the feeding demo.

@gilwoolee Did you run any other tests?

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

@egordon egordon self-assigned this Aug 20, 2019
brianhou
brianhou reviewed Aug 20, 2019
View reviewed changes
include/aikido/constraint/FiniteSampleable.hpp Outdated
@@ -1,6 +1,7 @@
#ifndef AIKIDO_CONSTRAINT_FINITESAMPLEABLE_HPP_
#define AIKIDO_CONSTRAINT_FINITESAMPLEABLE_HPP_

#include "aikido/statespace/dart/MetaSkeletonStateSpace.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these change are right? They might also be duplicated from #529.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, my mistake, let me roll back the master merge and re-do it.

@brianhou
Copy link
Contributor

Didn't we just add aikido::common::make_unique in #532? I think that was perhaps so we could continue to compile on C++11?

@egordon
Copy link
Contributor Author

egordon commented Aug 20, 2019

@brianhou Yep, just noticed that, so I'm re-merging the change into this branch.

@egordon
Copy link
Contributor Author

egordon commented Aug 20, 2019

@brianhou Ok, ready for comment now I think.

@egordon egordon requested a review from brianhou August 20, 2019 23:21
brianhou
brianhou suggested changes Aug 20, 2019
View reviewed changes
Copy link
Contributor

@brianhou brianhou left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes!

Minor thought: does it make sense to also replace ::aikido with aikido? I don't see a need for the former.

src/constraint/dart/CollisionFree.cpp
{
// CollisionFreeOutcome* collisionFreeOutcome
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this just be deleted?

src/constraint/dart/CollisionFree.cpp
@@ -35,15 +35,22 @@ statespace::ConstStateSpacePtr CollisionFree::getStateSpace() const
//==============================================================================
bool CollisionFree::isSatisfied(
const aikido::statespace::StateSpace::State* _state,
TestableOutcome* outcome) const
__attribute__((unused)) TestableOutcome* outcome) const
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this just be deleted (or at least the parameter name, if we're not using it)?

src/constraint/dart/CollisionFree.cpp
@@ -65,6 +72,7 @@ bool CollisionFree::isSatisfied(
{
collisionFreeOutcome->mPairwiseContacts = collisionResult.getContacts();
}
std::cout << collisionFreeOutcome->toString() << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for debugging?

src/constraint/dart/InverseKinematicsSampleable.cpp
@@ -220,7 +220,8 @@ bool IkSampleGenerator::sample(statespace::StateSpace::State* _state)

mInverseKinematics->getTarget()->setTransform(poseState.getIsometry());

// Run the IK solver. If an exact solution is computed, apply it to the skeleton.
// Run the IK solver. If an exact solution is computed, apply it to the
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this comment inside the #if? Would that help the formatter put it at the right indentation level?

src/control/KinematicSimulationTrajectoryExecutor.cpp
@@ -139,6 +139,7 @@ void KinematicSimulationTrajectoryExecutor::step(
// Check if trajectory has completed.
if (executionTime >= mTraj->getEndTime())
{
std::cout << "Trajectory completed" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

src/trajectory/util.cpp
@@ -151,52 +155,60 @@ UniqueInterpolatedPtr concatenate(
//==============================================================================
double findTimeOfClosestStateOnTrajectory(
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, should this function remain in aikido::trajectory or would it make sense to have it in aikido::distance instead?

src/trajectory/CMakeLists.txt
@@ -9,6 +9,7 @@ add_library("${PROJECT_NAME}_trajectory" SHARED ${sources})
target_link_libraries("${PROJECT_NAME}_trajectory"
PUBLIC
"${PROJECT_NAME}_common"
"${PROJECT_NAME}_distance"
Copy link
Contributor

Choose a reason for hiding this comment

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

If we keep this dependency, we should make sure to update the comments of src/CMakeLists.txt too.

src/trajectory/util.cpp
return findTime;
}

//==============================================================================
UniqueSplinePtr createPartialTrajectory(
const Spline& traj, double partialStartTime)
{
std::cout << "traj starts at " << traj.getStartTime() << " ends at "
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete?

src/trajectory/util.cpp
if (partialStartTime < traj.getStartTime()
|| partialStartTime > traj.getEndTime())
{
throw std::runtime_error("Wrong partial start time");
dtwarn << "Wrong partial start time" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why warn instead of error?

src/trajectory/util.cpp
@@ -254,6 +266,8 @@ UniqueSplinePtr createPartialTrajectory(
traj.getSegmentStartState(i));
}

std::cout << "outputTrajectory starts at " << outputTrajectory->getStartTime()
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete?

@egordon
Copy link
Contributor Author

egordon commented Aug 23, 2019

Rendered out-dated by #543

@egordon egordon closed this Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brianhou brianhou brianhou requested changes

@gilwoolee gilwoolee Awaiting requested review from gilwoolee

@aditya-vk aditya-vk Awaiting requested review from aditya-vk

@sniyaz sniyaz Awaiting requested review from sniyaz

Assignees

@egordon egordon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@egordon @brianhou @gilwoolee