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

Edge Processing #123

Merged
merged 7 commits into from
Jan 10, 2017

Conversation

woodaaron
Copy link

Incorporates CLewis' surface segmentation for edge detection and following.

Aaron Wood added 7 commits January 9, 2017 12:07
…entation to find_surfaces_old.

Changed getBoundaryTrajectory return type to Eigen::Matrix4d to better interface with existing descartes planning

Changed signatures for generateProcessPath and requestEdgePath to allow for new edge detection implementation
Made gode_surface_detection a required node

Tuning for edge detection with ensenso.
…r blending and edge paths to the surface normal (Assumes part is planar)

Added publisher for visualization and added the ability to clear blend and edge visualizations on re-scan
…and removed previous CollisionGuard checks
@woodaaron woodaaron requested a review from Jmeyer1292 January 9, 2017 19:35

if (obeys_limits)
{
if (isValid(sol))

Choose a reason for hiding this comment

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

Note to self: Move all of these custom kinematics classes to the Descartes IKFast adapter.

@@ -0,0 +1,606 @@
#ifndef SURFACE_SEGMENTATION_H

Choose a reason for hiding this comment

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

It appears all of the new edge stuff is stuffed into one big header. We should look at:

  • Adding one or more implementation files
  • Putting this library into its own package

Aside from cleanly dividing impl from interface, this will also help us reduce build times which are getting pretty steep for this project.

At a high level, I'd like to begin building a library of useful tools to do much of this work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can do this before I merge my changes with @woodaaron 's repository.

/** @class world_background_subtraction
@brief Maintains record of baseline sensor data to provide method to remove them leaving only new objects in the scene
*/
class surfaceSegmentation

Choose a reason for hiding this comment

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

I hate to be stickler, but naming conventions for something like this is important: Should be SurfaceSegmentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can take care of this too.

{
input_cloud_ = pcl::PointCloud<pcl::PointXYZ>::Ptr(new pcl::PointCloud<pcl::PointXYZ>);
normals_ = pcl::PointCloud<pcl::Normal>::Ptr(new pcl::PointCloud<pcl::Normal>);
input_cloud_->clear();

Choose a reason for hiding this comment

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

This stuff is unnecessary. The shared pointers will be de-allocated automatically.

normals_ = pcl::PointCloud<pcl::Normal>::Ptr(new pcl::PointCloud<pcl::Normal>);
setInputCloud(icloud);
removeNans();
computeNormals();

Choose a reason for hiding this comment

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

Compute normals here?

}


bool SurfaceDetection::find_surfaces_old()

Choose a reason for hiding this comment

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

Unless there is a good reason to keep this around in the active code, we should dispense with the original functions.

Eigen::Affine3d result;
// Get boundary trajectory and trim last two poses (last poses are susceptible to large velocity changes)
SS.getBoundaryTrajectory(boundaries, index, poses);
poses.resize(poses.size() - 2);

Choose a reason for hiding this comment

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

We should assert that poses.size() is bigger than 2.

We should understand why the last points are wonky, if they are.

k++;
}

ROS_INFO_STREAM("Cloud has " + std::to_string(boundary_cloud_ptr->points.size()) + " boundary points\n");

Choose a reason for hiding this comment

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

Just use the operator<< overload rather than to_string.

int max=0;
int max_idx=0;

for(int i=0;i<sorted_boundaries.size();i++)

Choose a reason for hiding this comment

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

The results of this for loop don't seem to go anywhere

/*
* Set the orientation for all edge points to be the orientation of the surface normal
* This is a hack that should be removed when planar surfaces assumption is dropped
* The main purpose here is to "smooth" the trajectory of the edges w.r.t. the z axis

Choose a reason for hiding this comment

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

We should open an issue to address this assumption

@Jmeyer1292
Copy link

Merging, will open issues

@woodaaron Please add the next one.

@Jmeyer1292 Jmeyer1292 merged commit 0a2cccc into ros-industrial-attic:kinetic-devel Jan 10, 2017
@Jmeyer1292 Jmeyer1292 mentioned this pull request Jan 27, 2017
3 tasks
@woodaaron woodaaron deleted the base_kinetic branch March 13, 2017 19:41
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.

3 participants