Refactor the project layout

[balazsdukai]

Implements my proposal from #16 :

• Use vcpkg for most of the libraries. Vcpkg is not a git submodule, because it's super heavy. Currently everyone needs to set it up on their own. There are two options for using vcpkg:
  • Use the build preset with --preset build and set the VCPKG_ROOT environment variable.
  • Set the CMAKE_TOOLCHAIN_FILE cmake variable directly.
• Use CPM for libraries that are not on vcpkg (val3dity, rerun)
• Restructure src into libraries
• Remove the umbrella roofer lib and link to the required libraries in the apps. This shouldn't cause any problems, right @Ylannl ?
• Clean up a bit and reformat the cmakelists

Fixes #16

[Ylannl]

I did some more refactoring:

• Organise src folder into core and extra. core contains only what is need for reconstruction algo/api. extra contains the rest that is needed for building the apps. These folders correspond to 2 libraries: roofer-core (all that is needed for API) and roofer-extra (all that is needed for apps, it links to roofer-core)
• separating the header files in a include dir for easier installation and so we can do #include <roofer/....> for all the roofer header files
• solving some duplicated linking issues with libs, file size of the build decreased with a few dozen Mb. However there are still some duplicated libraries linked when building with VCPKG, justing from the library names it could be GDAL related.
• Added BUILD_SHARED_LIBS option so that we can build statically and dynamically easily.
• added RF_ prefix to USE_* cmake options

[Ylannl]

Should we use target_compile_definitions() (https://cmake.org/cmake/help/latest/command/target_compile_definitions.html) instead of the config.hpp in the configured folder? Or are there other reasons to have that other than just adding defines?

[balazsdukai]

Should we use target_compile_definitions() (https://cmake.org/cmake/help/latest/command/target_compile_definitions.html) instead of the config.hpp in the configured folder? Or are there other reasons to have that other than just adding defines?

Yes, as far as I understand, target_compile_definitions and configure_file serve different purposes. In the examples I've seen, the latter is used for the communication between cmake and the source code. By far the most common use case is to pass the version to the code. I haven't set up the version here, because I was not sure what's it gonna be. I also use it for setting flags/defines, so that we have all variable definitions that need to be passed to the source code in one place. A more complete example would be in the logger experiment config.hpp.in and the use of its variables in the main.cpp

The cmake book uses the configure files basically only for passing the version number to the code, while it uses target_compile_definitions for defines.

I guess we could do the same? I've been looking for different recommendations, but I haven't found much.

[balazsdukai]

With 0761c0b the github action was set up for building all targets with vcpkg on all platforms that we support. These are ubuntu 22.04, 24.04, macos 13, 14, windows server 2022.

For future reference, our aim is to be able to build roofer with the default compiler on each platform. Although, this is a not a hard rule, and we might eventually require a newer compiler if a feature justifies it.

[balazsdukai]

I've unified the various test directories and set up two integration test, one for crop and one for the reconstruct app. The tests use the main executables, there is no separate exe just for the test. Now the build action also includes the running of the tests. I removed the Catch2 dependency, since we don't have unit tests atm, and CTest is sufficient for what is currently set up.

The test data is managed with FetchContent and the data is stored on the 3DGI server at https://data.3dgi.xyz/roofer-test-data. FetchContent downloads and extracts the data during the configuration. If the files already exist on disk, the download is skipped.

The toml config files for running crop and reconstruct are checked into git, because I consider them part of the test definition.

The tests fail, but it seems that the errors are not related to the CI setup.

[balazsdukai]

I added a pre-commit hook to enforce clang-fomat on the files. It checks if the files change after running clang-format on them, and it fails if there are formatting changes.

In addition to clang-format on the header and source files, it has additional checks for all files:

• Makes sure files end in a newline and only a newline.
• Trims trailing whitespace.

It uses the pre-commit tool which has its official github action. It can be run locally too, which will automatically fix the files, e.g. 8d1eaf9 is the result of running pre-commit run --all-files.

[Ylannl]

I added a pre-commit hook to enforce clang-fomat on the files. It checks if the files change after running clang-format on them, and it fails if there are formatting changes.

In addition to clang-format on the header and source files, it has additional checks for all files:

• Makes sure files end in a newline and only a newline.
• Trims trailing whitespace.

It uses the pre-commit tool which has its official github action. It can be run locally too, which will automatically fix the files, e.g. 8d1eaf9 is the result of running pre-commit run --all-files.

I suggest holding off this until we have merged with the API branch. I applied this formatting before, but than had to undo it because it made that merge very difficult.

In addition, we should probably exclude the thirdparty sources like toml.hpp and argh.h (in apps/external) from the formatting.

[balazsdukai]

I saw 5541a0a, but the problem with it is that the test input depends on the output of the "crop" test. Tests should be independent, because now if crop fails for some reason, "reconstruct" will fail too, even if "reconstruct" would succeed on it's own.

Thus the correct config for reconstruct needs to be added to the repo and the input data added to the wippolder test set.