Skip to content

Noise model #2

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.

Sign up for GitHub

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

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

Noise model #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
425f9f5
intial commit for noise model, not working
Jul 23, 2018
1e0e5dc
CMakelist for my intial commit
Jul 23, 2018
3e157f1
working model with noise reary for review
Jul 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ target_link_libraries(${PROJECT_NAME}
add_library(${PROJECT_NAME}_interfaces
src/interfaces/pcl_interface.cpp
src/interfaces/opencv_interface.cpp
src/interfaces/noise_model.cpp
)

add_dependencies(${PROJECT_NAME}_interfaces ${${PROJECT_NAME}_EXPORTED_TARGETS} ${catkin_EXPORTED_TARGETS})
Expand Down
8 changes: 6 additions & 2 deletions include/gl_depth_sim/camera_properties.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
#ifndef GL_DEPTH_SIM_CAMERA_PROPERTIES_H
#define GL_DEPTH_SIM_CAMERA_PROPERTIES_H

#include <math.h>
#include <vector>

#include <iostream>
#include <iomanip>
#include <string>
#include <map>
#include <random>
namespace gl_depth_sim
{

Expand Down
17 changes: 17 additions & 0 deletions include/gl_depth_sim/interfaces/noise_model.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#ifndef NOISE_MODEL_H
#define NOISE_MODEL_H

#include <math.h>
Copy link
Owner

Choose a reason for hiding this comment

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

Prefer the C++ equivalent header <cmath> over the C include math.h.

They define the same things but the cmath includes functions in the std:: namespace.

#include <vector>
#include <iostream>
#include <iomanip>
Copy link
Owner

Choose a reason for hiding this comment

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

I think string, iostream, iomanip, map are left over copy paste from an example. My gut tells me that only vector, math, and random are needed.

#include <string>
#include <map>
#include <random>
Copy link
Owner

Choose a reason for hiding this comment

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

Include files that aren't used directly in the header itself should go in the .cpp file. This keeps your interfaces cleaner and helps with build times on large projects.

In this case all the includes but vector can go into the source file.


namespace gl_depth_sim
{
std::vector<float> noise(std::vector<float> distance);
Copy link
Owner

Choose a reason for hiding this comment

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

While I know what you're trying to do here, others will not. Instead of noise, please consider a name that is more descriptive. Perhaps applyKinectNoise. I'd also like to see a brief comment (see the other headers) that describes what this does and gives a shout-out to the paper used as a guide.

Copy link
Owner

Choose a reason for hiding this comment

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

Passing and returning very large objects (like these vectors) can be very expensive. Consider passing the input as a constant reference: const std::vector<float>& distance.

There's arguments to be made that returning by value is not so bad, but you do have options:

  1. Return with an output parameter (so that memory can be re-used)
void noise(const std::vector<float>& distance_in,  std::vector<float>& distance_out);
  1. Modify the vector "in-place":
void (std::vector<float>& distance);


}
#endif // NOISE_MODEL_H
6 changes: 4 additions & 2 deletions src/camera_ros_example.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#include "gl_depth_sim/sim_depth_camera.h"
#include "gl_depth_sim/mesh_loader.h"
#include "gl_depth_sim/interfaces/pcl_interface.h"

#include "gl_depth_sim/interfaces/noise_model.h"
#include <pcl_ros/point_cloud.h>
#include <pcl_conversions/pcl_conversions.h>
#include <ros/ros.h>
Expand Down Expand Up @@ -97,7 +97,9 @@ int main(int argc, char** argv)

const auto pose = lookat(camera_pos, look_at, Eigen::Vector3d(0,0,1));

const auto depth_img = sim.render(pose);
auto depth_img = sim.render(pose);

depth_img.data = gl_depth_sim::noise(depth_img.data);
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a ROS parameter that optionally applies the noise. This should be something that people opt into.


frame_counter++;

Expand Down
19 changes: 19 additions & 0 deletions src/interfaces/noise_model.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#include "gl_depth_sim/interfaces/noise_model.h"

std::vector<float> gl_depth_sim::noise(std::vector<float> distance)
{
float segma;
std::random_device r;
std::seed_seq seed2{r(), r(), r(), r(), r(), r(), r(), r()};
std::mt19937 e2(seed2);
Copy link
Owner

Choose a reason for hiding this comment

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

A thought:

Re-creating these every function call is probably quite expensive. You might consider moving the std::mt19937 object into a struct or something thats passed as an additional argument to the function. That would also let people control the seed sequence used to generate the noise. As it stands, they'll get a totally new one for each operation.

Don't worry about it for now, but if you don't understand the idea of random number seeds then please look it up.


for (int i = 0 ; i < distance.size() ;++i){
Copy link
Owner

Choose a reason for hiding this comment

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

Sign/unsigned comparison.

distances.size() returns an unsigned type. When using STL containers like this, using std::size_t is usually good enough. E.g. int -> std::size_t.

Copy link
Owner

@Jmeyer1292 Jmeyer1292 Jul 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Alternatively you can re-write the entire loop using C++11 range-for. For example.

for (float& r : distance)
{
  // ...

 r = normal_dist(e2);
}

// Seed with a real random value, if available
segma= 0.0012+0.0019* pow(distance[i]-0.4,2);
Copy link
Owner

Choose a reason for hiding this comment

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

All of these numbers, or literals, are of the double type. Not a huge deal, but given that you're starting with floats its not unreasonable to stick with the floating point math here. You can change a literal to a float by appending f, e.g. 0.0012f.

float r = distance[i];
// Generate a normal distribution around that mean
std::normal_distribution<> normal_dist(r, segma);
Copy link
Owner

Choose a reason for hiding this comment

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

Likewise this line creates a normal distribution that is of type double. The <> brackets indicate that it is of the default template type. If you look at the docs here you'll see that means double. This could actually make difference because the random number generator will be asking for twice the random bits that it absolutely needs. Consider specifying the distribution as having template type float: std::normal_distribution<float>.

distance[i] = normal_dist(e2);
}
return distance;
}