Skip to content
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

Move OpenNI and VTK support from io to their own modules #5798

Open
jackjansen opened this issue Sep 3, 2023 · 6 comments
Open

Move OpenNI and VTK support from io to their own modules #5798

jackjansen opened this issue Sep 3, 2023 · 6 comments
Labels
kind: request Type of issue status: triage Labels incomplete

Comments

@jackjansen
Copy link

In the https://github.com/cwi-dis/cwipc toolkit that I maintain I depend on libpcl for some features. Specifically for this bug report: I use the ply-file readers from the io package. My toolkit is cross-platform.

But the io package is rather heavy: it also depends on vtk and OpenNI (at least when it is built for the Windows all-in-one installer).

So this means my users have a whole lot of packages they need to install, even though my toolkit doesn't actually need them.

Both of the mentioned dependencies have their own specific problems, especially on Windows:

  • vtk is large and unwieldy and versions seem to follow each other quickly, making it difficult to instal.
  • OpenNI is very sensitive for how it is installed, and how it is added to PATH.

(I have a similar issue with the boost dependency, but I get the impression that the libpcl developers are already moving away from boost as much as possible, so I'm hoping that problem will eventually go away by itself).

Context

See above.

Expected behavior

If the OpenNI and VTK I/O handlers were in their own package, in stead of in io, I wouldn't have to worry about them (because I don't use them).

Current Behavior

Currently all I/O is in a single package io, which means that the only way to disable (or enable) certain features is to build libpcl from source.

Describe the solution you'd like

If the OpenNI and VTK I/O handlers were in their own package, in stead of in io, I wouldn't have to worry about them (because I don't use them).

Describe alternatives you've considered

An alternative is that I build libpcl from source with exactly the options I need, and that I distribute that version with my cwipc package. But that would "lock in" the users of my package to have access only to the libpcl features that I happen to have selected, whereas with the setup where I don't distribute a private copy of libpcl my users have the freedom to use any libpcl feature they want (because I'm linking to the libpcl that they have installed).

Additional context

@jackjansen jackjansen added kind: request Type of issue status: triage Labels incomplete labels Sep 3, 2023
@Aposhian
Copy link

I was so surprised to realize that io pulls in vtk. It makes no sense to have to install vtk (which pulls in a lot of dependencies itself, like Java) on a headless platform that just needs PCD support. +1 to separating out these things from the core io functionality (compression, pcd, etc.)

@mvieth
Copy link
Member

mvieth commented Sep 17, 2023

Hi, I understand your point. The problem that PCL's io module depends on VTK (and thus entails a lot of dependencies on Debian/Ubuntu) also came up in perception_pcl (the ROS-PCL bridge) some time ago.
However, I am not sure if creating (an) additional io module(s) is a good idea. This would create a lot of work for package maintainers (e.g. apt package maintainers), users who potentially have to update their CMakeLists.txt, etc. Additionally, it will often be unclear to a user whether the pcl-io library is needed, or (one of) the new library/libraries. I feel like dividing pcl-io based on dependencies is quite "unnatural"/not intuitive.

I compiled an overview where PCL's io module needs vtk:

  • vtk_lib_io.h[pp], vtk_lib_io.cpp: saving and loading e.g. from ply, vtk, obj, stl files (note: PCL has native ply and obj readers/writers which could replace these, and also some vtk file format writers not depending on VTK libraries, but currently nothing for stl)
  • davidsdk_grabber.cpp: includes vtk_lib_io.h for loading obj, ply, stl
  • image_grabber.cpp: uses vtk classes to read from image files like png, tiff, jpeg, ...
  • png_io.cpp: uses vtk classes to write png files (might be replaceable with something from libpng)

My favourite solution would be to replace everything from VTK in io with PCL-native functions. However, this would be quite some work and would take some time.

Another idea: could delayed loading be the solution? (on Windows) See #5642

I have a similar issue with the boost dependency, but I get the impression that the libpcl developers are already moving away from boost as much as possible

This is true, we replace dependencies on boost modules whenever there is a good alternative.

@CSBVision
Copy link
Contributor

Another idea: could delayed loading be the solution? (on Windows) See #5642

Since we worked on delayed loading, we wanted to add here: Delay loading is well suited to resolve this kind of issues at runtime. Of course at compile time, still all optional dependencies are required. It's rather simple to use, only the linker flags have to be extended by the /DELAYLOAD:whatever.dll flags and the delayimp.lib flag for Ninja generators (VS seems to add this automatically).
For VTK, our PR is open for some time now (we just extended it for Ninja generators as explained), still merging it will already fix this issue with respect to the VTK dependencies. For OpenNI, a similar CMake patch is relatively easy possible, only the names of the DLLs have to be set accordingly.
Nevertheless, refactoring the module structure is more than reasonable as well, however is much more work than using delay loading as an intermediate step.

Btw: Another particularly useful use case of delay loading is CUDA, whose dependencies are also rather heavy. Activating CUDA at compile time and combining it with delayed loading, the CUDA dependencies can be skipped on any machine not needing CUDA at all (e.g., only non-CUDA functions are used or even no CUDA hardware is available). We implented this into OpenCV, transferring it to PCL is straightforward.

@jackjansen
Copy link
Author

@mvieth I didn't realise that all of the functionality you mention in your overview list is actually dependent on VTK.

That makes my suggestion of splitting rather pointless: I was hoping/expecting that things like loading ply and obj files were done by libpcl itself.

It also makes it pointless for my specific use case: the one thing for which I need the io module is ply file loading and saving, so I would still have to pull in vtk.

Feel free to close this issue.

And I will (eventually:-) implement my ply file load/save in my own code. If I manage to do this in a way that is "libpcl-compatible" I will try and donate the code back to libpcl.

@mvieth
Copy link
Member

mvieth commented Oct 10, 2023

@jackjansen Sorry for the misunderstanding: PCL does have functions to load and save ply and obj files, which do not depend on VTK:
loadPolygonFilePLY, loadPolygonFileOBJ, and savePolygonFilePLY for example depend on VTK.
But loadPLYFile, loadOBJFile, savePLYFile, and savePLYFileBinary do the same thing and do not depend on VTK.
I believe the only file format where we have to depend on VTK currently (where so far no counterpart exists that does not depend on VTK) is stl.
In my list above I mentioned that we could replace all calls to loadPolygonFilePLY inside of PCL with calls to loadPLYFile, all calls to loadPolygonFileOBJ with calls to loadOBJFile, etc, to reduce dependency on VTK step by step.

@jackjansen
Copy link
Author

@mvieth thanks a lot for the clarification!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: request Type of issue status: triage Labels incomplete
Projects
None yet
Development

No branches or pull requests

4 participants