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

Enhancement/pose estimator #497

Merged
merged 29 commits into from
Feb 14, 2019
Merged

Enhancement/pose estimator #497

merged 29 commits into from
Feb 14, 2019

Conversation

yskim041
Copy link
Member

@yskim041 yskim041 commented Jan 27, 2019

  • Cleaned up hacks for nips demo.
  • Added DetectedObject class
  • PoseEstimatorModule gets an object's unique id and additional information in JSON format from the text field of a Marker. DetectedObject is a wrapper for the information and delegates a detected object.

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

@yskim041 yskim041 requested a review from gilwoolee January 27, 2019 04:07
@yskim041 yskim041 self-assigned this Jan 27, 2019
@yskim041 yskim041 requested review from schmittlema and egordon and removed request for egordon and schmittlema January 27, 2019 04:08
@yskim041 yskim041 added this to the Aikido 0.3.0 milestone Jan 27, 2019
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.

Looks good! Requested some minor changes, mostly since I'm still unclear what JSON information is being sent by the external pose estimator.

src/perception/PoseEstimatorModule.cpp Outdated Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Outdated Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Outdated Show resolved Hide resolved
include/aikido/perception/PoseEstimatorModule.hpp Outdated Show resolved Hide resolved
include/aikido/perception/PoseEstimatorModule.hpp Outdated Show resolved Hide resolved
include/aikido/perception/PoseEstimatorModule.hpp Outdated Show resolved Hide resolved
include/aikido/perception/PoseEstimatorModule.hpp Outdated Show resolved Hide resolved
@yskim041 yskim041 requested review from wagnew3 and removed request for schmittlema February 3, 2019 06:25
@yskim041 yskim041 added the pr: work-in-progress Calls for preliminary review of the PR. Remove when PR is ready for complete review. label Feb 3, 2019
@codecov
Copy link

codecov bot commented Feb 3, 2019

Codecov Report

Merging #497 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #497   +/-   ##
=======================================
  Coverage   77.87%   77.87%           
=======================================
  Files         262      262           
  Lines        6712     6712           
=======================================
  Hits         5227     5227           
  Misses       1485     1485

@yskim041 yskim041 removed the pr: work-in-progress Calls for preliminary review of the PR. Remove when PR is ready for complete review. label Feb 4, 2019
@yskim041 yskim041 force-pushed the enhancement/pose_estimator branch from 1fdaf4d to 286359d Compare February 4, 2019 04:31
include/aikido/perception/DetectedObject.hpp Outdated Show resolved Hide resolved
include/aikido/perception/DetectedObject.hpp Outdated Show resolved Hide resolved
include/aikido/perception/DetectedObject.hpp Outdated Show resolved Hide resolved
include/aikido/perception/DetectedObject.hpp Outdated Show resolved Hide resolved
include/aikido/perception/DetectedObject.hpp Outdated Show resolved Hide resolved
src/perception/DetectedObject.cpp Show resolved Hide resolved
include/aikido/perception/DetectedObject.hpp Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Outdated Show resolved Hide resolved
include/aikido/perception/DetectedObject.hpp Show resolved Hide resolved
@gilwoolee gilwoolee requested a review from aditya-vk as a code owner February 11, 2019 06:34
@yskim041 yskim041 added the pr: work-in-progress Calls for preliminary review of the PR. Remove when PR is ready for complete review. label Feb 11, 2019
@egordon
Copy link
Contributor

egordon commented Feb 13, 2019

@brianhou @gilwoolee Ready for re-review!

include/aikido/perception/PerceptionModule.hpp Outdated Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Outdated Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Outdated Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Outdated Show resolved Hide resolved
@egordon
Copy link
Contributor

egordon commented Feb 13, 2019

@gilwoolee Ready for re-review!

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.

I think Travis is failing because of formatting issues, so feel free to ping if you need help reformatting after your changes.

CHANGELOG.md Outdated Show resolved Hide resolved
include/aikido/perception/DetectedObject.hpp Outdated Show resolved Hide resolved
include/aikido/perception/DetectedObject.hpp Show resolved Hide resolved
include/aikido/perception/DetectedObject.hpp Outdated Show resolved Hide resolved
include/aikido/perception/DetectedObject.hpp Outdated Show resolved Hide resolved
include/aikido/perception/DetectedObject.hpp Outdated Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Outdated Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Outdated Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Outdated Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@gilwoolee gilwoolee left a comment

Choose a reason for hiding this comment

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

Getting close! Left a few nit comments but otherwise looks good to me.

include/aikido/perception/AssetDatabase.hpp Outdated Show resolved Hide resolved
include/aikido/perception/DetectedObject.hpp Outdated Show resolved Hide resolved
include/aikido/perception/PerceptionModule.hpp Outdated Show resolved Hide resolved
src/perception/DetectedObject.cpp Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Outdated Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Outdated Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Outdated Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Show resolved Hide resolved
Copy link
Contributor

@gilwoolee gilwoolee left a comment

Choose a reason for hiding this comment

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

Three nit comments to go!

include/aikido/perception/DetectedObject.hpp Outdated Show resolved Hide resolved
include/aikido/perception/PoseEstimatorModule.hpp Outdated Show resolved Hide resolved
src/perception/PoseEstimatorModule.cpp Outdated Show resolved Hide resolved
Copy link
Member Author

@yskim041 yskim041 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I leave one comment. Please address it, and then this PR should be fine for me.

@gilwoolee gilwoolee removed the pr: work-in-progress Calls for preliminary review of the PR. Remove when PR is ready for complete review. label Feb 14, 2019
@gilwoolee gilwoolee merged commit a1ef21a into master Feb 14, 2019
@gilwoolee gilwoolee deleted the enhancement/pose_estimator branch February 14, 2019 07:52
@gilwoolee gilwoolee mentioned this pull request Feb 17, 2019
5 tasks
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.

4 participants