Camera class added and fiducial marker detections #7

chris24sahadeo · 2019-07-03T20:59:34Z

Milestone 1a v1

End of milestone 1 draft 1

sarika93

Not done reviewing this, but there is definitely enough here to tackle while I finish reviewing the rest of it.
The YAML formatting needs a big refactor, as well as determining if something is too far away.

rviz_simulator/launch/simulate.launch

rviz_simulator/include/rviz_simulator/target.h

rviz_simulator/src/target.cpp

rviz_simulator/include/rviz_simulator/camera.h

rviz_simulator/src/camera.cpp

TODO: - Function to detect if target is too far away - Change code to confirm to new YAML output requirements - Document desired YAML output with David

Still working on milestone 1 Big Discussion Fixes. Committing just to save progress.

The camera detects more targets on the right side then the left side

Code now generates the new YAML file formats, as specified in the google doc.

sarika93

Requested a few changes. When I ran this there was one instance where I was unable to generate an detections even though there was a tag within range of the camera. I was unable to recreate this afterwards though; if I am able to, I will let you know.

Also you have some very long variable names. Generally, you want names to be clear but with fewer words; abbreaviations are fine once they are clear e.g. dist instead of distance.

rviz_simulator/include/rviz_simulator/camera.h

rviz_simulator/launch/simulate.launch

rviz_simulator/src/camera.cpp

rviz_simulator/launch/simulate.launch

sarika93 · 2019-08-14T13:08:46Z

rviz_simulator/src/simulate.cpp

-  std_msgs::ColorRGBA blue = loadColor(n, "blue");
-  double target_scale;
-  n.getParam("target_scale", target_scale);
+  double target_scale = 0.1;


Tag size should not be hardcoded. This should be a param as was done previously. However, there should be two params instead of one; one for target_x_scale and one for target_y_scale. You can use a default of target_x_scale = target_y_scale = 0.1.

I made the param in the initialize_simulator.yaml file a sequence of length 3 (length, width, depth), example:
target_size: [0.1, 0.1, 0.01]
which is read into a std::vector<double> from the parameter server in the simulate.cpp file (since rosparam cpp can automatically read vectors with no additional code)

The depth is theoretically 0 since the target is a plane, so I gave it a depth of 1cm and set the target class to accept the vector and initialize the visualization marker dimensions accordingly.

rviz_simulator/src/camera.cpp

rviz_simulator/src/simulate.cpp

Commit before converting to iterators

sarika93

Some minor things to fix. Verifying pixel values before I approve.

sarika93 · 2019-08-23T14:58:13Z

rviz_simulator/README.md

 > `camera_intrinsics_file:=<camera_intrinsics_file_name.yaml>`  

-The `simulator` node creates a new folder `"detections_ROS_timestamp"` in the `rviz_simulator` package folder. A new detections folder with a new timestamp is created each time `roslaunch` is run.
+The `simulator` node creates a new folder with the name `"detections_<ROS_timestamp>"` in the `rviz_simulator` package folder.  


The comment you are resolving only said you needed to include an example to the ROS Timestamp; not the entire detections file. The file sample clutters the README.

Additionally, in the below example, it looks like the time-stamped name is associated with the detections YAML file and this is not the case. It's actually used in the folder name.

Instead, if you want to provide detections examples, create an "examples" folder and upload a few examples of the file format to the repo. If you take this approach (which I would recommend), add in comments so anyone who wants to read this through knows what the fields mean and what the format is (e.g. if matrices are row or column major).

sarika93 · 2019-08-23T14:59:12Z

rviz_simulator/include/rviz_simulator/camera.h

@@ -39,8 +39,9 @@
 #include <fstream>

 // for making directories
-#include <sys/stat.h>
-#include <sys/types.h>
+// #include <sys/stat.h>


If these are no longer used, just remove them, rather than commenting it out.

sarika93 · 2019-08-23T15:01:18Z

rviz_simulator/src/camera.cpp

@@ -107,17 +108,17 @@ void Camera::makeOutputDirectories()
  std::string detections_root_folder = package_path + "/detections";

  // if detections root directory does not exist then create it
-  struct stat info;
-  if (stat(detections_root_folder.c_str(), &info) != 0)
+  // struct stat info;


Remove this commented out line.

