Skip to content

Conversation

@nesnes
Copy link
Contributor

@nesnes nesnes commented Feb 26, 2016

Now the wrapper is able to provide the 3D point cloud, the depth map and the camera images at the same time. There is only one zed.launch now, the wrapper automatically adapts on subscribers change.

@Myzhar
Copy link
Member

Myzhar commented Feb 26, 2016

I was thinking about using CUDA to remap Depth Mat to pointcloud.
The loop at
https://github.com/nesnes/zed-ros-wrapper/blob/master/src/zed_wrapper_node.cpp#L122
is really heavy, don't you think that CUDA can give better performances?

Furthermore Depth and RGB information are available in GPU memory, so you have not bottleneck...

adujardin added a commit that referenced this pull request Feb 26, 2016
Point cloud integration and refactoring
@adujardin adujardin merged commit 8054a10 into stereolabs:master Feb 26, 2016
@eric-wieser
Copy link
Contributor

Could you not have squashed these commits a little?

SET(OPENCV_DIR $ENV{OPENCV_DIR})
ELSE() # Linux
find_package(ZED 0.9 REQUIRED)
ENDIF(WIN32)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this change remove windows support?

for (int i = 0; i < size; i++) {
if(p_cloud[index4+2] < 0) { // Check if it's an unvalid point, the depth is lower than 0
index4 += 4;
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

continue is not ok here - you've now left one of the entries in point_cloud.points with unitialized memory, which is impossible for anything downsteam to detect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what we can think, but when you read the std::vector::resize documentation, you see that if no default object is given to the resize as second arg, the default constructor is use the create the new objects. So the continue is possible because all the points in the cloud are initialized by the resize.

http://www.cplusplus.com/reference/vector/vector/resize/

Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably the default constructor for this type is zero-filled? Isn't NaN more desirable for a missing data point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a logic that should be handled by the PointCloud object itself and it will be a bad idea to try to overwrite it. You can see in rviz that the "missing" points aren't displayed, so the information is already in the message.

@eric-wieser
Copy link
Contributor

Ok, there's definitely a bug here due to the multithreading. This worked just fine with the std::thread constructor replaced with a direct function call.

I think the issue is a concurrent interaction like this:

thread1: camera->grab();
thread1: float buffer = camera->retrieveMeasure(...).data
thread2:          do_something_with(*buffer)
thread2:          do_something_with(*buffer)
thread1: camera->grab();
thread2:          do_something_with(*buffer); // either this causes a segfault
thread1: camera->retrieveImage();
thread2:          do_something_with(*buffer); // or this does

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants