Skip to content

Perception tutorial update - #178

Closed
Ridhwanluthra wants to merge 30 commits into
moveit:kinetic-develfrom
Ridhwanluthra:perception_tutorial_update
Closed

Perception tutorial update#178
Ridhwanluthra wants to merge 30 commits into
moveit:kinetic-develfrom
Ridhwanluthra:perception_tutorial_update

Conversation

@Ridhwanluthra

@Ridhwanluthra Ridhwanluthra commented Jun 5, 2018

Copy link
Copy Markdown
Contributor

I have added tutorials for obstacle avoidance and how to use a preprocessed pointcloud with moveit's perception pipeline.
@v4hn @mlautman @feroze

You should now see something like this:

.. image:: preprocess_3d_demo.png
:width: 700px

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not good enough.
The example should leave the point cloud that is used for the octomap as-is and spawn a (finite-length) cylinder CollisionObject that can be used as the object model to grasp.

@mlautman mlautman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a good start. Great job!

This needs significant cleanup before we can review it for content. Ping me when it's ready for another pass.


If you set the initial and the final location of the robot in a way that there is no straight path between them, then the planner will automatically avoid the octomap and plan around it.

.. image:: obstacle_avoidance.gif

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This gif is unnecessary and distracts from the tutorial itself. Please remove

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

based on the discussion I have removed the text

:width: 700px

Getting Started
===============

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See above. Please match syntax to other tutorials for consistency

Comment thread CMakeLists.txt Outdated
add_subdirectory(doc/state_display)
add_subdirectory(doc/interactivity)
add_subdirectory(doc/pick_place)
add_subdirectory(doc/perception_configuration)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please rename directory perception_pipeline to match the name of the tutorial itself

=================================
#################################
Perception Pipeline Tutorial
#################################

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use the same hierarchy for headers as in every other tutorial. This should be ===== to match other tutorials. All section titles below should be adjusted appropriately. I added notes for most of them but be sure to check the rest of your changes to ensure consistency

If you haven't already done so, make sure you've completed the steps in `Getting Started <../getting_started/getting_started.html>`_.

Configuration
=============

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See above. Please match syntax to other tutorials for consistency

seg.setMethodType(pcl::SAC_RANSAC);
seg.setNormalDistanceWeight(0.1);
seg.setMaxIterations(10000);
seg.setDistanceThreshold(0.05);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add comments when using magic numbers

seg.setNormalDistanceWeight(0.1);
seg.setMaxIterations(10000);
seg.setDistanceThreshold(0.05);
seg.setRadiusLimits(0, 1);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add comments when using magic numbers

pcl::PointCloud<pcl::PointXYZRGB>::Ptr cloud_cylinder(new pcl::PointCloud<pcl::PointXYZRGB>());
extract.filter(*cloud_cylinder);
if (cloud_cylinder->points.empty())
std::cerr << "Can't find the cylindrical component." << std::endl;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

see above

std::cerr << "Can't find the cylindrical component." << std::endl;
else
{
std::cerr << "PointCloud representing the cylindrical component: " << cloud_cylinder->points.size()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

see above

---------------
In addition to the steps given in `Getting Started <../getting_started/getting_started.html>`_, you will also need to install the ``perception_pcl`` package: ::

sudo apt install ros-kinetic-perception-pcl

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add new dependencies to package.xml


sudo apt install ros-kinetic-perception-pcl

Running the Code

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general we've been standardizing to a show then tell model. ie. Demo's first then, explanations second

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

need to update the preprocessing section, will update this with that

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is this solved?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The other tutorials put the launch commands for running the demo at the beginning of the tutorial then they go through the steps for integrating this with their custom robot_moveit_config package. It would be great if you made the necessary changes to ros-planning/panda_moveit_config so that this can be run out of the box with a few commands at the start of the tutorial.

@davetcoleman

Copy link
Copy Markdown
Member

I'm excited about this!

@Ridhwanluthra
Ridhwanluthra force-pushed the perception_tutorial_update branch from 65f6a55 to b8b65c0 Compare June 8, 2018 09:49
@Ridhwanluthra
Ridhwanluthra force-pushed the perception_tutorial_update branch from b8b65c0 to 3e96e8b Compare June 8, 2018 10:28
@Ridhwanluthra

Copy link
Copy Markdown
Contributor Author

Here the build fail is because of this link
https://github.com/ros-planning/moveit_tutorials/blob/kinetic-devel/doc/perception_pipeline/perception_pipeline_tutorial.rst

Here the kinetic-devel doesnt contain the perpection pipeline name change and this link should work once merged.

// ModelCoefficients will hold the parameters using which we can define a cylinder of infinite length.
// It has a public attribute values of type std::vector< float >.
// values[0-2] hold a point on the center line of the cylinder.
// values[3-5] hold direction vector of the z-axis.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mlautman I want to write these to be shown as separate lines in the html. how can that be done?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Found a workaround for this using raw tags, I hope that is ok

// radius of cylinder
cylinder_params.push_back(coefficients_cylinder->values[6]);
// i,j,k values of the direction vector of z-axis
cylinder_params.push_back(coefficients_cylinder->values[3]);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't want to include these lines in the tutorial, is there a way to do that?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The best way to do this would be to use BEGIN_SUB_TUTORIAL and END_SUB_TUTORIAL to include things you want and exclude things you don't. Check out the Python MoveGroup tutorial for an aggressive example of this method.

@mlautman mlautman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is going in the right direction. Ping me when these changes are made for another pass


<node pkg="rosbag" type="play" name="point_clouds" args="--clock -k $(find moveit_tutorials)/doc/perception_configuration/bags/2018-05-28-07-49-11.bag" />
<node pkg="rosbag" type="play" name="point_clouds" args="--clock -k $(find moveit_tutorials)/doc/perception_pipeline/bags/2018-05-28-07-49-11.bag" />
<!-- <node pkg="moveit_tutorials" type="add_object" name="adding" /> -->

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please remove commented out code. If it needs to be here for some reason, add a comment explaining why

<node pkg="tf" type="static_transform_publisher" name="to_panda_base" args="0 0 0 0 0.2 1.92 camera_rgb_optical_frame temp_link 100" />

<node pkg="rosbag" type="play" name="point_clouds" args="--clock -k $(find moveit_tutorials)/doc/perception_configuration/bags/2018-05-28-07-49-11.bag" />
<node pkg="rosbag" type="play" name="point_clouds" args="--clock -k $(find moveit_tutorials)/doc/perception_pipeline/bags/2018-05-28-07-49-11.bag" />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add comment

<arg name="debug" value="$(arg debug)"/>
</include>

<node pkg="moveit_tutorials" type="cylinder_segment" name="point_cloud_preprocessor" />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add comment

box_pose.orientation.z = axis.z() * sin(angle / 2);
box_pose.orientation.w = cos(angle / 2);

box_pose.position.x = cylinder_params->at(4);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No good reason to use ->at() here. Might as well just reference the index directly. See above comment about passing in a struct rather than a vector.

#include <moveit_msgs/CollisionObject.h>

void cloud_cb(const sensor_msgs::PointCloud2ConstPtr& input)
void add_object(std::vector<double>* cylinder_params)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rename function something like add_cylinder

Also, it is generally bad practice to pass in a vector as the container for some list of inputs. You should create a struct and pass that in.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

extract.filter(*cloud);
}

void cloud_cb(const sensor_msgs::PointCloud2ConstPtr& input)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

much better! Now add a comment for the callback :)

extractCylinder(cloud, coefficients_cylinder, cloud_normals);
if (cloud->points.empty())
{
ROS_ERROR_STREAM("Can't find the cylindrical component.");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

return here

{
ROS_ERROR_STREAM("Can't find the cylindrical component.");
}
else

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No need for else if you return above

// radius of cylinder
cylinder_params.push_back(coefficients_cylinder->values[6]);
// i,j,k values of the direction vector of z-axis
cylinder_params.push_back(coefficients_cylinder->values[3]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The best way to do this would be to use BEGIN_SUB_TUTORIAL and END_SUB_TUTORIAL to include things you want and exclude things you don't. Check out the Python MoveGroup tutorial for an aggressive example of this method.

cylinder_params.push_back(coefficients_cylinder->values[4]);
cylinder_params.push_back(coefficients_cylinder->values[5]);
get_coords(cloud, &cylinder_params);
// for (auto i: cylinder_params)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

delete

@mlautman

Copy link
Copy Markdown
Contributor

@Ridhwanluthra This PR is getting really big. Let's get these changes fixed up and merged in before adding new features/content to the tutorial.

ridhwanluthra added 4 commits June 12, 2018 04:31
used workaround for new line and code formatting in the tutorial-formatter
* passing struct instead of vector to funcs
* variable names improved
* comments explaining functions added
* other minor fixes
@Ridhwanluthra

Copy link
Copy Markdown
Contributor Author

@mlautman I think I have handled all the reviews and comments. Could you please give it another pass and see if there is anything else that needs to be done

@Ridhwanluthra

Copy link
Copy Markdown
Contributor Author

Where should the bag file be maintained? It is too large to be uploaded on git. Its around 2GB.

@mlautman

Copy link
Copy Markdown
Contributor

I'll take a look in the next few days. Regarding the bag file, check out https://git-lfs.github.com/ and see if you can't get that setup.

@Ridhwanluthra

Copy link
Copy Markdown
Contributor Author

I had set that up but had to remove it as it only works for enterprises and is not available for public repos

@mlautman mlautman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is really good! So close.

I feel bad that this is taking so long to get merged but there are still a few things that have to get fixed before then. The biggest blocker is that you will need to make the necessary changes to ros-planning/panda_moveit_config so that these demo's can be run out of the box. Other than that, we need to be a little more specific on the instructions so that someone who is a complete noob can get through the tutorial with their custom robot_moveit_config. Once these two issues and some minor changes are made this should be good to go!

Ping me when you've made the suggested changes and after one more pass we should be good to merge!

@@ -0,0 +1,56 @@
<launch>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems rather redundant. Is there a reason you didn't <include> demo.launch and add the additional nodes specific to this demo?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess I started testing that way and didnt change

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please refactor when you get a chance

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ya did that

@@ -0,0 +1,53 @@
<launch>
<param name ="/use_sim_time" value="true"/>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above. At minimum the common elements should be refactored into a common launch file that is included by each of these two scripts.

// There are 4 fields and a total of 7 parameters used to define this.
struct add_cylinder_params
{
// Radius of the cylinder.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit pick: If you use /* comment */ instead of // comment for minor comments like these the auto-generated tutorial tends to look a little better

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is much better

Comment thread CMakeLists.txt
moveit_ros_planning_interface
pluginlib
geometric_shapes
pcl_ros

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@v4hn Is adding this dependency going to be an issue?

<node pkg="tf" type="static_transform_publisher" name="to_panda_base" args="0 0 0 0 0.2 1.92 camera_rgb_optical_frame temp_link 100" />

<!-- Play the rosbag that contains the pointcloud data -->
<node pkg="rosbag" type="play" name="point_clouds" args="--clock -k $(find moveit_tutorials)/doc/perception_pipeline/bags/2018-05-28-07-49-11.bag" />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just created a S3 account for hosting this. If you send me this and other large files I can give you permalinks so that users can download them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

segmentor.setOptimizeCoefficients(true);
segmentor.setModelType(pcl::SACMODEL_PLANE);
segmentor.setMethodType(pcl::SAC_RANSAC);
// run at max 1000 iterations before giving up

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

segmentor.setMethodType(pcl::SAC_RANSAC);
// run at max 1000 iterations before giving up
segmentor.setMaxIterations(1000);
// tolerence for variation from model

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

// tolerence for variation from model
segmentor.setDistanceThreshold(0.01);
segmentor.setInputCloud(cloud);
// Create the segmentation object for the planar model and set all the parameters

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

// Create the segmentation object for the planar model and set all the parameters
pcl::ModelCoefficients::Ptr coefficients_plane(new pcl::ModelCoefficients);
segmentor.segment(*inliers_plane, *coefficients_plane);
// Extract the planar inliers from the input cloud

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

pcl::ExtractIndices<pcl::PointXYZRGB> extract_indices;
extract_indices.setInputCloud(cloud);
extract_indices.setIndices(inliers_plane);
// Remove the planar inliers, extract the rest

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

* /**/ comments
* refactor launch
* details on config storage and use
* better dependency management
@mlautman

mlautman commented Jun 25, 2018

Copy link
Copy Markdown
Contributor

I've uploaded the bag to S3.
https://s3.us-east-2.amazonaws.com/picknik-basket/2018-05-28-07-49-11.bag

Please add instructions for downloading to the correct location using wget. (you can assume their workspace is located at ~/ws_moveit

@Ridhwanluthra

Copy link
Copy Markdown
Contributor Author

@mlautman does it make sense to create an issue for this problem?

@Ridhwanluthra

Copy link
Copy Markdown
Contributor Author

Also, there is no license in this yet.

@mlautman

mlautman commented Jul 9, 2018

Copy link
Copy Markdown
Contributor

@Ridhwanluthra Please add the license. Copy it in from one of the other tutorials


YAML Configuration file (Point Cloud)
-------------------------------------
+++++++++++++++++++++++++++++++++++++

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Ridhwanluthra are you aware of @mohmadAyman's work relating to setting up 3D sensors moveit/moveit#969

Can both of cross-review these PRs to ensure there is no duplicate or contradictory effort? Thanks!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure!

+++++++++++++++++++++++++++++++++++++

We will have to generate a YAML configuration file for configuring the 3D sensors. Please see `this example file <https://github.com/ros-planning/panda_moveit_config/blob/master/config/sensors_kinect_pointcloud.yaml>`_ for processing point clouds, located in the `panda_moveit_config repository for Kinetic <https://github.com/ros-planning/panda_moveit_config>`_: ::
We will have to generate a YAML configuration file for configuring the 3D sensors. Please see `this example file <https://github.com/ros-planning/panda_moveit_config/blob/master/config/sensors_kinect_pointcloud.yaml>`_ for processing point clouds, located in the `panda_moveit_config repository for Kinetic <https://github.com/ros-planning/panda_moveit_config>`_.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd prefer we use the examples that are almost available in the moveit setup assistant:

moveit_setup_assistant/resources/default_config/sensors_3d.yaml

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The challenge with including that would be that it won't work out of the box as we can't directly add that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From a naming stand point, would sensors_3d.yaml be better? as more generalized.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 to sensors_3d.yaml for the name

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As of now it matches the files in panda_moveit_config.
depthmap
pointcloud

Also, If the name is same the user will have to delete 1 ffile and then use it. or if its all included in a single file they will have to comment out one of the plugins. Which would lead to unnecessary confusion for a newcomer.

ros.moveit_ros_planning.planning_scene_monitor: Transform error: Lookup would require extrapolation into the future. Requested time 1527473962.793050157 but the latest data is at time 1527473962.776993978, when looking up transform from frame [panda_link2] to frame [camera_rgb_optical_frame]
ros.moveit_ros_perception: Transform cache was not updated. Self-filtering may fail.

We are working on fixing it, it should not break the working of the demo.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a Github issue tracking the progress of this problem? Please link to it (and create it if needed)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,46 @@
#include "ros/ros.h"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add license

int main(int argc, char **argv)
{
ros::init(argc, argv, "bag_publisher_maintain_time");
ros::NodeHandle n;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nh preferred, or something even longer

cylinder_params->direction_vec[2]);
Eigen::Vector3d origin_z_direction(0., 0., 1.);
Eigen::Vector3d axis;
double angle;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

move inline with angle = ...


/** \brief Given the pointcloud containing just the cylinder, compute its center point and its height and store in
cylinder_params.
@param cloud - Pointcloud containning just the cylinder.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"containing"

extract.filter(*cloud);
}

void cloud_cb(const sensor_msgs::PointCloud2ConstPtr& input)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cloudCB

// END_SUB_TUTORIAL
if (cloud->points.empty())
{
ROS_ERROR_STREAM("Can't find the cylindrical component.");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ROS_ERROR_STREAM_NAMED("file_name", "...

ROS_ERROR_STREAM("Can't find the cylindrical component.");
return;
}
static bool points_not_found = true;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you should avoid static - is it really needed here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

static is needed here as the function will be called again and again but we want to compute the cylinder params only once and also publish it only once

Comment thread CMakeLists.txt Outdated
add_subdirectory(doc/state_display)
add_subdirectory(doc/interactivity)
add_subdirectory(doc/pick_place)
add_subdirectory(doc/perception_pipeline) No newline at end of file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add a new line here :)


YAML Configuration file (Point Cloud)
-------------------------------------
+++++++++++++++++++++++++++++++++++++

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure!

/*********************************************************************
* Software License Agreement (BSD License)
*
* Copyright (c) 2012, Willow Garage, Inc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is there a need to update this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yea make it this year and your name

mlautman and others added 7 commits July 16, 2018 21:38
* added tests for running CHOMP in scenes with obstacles into the CHOMP tutorial

* added section on Tweaking some of the parameters for CHOMP_under-construction commit

* added: CHOMP parameters, difference in plans OMPL/CHOMP, python scripts edited

* corrected scripts link

* fixed formatting issues

* PR changes requested

* add_randomness is not used in the CHOMP code

* addressed the PR changes requested

* fixed python script bugs, removed sleep

* minor bug changed the file name

* changed class-names to be same as file-name

* addressed PR requested changes for python script and tutorial document

* addition of correct number of arguments check

* added chomp_planner into index.rst and removed ros spin call

* fixed print statement typo

* fixed travis build errors
Add a description of the controller-specific duration parameters,
including a discussion of the difference to the global values,
as well as an example how to set them.
* Added tutorial for STOMP Motion planner

* addressed PR#185 requested changes

* addresses PR suggested changes

* placed collision_scene_example.py in collision_environments folder

* added section on running STOMP unit tests
* updated move_group_interface tutorial's Cartesian path waypoints (example moveit#4) so path is triangular
* updated description of the path
+++++++++++++++++++++++++++++++++++

We will have to generate a YAML configuration file for configuring the 3D sensors. An `example file for processing depth images <https://github.com/ros-planning/panda_moveit_config/blob/master/config/sensors_kinect_depthmap.yaml>`_ can be found in the `panda_moveit_config repository <https://github.com/ros-planning/panda_moveit_config>`_ as well: ::
We will have to generate a YAML configuration file for configuring the 3D sensors. An `example file for processing depth images <https://github.com/ros-planning/panda_moveit_config/blob/master/config/sensors_kinect_depthmap.yaml>`_ can be found in the `panda_moveit_config repository <https://github.com/ros-planning/panda_moveit_config>`_ as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is something I want to ask about since I'm not sure, is it possible to have both plugins working at the same time?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i don't think so but im not 100% on that

<rosparam command="load" file="$(find panda_moveit_config)/config/sensors_kinect.yaml" />
<rosparam command="load" file="$(find panda_moveit_config)/config/sensors_kinect_pointcloud.yaml" />

If you are using depthmap change the name of the yaml file to ``sensors_kinect_depthmap.yaml``.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Depending if the perception screen was to be merged first, adding this line would be automated.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

agreed. this tutorial can be simplified by relying on the perception screen

+++++++++++++++++++++++++++++++++++++

We will have to generate a YAML configuration file for configuring the 3D sensors. Please see `this example file <https://github.com/ros-planning/panda_moveit_config/blob/master/config/sensors_kinect_pointcloud.yaml>`_ for processing point clouds, located in the `panda_moveit_config repository for Kinetic <https://github.com/ros-planning/panda_moveit_config>`_: ::
We will have to generate a YAML configuration file for configuring the 3D sensors. Please see `this example file <https://github.com/ros-planning/panda_moveit_config/blob/master/config/sensors_kinect_pointcloud.yaml>`_ for processing point clouds, located in the `panda_moveit_config repository for Kinetic <https://github.com/ros-planning/panda_moveit_config>`_.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 to sensors_3d.yaml for the name

+++++++++++++++++++++++++++++++++++

We will have to generate a YAML configuration file for configuring the 3D sensors. An `example file for processing depth images <https://github.com/ros-planning/panda_moveit_config/blob/master/config/sensors_kinect_depthmap.yaml>`_ can be found in the `panda_moveit_config repository <https://github.com/ros-planning/panda_moveit_config>`_ as well: ::
We will have to generate a YAML configuration file for configuring the 3D sensors. An `example file for processing depth images <https://github.com/ros-planning/panda_moveit_config/blob/master/config/sensors_kinect_depthmap.yaml>`_ can be found in the `panda_moveit_config repository <https://github.com/ros-planning/panda_moveit_config>`_ as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i don't think so but im not 100% on that

<rosparam command="load" file="$(find panda_moveit_config)/config/sensors_kinect.yaml" />
<rosparam command="load" file="$(find panda_moveit_config)/config/sensors_kinect_pointcloud.yaml" />

If you are using depthmap change the name of the yaml file to ``sensors_kinect_depthmap.yaml``.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

agreed. this tutorial can be simplified by relying on the perception screen

/*********************************************************************
* Software License Agreement (BSD License)
*
* Copyright (c) 2012, Willow Garage, Inc.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yea make it this year and your name

* copyright notice, this list of conditions and the following
* disclaimer in the documentation and/or other materials provided
* with the distribution.
* * Neither the name of Willow Garage nor the names of its

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

your name

* copyright notice, this list of conditions and the following
* disclaimer in the documentation and/or other materials provided
* with the distribution.
* * Neither the name of Willow Garage nor the names of its

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add your name here


// BEGIN_SUB_TUTORIAL param_struct
// There are 4 fields and a total of 7 parameters used to define this.
struct addCylinderParams

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The names of all types — classes, structs, type aliases, enums, and type template parameters — have the same naming convention. Type names should start with a capital letter and have a capital letter for each new word. No underscores.

https://google.github.io/styleguide/cppguide.html#Type_Names

// angle the point made horizontally- |code_start| atan2(pt.z,pt.x);\ |code_end| |br|
// angle the point made Vertically- |code_start| atan2(pt.z, pt.y);\ |code_end| |br|
// Loop over the entire pointcloud.
for (auto const pt : cloud->points)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Longer variable name:

pt -> point

pointcloud and store the cylinder parameters in coefficients_cylinder.
@param cloud - Pointcloud whose plane is removed.
@param coefficients_cylinder - Cylinder parameters used to define an infinite cylinder will be stored here.
@param cloud_normals - Point normals corresponding to the plane on which cylinder is kept */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is awesome documentation

ROS_ERROR_STREAM_NAMED("cylinder_segment", "Can't find the cylindrical component.");
return;
}
static bool points_not_found = true;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

rather than use static, make these functions part of a class and use a member variable

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have converted it to a class but I was wondering why is keeping static such a bad idea?

ridhwanluthra added 3 commits August 6, 2018 11:29
* license naming and year
* conversion to class to remove static
* naming and styling fix
@@ -0,0 +1,319 @@
#include <ros/ros.h>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add license

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is referenced from an earlier commit. The license has already been added

+++++++++++++++++++++++++++++++++++++

We will have to generate a YAML configuration file for configuring the 3D sensors. Please see `this example file <https://github.com/ros-planning/panda_moveit_config/blob/master/config/sensors_kinect_pointcloud.yaml>`_ for processing point clouds, located in the `panda_moveit_config repository for Kinetic <https://github.com/ros-planning/panda_moveit_config>`_: ::
We will have to generate a YAML configuration file for configuring the 3D sensors. Please see `this example file <https://github.com/ros-planning/panda_moveit_config/blob/master/config/sensors_kinect_pointcloud.yaml>`_ for processing point clouds, located in the `panda_moveit_config repository for Kinetic <https://github.com/ros-planning/panda_moveit_config>`_.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

v4hn pushed a commit that referenced this pull request Aug 13, 2018
includes
 - configuration
 - obstacle avoidance
 - cylinder segmentation
 - planning scene update

This commit adds a rather small rosbag file as an example for the tutorials.
@v4hn
v4hn dismissed davetcoleman’s stale review August 13, 2018 17:21

feedback addressed or too detailed to get this merged eventually..

@v4hn

v4hn commented Aug 13, 2018

Copy link
Copy Markdown
Contributor

I just looked through the tutorial, fixed a few minor issues and merged it as 160e941 .
Thank you a lot @Ridhwanluthra !

Not all feedback in this pull-request has been addressed or thoroughly discussed, but the comment history was way to long already and it took a lot of effort to look through everything to judge the current quality.
I believe this tutorial could use some further contributions in the future,
especially with respect to the other GSoC contributions, but so could a lot of other parts of the tutorials.

@v4hn v4hn closed this Aug 13, 2018
rhaschke pushed a commit to moveit/panda_moveit_config that referenced this pull request Nov 13, 2018
Rename camera to match perception tutorial: moveit/moveit_tutorials#178.
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.

7 participants