Alternative way to structure the code files

[SunBlack]

Every time I check automatic code changes by clang tidy I have the feeling the code structure of PCL has more disadvantages than benefits:

Current file structure

File structure

pcl/<module>/include/pcl/<module>/impl/<file>.hpp
pcl/<module>/include/pcl/<module>/<file>.h
pcl/<module>/src/<file>.cpp

CMake structure

set(srcs
  src/<file>.cpp
)

set(incs
  "include/pcl/${SUBSYS_NAME}/<file>.h"
)

set(impl_incs
  "include/pcl/${SUBSYS_NAME}/impl/<file>.hpp"
)

Advantages:

• You can copy the whole directory to install directory
• You can easily skip whole directories if you have private headers

Disadvantages

• You have multiple list in CMake
• If you review sth. changes to header file and related source file are sticking not together.

Example where you can see it

Local git tool:

GitHub:

Alternatively file structure

File structure

pcl/<module>/<file>.cpp
pcl/<module>/<file>.h
pcl/<module>/<file>.hpp

CMake structure

set(srcs
  <file>.cpp
  <file>.h
  <file>.hpp
)

Advantages:

• Corresponding files stick together => easier to understand a diff & easier to switch between files without IDE
• Simplify CMake code

(Dis)advantages

• You need a schema to differ private header files from other, e.g. <file>_priv.h to say this file should not be copied via install target by CMake (advantage by this: It is always fast to see it this is a public or private header)

Last file structure is e.g. used by Qt as fast I saw until now. We have a similar code structure in our project and it is really always a pain in the ass to switch between files or review sth. here ;-)

Hint: In case you agree I would see it as long term issue, as we should then discuss when to apply this changes, as a lot of PRs will get broken (expect Git & Github manages to follow renaming of files as we don't need to touch source files)

[0sidharth]

I believe this produces issues with #pragma once and would require a change back to #ifndef include header guards, going against #3774 . But maybe wait for a maintainer comment.

[SunBlack]

Why it should produce issues with #pragma once?

[0sidharth]

On a closer look, I don't think there'll be an issue. My concern was that #pragma once ignores different files in different locations if they gave the same name. I don't think that will arise here, so #pragma once shouldn't be an issue.

[kunaltyagi]

We need a migration path for any breaking change. If we can prevent any breaking change via CMake, it might be easier.

Regarding the organization of files, I've seen a lot of usage of the following layout:

• API headers
• non-API files such as implementation detail headers, headers for templates, cpp files, etc.

Sticking everything together would have its own drawbacks, such as a huge number of files in one folder (thrice in worst case), which might prompt another PR in future: "separate files because it's difficult to find the correct one"

[SergioRAgostinho]

I'm a little with @kunaltyagi on this one. I don't believe there's an optimal way of addressing this organization, otherwise we would have seen something coming out of the CppGuidelines and everyone on the cpp community would be preaching about it. Ultimately is a matter of preference and every approach will have strong points and drawbacks. I'm ok with the current organization and really only have issues with some modules (cough gpu) which don't adhere to it properly.

[kunaltyagi]

You need to take care of cold and coughs in this weather @SergioRAgostinho

I'll be fine with an explicit difference between API and implementation detail (and in fact, that'll help us in the long run with deprecation) (even if we start with or touch only the offending modules)

Now for the controversial part: I believe it might be better to undertake this effort with an eye on Modules (not supporting modules, just making sure we don't do the reorg twice)

[taketwo]

You missed one more advantage of the current structure. In fact, it's probably the raison d'être of it! The apparent redundancy in the paths:

pcl/<module>/include/pcl/<module>/<file>.h

where the library name and the module name are repeated twice is needed to make sure that the same include paths (pcl/<module>/<file>.h) are valid both in build tree and when distributed.

As @SergioRAgostinho wrote, there is no golden standard of organizing C++ libraries. There are different approaches with their own pros and cons. In such situations, I strongly prefer to keep the status quo.

[SunBlack]

Just coming back to this, as I saw a lot of more code of the PCl during applying .clang-format ;-)

You missed one more advantage of the current structure. In fact, it's probably the raison d'être of it! The apparent redundancy in the paths:

pcl/<module>/include/pcl/<module>/<file>.h

where the library name and the module name are repeated twice is needed to make sure that the same include paths (pcl/<module>/<file>.h) are valid both in build tree and when distributed.

There is no difference between both approaches, as we move only the cpp & hpp files into the header directories and remove unnecessary directories. So fully structure would look like this:

apps/<app>/...
pcl/<module>/<file>.cpp
pcl/<module>/<file>.h
pcl/<module>/<file>.hpp
tests/<module>/...
CMakeLists.txt

Okay, that is the only really disadvantage: You don't have the module in root directory, instead you have a pcl directory (like Boost it is handling in its installed version).

In general PCL doesn't really completely follow even the current rule: pcl/<module>/include/pcl/<module>/<file>.h. Example: compression is part of io but included like they are separate modules (#include <pcl/compression/color_coding.h). In gpu a lot of includes are via #include "..." to include headers which lays aside of the current header or in simulation/tools you don't have even a proper include_directories defined, so we can't exchange #include "..." without much effort. What I want to say with it: Even the current structure does not prevent the #includes from being revived manually in a PR. The only thing: You prevent now to include header files from cpp via #include ""

As @SergioRAgostinho wrote, there is no golden standard of organizing C++ libraries. There are different approaches with their own pros and cons. In such situations, I strongly prefer to keep the status quo.

I agree that there is no golden standard (e.g. where to locate gpu submodules, tests, ...). But currently I ask myself why the PCl seems the only bigger project with this architecture. LLVM, Mozilla, Google, Qt... - all have the source at the side of the header.

Why I'm currently discussing this? I'm currently think about restructure some bad directories. E.g. in apps you have no idea which files belong together, as only a few apps are separated into different directories.

We have currently following issues with folder structure (only a fast check):

• ✔️ 2d
• ❌ apps: Ugly as hell - no further comment necessary^^
• ❌ common: Some components must be included via pcl/common/<header>.h, during other via pcl/<header>.h. Even pcl/console/<header>.h doesn't find the structure pcl/<module>/<header> (if you see it, you can think console is a separate module)
• ❌ cuda: nn doesn't follow the rule
• ✔️ examples: Maybe group into separate directories to keep main CMakeFile smaller and it is faste to see that each example have just one header
• ❓ gpu: The tools directory should be separated
• ❌ io: compression doesn't fit into the rule pcl/<module>/<header>. Tools should be separated too
• ✔️kdtree
• ✔️keypoints
• ✔️ml
• ✔️octree
• ❓ outofcore: Tools be splitted
• ❓ people: apps should be separated
• ✔️ recognition
• ✔️ registration
• ✔️ sample_consensus
• ✔️ search
• ✔️ segmentation
• ❓ simulation: Tools should be separated
• ✔️ stereo
• ✔️ surface
• ✔️ tests
• ✔️ tools: Maybe group into separate directories to keep main CMakeFile smaller and it is faste to see that each example have just one header
• ✔️ tracking
• ❌ visualization: Move tests to test directory

As you can see: Maybe half of the modules may be okay currently (✔️), some should be restructured slightly (❓) (mostly move tools out of the module), there are still some modules (❌) which should be restructured to follow current rules. And before we start it, how we this result be to prevent unnecessary move commits.

In general imho:

• Each lib & exe should have a separate directory, so it is faster to see which files belongs together and make sure adjusting one target doesn't touch other targets (that's why each target should have a separate CMakeFile to make it sure). This would lead to a lot of extra directories for includes for the apps (as we have always a few cpp files, then include/pcl/apps as 3 directories and then 2-3 header files).
• We should think about the difference between tools, apps and examples. Where is the real difference? Tools and apps sounds really similar and most of the current apps are more like examples.

[kunaltyagi]

• I like the first suggestion (each target (exe or lib) in separate folder)
• Maybe just have tools and examples. Tools will have certain standard while examples will have more leeway (eg: no proper cmdline interface, less flexible, more hardcoded values, etc.)

It makes sense to split tools (just like examples) separate from pcl_libs (other modules) and organize it (I think there was some work underway for that). It can be done either

• similar to pcl/ (aka tools/<module>/<tool_name>): helps in discovery
• flat (aka tools/<tool_name>): discovery via README or something else. Flatter structure, less worry about what module tool is intended for

[SunBlack]

• Maybe just have tools and examples. Tools will have certain standard while examples will have more leeway (eg: no proper cmdline interface, less flexible, more hardcoded values, etc.)

Agree, but it will be hard to decide in some cases, as some tools are really short, so you don't know is this an example or or a tool. See e.g. this. Maybe it is enough to have apps and the example within the documentation and remove all other old examples.

It makes sense to split tools (just like examples) separate from pcl_libs (other modules) and organize it (I think there was some work underway for that). It can be done either

• similar to pcl/ (aka tools/<module>/<tool_name>): helps in discovery
• flat (aka tools/<tool_name>): discovery via README or something else. Flatter structure, less worry about what module tool is intended for

As some tools may use multiple modules tools/<module>/<tool_name> makes no sense, or you wan't e.g. tools/io_kdtree/<tool_name>? ;-)

[kunaltyagi]

The name itself of the example you provided has example in it, so I'd say in reorg, it'll go to examples. And yeah, flat makes more sense for tools

[SergioRAgostinho]

Just adding some more info for decision making. I believe the distinction between tools and apps was the gui. Apps are meant to be standalone gui enabled executables. Tool are supposed to be command line oriented executables.

It's not necessarily a happy division because semantically speaking point_cloud_editor is definitely a work tool.

Nevertheless, setting this distinction might help organize thoughts.

[SunBlack]

Just adding some more info for decision making. I believe the distinction between tools and apps was the gui. Apps are mean to be standalone gui enabled executables. Tool are supposed to be command line oriented executables.

So maybe this be reflected in the path, e.g. tools_gui and tools_cui (sound ugly, but... 😆)

[kunaltyagi]

Keeping GUI and CLI separate makes sense only since GUI has dependency on visualization libraries (either pcl_visualization or some other library too). No strong reason (if I'm not building visualization, I don't need to build tools_gui, just tools_cli to borrow the terms) since this can be managed by CMake.

Options:

• By name: _editor, _viewer, _composer, etc. : bit ambiguous that's all
• By path: tools/gui/, tools/cli/: works till we manage to create a tools with optional gui
• By module: tools_gui, tools_cli or apps and tools 😆 : works rn

[kunaltyagi]

@sunblack Could you please revive this thread by stating the conclusions made and the points left to discuss?

[SergioRAgostinho]

Keeping GUI and CLI separate makes sense only since GUI has dependency on visualization libraries (either pcl_visualization or some other library too).

I believe some of our CL tools breach this assumption as well. They are command line in the sense that you literally need to supply proper arguments so that they run, but some of them also open visualization windows.

This was also one of the issues with some of the tools for Mac. In order the properly parse mouse click events, the executables had to be converted to app bundles, but then you could no longer supply proper input arguments (at least not without some effort) because now the user sees an application bundle and double clicks to execute it (like any other app bundle on macOS).

[kunaltyagi]

This was also one of the issues with some of the tools for Mac

(#refactor, unrelated) Maybe we should have 2 binaries:

• launcher
• application with defaults (and configurable options)

If so, it'd make less sense to keep gui and cli in separate modules since they are "dependent" on each other