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

Add Intel LibRealSense grabber and viewer #1633

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@lebronzhang

lebronzhang commented Jun 15, 2016

This pull request adds a new grabber for Intel RealSense devices (e.g. Intel® RealSense™ F200, SR300 and R200 cameras) that supports Intel librealsense. I think it is ready to be merged into the PCL.

If there is anything needed to be done, please contact me immediately. Thanks.

@lebronzhang

This comment has been minimized.

Show comment
Hide comment
@lebronzhang

lebronzhang Jun 15, 2016

This grabber could also close #1215

lebronzhang commented Jun 15, 2016

This grabber could also close #1215

@taketwo

This comment has been minimized.

Show comment
Hide comment
@taketwo

taketwo Jun 15, 2016

Member

PCL definitely needs support for librealsense, but I strongly dislike an idea of having both RealSenseGrabber and LibRealSenseGrabber. In my opinion, this should be a single grabber which can choose between backend drivers. From the end-user perspective, there should be no difference if SDK of librealsense is used! Also, in it's current form, LibRealSenseGrabber is largely a copy-paste of the RealSenseGrabber, which suggests that most of the code could and should be shared. Finally, the proposed grabber exposes no control for the frame rate and resolution, which is a major missing feature.

Member

taketwo commented Jun 15, 2016

PCL definitely needs support for librealsense, but I strongly dislike an idea of having both RealSenseGrabber and LibRealSenseGrabber. In my opinion, this should be a single grabber which can choose between backend drivers. From the end-user perspective, there should be no difference if SDK of librealsense is used! Also, in it's current form, LibRealSenseGrabber is largely a copy-paste of the RealSenseGrabber, which suggests that most of the code could and should be shared. Finally, the proposed grabber exposes no control for the frame rate and resolution, which is a major missing feature.

@lebronzhang

This comment has been minimized.

Show comment
Hide comment
@lebronzhang

lebronzhang Jun 16, 2016

@taketwo Thank you for your replay, I am happy to know that PCL needs librealsense. Your points make sense, we should extract just one interface for the RealSense camera users and the control of frame rate and resolution should be added. However, I am not sure how to design the interface for this moment, I will discuss with my colleagues on how to design the grabber and if there is any update, I will post it in this thread. You are always welcomed to give me some suggestions, thank you again.

Good day!

lebronzhang commented Jun 16, 2016

@taketwo Thank you for your replay, I am happy to know that PCL needs librealsense. Your points make sense, we should extract just one interface for the RealSense camera users and the control of frame rate and resolution should be added. However, I am not sure how to design the interface for this moment, I will discuss with my colleagues on how to design the grabber and if there is any update, I will post it in this thread. You are always welcomed to give me some suggestions, thank you again.

Good day!

@lebronzhang

This comment has been minimized.

Show comment
Hide comment
@lebronzhang

lebronzhang Jun 23, 2016

@taketwo Hi, sorry for the late update. I have discussed with my colleague and take your words as a reference, we will change our grabber name as RealSenseGrabber too, and let PCL decide which one will be compiled based on the running operating system and existing library. Then we will extract the common part of two grabbers as pcl::io::real_sense::common, and both grabbers hold the object. Therefore, we could share the common code and deal with SDK related part in our own grabber. The attached files are the former RealSenseGrabber class diagram and the latest RealSenseGrabber class diagram respectively, please check it out. If you have any suggestions, please reach me at once, thank you.
The former RealSenseGrabber class diagram:
the former realsensegrabber class diagram
The latest RealSenseGrabber class diagram:
the latest realsensegrabber class diagram

Good day!

lebronzhang commented Jun 23, 2016

@taketwo Hi, sorry for the late update. I have discussed with my colleague and take your words as a reference, we will change our grabber name as RealSenseGrabber too, and let PCL decide which one will be compiled based on the running operating system and existing library. Then we will extract the common part of two grabbers as pcl::io::real_sense::common, and both grabbers hold the object. Therefore, we could share the common code and deal with SDK related part in our own grabber. The attached files are the former RealSenseGrabber class diagram and the latest RealSenseGrabber class diagram respectively, please check it out. If you have any suggestions, please reach me at once, thank you.
The former RealSenseGrabber class diagram:
the former realsensegrabber class diagram
The latest RealSenseGrabber class diagram:
the latest realsensegrabber class diagram

