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

Added Chisel to the perception module #281

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

Conversation

jeking04
Copy link
Contributor

I think we are ready to merge this. @Shushman and I checked that chisel is working. And I have verified simtrack also works with these changes. We should test vncc one more time to ensure I didn't introduce any bugs with by switching to use the kinbody_helper functions. But in the meantime, lets start gathering comments on this PR and working it through.

import rospy

try:
rospy.init_node('chisel_detector',anonymous=True)
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't call init_node here because it may only be called once per process. I prefer calling init_node only once, in the top-level script, to prevent this from happening twice.

# Remove any previous bodies in the camera
for b in self.camera_bodies:
self.camera.remove_body(b)
self.camera_bodies = []
Copy link
Member

Choose a reason for hiding this comment

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

Why are these on self? It seems like a local variable would suffice.

Copy link
Contributor

Choose a reason for hiding this comment

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

camera_bodies? It is the list of bodies that the camera has added since being started, and needs to have state between calls to DetectObject

@mkoval
Copy link
Member

mkoval commented Mar 27, 2016

This code has a lot of dependencies, including several that are specific to Chisel. Can we move this code to or_mesh_marker, which already has those dependencies? We can then make herbpy depend on or_mesh_marker without adding any dependencies to prpy.

This mirrors @psigen's suggestion in #275.

@psigen
Copy link
Member

psigen commented Mar 27, 2016

I've been starting to do this for our retimers/smoothers. See:
personalrobotics/or_parabolicsmoother#19 and
https://github.com/personalrobotics/or_circularblender

@mkoval mkoval changed the title Perception/chisel Added Chisel to the perception module Mar 30, 2016
@Shushman
Copy link
Contributor

Shushman commented May 6, 2016

I've gone through chisel.py and kinbody_helper.py and addressed most to all of @mkoval's initial comments. Could @jeking04 throw another pair of eyes on this, address the comment for simtrack and weigh in on a couple of questions I raised?

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.

4 participants