Commit: 1bf0f1a1192c5a15aae5718603f2d2266715edb4 Author: Julian Eisel Date: Tue Sep 21 16:01:31 2021 +0200 Branches: temp-asset-browser-catalogs https://developer.blender.org/rB1bf0f1a1192c5a15aae5718603f2d2266715edb4
Use existing C-APIs for path and filesystem handling The `ghc::filesystem` we wanted to use for a platform compatible replacement of `std::filesystem` (see D12197) doesn't behave as we expect or want to. We could probably find a solution, but it's not worth the extra time. To move this forward, we can switch the asset catalogs to use Blender's C functions for path and filesystem handling. Differential Revision: https://developer.blender.org/D12538 =================================================================== M source/blender/blenkernel/BKE_asset_catalog.hh M source/blender/blenkernel/BKE_asset_library.hh M source/blender/blenkernel/intern/asset_catalog.cc M source/blender/blenkernel/intern/asset_catalog_test.cc M source/blender/blenkernel/intern/asset_library.cc M source/blender/blenkernel/intern/asset_library_test.cc =================================================================== diff --git a/source/blender/blenkernel/BKE_asset_catalog.hh b/source/blender/blenkernel/BKE_asset_catalog.hh index 7483c82754e..7ed2dc187e8 100644 --- a/source/blender/blenkernel/BKE_asset_catalog.hh +++ b/source/blender/blenkernel/BKE_asset_catalog.hh @@ -24,7 +24,6 @@ # error This is a C++ header. The C interface is yet to be implemented/designed. #endif -#include "BLI_filesystem.hh" #include "BLI_function_ref.hh" #include "BLI_map.hh" #include "BLI_string_ref.hh" @@ -40,7 +39,9 @@ namespace blender::bke { using CatalogID = UUID; using CatalogPath = std::string; using CatalogPathComponent = std::string; -using CatalogFilePath = filesystem::path; +/* Would be nice to be able to use `std::filesystem::path` for this, but it's currently not + * available on the minimum macOS target version. */ +using CatalogFilePath = std::string; class AssetCatalog; class AssetCatalogDefinitionFile; diff --git a/source/blender/blenkernel/BKE_asset_library.hh b/source/blender/blenkernel/BKE_asset_library.hh index d12130ca368..68f7481574e 100644 --- a/source/blender/blenkernel/BKE_asset_library.hh +++ b/source/blender/blenkernel/BKE_asset_library.hh @@ -28,18 +28,14 @@ #include "BKE_asset_catalog.hh" -#include <filesystem> #include <memory> namespace blender::bke { -/* TODO(@sybren): revisit after D12117 has a conclusion. */ -namespace fs = blender::filesystem; - struct AssetLibrary { std::unique_ptr<AssetCatalogService> catalog_service; - void load(const fs::path &library_root_directory); + void load(StringRefNull library_root_directory); }; } // namespace blender::bke diff --git a/source/blender/blenkernel/intern/asset_catalog.cc b/source/blender/blenkernel/intern/asset_catalog.cc index 4519f796c40..618888d44c5 100644 --- a/source/blender/blenkernel/intern/asset_catalog.cc +++ b/source/blender/blenkernel/intern/asset_catalog.cc @@ -20,14 +20,12 @@ #include "BKE_asset_catalog.hh" -#include "BLI_filesystem.hh" +#include "BLI_fileops.h" +#include "BLI_path_util.h" #include "BLI_string_ref.hh" -#include <filesystem> #include <fstream> -namespace fs = blender::filesystem; - namespace blender::bke { const char AssetCatalogService::PATH_SEPARATOR = '/'; @@ -95,6 +93,16 @@ AssetCatalog *AssetCatalogService::create_catalog(const CatalogPath &catalog_pat return catalog_ptr; } +static std::string asset_definition_default_file_path_from_dir(StringRef asset_library_root) +{ + char file_path[PATH_MAX]; + BLI_join_dirfile(file_path, + sizeof(file_path), + asset_library_root.data(), + AssetCatalogService::DEFAULT_CATALOG_FILENAME.data()); + return file_path; +} + void AssetCatalogService::ensure_catalog_definition_file() { if (catalog_definition_file_) { @@ -102,7 +110,7 @@ void AssetCatalogService::ensure_catalog_definition_file() } auto cdf = std::make_unique<AssetCatalogDefinitionFile>(); - cdf->file_path = asset_library_root_ / DEFAULT_CATALOG_FILENAME; + cdf->file_path = asset_definition_default_file_path_from_dir(asset_library_root_); catalog_definition_file_ = std::move(cdf); } @@ -117,8 +125,8 @@ bool AssetCatalogService::ensure_asset_library_root() return false; } - if (fs::exists(asset_library_root_)) { - if (!fs::is_directory(asset_library_root_)) { + if (BLI_exists(asset_library_root_.data())) { + if (!BLI_is_dir(asset_library_root_.data())) { std::cerr << "AssetCatalogService: " << asset_library_root_ << " exists but is not a directory, this is not a supported situation." << std::endl; @@ -131,7 +139,7 @@ bool AssetCatalogService::ensure_asset_library_root() /* Ensure the root directory exists. */ std::error_code err_code; - if (!fs::create_directories(asset_library_root_, err_code)) { + if (!BLI_dir_create_recursive(asset_library_root_.data())) { std::cerr << "AssetCatalogService: error creating directory " << asset_library_root_ << ": " << err_code << std::endl; return false; @@ -148,17 +156,20 @@ void AssetCatalogService::load_from_disk() void AssetCatalogService::load_from_disk(const CatalogFilePath &file_or_directory_path) { - fs::file_status status = fs::status(file_or_directory_path); - switch (status.type()) { - case fs::file_type::regular: - load_single_file(file_or_directory_path); - break; - case fs::file_type::directory: - load_directory_recursive(file_or_directory_path); - break; - default: - // TODO(@sybren): throw an appropriate exception. - return; + BLI_stat_t status; + if (BLI_stat(file_or_directory_path.data(), &status) == -1) { + // TODO(@sybren): throw an appropriate exception. + return; + } + + if (S_ISREG(status.st_mode)) { + load_single_file(file_or_directory_path); + } + else if (S_ISDIR(status.st_mode)) { + load_directory_recursive(file_or_directory_path); + } + else { + // TODO(@sybren): throw an appropriate exception. } /* TODO: Should there be a sanitize step? E.g. to remove catalogs with identical paths? */ @@ -170,13 +181,13 @@ void AssetCatalogService::load_directory_recursive(const CatalogFilePath &direct { // TODO(@sybren): implement proper multi-file support. For now, just load // the default file if it is there. - CatalogFilePath file_path = directory_path / DEFAULT_CATALOG_FILENAME; - fs::file_status fs_status = fs::status(file_path); + CatalogFilePath file_path = asset_definition_default_file_path_from_dir(directory_path); - if (!fs::exists(fs_status)) { + if (!BLI_exists(file_path.data())) { /* No file to be loaded is perfectly fine. */ return; } + this->load_single_file(file_path); } @@ -289,18 +300,25 @@ std::unique_ptr<AssetCatalogTree> AssetCatalogService::read_into_tree() /* Go through the catalogs, insert each path component into the tree where needed. */ for (auto &catalog : catalogs_.values()) { - /* #fs::path adds useful behavior to the path. Remember that on Windows it uses "\" as - * separator! For catalogs it should always be "/". Use #fs::path::generic_string if needed. */ - fs::path catalog_path = catalog->path; - const AssetCatalogTreeItem *parent = nullptr; AssetCatalogTreeItem::ChildMap *insert_to_map = &tree->children_; - BLI_assert_msg(catalog_path.is_relative() && !catalog_path.has_root_path(), - "Malformed catalog path: Path should be a relative path, with no root-name or " - "root-directory as defined by std::filesystem::path."); - for (const fs::path &component : catalog_path) { - std::string component_name = component.string(); + BLI_assert_msg(!ELEM(catalog->path[0], '/', '\\'), + "Malformed catalog path: Path should be formatted like a relative path"); + + const char *next_slash_ptr; + /* Looks more complicated than it is, this just iterates over path components. E.g. + * "just/some/path" iterates over "just", then "some" then "path". */ + for (const char *name_begin = catalog->path.data(); name_begin && name_begin[0]; + /* Jump to one after the next slash if there is any. */ + name_begin = next_slash_ptr ? next_slash_ptr + 1 : nullptr) { + next_slash_ptr = BLI_path_slash_find(name_begin); + + /* Note that this won't be null terminated. */ + StringRef component_name = next_slash_ptr ? + StringRef(name_begin, next_slash_ptr - name_begin) : + /* Last component in the path. */ + name_begin; /* Insert new tree element - if no matching one is there yet! */ auto [item, was_inserted] = insert_to_map->emplace( diff --git a/source/blender/blenkernel/intern/asset_catalog_test.cc b/source/blender/blenkernel/intern/asset_catalog_test.cc index 65cff8c58df..48179576a33 100644 --- a/source/blender/blenkernel/intern/asset_catalog_test.cc +++ b/source/blender/blenkernel/intern/asset_catalog_test.cc @@ -21,13 +21,10 @@ #include "BKE_asset_catalog.hh" #include "BLI_fileops.h" +#include "BLI_path_util.h" #include "testing/testing.h" -#include <filesystem> - -namespace fs = blender::filesystem; - namespace blender::bke::tests { /* UUIDs from lib/tests/asset_library/blender_assets.cats.txt */ @@ -61,12 +58,12 @@ class AssetCatalogTest : public testing::Test { void SetUp() override { - const fs::path test_files_dir = blender::tests::flags_test_asset_dir(); + const std::string test_files_dir = blender::tests::flags_test_asset_dir(); if (test_files_dir.empty()) { FAIL(); } - asset_library_root_ = test_files_dir / "asset_library"; + asset_library_root_ = test_files_dir + "/" + "asset_library"; temp_library_path_ = ""; } @@ -74,33 +71,31 @@ class AssetCatalogTest : public testing::Test { CatalogFilePath use_temp_path() { const CatalogFilePath tempdir = BKE_tempdir_session(); - temp_library_path_ = tempdir / "test-temporary-path"; + temp_library_path_ = tempdir + "test-temporary-path"; return temp_library_path_; } - int count_path_parents(const fs::path &path) - { - int counter = 0; - for (const fs::path &segment : path.parent_path()) { - counter++; - UNUSED_VARS(segment); - } - return counter; - } + struct CatalogPathInfo { + StringRef name; + int parent_count; + }; void assert_expected_tree_items(AssetCatalogTree *tree, - const std::vector<fs::path> &expected_paths) + const std::vector<CatalogPathInfo> &expected_paths) { int i = 0; tree->foreach_item([&](const AssetCatalogTreeItem &actual_item) { ASSERT_LT(i, expected_paths.size()) << "More catalogs in tree than expected; did not expect " << actual_item.catalog_path(); + char expected_filename[FILE_MAXFILE]; /* Is the catalog name as expected? "character", "Ellie", ... */ - EXPECT_EQ(expected_paths[i].filename().string(), actual_item.get_name()); - /* Does the number of parents match? */ - EXPECT_EQ(count_path_parents(expected_paths[ @@ Diff output truncated at 10240 characters. @@ _______________________________________________ Bf-blender-cvs mailing list [email protected] List details, subscription details or unsubscribe: https://lists.blender.org/mailman/listinfo/bf-blender-cvs
