Add Ensenso support #888

VictorLamoine · 2014-09-05T20:52:12Z

If you want to test please read the tutorial first. For the last steps you will, of course, need an Ensenso device!

The Ensenso grabber has been written like the OpenNI grabber class
The code contains 2 examples, with/without the Ensenso grabber class
The code is documented
Support for Windows has not been tested
A tutorial has been written to explain how to use the Ensenso in PCL

VictorLamoine · 2014-10-02T09:29:27Z

Does PCL even use the .pc files it generates? I deleted all .pc files on my system and PCL projects still builds.

How does PCL projects builds when using a simple CMakeFile?

jspricke · 2014-10-02T09:40:36Z

PCL provides PCLConfig.cmake as well as pkg-config (.pc) files. Your CMakeLists.txt only depends on the PCLConfig.cmake. If you want to use pkg-config, use it, or use it from within cmake, like this:

find_package(PkgConfig)
pkg_check_modules(pcl_cmmon-1.8)

VictorLamoine · 2014-10-06T11:31:24Z

Thank you Jochen you spared a lot of my time 😄
This pull request is ready to be reviewed/merged!

I edited my first message to explain the modifications done.

taketwo · 2014-10-06T20:13:33Z

PCLConfig.cmake.in

@@ -247,6 +247,37 @@ macro(find_openni2)
  endif(OPENNI2_FOUND)
 endmacro(find_openni2)

+#remove this as soon as the Ensenso SDK is shipped with FindEnsenso.cmake
+macro(find_ensenso)


I wonder why we need both this and cmake/Modules/FindEnsenso.cmake?

cmake/Modules/FindEnsenso.cmake is checked by cmake to determine whether the system has Ensenso SDK or not: CMakeLists.txt

Yes, that part I understand. But why this macro?

If we don't have the macro in PCLConfig.cmake.in then the generated PCLConfig.cmake will not be able to check whether the system has the Ensenso SDK or not.

The Ensenso SDK does not install a cmake file (eg: in /usr/share/cmake-2.8/Modules/) in the system. Thus PCL cmake file has to embed its own way to find if the Ensenso SDK is there or not.

This is what I understood. Hope it's right.. 😅

Okay, thanks for the explanation. I am not very familiar with this beast :)

PCLConfig.cmake is used by projects using the PCL to find the headers and libs. Adding Ensenso in there allows these projects to transparently use the Ensenso SDK.

taketwo · 2014-10-08T08:48:34Z

That's all from my side. @jspricke please merge if you think the PR is in good shape.

Thanks for contributing, Victor!

jspricke · 2014-10-08T13:56:54Z

Apart from my comments above, I'm fine with merging this. Thanks for the work Victor!

VictorLamoine · 2014-10-09T12:49:15Z

No more pkg-config file. I've came up with a solution that seems perfect:

In PCLConfig.cmake.in the guess path are taken from the cmake variables ENSENSO_INCLUDE_DIR and ENSENSO_LIBRARY. So whatever was the user configuration in cmake the path are copied.

ENSENSO_INCLUDE_DIR and ENSENSO_LIBRARY are set by cmake at default values here, it can be changed by the user if he has moved the Ensenso SDK files.

VictorLamoine · 2014-10-09T13:19:27Z

I tested how this behaves by moving my include directory and it seems to be working fine.

taketwo · 2014-10-09T20:57:58Z

@jspricke As a CMake expert, could you have another look at config files and merge if they are okay? :)

jspricke · 2014-10-12T22:34:38Z

@VictorLamoine looks like the CMake stuff is back to it's beginning. Did you remove the version without pkg-config intentionally?

VictorLamoine · 2014-10-13T06:51:31Z

@jspricke I pushed the wrong branch, it is now back sorry.

Did you remove the version without pkg-config intentionally?

Yes we can get rid of it as long as the Ensenso SDK doesn't provide a pkg-config file.

jspricke · 2014-10-16T08:07:15Z

CMakeLists.txt

+option(WITH_ENSENSO "Build IDS-Imaging Ensenso's camera support" TRUE)
+if(WITH_ENSENSO)
+  set(ENSENSO_API_DIR "/opt/ensenso/development/c/" CACHE PATH "directory of Ensenso API")
+  set(ENSENSO_LIB_DIR "/usr/lib/" CACHE PATH "directory of Ensenso library")


AFAIR find_package will search in /usr/lib by default.

find_library that is.

jspricke · 2014-10-16T08:17:22Z

I'm still not sure we should merge the ensenso_grabber_example.cpp in it's current form, as it doubles the code from the pcl::EnsensoGrabber. I would propose to rewrite it and to host the original version somewhere else. @taketwo what do you think?

