-
Notifications
You must be signed in to change notification settings - Fork 7
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
Added a gesture interface #54
Added a gesture interface #54
Conversation
goal_name = None | ||
else: | ||
raise prpy.exceptions.PrPyException('Focus of the point is an \ | ||
unknown object') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type inference is un-Pythonic and makes me nervous. For example, the "pointing at a point in space" logic will break if the user passes a list
, tuple
, or any other type of collection that is not an ndarray
. This violates Python's affinity for duck typing.
I would prefer if you restructured this slightly:
- Make a
Point
action that takesfocus_trans
and (maybe, see above) agoal_name
. - Add
PointAtKinBody
andPointAtPoint
functions for, respectively points andKinBody
's - If you think it is necessary, write a generic
PointAt
function that infers whichPointAtX
function to call.
Also, you should always use instanceof
, not type
equality checks to compare types.
Overall, this looks pretty good. I left a number of relatively minor comments, mostly about missing locks and |
""" | ||
env = robot.GetEnv() | ||
pointing_coord = GetPointFrom(env, focus) | ||
Point(robot, pointing_coord, manip, render) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should propagate the return
value; i.e.
return Point(robot, pointing_coord, manip, render)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This applies to all of the wrapper functions.
p = openravepy.KinBody.SaveParameters | ||
with robot.CreateRobotStateSaver(p.ActiveManipulator): | ||
manip = robot.right_arm | ||
robot.SetActiveManipulator(manip) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to set the manipulator as active here.
@@ -1,2 +1,3 @@ | |||
from grasping import PushGrasp, Grasp | |||
from rogue import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import *
is evil. Could you explicitly list the functions you wish to import here?
All of the major issues seem to be resolved. I just left a few comments on code cleanup. @rachelholladay Could you also add (at least) one test to each of these functions that calls it on a very simple environment? It's fine if it's something as simple as just running each function on a Fuze bottle. We have a tutorial for doing this on the website and there are some simple examples in @jeking04, @Shushman, or myself can help if you run into trouble. |
@mkoval I made a PR on prpy for the util function, so I think that should be resolved first. I've also made most of the changes you asked for on this. |
Thanks. The only comment I have left is that the default should always be |
Alright @mkoval. Last comment addressed. |
|
||
env, robot = herbpy.initialize(sim=True) | ||
robot.right_arm.SetActive() | ||
fuze = prpy.rave.add_object(env, 'fuze_bottle', 'objects/fuze_bottle.kinbody.xml') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should occur in setUp
, not in the file-level scope.
Testing addressed. Can we get it merged before class? :) |
Adding the gesture interface for the demo. This involves: