Vulkan-Samples icon indicating copy to clipboard operation
Vulkan-Samples copied to clipboard

Move vkb::filesystem to components

Open tomadamatkinson opened this issue 1 year ago • 2 comments

Description

Simplifies our filesystem implementation by using std::filesystem underneath. This is the filesystem used in #846 which can be easily used in different executables to perform small tasks (See shader_reflector and variant_reflector. Moved to components and can be used using vkb__filesystem. Filesystem is influenced by the decisions made on the framework/2.0 branch

The existing "legacy" api is kept to maintain compatibility with samples. In the future asset loaders may choose to use the filesystem directly with vkb::filesystem::get()

Merging this will help simplify the review on #846

Tested on MacOS (M1), Linux (Ubuntu 22.04), Windows (11) and Android (Pixel 3). Samples work as expected

General Checklist:

Please ensure the following points are checked:

  • [x] My code follows the coding style
  • [x] I have reviewed file licenses
  • [x] I have commented any added functions (in line with Doxygen)
  • [x] I have commented any code that could be hard to understand
  • [x] My changes do not add any new compiler warnings
  • [x] My changes do not add any new validation layer errors or warnings
  • [x] I have used existing framework/helper functions where possible
  • [x] My changes do not add any regressions
  • [x] I have tested every sample to ensure everything runs correctly
  • [x] This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

  • [ ] I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
  • [ ] My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

Sample Checklist

If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist:

  • [ ] I have tested the sample on at least one compliant Vulkan implementation
  • [ ] If the sample is vendor-specific, I have tagged it appropriately
  • [ ] I have stated on what implementation the sample has been tested so that others can test on different implementations and platforms
  • [ ] Any dependent assets have been merged and published in downstream modules
  • [ ] For new samples, I have added a paragraph with a summary to the appropriate chapter in the samples readme
  • [ ] For new samples, I have added a tutorial README.md file to guide users through what they need to know to implement code using this feature. For example, see conditional_rendering
  • [ ] For new samples, I have added a link to the Antora navigation so that the sample will be listed at the Vulkan documentation site

tomadamatkinson avatar Feb 07 '24 00:02 tomadamatkinson

I'm seeing a couple of build problems. The first is a redefinition of LOGE:

[100%] Building CXX object app/CMakeFiles/vulkan_samples.dir/main.cpp.o
In file included from Vulkan-Samples/components/filesystem/include/filesystem/filesystem.hpp:26,
                 from Vulkan-Samples/app/main.cpp:23:
Vulkan-Samples/components/core/include/core/util/logging.hpp:28: warning: "LOGE" redefined
   28 | #define LOGE(...) spdlog::error("{}", fmt::format(__VA_ARGS__));
      | 
In file included from Vulkan-Samples/app/main.cpp:18:
Vulkan-Samples/framework/common/logging.h:35: note: this is the location of the previous definition
   35 | #define LOGE(...) spdlog::error("[{}:{}] {}", __FILENAME__, __LINE__, fmt::format(__VA_ARGS__));

gary-sweet avatar Feb 14 '24 09:02 gary-sweet

The second issue is that std::filesystem isn't available before gcc8 unless you explicitly include stdc++fs as a link library.

We've had to work around that a number of times using something like:

if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 9.0)
   target_link_libraries(${TGT} PRIVATE stdc++fs)
endif()

gary-sweet avatar Feb 14 '24 09:02 gary-sweet

I'm seeing a couple of build problems. The first is a redefinition of LOGE:

[100%] Building CXX object app/CMakeFiles/vulkan_samples.dir/main.cpp.o
In file included from Vulkan-Samples/components/filesystem/include/filesystem/filesystem.hpp:26,
                 from Vulkan-Samples/app/main.cpp:23:
Vulkan-Samples/components/core/include/core/util/logging.hpp:28: warning: "LOGE" redefined
   28 | #define LOGE(...) spdlog::error("{}", fmt::format(__VA_ARGS__));
      | 
In file included from Vulkan-Samples/app/main.cpp:18:
Vulkan-Samples/framework/common/logging.h:35: note: this is the location of the previous definition
   35 | #define LOGE(...) spdlog::error("[{}:{}] {}", __FILENAME__, __LINE__, fmt::format(__VA_ARGS__));

Due to a conflict with logging.h and logging.hpp I will remove the original version in favour of the one that is ported to the components folders

tomadamatkinson avatar Feb 24 '24 14:02 tomadamatkinson

Thanks all for taking the time to review! Branch is updated

tomadamatkinson avatar Feb 24 '24 14:02 tomadamatkinson