personalrobotics / prpy

Python utilities used by the Personal Robotics Laboratory.
BSD 3-Clause "New" or "Revised" License
62 stars 19 forks source link

issue #292 | issue #293 #294

Closed DavidB-CMU closed 8 years ago

DavidB-CMU commented 8 years ago

issue #292 DetectObjects() returns duplicate kinbodies in prpy.perception.apriltags.py

issue #293 env is not defined, in DetectObject() in prpy.perception.apriltags.py

mkoval commented 8 years ago

@Shushman @jeking04 Will this change in DetectObject behavior break any of our demos?

Shushman commented 8 years ago

I don't think so but I'd like to test them out before committing.

jeking04 commented 8 years ago

I definitely agree with the change to fix #293. But I am not sure I understand the change for #292. It seems to me like the true bug is further down the pipeline: the kinbody_detector is incorrectly putting those bodies in both the added and updated lists. I need a little convincing that the right thing to do is just ignore one list. Either we are incorrectly interpretting the results from the kinbody_detector or there is a bug in kinbody_detector.Update. I think we need to understand which of these is the true problem and either a) update the code to correctly name and use the two elements returned by the detector or b) fix the bug in kinbody_detector

Shushman commented 8 years ago

On a (somewhat) related note, for the aikido perception re-write, I am not making a separate KinBody Detector, since it seems quite specific to AprilTags. I have just moved the relevant part of the functionality to AprilTags. And it definitely only returns one set of objects - so our future DetectObjects() should be fine.

jeking04 commented 8 years ago

To follow up, I think this is definitely a problem with the kinbody_detector. You can see here:

https://github.com/personalrobotics/kinbody_detector/blob/master/src/kinbody_detector/kinbody_detector.py#L102

Objects added to added_kinbodies also get put in the updated_kinbodies list here:

https://github.com/personalrobotics/kinbody_detector/blob/master/src/kinbody_detector/kinbody_detector.py#L107

I propose we remove the change from here and open an issue against the kinbody_detector repository.

DavidB-CMU commented 8 years ago

I discussed this with @jeking04 - I'll take a look at kinbody_detector

DavidB-CMU commented 8 years ago

Should we move this block up to the constructor?

        # TODO: Creating detector is not instant...might want
        #  to just do this once in the constructor
        import kinbody_detector.kinbody_detector as kd
        detector = kd.KinBodyDetector(env,
                                      marker_data_path,
                                      kinbody_path,
                                      marker_topic,
                                      detection_frame,
                                      destination_frame,
                                      reference_link)
DavidB-CMU commented 8 years ago

I decided not to move that block into the constructor. There's a lot of parameters that can potentially be overridden.

DavidB-CMU commented 8 years ago

I also modified DetectObject() and DetectObjects() to suit the changes to kinbody_detector. For example, you can call DetectObject('cup5') and it won't update the pose of the table.

This has also been tested on the robot but hopefully it won't break any demos.

mkoval commented 8 years ago

I am fine with merging these changes, and https://github.com/personalrobotics/kinbody_detector/pull/4, if @Shushman and @jeking04 can confirm that this will not break the demo.

DavidB-CMU commented 8 years ago

@Shushman and I tested this again this morning. @jeking04 can we merge this in please?

DavidB-CMU commented 8 years ago

We decided to strip the latest commit so this PR is finished.

The other issue is now here: https://github.com/personalrobotics/prpy/issues/300