Also, I think we should be more strict about the namespace in future. So pcl::EnsensoGrabber should be pcl::io::EnsensoGrabber.

taketwo · 2014-10-16T09:04:22Z

I'm still not sure we should merge the ensenso_grabber_example.cpp

I understand that Victor followed the footsteps of openni grabber developers, who also provided such an example. Though I do not see what's the purpose. The point of this contribution is to provide a grabber class that abstracts away the API specific to the device driver, so why would anyone in "PCL world" be interested in using nxLib.h directly? So +1 for removing the example.

Also, I think we should be more strict about the namespace in future.

So far none of the existing grabbers sit in pcl::io namespace, so I don't think it is reasonable to add this one to that namespace. However, in principle I agree, proper namespacing is a good thing to do.

What about dropping "simple" from pcl_ensenso_simple_viewer? There is no other "complex" viewer anyways. Also the viewers for openni/openni2 are called pcl_XXX_viewer.

jspricke · 2014-10-16T09:09:21Z

Sergey Alexandrov [email protected] [2014-10-16 02:04]:

I understand that Victor followed the footsteps of openni grabber developers, who also provided such an example. Though I do not see what's the purpose. The point of this contribution is to provide a grabber class that abstracts away the API specific to the device driver, so why would anyone in "PCL world" be interested in using nxLib.h directly? So +1 for removing the example.

find -name "*grabber_example*"
./io/tools/openni_grabber_example.cpp
./io/tools/hdl_grabber_example.cpp
./apps/src/dinast_grabber_example.cpp

..all using their internal grabber, btw ;).

So far none of the existing grabbers sit in pcl::io namespace, so I don't think it is reasonable to add this one to that namespace. However, in principle I agree, proper namespacing is a good thing to do.

I think we should start this now ;).

What about dropping "simple" from pcl_ensenso_simple_viewer? There is no other "complex" viewer anyways. Also the viewers for openni/openni2 are called pcl_XXX_viewer.

+1

taketwo · 2014-10-16T09:11:17Z

I think we should start this now ;).

Perhaps a separate PR later that puts all of them into pcl::io?

jspricke · 2014-10-16T09:12:42Z

Sergey Alexandrov [email protected] [2014-10-16 02:11]:

Perhaps a separate PR later that puts all of them into pcl::io?

Can we make forward declarations to be backward compatible?

taketwo · 2014-10-16T09:35:00Z

As far as I see, there is just a single forward-declaration for a grabber in the whole PCL codebase:

apps/in_hand_scanner/include/pcl/apps/in_hand_scanner/in_hand_scanner.h
59:  class OpenNIGrabber;

Shouldn't something like the following work?

namespace pcl
{

  namespace io
  {

    class OpenNIGrabber
    {
      // ...
    };

  }

  using io::OpenNIGrabber;

}

jspricke · 2014-10-16T11:45:39Z

Sergey Alexandrov [email protected] [2014-10-16 02:35]:

Shouldn't something like the following work?

I guess so. Maybe we should move this discussion to pcl-dev@?

VictorLamoine · 2014-10-16T12:26:17Z

I've done the modifications to the CMake files using Windows guess paths.

I understand that Victor followed the footsteps of openni grabber developers, who also provided such an example. Though I do not see what's the purpose. The point of this contribution is to provide a grabber class that abstracts away the API specific to the device driver, so why would anyone in "PCL world" be interested in using nxLib.h directly? So +1 for removing the example.

If the user wants to do something that has not been coded in the pcl::EnsensoGrabber he will HAVE to use the device specific API by using the root_ member of the grabber. This explain why root_ is public.

taketwo · 2014-10-16T12:31:35Z

@VictorLamoine please update the tutorial text.

VictorLamoine · 2014-10-16T12:39:23Z

All good on my side.

jspricke · 2014-10-16T12:41:54Z

+1 from me, thanks a lot @VictorLamoine!

Add Ensenso support

taketwo · 2014-10-19T17:28:21Z

@VictorLamoine any idea about the issue brought up in this comment?

VictorLamoine · 2014-10-19T20:00:45Z

Configuring a simple PCL program (pcd write example):

-- Could NOT find OpenNI2 (missing:  OPENNI2_LIBRARY OPENNI2_INCLUDE_DIRS) 
** WARNING ** io features related to openni2 will be disabled
-- Could NOT find ensenso (missing:  ENSENSO_LIBRARY ENSENSO_INCLUDE_DIR) 
** WARNING ** io features related to ensenso will be disabled

Everything is normal at this point (I removed Ensenso libraries/include dirs from my OS)

CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
ENSENSO_LIBRARY
    linked by target "pcd_write" in directory /home/victor/Copy/Autres/pcd_write/src

ccmake shows:

 ENSENSO_INCLUDE_DIR              ENSENSO_INCLUDE_DIR-NOTFOUND
 ENSENSO_LIBRARY                  ENSENSO_LIBRARY-NOTFOUND
[...]
 OPENNI2_INCLUDE_DIRS             OPENNI2_INCLUDE_DIRS-NOTFOUND
 OPENNI2_LIBRARY                  OPENNI2_LIBRARY-NOTFOUND

I don't know why OPENNI2_LIBRARY-NOTFOUND is ok and ENSENSO_LIBRARY-NOTFOUND is not.

taketwo · 2014-10-19T21:18:27Z

I suspect these two lines: https://github.com/PointCloudLibrary/pcl/blob/master/PCLConfig.cmake.in#L267-L268

VictorLamoine · 2015-06-10T18:51:15Z

@simonschmeisser you might be interested in the fact that PCL trunk version supports the Ensenso cameras (thus, ROS); tutorial.

simonschmeisser · 2015-06-10T21:03:45Z

Thanks for the hint, I came across your tutorial some while ago already and definitely plan to compare it to my current halcon based approach. Nice job!

taketwo reviewed Oct 6, 2014
View reviewed changes

jspricke reviewed Oct 16, 2014
View reviewed changes

Add Ensenso support in CMake files

7ce6008

VictorLamoine added 3 commits October 16, 2014 14:32

Add EnsensoGrabber class

da40d1f

Add Ensenso viewer

398dd73

Add Ensenso documentation & tutorial

f513f15

taketwo added a commit that referenced this pull request Oct 16, 2014

Merge pull request #888 from VictorLamoine/ensenso

75d97aa

Add Ensenso support

taketwo merged commit 75d97aa into PointCloudLibrary:master Oct 16, 2014

VictorLamoine deleted the ensenso branch October 16, 2014 17:23

VictorLamoine mentioned this pull request Oct 24, 2014

Move all grabbers into pcl::io namespace #975

Open

taketwo mentioned this pull request Jun 18, 2017

pcl::io::OpenNI2Grabber is not included in API reference #1864

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Ensenso support #888

Add Ensenso support #888

VictorLamoine commented Sep 5, 2014

VictorLamoine commented Oct 2, 2014

jspricke commented Oct 2, 2014

VictorLamoine commented Oct 6, 2014

taketwo Oct 6, 2014

VictorLamoine Oct 7, 2014

taketwo Oct 7, 2014

VictorLamoine Oct 7, 2014

taketwo Oct 7, 2014

jspricke Oct 8, 2014

taketwo commented Oct 8, 2014

jspricke commented Oct 8, 2014

VictorLamoine commented Oct 9, 2014

VictorLamoine commented Oct 9, 2014

taketwo commented Oct 9, 2014

jspricke commented Oct 12, 2014

VictorLamoine commented Oct 13, 2014

jspricke Oct 16, 2014

jspricke Oct 16, 2014

jspricke commented Oct 16, 2014

taketwo commented Oct 16, 2014

jspricke commented Oct 16, 2014

taketwo commented Oct 16, 2014

jspricke commented Oct 16, 2014

taketwo commented Oct 16, 2014

jspricke commented Oct 16, 2014

VictorLamoine commented Oct 16, 2014

taketwo commented Oct 16, 2014

VictorLamoine commented Oct 16, 2014

jspricke commented Oct 16, 2014

taketwo commented Oct 19, 2014

VictorLamoine commented Oct 19, 2014

taketwo commented Oct 19, 2014

VictorLamoine commented Jun 10, 2015

simonschmeisser commented Jun 10, 2015

Add Ensenso support #888

Add Ensenso support #888

Conversation

VictorLamoine commented Sep 5, 2014

VictorLamoine commented Oct 2, 2014

jspricke commented Oct 2, 2014

VictorLamoine commented Oct 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taketwo commented Oct 8, 2014

jspricke commented Oct 8, 2014

VictorLamoine commented Oct 9, 2014

VictorLamoine commented Oct 9, 2014

taketwo commented Oct 9, 2014

jspricke commented Oct 12, 2014

VictorLamoine commented Oct 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jspricke commented Oct 16, 2014

taketwo commented Oct 16, 2014

jspricke commented Oct 16, 2014

taketwo commented Oct 16, 2014

jspricke commented Oct 16, 2014

taketwo commented Oct 16, 2014

jspricke commented Oct 16, 2014

VictorLamoine commented Oct 16, 2014

taketwo commented Oct 16, 2014

VictorLamoine commented Oct 16, 2014

jspricke commented Oct 16, 2014

taketwo commented Oct 19, 2014

VictorLamoine commented Oct 19, 2014

taketwo commented Oct 19, 2014

VictorLamoine commented Jun 10, 2015

simonschmeisser commented Jun 10, 2015