[RFC] Added simple kinectv2 grabber and viewer #1410

larshg · 2015-10-29T19:55:17Z

However as of now you need to edit the freenect2config.cmake to add a few variables and make some of them with CAPS - why is is necessary? Or should I make a findfreenect2.cmake instead?
@VictorLamoine can you guide me here?

~~For some reason the pointcloud behaves flaky and the colors are acting weird also - so would like some help in localizing the error(s).~~
Set is_dense to false seem to do the trick here.

@giacomodabisias can you have a look?

TODO:

Selectable packetpipline - thanks to @giacomodabisias
Selectable kinectv2 unit by seriel number
Cmake search paths for windows (will probably be a envirotment variable as libfreenect2 is currently installed in the build folder, which can be arbitrary paths)

giacomodabisias · 2015-10-29T23:07:17Z

Nice,
I will have a better look tomorrow, but why is there only the OpenCLPacketPipeline ?

larshg · 2015-10-30T07:20:12Z

Mostly due to the short time I have worked on it and that the opengl processor outputs black windows atm - but that I is a matter at freenect2 :)

And I lack implementation to choose PacketPipline and certain kinectv2 unit :)

giacomodabisias · 2015-11-25T14:25:04Z

Hi, what is the status right now? I am pulling right now everything to have a look.

larshg · 2015-11-25T15:04:17Z

Hi, well you can get colored pointclouds using the so far hardcoded opencl pipeline. I haven't got any spare time for the two features on the todo. Thanks for trying it out ;-)

larshg · 2015-11-25T15:06:40Z

Also, One might need to create a findFreenect2 cmake file, as the exported cmake names of the libfreenect2 doesn't fit the PCL way of doing things...

giacomodabisias · 2015-11-25T15:14:16Z

Ok,
I had a look at the code. I managed to build it even if the system was not finding freenect2, yeah there is some problem in the cmake.
I am adding the different pipelines. The grabber is too slow, mainly its because the way you build the cloud. I will fix that also and then make a pull request :)

larshg · 2015-11-25T15:31:26Z

Sounds nice - looking forward to see the changes!

giacomodabisias · 2015-11-25T15:46:46Z

done, working.
I will upload it to your branch

SergioRAgostinho · 2015-11-25T15:53:57Z

Here's some options
1 - @larshg pulls your changes into this pull request
2 - @larshg gives you (temporary) write permission into his fork and you can push your changes into this pr.
3 - You create a new pull request (let's avoid this)

giacomodabisias · 2015-11-25T15:57:13Z

Yeah thanks, just made a pull request on his branch.
There are still some minor fixes to do; for example I created an enum to choose the freenect2 pipeline but I don't know if leaving it in the pcl namespace is correct or if it should have an own namespace.

larshg · 2015-11-25T15:59:42Z

Yes, I guess actually it should be an enum created in libfreenect2 - as when 0.2 version is released there properly will be a CUDA packetpipline, which in turn will require updates to PCL?

Anyway merged your changes :)

giacomodabisias · 2015-11-25T16:01:16Z

yeah exactly. the biggest trouble is right now the CMake. I am not familiar with the pcl cmake structure so someone should have a look to fix it. There is the need to create a FindFreenect2.cmake I suppose.
@VictorLamoine what would be the best way to fix the cmake?

giacomodabisias · 2015-12-14T10:35:25Z

I fixed the cmake issue by creating a findModule for freenect2. Waiting for @larshg to merge. Next I will fix the Selectable kinectv2 unit by seriel number issue

ahundt · 2015-12-16T21:50:01Z

io/src/k4w2_grabber.cpp

This won't compile if the user doesn't have OpenCL installed due to this line.

Thanks, will add a macro

giacomodabisias · 2016-01-07T10:22:07Z

Hi,
did anyone test this last version? I think we got everything and just need to fix the code with the pcl style guide.
@larshg I fixed also the selectable kinect by serial

giacomodabisias · 2016-01-11T10:09:44Z

@VictorLamoine could you have a look? Just to know if we are missing something.

VictorLamoine · 2016-01-11T14:49:07Z

The grabber codes lacks documentation, it would be nice to add some.
You should fix your code to match PCL style guide, but you already know that.
I don't have a Kinect v2 for testing but the code looks ok.

VictorLamoine · 2016-01-11T14:51:24Z

visualization/tools/k4w2_viewer.cpp

You should complete this explaining you can choose which Kinect to open by specifying an argument.

@VictorLamoine thanks.
Like this?

void printHelp(int, char **argv) { std::cout << std::endl; std::cout << "****************************************************************************" << std::endl; std::cout << "* *" << std::endl; std::cout << "* FREENECT2 - Usage Guide *" << std::endl; std::cout << "* *" << std::endl; std::cout << "****************************************************************************" << std::endl; std::cout << "Creates a point cloud grabber for the kinect2. Accepts as constructor " << std::endl; std::cout << "parameter a string specifying a kinect2 serial number " << std::endl; std::cout << std::endl; }

Yes, you can even make it shorter. I like this way of writing program helps, its very close to what is common on Linux program helps.

PCL_INFO ("Creates a point cloud grabber for the kinect2\n"); PCL_INFO ("Accepts as constructor parameter a string specifying a kinect2 serial number\n");

Yes this looks good!

taketwo · 2016-01-13T20:50:31Z

I think the name k2w2Grabber is quite cryptic. Also, typically the name of the grabber and the viewer program that exploits it are the same. Since we have pcl_freenect2_viewer here, why not calling the grabber Freenect2Grabber?

giacomodabisias · 2016-01-14T09:57:16Z

Thanks @taketwo for having a look. I fixed all the things.
I had to change the FREENECT2_LIBRARIES var in the io/CMakeLists.txt to FREENECT2_LIBRARY to avoid setting it in the FindFreenect2.cmake in order to use the suggested find_package_handle_standard_args; it was useless anyway.

larshg · 2016-01-14T10:44:41Z

Thanks for helping out with the last @giacomodabisias, @taketwo and @VictorLamoine. I'll try to make time to add the default paths for windows for finding libfreenect2 to the Cmake tonight, before its complete for testing etc.

giacomodabisias · 2016-01-14T11:06:48Z

@larshg you can fix the second checkbox also

taketwo · 2017-11-22T14:22:24Z

AFAIK both Kinect v1 and v2 are end-of-life and are not manufactured anymore. I'm not sure if we need to merge drivers for dead hardware into PCL.

SergioRAgostinho · 2017-11-22T14:30:03Z

There might still be people with that hardware around. I would simply disable its compilation by default.

UnaNancyOwen · 2017-11-22T14:43:38Z

AFAIK both Kinect v1 and v2 are end-of-life and are not manufactured anymore. I'm not sure if we need to merge drivers for dead hardware into PCL.

However, Kinect v2 sensor is still used by many users. Unfortunately, Kinect v2 sensor production was discontinued, But it is still representative of RGB-D sensors for consumers.
I strongly think it is worthwhile for PCL to support it.

I'm publishing grabber for Kinect v2 that based Kinect SDK v2 which is different from this grabber. (It is works on only Windows.) It still has a lot of demand.
I think this grabber that works on cross platform is welcomed by many users.

SergioRAgostinho · 2017-11-22T16:04:55Z

The freenect2 dependency is not being met in the CI and therefore this code is not being compiled.

libfreenect2 is not being packaged in any ubuntu version, from what I can tell. I also can't seem to find any ppa bundling it.
It's also not a part of homebrew universe: some random references of attempts at it
- Formula cannot find certain libraries Homebrew/homebrew-core#1833
- https://groups.google.com/forum/#!topic/openkinect/yq01TpMfTbA
Fortunately it exists in vcpkg https://github.com/Microsoft/vcpkg/tree/master/ports/libfreenect2

Our build time does not allow us to be manually compiling third parties. I would like to have this properly compiling on our CI.

SergioRAgostinho

My first review just focused on your CMake files.

You have a number of indentation problem to solve in the other files as well.

I recommend you have another look at the PCL style guide. http://pointclouds.org/documentation/advanced/pcl_style_guide.php#pcl-style-guide

SergioRAgostinho · 2017-11-28T10:53:03Z

io/CMakeLists.txt

 PCL_SUBSYS_OPTION(build "${SUBSYS_NAME}" "${SUBSYS_DESC}" ON)
 if(WIN32)
-    PCL_SUBSYS_DEPEND(build "${SUBSYS_NAME}" DEPS ${SUBSYS_DEPS} OPT_DEPS openni openni2 ensenso davidSDK dssdk rssdk pcap png vtk)
+    PCL_SUBSYS_DEPEND(build "${SUBSYS_NAME}" DEPS ${SUBSYS_DEPS} OPT_DEPS openni openni2 ensenso davidSDK dssdk rssdk pcap png vtk freenect2)


On FindFreenect2.cmake, you specify paths which belong to unix style file system, but here you're only setting the decency on the windows. Was this intentional? I thought libfreenect was supported on all platforms.

SergioRAgostinho · 2017-11-28T10:59:23Z

io/CMakeLists.txt

+	if(WITH_FREENECT2)
+        option(WITH_OPENCL "adds opencl support for freenect2" OFF)
+        if(${WITH_OPENCL})
+            set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DWITH_OPENCL")


There's no find scripts in pcl which set opencl include dirs and libraries.

OpenCL is something other modules might later use, so doesn't make sense to add this option here. It would should be moved to project root level.

If you want to set preprocessor definitions use pcl_config.h.in and include it in your header files.

Given all these three options, I say to delete all these OpenCL related lines and include pcl_config.h in your headers, which will provide you with the definitions

SergioRAgostinho · 2017-11-28T11:01:20Z

io/CMakeLists.txt

        ${DAVIDSDK_GRABBER_SOURCES}
        ${DSSDK_GRABBER_SOURCES}
        ${RSSDK_GRABBER_SOURCES}
+		${FREENECT2_GRABBER_SOURCES}


Indentation is messed up. Don't use tabs.

SergioRAgostinho · 2017-11-28T11:02:10Z

io/CMakeLists.txt

        ${DAVIDSDK_GRABBER_INCLUDES}
        ${DSSDK_GRABBER_INCLUDES}
        ${RSSDK_GRABBER_INCLUDES}
+		${FREENECT2_GRABBER_INCLUDES}


SergioRAgostinho · 2017-11-28T11:04:02Z

io/CMakeLists.txt

+		set(FREENECT2_GRABBER_SOURCES
+			src/freenect2_grabber.cpp
+		)
+	endif()


The indentation for this block is off

SergioRAgostinho · 2017-11-28T11:15:58Z

io/CMakeLists.txt

+        option(WITH_CUDA "adds cuda support for freenect2" OFF)
+        if(${WITH_CUDA})
+            set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DWITH_CUDA")
+        endif()


Same comments as above. this time for cuda.

SergioRAgostinho · 2017-11-28T11:17:10Z

io/CMakeLists.txt

+        else()
+          target_link_libraries(${LIB_NAME} ${FREENECT2_LIBRARY})
+        endif()
+	endif()


Indentation is off again. Don't use tabs.

stale · 2018-01-27T11:51:14Z

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.

ahundt · 2018-01-29T03:48:35Z

Can this be bumped up? It is quite useful and the changes needed seem fairly minor. As for CI simply cache the dependency with travis so it is only built once. https://docs.travis-ci.com/user/caching/

SergioRAgostinho · 2018-01-29T08:08:31Z

Hey,

The review is not complete, I probably just advanced some things so that the authors could start working
We need people with the corresponding hardware to help. I personally don't have one.
Regarding your CI comment:
- To enforce what you'll need to change the way we use our env vars on travis
- They used to suggest using an external server to offload the result dependency compilation and pull it back in on every other stage, but as a first stage one can try to cache the installation directory and see if that's enough.

Bottom line. If you're interested in seeing this feature pushed, lend us a help.

stale · 2018-05-23T03:54:37Z

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 · 2018-10-25T19:14:41Z

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.

ahundt · 2018-11-11T19:12:06Z

Cool this looks like it passes CI! Could it be merged soon now that we have passed the 3 year old PR milestone? 🥇

taketwo · 2018-11-12T10:07:42Z

There are major review comments (besides from the indentation stuff) that have not been addressed.

stale · 2019-01-12T19:24:39Z

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.

Fixes PointCloudLibrary#3215

larshg force-pushed the master branch from fd874d3 to ba55399 Compare November 15, 2015 07:47

larshg changed the title ~~[WIB] Added simple kinectv2 grabber and viewer~~ [WIP] Added simple kinectv2 grabber and viewer Nov 15, 2015

larshg force-pushed the master branch from ba55399 to 2ffe0ad Compare November 17, 2015 11:13

giacomodabisias mentioned this pull request Dec 14, 2015

Kinect2Grabber with example #1032

Closed

ahundt reviewed Dec 16, 2015
View reviewed changes

ahundt mentioned this pull request Dec 16, 2015

#11 cmdline opts to select processor and output a PLY file giacomodabisias/libfreenect2pclgrabber#12

Merged

ahundt mentioned this pull request Dec 27, 2015

point cloud support and/or helpers OpenKinect/libfreenect2#515

Open

VictorLamoine reviewed Jan 11, 2016
View reviewed changes

SergioRAgostinho added the status: needs upstream fix label Nov 22, 2017

SergioRAgostinho requested changes Nov 28, 2017

View reviewed changes

SergioRAgostinho added needs: author reply Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Nov 28, 2017

stale bot added the status: stale label Jan 27, 2018

stale bot removed the status: stale label Jan 29, 2018

larshg force-pushed the master branch from 6664a5f to a5a8411 Compare February 21, 2018 09:54

stale bot added the status: stale label May 23, 2018

SergioRAgostinho removed this from the pcl-1.9.0 milestone Aug 26, 2018

stale bot removed the status: stale label Aug 26, 2018

stale bot added the status: stale label Oct 25, 2018

stale bot removed the status: stale label Nov 11, 2018

stale bot added the status: stale label Jan 12, 2019

larshg force-pushed the master branch from a5a8411 to dcb4a36 Compare July 9, 2019 09:37

stale bot removed the status: stale label Jul 9, 2019

Fix callback declarations per @SergioRAgostinho

9502463

Fixes PointCloudLibrary#3215

larshg force-pushed the master branch from dcb4a36 to 9502463 Compare July 9, 2019 09:47

larshg closed this Jul 9, 2019

Uh oh!

[RFC] Added simple kinectv2 grabber and viewer #1410

[RFC] Added simple kinectv2 grabber and viewer #1410

Uh oh!

Conversation

larshg commented Oct 29, 2015 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giacomodabisias commented Oct 29, 2015

Uh oh!

larshg commented Oct 30, 2015

Uh oh!

giacomodabisias commented Nov 25, 2015

Uh oh!

larshg commented Nov 25, 2015

Uh oh!

larshg commented Nov 25, 2015

Uh oh!

giacomodabisias commented Nov 25, 2015

Uh oh!

larshg commented Nov 25, 2015

Uh oh!

giacomodabisias commented Nov 25, 2015

Uh oh!

SergioRAgostinho commented Nov 25, 2015

Uh oh!

giacomodabisias commented Nov 25, 2015

Uh oh!

larshg commented Nov 25, 2015

Uh oh!

giacomodabisias commented Nov 25, 2015

Uh oh!

giacomodabisias commented Dec 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giacomodabisias commented Jan 7, 2016

Uh oh!

giacomodabisias commented Jan 11, 2016

Uh oh!

VictorLamoine commented Jan 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taketwo commented Jan 13, 2016

Uh oh!

giacomodabisias commented Jan 14, 2016

Uh oh!

larshg commented Jan 14, 2016

Uh oh!

giacomodabisias commented Jan 14, 2016

Uh oh!

taketwo commented Nov 22, 2017

Uh oh!

SergioRAgostinho commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UnaNancyOwen commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergioRAgostinho commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergioRAgostinho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larshg commented Oct 29, 2015 •

edited

Loading

SergioRAgostinho commented Nov 22, 2017 •

edited

Loading

UnaNancyOwen commented Nov 22, 2017 •

edited

Loading

SergioRAgostinho commented Nov 22, 2017 •

edited

Loading