Good day!

@taketwo

This comment has been minimized.

Show comment
Hide comment
@taketwo

taketwo Jun 29, 2016

Member

Since we decide which driver back-end to use during the configuration of PCL, we don't need any "real" polymorphism. We just need to separate functions into common and backend-specific at the level of source code files.

That means, there is one and only "real_sense_grabber.h", it has no backend-specific code. And in the implementation file "real_sense_grabber.cpp" we keep only common functions, and put SDK and librealsense-dependent stuff into separate files (e.g. "real_sense/sdk/real_sense_grabber.cpp" and "real_sense/librealsense/real_sense_grabber.cpp"). And CMake decides which one to compile. The same for the device manager code.

What do you think?

Member

taketwo commented Jun 29, 2016

Since we decide which driver back-end to use during the configuration of PCL, we don't need any "real" polymorphism. We just need to separate functions into common and backend-specific at the level of source code files.

That means, there is one and only "real_sense_grabber.h", it has no backend-specific code. And in the implementation file "real_sense_grabber.cpp" we keep only common functions, and put SDK and librealsense-dependent stuff into separate files (e.g. "real_sense/sdk/real_sense_grabber.cpp" and "real_sense/librealsense/real_sense_grabber.cpp"). And CMake decides which one to compile. The same for the device manager code.

What do you think?

@lebronzhang

This comment has been minimized.

Show comment
Hide comment
@lebronzhang

lebronzhang Jun 30, 2016

@taketwo Thank you for your suggestion! I love your solution, actually I am doing it mostly like your suggestion since I found it is not a good way to extract a common class, and there is a little bit difference between what I have done and your suggestion. But I think your solution is better than mine, I will use your solution and test it when I have finished.
You are always welcomed to give me another suggestion :)
Thank you again!

Good day!

lebronzhang commented Jun 30, 2016

@taketwo Thank you for your suggestion! I love your solution, actually I am doing it mostly like your suggestion since I found it is not a good way to extract a common class, and there is a little bit difference between what I have done and your suggestion. But I think your solution is better than mine, I will use your solution and test it when I have finished.
You are always welcomed to give me another suggestion :)
Thank you again!

Good day!

@huningxin

This comment has been minimized.

Show comment
Hide comment
@huningxin

huningxin Jul 11, 2016

@lebronzhang , please rebase to master. Thanks!

huningxin commented Jul 11, 2016

@lebronzhang , please rebase to master. Thanks!

@lebronzhang

This comment has been minimized.

Show comment
Hide comment
@lebronzhang

lebronzhang Jul 11, 2016

@taketwo please take a look. Thanks!

lebronzhang commented Jul 11, 2016

@taketwo please take a look. Thanks!

@huningxin

This comment has been minimized.

Show comment
Hide comment
@huningxin

huningxin Jul 11, 2016

@lebronzhang , the buildbot failed, please take a look.

huningxin commented Jul 11, 2016

@lebronzhang , the buildbot failed, please take a look.

@lebronzhang

This comment has been minimized.

Show comment
Hide comment
@lebronzhang

lebronzhang Jul 11, 2016

@huningxin Thank you for your reminder, all checks have passed now:)
@taketwo please take a look, if there is anything that needed to be done, please let me know, thanks!

lebronzhang commented Jul 11, 2016

@huningxin Thank you for your reminder, all checks have passed now:)
@taketwo please take a look, if there is anything that needed to be done, please let me know, thanks!

@taketwo

This comment has been minimized.

Show comment
Hide comment
@taketwo

taketwo Jul 11, 2016

Member

In general looks fine, pretty much how I expected it.

One thing that I'm not satisfied with though is the CMake-related bits. Now we have two options: WITH_RSSDK and WITH_LIBREALSENSE. What happens if both are enabled? Not clear for an end-user. (Actual answer is: SDK will be used.) I think we should have one switch to enable Real Sense support, and another one to choose between backends. But this is something I can look into later after this pull request is merged.

Would be nice to have some people confirming that this works with both backends on Windows and with librealsense on Linux.

Member

taketwo commented Jul 11, 2016

In general looks fine, pretty much how I expected it.

One thing that I'm not satisfied with though is the CMake-related bits. Now we have two options: WITH_RSSDK and WITH_LIBREALSENSE. What happens if both are enabled? Not clear for an end-user. (Actual answer is: SDK will be used.) I think we should have one switch to enable Real Sense support, and another one to choose between backends. But this is something I can look into later after this pull request is merged.

Would be nice to have some people confirming that this works with both backends on Windows and with librealsense on Linux.

{
PCL_WARN ("[pcl::RealSenseGrabber::setConfidenceThreshold] Attempted to set threshold outside valid range (0-15)");
}
else if (!device_->getDevice ()->supports_option (rs::option::f200_confidence_threshold))

This comment has been minimized.

@taketwo

taketwo Jul 11, 2016

Member

Why f200_confidence_threshold? Don't other cameras support it?

@taketwo

taketwo Jul 11, 2016

Member

Why f200_confidence_threshold? Don't other cameras support it?

This comment has been minimized.

@huningxin

huningxin Jul 12, 2016

@SergioRAgostinho

This comment has been minimized.

Show comment
Hide comment
@SergioRAgostinho

SergioRAgostinho Jul 11, 2016

Member

I'm not sure who has hardware for testing this, but we can always request some help on the dev's mailing list.

Although working with grabbers tends to be more of the same, it would also be cool if you guys could provide a tutorial, especially detailing how to select your backend.

Member

SergioRAgostinho commented Jul 11, 2016

I'm not sure who has hardware for testing this, but we can always request some help on the dev's mailing list.

Although working with grabbers tends to be more of the same, it would also be cool if you guys could provide a tutorial, especially detailing how to select your backend.

@taketwo

This comment has been minimized.

Show comment
Hide comment
@taketwo

taketwo Jul 11, 2016

Member

Hardware I have, if only someone gives me some time... :D
I'll give it a try later, but don't know when.

Member

taketwo commented Jul 11, 2016

Hardware I have, if only someone gives me some time... :D
I'll give it a try later, but don't know when.

@SergioRAgostinho

This comment has been minimized.

Show comment
Hide comment
@SergioRAgostinho

SergioRAgostinho Jul 11, 2016

Member

Well, I think this will be integrated into 1.9.0 and it would be cool to release a 1.8.1 before, so I would say there's no rush.

Member

SergioRAgostinho commented Jul 11, 2016

Well, I think this will be integrated into 1.9.0 and it would be cool to release a 1.8.1 before, so I would say there's no rush.

@huningxin

This comment has been minimized.

Show comment
Hide comment
@huningxin

huningxin Jul 12, 2016

I'm not sure who has hardware for testing this, but we can always request some help on the dev's mailing list.

@lebronzhang did test the RSSDK backend on Windows and librealsense backend on Linux. They are the most common use cases IMO.

@lebronzhang , could you please share the test results here?

Although working with grabbers tends to be more of the same, it would also be cool if you guys could provide a tutorial, especially detailing how to select your backend.

Great idea. @taketwo , did you have the RSSDK grabber tutorial? It would be good to have a tutorial of RealSense cameras and include backend selection steps. What do you think?

huningxin commented Jul 12, 2016

I'm not sure who has hardware for testing this, but we can always request some help on the dev's mailing list.

@lebronzhang did test the RSSDK backend on Windows and librealsense backend on Linux. They are the most common use cases IMO.

@lebronzhang , could you please share the test results here?

Although working with grabbers tends to be more of the same, it would also be cool if you guys could provide a tutorial, especially detailing how to select your backend.

Great idea. @taketwo , did you have the RSSDK grabber tutorial? It would be good to have a tutorial of RealSense cameras and include backend selection steps. What do you think?

@lebronzhang

This comment has been minimized.

Show comment
Hide comment
@lebronzhang

lebronzhang Jul 12, 2016

Hi, guys, I have tested the RSSDK backend on Windows and librealsense backend on Linux. Here is the details:
RSSDK backend on Windows
Machine:
OS version: 10.0
CPU: Intel i5-5200U @ 2.20GHz
GPU: NVIDIA GeForce 840M
RSSDK version: 8.0.24.6528
IDE: Visual Studio Professional 2015
The new RealSenseGrabber has same performance as the old one, but there are two problems with my test:

  1. the framerate is low (around 11) with pcl_real_sense_viewer_debug.exe.
  2. When I closed the viewer's window, an exception occurs.

I found the same problems in the PCL master branch, so do you ever come across these problems? What do you think the problem might be? @taketwo

Librealsense backend on Linux
Machine:
OS version: ubuntu 14.04 LTS
CPU: Intel i7-5960X @ 3.00GHz × 16
GPU: NVIDIA GeForce GTX 960
librealsesne version: Compiled with the source
Everything works fine with new RealSenseGrabber, but one thing should be noticed:
The librealsense is written in standards-conforming C++11.
So the CMAKE_CXX_FLAGS at least should be "-std=c++11" to compile PCL with librealsense, maybe you want to mention this in the tutorial of RealSense cameras. @taketwo

lebronzhang commented Jul 12, 2016

Hi, guys, I have tested the RSSDK backend on Windows and librealsense backend on Linux. Here is the details:
RSSDK backend on Windows
Machine:
OS version: 10.0
CPU: Intel i5-5200U @ 2.20GHz
GPU: NVIDIA GeForce 840M
RSSDK version: 8.0.24.6528
IDE: Visual Studio Professional 2015
The new RealSenseGrabber has same performance as the old one, but there are two problems with my test:

  1. the framerate is low (around 11) with pcl_real_sense_viewer_debug.exe.
  2. When I closed the viewer's window, an exception occurs.

I found the same problems in the PCL master branch, so do you ever come across these problems? What do you think the problem might be? @taketwo

Librealsense backend on Linux
Machine:
OS version: ubuntu 14.04 LTS
CPU: Intel i7-5960X @ 3.00GHz × 16
GPU: NVIDIA GeForce GTX 960
librealsesne version: Compiled with the source
Everything works fine with new RealSenseGrabber, but one thing should be noticed:
The librealsense is written in standards-conforming C++11.
So the CMAKE_CXX_FLAGS at least should be "-std=c++11" to compile PCL with librealsense, maybe you want to mention this in the tutorial of RealSense cameras. @taketwo

@SergioRAgostinho

This comment has been minimized.

Show comment
Hide comment
@SergioRAgostinho

SergioRAgostinho Jul 12, 2016

Member

When I closed the viewer's window, an exception occurs.

Backtrace it if possible.

The librealsense is written in standards-conforming C++11.

Well now this definitely pushes it to 1.9.0. ^^

Member

SergioRAgostinho commented Jul 12, 2016

When I closed the viewer's window, an exception occurs.

Backtrace it if possible.

The librealsense is written in standards-conforming C++11.

Well now this definitely pushes it to 1.9.0. ^^

void
populateDeviceList ();
std::vector<DeviceInfo> device_list_;

This comment has been minimized.

@UnaNancyOwen

UnaNancyOwen Jul 12, 2016

Member

This line will occure compile error in MSVC because vector header has not been included. (real_sense_device_manager.h #L123)

@UnaNancyOwen

UnaNancyOwen Jul 12, 2016

Member

This line will occure compile error in MSVC because vector header has not been included. (real_sense_device_manager.h #L123)

This comment has been minimized.

@SergioRAgostinho

SergioRAgostinho Jul 12, 2016

Member

I don't think an error like that would go by unnoticed in Travis. Did you get a compile error while trying to compile this yourself?

@SergioRAgostinho

SergioRAgostinho Jul 12, 2016

Member

I don't think an error like that would go by unnoticed in Travis. Did you get a compile error while trying to compile this yourself?

This comment has been minimized.

@UnaNancyOwen

UnaNancyOwen Jul 12, 2016

Member

Yes, I was confirm that the error occur when trying to compile this on MSVC.

Possibly, I think "WITH_LIBREALSENSE" option is disabled (or librealsense is not found) when checked by Travis. In that case, this code is not compiled in test.
https://travis-ci.org/PointCloudLibrary/pcl/jobs/144050092#L846
https://travis-ci.org/PointCloudLibrary/pcl/jobs/144050093#L891

@UnaNancyOwen

UnaNancyOwen Jul 12, 2016

Member

Yes, I was confirm that the error occur when trying to compile this on MSVC.

Possibly, I think "WITH_LIBREALSENSE" option is disabled (or librealsense is not found) when checked by Travis. In that case, this code is not compiled in test.
https://travis-ci.org/PointCloudLibrary/pcl/jobs/144050092#L846
https://travis-ci.org/PointCloudLibrary/pcl/jobs/144050093#L891

This comment has been minimized.

@lebronzhang

lebronzhang Jul 13, 2016

I haven't tried to compile librealsense backend on MSVC, I will try it and fix the problem.

@lebronzhang

lebronzhang Jul 13, 2016

I haven't tried to compile librealsense backend on MSVC, I will try it and fix the problem.

This comment has been minimized.

@lebronzhang

lebronzhang Jul 13, 2016

Backtrace it if possible.

Exception thrown at 0x00007FFBB6377ACE (intel-ias2.dll) in pcl_real_sense_viewer_debug.exe: 0xC0000005: Access violation reading location 0x0000000000000050.
Does anyone have any ideas?

@lebronzhang

lebronzhang Jul 13, 2016

Backtrace it if possible.

Exception thrown at 0x00007FFBB6377ACE (intel-ias2.dll) in pcl_real_sense_viewer_debug.exe: 0xC0000005: Access violation reading location 0x0000000000000050.
Does anyone have any ideas?

This comment has been minimized.

@lebronzhang

lebronzhang Jul 13, 2016

I have compiled librealsense backend on MSVC and fixed the problems, please check it out. @UnaNancyOwen :)

@lebronzhang

lebronzhang Jul 13, 2016

I have compiled librealsense backend on MSVC and fixed the problems, please check it out. @UnaNancyOwen :)

This comment has been minimized.

@UnaNancyOwen

UnaNancyOwen Jul 13, 2016

Member

I was confirm that it can successfully compile.
I will do operation test within a few days. (I will get the RealSense camera in a few days.)

@UnaNancyOwen

UnaNancyOwen Jul 13, 2016

Member

I was confirm that it can successfully compile.
I will do operation test within a few days. (I will get the RealSense camera in a few days.)

@UnaNancyOwen

This comment has been minimized.

Show comment
Hide comment
@UnaNancyOwen

UnaNancyOwen Jul 13, 2016

Member

Do you have any ideas that to support static link? (realsense-s)
I think it is should implement LIBREALSENSE_LIBRARY_DEBUG to Findlibrealsense.cmake.
(like RSSDK_LIBRARY_DEBUG of FindRSSDK.cmake)

What do you think?

Member

UnaNancyOwen commented Jul 13, 2016

Do you have any ideas that to support static link? (realsense-s)
I think it is should implement LIBREALSENSE_LIBRARY_DEBUG to Findlibrealsense.cmake.
(like RSSDK_LIBRARY_DEBUG of FindRSSDK.cmake)

What do you think?

@lebronzhang

This comment has been minimized.

Show comment
Hide comment
@lebronzhang

lebronzhang Jul 14, 2016

Hi, @UnaNancyOwen :)
I have updated the Findlibrealsense.cmake, please check it out.
Any comments would be appreciated. Thanks!

lebronzhang commented Jul 14, 2016

Hi, @UnaNancyOwen :)
I have updated the Findlibrealsense.cmake, please check it out.
Any comments would be appreciated. Thanks!

@UnaNancyOwen

This comment has been minimized.

Show comment
Hide comment
@UnaNancyOwen

UnaNancyOwen Jul 18, 2016

Member

I has been finished check on Windows. LGTM 👍
Is there anything else?

Member

UnaNancyOwen commented Jul 18, 2016

I has been finished check on Windows. LGTM 👍
Is there anything else?

@lebronzhang

This comment has been minimized.

Show comment
Hide comment
@lebronzhang

lebronzhang Aug 8, 2016

@SergioRAgostinho
Hi, It has been a long time since @UnaNancyOwen had done the test on Windows with good result, so do you have anything else needed to be checked or should we changed the label to "ready to merge"?
I am looking forward to your comment. Thanks!

lebronzhang commented Aug 8, 2016

@SergioRAgostinho
Hi, It has been a long time since @UnaNancyOwen had done the test on Windows with good result, so do you have anything else needed to be checked or should we changed the label to "ready to merge"?
I am looking forward to your comment. Thanks!

@SergioRAgostinho

This comment has been minimized.

Show comment
Hide comment
@SergioRAgostinho

SergioRAgostinho Aug 17, 2016

Member

Hey guys. Thanks a lot for the hard work you put into this, as well from the reviewers.

This is not really a priority at the moment, since it's a new feature it's probably going to be merged only after 1.8.1 is released. Since there's still time, we can definitely address some of the unfinished points in the meantime. @taketwo made this comment a while ago

One thing that I'm not satisfied with though is the CMake-related bits. Now we have two options: WITH_RSSDK and WITH_LIBREALSENSE. What happens if both are enabled? Not clear for an end-user. (Actual answer is: SDK will be used.) I think we should have one switch to enable Real Sense support, and another one to choose between backends.

Also the dependency on c++11 flags from librealsense requires this to be merged after #1638 only.

Like I mentioned before, having a tutorial would be awesome. I cannot stress this enough, it really helps to promote your contribution as users tend to explore the tutorials, not really the repository. Take inspiration from this one of @taketwo.

And finally, we will need to squash those commits at the end.

Member

SergioRAgostinho commented Aug 17, 2016

Hey guys. Thanks a lot for the hard work you put into this, as well from the reviewers.

This is not really a priority at the moment, since it's a new feature it's probably going to be merged only after 1.8.1 is released. Since there's still time, we can definitely address some of the unfinished points in the meantime. @taketwo made this comment a while ago

One thing that I'm not satisfied with though is the CMake-related bits. Now we have two options: WITH_RSSDK and WITH_LIBREALSENSE. What happens if both are enabled? Not clear for an end-user. (Actual answer is: SDK will be used.) I think we should have one switch to enable Real Sense support, and another one to choose between backends.

Also the dependency on c++11 flags from librealsense requires this to be merged after #1638 only.

Like I mentioned before, having a tutorial would be awesome. I cannot stress this enough, it really helps to promote your contribution as users tend to explore the tutorials, not really the repository. Take inspiration from this one of @taketwo.

And finally, we will need to squash those commits at the end.

@lebronzhang

This comment has been minimized.

Show comment
Hide comment
@lebronzhang

lebronzhang Aug 18, 2016

Hi @SergioRAgostinho , thanks for your comment.

The problem which @taketwo commented before should be resolved, and it's not right to leave this trouble to @taketwo as well as the tutorial, I will solve them later.

AS for the c++11 flag, in my opinion, we could add the description in the tutorial that if the user wants to use librealsense backend, he should add c++11 flag when compiling PCL. So we could merge the PR once the tutorial is done and checked, and it's unnecessary to wait until #1638 is solved.

Also, you are right that the commits should be squashed at the end, I will do this when the PR is ready to be merged.

What do you think?

lebronzhang commented Aug 18, 2016

Hi @SergioRAgostinho , thanks for your comment.

The problem which @taketwo commented before should be resolved, and it's not right to leave this trouble to @taketwo as well as the tutorial, I will solve them later.

AS for the c++11 flag, in my opinion, we could add the description in the tutorial that if the user wants to use librealsense backend, he should add c++11 flag when compiling PCL. So we could merge the PR once the tutorial is done and checked, and it's unnecessary to wait until #1638 is solved.

Also, you are right that the commits should be squashed at the end, I will do this when the PR is ready to be merged.

What do you think?

@SergioRAgostinho

This comment has been minimized.

Show comment
Hide comment
@SergioRAgostinho

SergioRAgostinho Aug 25, 2016

Member

I was hoping you'd get more feedback from the other maintainers but I guess their silence might mean exactly that everything is already said. My opinion is the same as 1 week ago :x

Although not explicitly stated anywhere, when there's a patch release, like 1.8.1, we enforce the (unspoken barely ever discussed in public) rule:

  • No new features
  • No ABI/API breakage

In your case, at least the first bullet is broken. That's why I said, there's time to address the proposals from @taketwo.

Member

SergioRAgostinho commented Aug 25, 2016

I was hoping you'd get more feedback from the other maintainers but I guess their silence might mean exactly that everything is already said. My opinion is the same as 1 week ago :x

Although not explicitly stated anywhere, when there's a patch release, like 1.8.1, we enforce the (unspoken barely ever discussed in public) rule:

  • No new features
  • No ABI/API breakage

In your case, at least the first bullet is broken. That's why I said, there's time to address the proposals from @taketwo.

@lebronzhang

This comment has been minimized.

Show comment
Hide comment
@lebronzhang

lebronzhang Aug 26, 2016

@SergioRAgostinho Ok, I get it, rules are rules, thanks for explanation, it's very kind of you:)

lebronzhang commented Aug 26, 2016

@SergioRAgostinho Ok, I get it, rules are rules, thanks for explanation, it's very kind of you:)

@nouei

This comment has been minimized.

Show comment
Hide comment
@nouei

nouei Dec 5, 2016

realsense produces pcl::PointXYZRGBA but what we should do if we want to use pcl::PointXYZRGB ?

nouei commented Dec 5, 2016

realsense produces pcl::PointXYZRGBA but what we should do if we want to use pcl::PointXYZRGB ?

@Aphoh

This comment has been minimized.

Show comment
Hide comment
@Aphoh

Aphoh Jul 21, 2017

Any update on this?

Aphoh commented Jul 21, 2017

Any update on this?

@SergioRAgostinho

This comment has been minimized.

Show comment
Hide comment
@SergioRAgostinho

SergioRAgostinho Jul 21, 2017

Member
Member

SergioRAgostinho commented Jul 21, 2017

@cjue

This comment has been minimized.

Show comment
Hide comment
@cjue

cjue Aug 24, 2017

For those interested in using this on Linux:

  • Merging this PR into current master works without a hitch.
  • Tested on Ubuntu 16.04.2 with a RealSense R200.
  • pcl_real_sense_viewer worked like a charm.

Setup

  • add -std=c++11 to CMAKE_CXX_FLAGS
  • get librealsense and set LIBREALSENSE_DIR. Example:
 sudo apt-get install ros-kinetic-librealsense
 export LIBREALSENSE_DIR=/opt/ros/kinetic/
  • run cmake and build
  • run pcl_real_sense_viewer

cjue commented Aug 24, 2017

For those interested in using this on Linux:

  • Merging this PR into current master works without a hitch.
  • Tested on Ubuntu 16.04.2 with a RealSense R200.
  • pcl_real_sense_viewer worked like a charm.

Setup

  • add -std=c++11 to CMAKE_CXX_FLAGS
  • get librealsense and set LIBREALSENSE_DIR. Example:
 sudo apt-get install ros-kinetic-librealsense
 export LIBREALSENSE_DIR=/opt/ros/kinetic/
  • run cmake and build
  • run pcl_real_sense_viewer
endif ()
else (WIN32)
set(LIBREALSENSE_RELEASE_NAME librealsense.so)
set(LIBREALSENSE_DEBUG_NAME librealsense.a)

This comment has been minimized.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

This doesn't make much sense, in contrast with the windows branch. The first one is the shared lib and the second the static one. But both can be either release or debug.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

This doesn't make much sense, in contrast with the windows branch. The first one is the shared lib and the second the static one. But both can be either release or debug.

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(LIBREALSENSE LIBREALSENSE_LIBRARIES LIBREALSENSE_INCLUDE_DIRS)
endif (LIBREALSENSE_LIBRARIES AND LIBREALSENSE_INCLUDE_DIRS)

This comment has been minimized.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

Lack of new line at the end of the file.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

Lack of new line at the end of the file.

class RealSenseDevice;
class PCL_EXPORTS RealSenseDeviceManager : boost::noncopyable

This comment has been minimized.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

The moment you define your custom destructor the class is stripped of default copy operators, so this is not so usefull anymore. But then again, your destructor is not doing anything so it should just be deleted.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

The moment you define your custom destructor the class is stripped of default copy operators, so this is not so usefull anymore. But then again, your destructor is not doing anything so it should just be deleted.

typedef boost::shared_ptr<RealSenseDeviceManager> Ptr;
static Ptr&

This comment has been minimized.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

Shouldn't this be a const reference? Your class is implemented as a lazy initialized singleton, so I assume you don't want to allow it to be reset and a new manager allocated later on.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

Shouldn't this be a const reference? Your class is implemented as a lazy initialized singleton, so I assume you don't want to allow it to be reset and a new manager allocated later on.

} // namespace io
} // namespace pcl
#endif /* PCL_IO_REAL_SENSE_DEVICE_MANAGER_H */

This comment has been minimized.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

New line at the end of the file.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

New line at the end of the file.

{
// This is called from public captureDevice() functions and should already be
// under scoped lock
RealSenseDevice::Ptr device (new RealSenseDevice (device_info.serial));

This comment has been minimized.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

Use make_shared instead.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

Use make_shared instead.

device->device_ = context_.get_device (device_info.index);
device_info.isCaptured = true;
return device;
}

This comment has been minimized.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

No new line at the end of the file.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

No new line at the end of the file.

friend class RealSenseDeviceManager;
std::string device_id_;
rs::device* device_;

This comment has been minimized.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

I don't like this very much. You're passing a pointer to the user which is managed by the realsense. If the user wants he can delete the pointer. Instead I propose to hold a reference to the rs::device. The user can do whatever he wants with the reference without being able to delete the resource.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

I don't like this very much. You're passing a pointer to the user which is managed by the realsense. If the user wants he can delete the pointer. Instead I propose to hold a reference to the rs::device. The user can do whatever he wants with the reference without being able to delete the resource.

}
}
device->stop ();
}

This comment has been minimized.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

Needs a new line at the end of the file.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

Needs a new line at the end of the file.

pcl::io::real_sense::RealSenseDeviceManager::~RealSenseDeviceManager ()
{
}

This comment has been minimized.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

To be deleted.

@SergioRAgostinho

SergioRAgostinho Sep 3, 2017

Member

To be deleted.

@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot Dec 14, 2017

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

stale bot commented Dec 14, 2017

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

@stale stale bot added the status: stale label Dec 14, 2017

@SergioRAgostinho SergioRAgostinho removed this from the pcl-1.9.0 milestone Dec 19, 2017

@stale stale bot removed the status: stale label Dec 19, 2017

@nawalgupta

This comment has been minimized.

Show comment
Hide comment
@nawalgupta

nawalgupta Jan 19, 2018

Does this grabber supports Intel® RealSense™ SDK 2.0 (Librealsense) 2 for D400, D415, D430 Camera?

nawalgupta commented Jan 19, 2018

Does this grabber supports Intel® RealSense™ SDK 2.0 (Librealsense) 2 for D400, D415, D430 Camera?

@kne1p

This comment has been minimized.

Show comment
Hide comment
@kne1p

kne1p Jan 20, 2018

I'd like to see this merged on master.

If I apply the requested changes, should I create my own pull request (with attribution to lebronzhang), or can I attach it to this one in some way?

kne1p commented Jan 20, 2018

I'd like to see this merged on master.

If I apply the requested changes, should I create my own pull request (with attribution to lebronzhang), or can I attach it to this one in some way?

@SergioRAgostinho

This comment has been minimized.

Show comment
Hide comment
@SergioRAgostinho

SergioRAgostinho Jan 22, 2018

Member

Hey. This one requires C++11 support which we currently don't officially support but it's a milestone for the first quarter/half of this year. So merging to master (even after applying whatever change might be necessary) is not really possible at this point.

I would apply it on your personal fork and keep it public for the time being.

Member

SergioRAgostinho commented Jan 22, 2018

Hey. This one requires C++11 support which we currently don't officially support but it's a milestone for the first quarter/half of this year. So merging to master (even after applying whatever change might be necessary) is not really possible at this point.

I would apply it on your personal fork and keep it public for the time being.

@UnaNancyOwen

This comment has been minimized.

Show comment
Hide comment
@UnaNancyOwen

UnaNancyOwen Jan 22, 2018

Member

@nawalgupta No, This grabber based on librealsense 1.x.
I think RealSense D400 series is one candidate of sensor to replace Kinect series.
In the future, I plan to implement the grabber based on librealsense 2.x.

Member

UnaNancyOwen commented Jan 22, 2018

@nawalgupta No, This grabber based on librealsense 1.x.
I think RealSense D400 series is one candidate of sensor to replace Kinect series.
In the future, I plan to implement the grabber based on librealsense 2.x.

@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot Mar 23, 2018

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

stale bot commented Mar 23, 2018

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

@stale stale bot added the status: stale label Mar 23, 2018

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