sarika93 · 2019-08-23T17:50:30Z

rviz_simulator/src/camera.cpp

      ROS_ERROR_STREAM("Error making directory :  " << strerror(errno) << std::endl);
  }

  this->output_folder_path_ = detections_root_folder + "/" + output_folder_name;

-  if (mkdir(this->output_folder_path_.c_str(), 0777) == -1)
+  if (!boost::filesystem::create_directories(this->output_folder_path_))
    ROS_ERROR_STREAM("Error :  " << strerror(errno) << std::endl);



Nit: Remove extra line space for consistency

sarika93 · 2019-08-23T17:52:24Z

rviz_simulator/src/simulate.cpp

    }
    else  // invalid point name
    {
-      ROS_ERROR_STREAM(point_name << " is an invalid param name.");
+      ROS_ERROR_STREAM("The param " << point_name << "was not found or is of an invalid type. Exiting application!");


Nit: Add in a space before "was" for cleaner formatting.

sarika93

Approving after testing (which admittedly wasn't super rigorous), with the assumption that the fixes from the previous review will be made before merging.

chris24sahadeo added 2 commits July 3, 2019 16:41

Testing package

6f31efe

Added camera class.

1d87386

End of milestone 1 draft 1

chris24sahadeo requested review from sarika93, Keyran-H and vpradeep07 July 3, 2019 20:59

ROSCpp Code Format

332954b

sarika93 requested changes Jul 15, 2019

View reviewed changes

chris24sahadeo added 5 commits July 17, 2019 18:26

Commit before making "Big Discussion" changes

93dfd61

TODO: - Function to detect if target is too far away - Change code to confirm to new YAML output requirements - Document desired YAML output with David

Midway commit.

85da2ae

Still working on milestone 1 Big Discussion Fixes. Committing just to save progress.

FIXME:

affcb7d

The camera detects more targets on the right side then the left side

REVIEW ME

39ba34f

Code now generates the new YAML file formats, as specified in the google doc.

Changed target orientations.

cd92303

sarika93 requested changes Jul 29, 2019

View reviewed changes

chris24sahadeo added 2 commits July 29, 2019 14:44

Fixing PR changes

e992ef1

Requested changes made.

b72067d

chris24sahadeo requested a review from sarika93 July 30, 2019 19:09

sarika93 reviewed Aug 14, 2019

View reviewed changes

chris24sahadeo added 3 commits August 15, 2019 11:38

Addressed all of Sarika's milestone 1b comments

73b26ac

Commit before implementing boot::filesystem

38d08de

Switched to boost::filesystem

aa610a4

Commit before converting to iterators

chris24sahadeo requested a review from sarika93 August 23, 2019 00:12

sarika93 reviewed Aug 23, 2019

View reviewed changes

sarika93 approved these changes Aug 30, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Camera class added and fiducial marker detections #7

Camera class added and fiducial marker detections #7

chris24sahadeo commented Jul 3, 2019

sarika93 left a comment

sarika93 left a comment

sarika93 Aug 14, 2019

chris24sahadeo Aug 15, 2019

sarika93 left a comment

sarika93 Aug 23, 2019

sarika93 Aug 23, 2019

sarika93 Aug 23, 2019

sarika93 Aug 23, 2019

sarika93 Aug 23, 2019

sarika93 left a comment

Camera class added and fiducial marker detections #7

Are you sure you want to change the base?

Camera class added and fiducial marker detections #7

Conversation

chris24sahadeo commented Jul 3, 2019

sarika93 left a comment

Choose a reason for hiding this comment

sarika93 left a comment

Choose a reason for hiding this comment

sarika93 Aug 14, 2019

Choose a reason for hiding this comment

chris24sahadeo Aug 15, 2019

Choose a reason for hiding this comment

sarika93 left a comment

Choose a reason for hiding this comment

sarika93 Aug 23, 2019

Choose a reason for hiding this comment

sarika93 Aug 23, 2019

Choose a reason for hiding this comment

sarika93 Aug 23, 2019

Choose a reason for hiding this comment

sarika93 Aug 23, 2019

Choose a reason for hiding this comment

sarika93 Aug 23, 2019

Choose a reason for hiding this comment

sarika93 left a comment

Choose a reason for hiding this comment