PointCloudLibrary / pcl

Point Cloud Library (PCL)
https://pointclouds.org/
Other
9.89k stars 4.61k forks source link

Remove global includes and use target include for GPU/CUDA #6010

Closed larshg closed 1 month ago

larshg commented 5 months ago

Follows after #6009

larshg commented 1 month ago

@mvieth Would you review this one? I have moved some files between gpu utils and gpu containers, as there was cyclic depedencies/includes - do you agree that we can go ahead with this, without keeping a placeholder for the originals, not sure we can cross-include for deprecation cycle etc. Since its GPU module and its not shipped by default - we aren't as strict with this module?

mvieth commented 1 month ago

Could you please explain the cyclic dependencies? I see that you moved internal.hpp, repacks.hpp, repacks.cu, and texture_binder.hpp to gpu_containers, and initialization.h, initialization.cpp, and error.cpp to gpu_utils. And gpu_containers now depends on gpu_utils. I don't understand yet why you arranged it like this.

larshg commented 1 month ago

Before this PR, texture_binder.hpp and repacks.hpp in gpu/utils depended on containers/device_array, and containers/device_memory and containers/initialization depend on utils/safe_call.

Hence containers depend on utils and utils depend on containers. And my reasoning was that containers should depend on utils rather than the other way around.

mvieth commented 1 month ago

Regarding initialization.h, initialization.cpp, and error.cpp: As far as I see, safe_call.hpp only needs the function error from initialization.h. So we could just copy the std::cout command to safe_call.hpp instead of moving the three files.

Regarding internal.hpp (formerly in gpu/utils): the only function in internal.hpp (copyFields) does not seem to be used anywhere and does not seem to have a definition. So I think we can remove #include "internal.hpp" from repacks.cu/repacks.cpp and deprecate internal.hpp instead of moving it.

We could consider keeping a placeholder for the moved header files (e.g. texture_binder.hpp and repacks.hpp) which does not do a cross-include, but has only the deprecation macro with the hint where the header file is located now. So if a user tried to include the header from the old location, the compilation would still fail but at least a helpful message what to include instead would be displayed. Just an idea. :slightly_smiling_face:

larshg commented 1 month ago

It seems that internal.hpp (copyFields) was the entrypoint to all the functions in repack - see https://github.com/PointCloudLibrary/pcl/commit/353a967cd45ed54a1d4176c03597357f883052dc#diff-63446a445fe62a09821ab34e69efddb8fd3153b3c17439ed532d40e36ca0366f So it seems we can remove all of repacks, as its not used anywhere and was added as experimental?

mvieth commented 1 month ago

It seems that internal.hpp (copyFields) was the entrypoint to all the functions in repack - see 353a967#diff-63446a445fe62a09821ab34e69efddb8fd3153b3c17439ed532d40e36ca0366f So it seems we can remove all of repacks, as its not used anywhere and was added as experimental?

I don't know, the rest of repacks might still be useful, at least everything has a definition. internal.hpp has just the function declaration, with no corresponding definition, so it is definitely of no use to anyone.

larshg commented 1 month ago

It seems that internal.hpp (copyFields) was the entrypoint to all the functions in repack - see 353a967#diff-63446a445fe62a09821ab34e69efddb8fd3153b3c17439ed532d40e36ca0366f So it seems we can remove all of repacks, as its not used anywhere and was added as experimental?

I don't know, the rest of repacks might still be useful, at least everything has a definition. internal.hpp has just the function declaration, with no corresponding definition, so it is definitely of no use to anyone.

Ahh, I missread the copyfields signature in internal.hpp. Anyways, we can keep the rest of repacks around, even though its not used atm. Did I else manage to reorganize as you suggested?