Skip to content

Make the dependency of "outofcore" on "visualization" optional. #6254

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

Merged

Conversation

rhuitl
Copy link
Contributor

@rhuitl rhuitl commented Mar 18, 2025

I would like to propose the following patch, to make the outofcore component not depend on the visualization component, because that brings in a pretty large dependency chain via VTK.

I noticed that outofcore needs only a single method from visualization, which is this overload of queryFrustum.

I'm not sure if my approach to make this dependency optional in CMake is correct. But it seems to work fine on my end.

What it does is:

  • if visualization is enabled, and outofcore is enabled, the latter will be built as normal
  • if visualization is disabled, but outofcore is enabled, the latter will be built without the method.

@mvieth
Copy link
Member

mvieth commented Mar 20, 2025

Thanks for the pull request. I understand your wish to make the dependency of outofcore on visualization optional. Some thoughts I have on this pull request:

  • I am not sure how user-friendly it is that the overload of queryFrustum can simply not be available (if the visualization module has not been built). I guess the user can check BUILD_visualization to find out whether it is available or not, but that should be well documented
  • queryFrustum seems to only need pcl::visualization::viewScreenArea from the visualization module. And viewScreenArea does not need VTK, so maybe we can find a way without disabling queryFrustum when VTK is not available?

I will look further into this and try to come up with a suggestion.

@mvieth mvieth added changelog: enhancement Meta-information for changelog generation module: outofcore labels Apr 4, 2025
Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I thought about it and while there is likely a way to move some code around and make the queryFrustum function work even without the visualization module, I think that the outofcore module and the queryFrustum function in particular are not used enough to justify putting a lot of work in. So I am happy with your approach, just have some minor remarks.

pcl_config.h.in Outdated
@@ -100,3 +100,5 @@

#cmakedefine HAVE_QVTK 1

#cmakedefine BUILD_visualization
Copy link
Member

Choose a reason for hiding this comment

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

I feel like BUILD_visualization might not send a clear message to a person using PCL. Maybe it would be better to introduce a separate preprocessor symbol, like PCL_VISUALIZATION_AVAILABLE or similar, and use that in the *.h and *.hpp files (this should then be set to true in visualization/CMakeLists.txt if the visualization module is indeed built).

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, a better name would be nice. I tried your suggestion, but couldn't make it work. The problem is that pcl_config.h.in is generated before the PCL module CMake files are processed (generating it afterwards does not work). So I'm setting in in the main CMakeLists.txt now.

In outofcore/CMakeLists.txt, I still use BUILD_visualization. We could switch that to PCL_VISUALIZATION_AVAILABLE if you want.

Without "visualization", one of the overloads of queryFrustum is unavailable.
@rhuitl rhuitl force-pushed the outofcore-without-visualization branch from e63be17 to a8f0e4c Compare April 4, 2025 20:00
@rhuitl rhuitl requested a review from mvieth April 4, 2025 20:06
Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good to me.

@larshg larshg added this to the pcl-1.15.1 milestone Apr 7, 2025
@mvieth mvieth merged commit bc88ade into PointCloudLibrary:master Apr 7, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: enhancement Meta-information for changelog generation module: outofcore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants