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

Block sorting #189

Merged
merged 54 commits into from
Oct 12, 2015
Merged

Block sorting #189

merged 54 commits into from
Oct 12, 2015

Conversation

jeking04
Copy link
Contributor

This pulls in the perception pipeline and a few other enhancements made during the block sorting demo.

jeking04 and others added 30 commits June 26, 2015 11:50
…mpty list when service call fails. Fixing minor bug in adding blocks to block list.
…n_pipeline

Conflicts:
	src/prpy/base/wam.py
…ne_merged

Merged PrPy changes into feature/perception_pipeline

I didn't read them all, I'm trusting you mike...
Conflicts:
	src/prpy/base/wam.py
	src/prpy/util.py
@@ -236,6 +238,7 @@ def MoveUntilTouch(manipulator, direction, distance, max_distance=None,
@param max_force maximum force in Newtons
@param max_torque maximum torque in Newton-Meters
@param ignore_collisions collisions with these objects are ignored when planning the path, e.g. the object you think you will touch
@param velocity_limit_scale A multiplier to use to scale velocity limits when executing MoveUntilTouch ( < 1 in most cases).
@param **kw_args planner parameters
Copy link
Member

Choose a reason for hiding this comment

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

Please wrap these lines to 80 characters to be PEP-8 compliant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

self.destination_frame = destination_frame

def __str__(self):
return 'RockModule'
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 return self.__class__.__name__ here, but this is just a nitpick.

def DetectBlocks(self, robot, table, blocks=None,timeout=10, **kw_args):
"""
Place blocks on the table
"""
Copy link
Member

Choose a reason for hiding this comment

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

Please add documentation for the arguments and the return value (see above).

@mkoval
Copy link
Member

mkoval commented Oct 10, 2015

There are three minor issues remaining before we can merge this:

  1. There are merge conflicts that must be resolved (via git merge master locally)
  2. Cleanup one function
  3. Add one missing __docstring__

@Shushman Can you address these?

@jeking04
Copy link
Contributor Author

Okay, @mkoval, I think we addressed all 3 above. After merging master into this branch I ran the unittests. The planning tests passed but the TSR test failed. Switching back to master I see it fails there too. So I think we should fix it under a different PR.

valid = False
while not valid:
rand_name = 'block' + `int(numpy.random.randint(1,10000))`
valid = env.GetKinBody(rand_name) is None
Copy link
Member

Choose a reason for hiding this comment

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

env.Add takes an optional second argument anonymous that will append an integer to the end of the KinBody's name to make it unique. Please replace this loop with that functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed

mkoval added a commit that referenced this pull request Oct 12, 2015
Changes for the block sorting demo.
@mkoval mkoval merged commit 62fca02 into master Oct 12, 2015
@mkoval mkoval deleted the block_sorting branch October 12, 2015 16:15
@mkoval mkoval mentioned this pull request Sep 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants