From 7778dc71c54126b3b21010c9e7bf52c092e01c4b Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Tue, 18 Mar 2025 16:42:04 +0900 Subject: [PATCH 01/27] test(asset-back): add more AssetLocation unit tests Tests for setPath, setName, setPackName and error handling for these methods too. --- engine/src/assets/AssetLocation.hpp | 12 ++- tests/engine/assets/AssetLocation.test.cpp | 95 ++++++++++++++++++++++ 2 files changed, 104 insertions(+), 3 deletions(-) diff --git a/engine/src/assets/AssetLocation.hpp b/engine/src/assets/AssetLocation.hpp index bd9da3652..816debe79 100644 --- a/engine/src/assets/AssetLocation.hpp +++ b/engine/src/assets/AssetLocation.hpp @@ -57,12 +57,18 @@ namespace nexo::assets { return *this; } - AssetLocation& setPackName(const std::optional>& packName) + AssetLocation& setPackName(const AssetPackName& packName) { _packName = packName; return *this; } + AssetLocation& clearPackName() + { + _packName.reset(); + return *this; + } + /** * @brief Get the asset's name * @return The asset's AssetName @@ -120,9 +126,9 @@ namespace nexo::assets { _path = extractedPath; } - bool operator==(const AssetLocation& asset_location) const + bool operator==(const AssetLocation& assetLocation) const { - return _name == asset_location._name && _packName == asset_location._packName && _path == asset_location._path; + return _name == assetLocation._name && _packName == assetLocation._packName && _path == assetLocation._path; } bool operator==(const std::string& fullLocation) const diff --git a/tests/engine/assets/AssetLocation.test.cpp b/tests/engine/assets/AssetLocation.test.cpp index 7ae1d42e5..d2ba86e03 100644 --- a/tests/engine/assets/AssetLocation.test.cpp +++ b/tests/engine/assets/AssetLocation.test.cpp @@ -152,4 +152,99 @@ namespace nexo::assets { EXPECT_EQ(location.getFullLocation(), "myPack::myAsset"); } + TEST(AssetLocationTest, SetName) + { + AssetLocation location("myPack::myAsset@path/to/asset"); + EXPECT_EQ(location.getFullLocation(), "myPack::myAsset@path/to/asset"); + + + const std::string newName = "newAssetName"; + location.setName(newName); + EXPECT_EQ(location.getFullLocation(), std::format("myPack::{}@path/to/asset", newName)); + } + + TEST(AssetLocationTest, InvalidSetName) + { + AssetLocation location("myPack::myAsset@path/to/asset"); + EXPECT_EQ(location.getFullLocation(), "myPack::myAsset@path/to/asset"); + + EXPECT_THROW({ + location.setName(""); + }, InvalidName); + + EXPECT_THROW({ + location.setName("newAssetName@"); + }, InvalidName); + } + + TEST(AssetLocationTest, SetPath) + { + AssetLocation location("myPack::myAsset@path/to/asset"); + EXPECT_EQ(location.getFullLocation(), "myPack::myAsset@path/to/asset"); + + const std::string newPath = "new/path/to/asset"; + location.setPath(newPath); + EXPECT_EQ(location.getFullLocation(), std::format("myPack::myAsset@{}", newPath)); + } + + TEST(AssetLocationTest, SetPackName) + { + AssetLocation location("myPack::myAsset@path/to/asset"); + EXPECT_EQ(location.getFullLocation(), "myPack::myAsset@path/to/asset"); + + const std::string newPackName = "newPackName"; + location.setPackName(newPackName); + EXPECT_EQ(location.getFullLocation(), std::format("{}::myAsset@path/to/asset", newPackName)); + } + + TEST(AssetLocationTest, ClearPackName) + { + AssetLocation location("myPack::myAsset@path/to/asset"); + EXPECT_EQ(location.getFullLocation(), "myPack::myAsset@path/to/asset"); + + location.clearPackName(); + EXPECT_EQ(location.getFullLocation(), "myAsset@path/to/asset"); + } + + TEST(AssetLocationTest, InvalidSetPackNameEmpty) + { + AssetLocation location("myPack::myAsset@path/to/asset"); + EXPECT_EQ(location.getFullLocation(), "myPack::myAsset@path/to/asset"); + + EXPECT_THROW({ + location.setPackName(""); + }, InvalidName); + + EXPECT_THROW({ + location.setPackName("myPack::"); + }, InvalidName); + } + + TEST(AssetLocationTest, EqualityOperators) + { + const std::string fullLocationEq1 = "myPack::myAsset@path/to/asset"; + const std::string fullLocationEq2 = "myPack::myAsset@path/to/asset"; + const std::string fullLocationNeq = "myPack::myAsset@path/to/otherAsset"; + + const AssetLocation locationEq1(fullLocationEq1); + const AssetLocation locationEq2(fullLocationEq2); + const AssetLocation locationNeq(fullLocationNeq); + + EXPECT_EQ(locationEq1, locationEq2); + EXPECT_EQ(locationEq1, fullLocationEq2); + + EXPECT_EQ(locationEq2, locationEq1); + EXPECT_EQ(locationEq2, fullLocationEq1); + + EXPECT_NE(locationEq1, locationNeq); + EXPECT_NE(locationEq2, locationNeq); + EXPECT_NE(locationNeq, locationEq1); + EXPECT_NE(locationNeq, locationEq2); + EXPECT_NE(locationNeq, fullLocationEq1); + EXPECT_NE(fullLocationNeq, locationEq1); + EXPECT_NE(fullLocationNeq, locationEq2); + + EXPECT_EQ(locationNeq, fullLocationNeq); + } + } // namespace nexo::assets From f144a68bc9a3607e716cfff0dfefef1d4cd23206 Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Tue, 18 Mar 2025 16:53:53 +0900 Subject: [PATCH 02/27] test(asset-back): add tests for AssetRef --- tests/engine/CMakeLists.txt | 1 + tests/engine/assets/AssetRef.test.cpp | 179 ++++++++++++++++++++++++++ 2 files changed, 180 insertions(+) create mode 100644 tests/engine/assets/AssetRef.test.cpp diff --git a/tests/engine/CMakeLists.txt b/tests/engine/CMakeLists.txt index 2372d5ab1..04d6d3b23 100644 --- a/tests/engine/CMakeLists.txt +++ b/tests/engine/CMakeLists.txt @@ -33,6 +33,7 @@ add_executable(engine_tests ${BASEDIR}/components/Camera.test.cpp ${BASEDIR}/assets/AssetName.test.cpp ${BASEDIR}/assets/AssetLocation.test.cpp + ${BASEDIR}/assets/AssetRef.test.cpp # Add other engine test files here ) diff --git a/tests/engine/assets/AssetRef.test.cpp b/tests/engine/assets/AssetRef.test.cpp new file mode 100644 index 000000000..9cbc79675 --- /dev/null +++ b/tests/engine/assets/AssetRef.test.cpp @@ -0,0 +1,179 @@ +//// AssetRef.test.cpp //////////////////////////////////////////////////////// +// +// zzzzz zzz zzzzzzzzzzzzz zzzz zzzz zzzzzz zzzzz +// zzzzzzz zzz zzzz zzzz zzzz zzzz +// zzz zzz zzz zzzzzzzzzzzzz zzzz zzzz zzz +// zzz zzz zzz z zzzz zzzz zzzz zzzz +// zzz zzz zzzzzzzzzzzzz zzzz zzz zzzzzzz zzzzz +// +// Author: Guillaume HEIN +// Date: 18/03/2025 +// Description: Unit tests for the AssetRef class +// +/////////////////////////////////////////////////////////////////////////////// + +#include +#include "assets/AssetRef.hpp" +#include "assets/Asset.hpp" +#include "assets/Assets/Texture/Texture.hpp" +#include "assets/Assets/Model/Model.hpp" + +namespace nexo::assets { + + class AssetRefTest : public ::testing::Test { + protected: + std::shared_ptr textureAsset; + std::shared_ptr modelAsset; + std::shared_ptr genericAsset; + + void SetUp() override { + textureAsset = std::make_shared(); + modelAsset = std::make_shared(); + genericAsset = std::make_shared(); // Using Texture as our generic asset + } + }; + + // GenericAssetRef Tests + TEST_F(AssetRefTest, DefaultConstructorCreatesNullReference) { + GenericAssetRef ref; + EXPECT_FALSE(ref.isValid()); + EXPECT_FALSE(ref); + EXPECT_EQ(ref.lock(), nullptr); + } + + TEST_F(AssetRefTest, ConstructorWithSharedPtrCreatesValidReference) { + GenericAssetRef ref(genericAsset); + EXPECT_TRUE(ref.isValid()); + EXPECT_TRUE(ref); + EXPECT_EQ(ref.lock(), genericAsset); + } + + TEST_F(AssetRefTest, NullStaticMethodReturnsNullReference) { + auto ref = GenericAssetRef::null(); + EXPECT_FALSE(ref.isValid()); + EXPECT_FALSE(ref); + } + + TEST_F(AssetRefTest, IsValidReturnsCorrectValue) { + GenericAssetRef ref(genericAsset); + EXPECT_TRUE(ref.isValid()); + + // Simulate expired pointer + { + auto tempAsset = std::make_shared(); + GenericAssetRef tempRef(tempAsset); + EXPECT_TRUE(tempRef.isValid()); + } + // tempAsset goes out of scope here, should expire + } + + TEST_F(AssetRefTest, LockReturnsCorrectPointer) { + GenericAssetRef validRef(genericAsset); + GenericAssetRef nullRef; + + EXPECT_EQ(validRef.lock(), genericAsset); + EXPECT_EQ(nullRef.lock(), nullptr); + } + + TEST_F(AssetRefTest, AsMethodCastsCorrectly) { + // Create refs + GenericAssetRef textureRef(textureAsset); + GenericAssetRef modelRef(modelAsset); + GenericAssetRef nullRef; + + // Cast to texture ref + auto castedTextureRef = textureRef.as(); + EXPECT_TRUE(castedTextureRef.isValid()); + EXPECT_EQ(castedTextureRef.lock(), textureAsset); + + // Cast to model ref + auto castedModelRef = modelRef.as(); + EXPECT_TRUE(castedModelRef.isValid()); + EXPECT_EQ(castedModelRef.lock(), modelAsset); + + // Cast null ref + auto castedNullRef = nullRef.as(); + EXPECT_FALSE(castedNullRef.isValid()); + + // Wrong cast should produce invalid reference + auto wrongCastRef = textureRef.as(); + EXPECT_FALSE(wrongCastRef.isValid()); + } + + TEST_F(AssetRefTest, BoolOperatorWorksAsExpected) { + GenericAssetRef validRef(genericAsset); + GenericAssetRef nullRef; + + EXPECT_TRUE(static_cast(validRef)); + EXPECT_FALSE(static_cast(nullRef)); + } + + TEST_F(AssetRefTest, CopyConstructorWorksCorrectly) { + GenericAssetRef original(genericAsset); + GenericAssetRef copy(original); + + EXPECT_TRUE(copy.isValid()); + EXPECT_EQ(copy.lock(), original.lock()); + } + + TEST_F(AssetRefTest, AssignmentOperatorWorksCorrectly) { + GenericAssetRef original(genericAsset); + GenericAssetRef assigned; + assigned = original; + + EXPECT_TRUE(assigned.isValid()); + EXPECT_EQ(assigned.lock(), original.lock()); + } + + // AssetRef Tests + TEST_F(AssetRefTest, TypedDefaultConstructorCreatesNullReference) { + AssetRef ref; + EXPECT_FALSE(ref.isValid()); + EXPECT_EQ(ref.lock(), nullptr); + } + + TEST_F(AssetRefTest, TypedConstructorWithSharedPtrCreatesValidReference) { + AssetRef ref(textureAsset); + EXPECT_TRUE(ref.isValid()); + EXPECT_EQ(ref.lock(), textureAsset); + } + + TEST_F(AssetRefTest, TypedNullStaticMethodReturnsNullReference) { + auto ref = AssetRef::null(); + EXPECT_FALSE(ref.isValid()); + } + + TEST_F(AssetRefTest, TypedLockReturnsCorrectPointer) { + AssetRef validRef(textureAsset); + AssetRef nullRef; + + EXPECT_EQ(validRef.lock(), textureAsset); + EXPECT_EQ(nullRef.lock(), nullptr); + } + + TEST_F(AssetRefTest, IsLoadedReturnsCorrectState) { + // Set up a loaded asset + auto loadedAsset = std::make_shared(); + loadedAsset->m_metadata.status = AssetStatus::LOADED; + + // Set up an unloaded asset + auto unloadedAsset = std::make_shared(); + unloadedAsset->m_metadata.status = AssetStatus::UNLOADED; + + AssetRef loadedRef(loadedAsset); + AssetRef unloadedRef(unloadedAsset); + AssetRef nullRef; + + EXPECT_TRUE(loadedRef.isLoaded()); + EXPECT_FALSE(unloadedRef.isLoaded()); + EXPECT_FALSE(nullRef.isLoaded()); + } + + TEST_F(AssetRefTest, LoadAndUnloadMethodsCalledSuccessfully) { + // These methods currently only log warnings, but we test they don't crash + GenericAssetRef ref(genericAsset); + EXPECT_NO_FATAL_FAILURE(ref.load()); + EXPECT_NO_FATAL_FAILURE(ref.unload()); + } + +} // namespace nexo::assets From f00d6f6dec50e09fdddff606af4c9563a9c9d46a Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Thu, 20 Mar 2025 13:08:30 +0900 Subject: [PATCH 03/27] feat(asset-back): improve return value for AssetCatalog::getAssetsView --- engine/src/assets/AssetCatalog.cpp | 10 +--------- engine/src/assets/AssetCatalog.hpp | 17 ++++++++++++++++- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/engine/src/assets/AssetCatalog.cpp b/engine/src/assets/AssetCatalog.cpp index 0be4b3dcc..4399c9c1b 100644 --- a/engine/src/assets/AssetCatalog.cpp +++ b/engine/src/assets/AssetCatalog.cpp @@ -61,19 +61,11 @@ namespace nexo::assets { return assets; } - std::ranges::view auto AssetCatalog::getAssetsView() const - { - return m_assets - | std::views::values - | std::views::transform([](const auto& asset) { - return GenericAssetRef(asset); - }); - } - GenericAssetRef AssetCatalog::registerAsset(const AssetLocation& location, IAsset* asset) { if (!asset) return GenericAssetRef::null(); + // TODO: implement error handling if already exists (once we have the folder tree) auto shared_ptr = std::shared_ptr(asset); shared_ptr->m_metadata.location = location; if (shared_ptr->m_metadata.id.is_nil()) diff --git a/engine/src/assets/AssetCatalog.hpp b/engine/src/assets/AssetCatalog.hpp index 1e772639d..50cffc2a6 100644 --- a/engine/src/assets/AssetCatalog.hpp +++ b/engine/src/assets/AssetCatalog.hpp @@ -21,6 +21,10 @@ #include #include +#if NEXO_TESTING +class AssetCatalogTest; +#endif + namespace nexo::assets { /** @@ -29,6 +33,10 @@ namespace nexo::assets { * @brief Singleton class that holds all the assets in the engine. */ class AssetCatalog { +#if NEXO_TESTING + friend class AssetCatalogTest; +#endif + private: // Singleton: private constructor and destructor AssetCatalog(); @@ -82,7 +90,14 @@ namespace nexo::assets { * @brief Get all assets in the catalog as a view. * @return A view of all assets in the catalog. */ - [[nodiscard]] std::ranges::view auto getAssetsView() const; + [[nodiscard]] auto getAssetsView() const + { + return m_assets + | std::views::values + | std::views::transform([](const auto& asset) { + return GenericAssetRef(asset); + }); + } /** * @brief Get all assets of a specific type in the catalog. From b62422b7a7ac9c0a2e0e00c56ac078fa654da678 Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Thu, 20 Mar 2025 13:10:34 +0900 Subject: [PATCH 04/27] feat(asset-back): add global macro for testing NEXO_TESTING Global macro NEXO_TESTING will be set when tests and source code are compiled. Makes it easier for test specific code in the engine. --- tests/CMakeLists.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 485fa9a11..fd4ad88cd 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -30,6 +30,10 @@ set(TEST_MAIN_FILES ${CMAKE_CURRENT_LIST_DIR}/test_main.cpp ) +# Add global macro to set NEXO_TESTING to 1 +add_definitions(-DNEXO_TESTING=1) +message(STATUS "Set global MACRO: NEXO_TESTING = 1") + include(${CMAKE_CURRENT_LIST_DIR}/engine/CMakeLists.txt) include(${CMAKE_CURRENT_LIST_DIR}/common/CMakeLists.txt) include(${CMAKE_CURRENT_LIST_DIR}/renderer/CMakeLists.txt) From 0d9fa54148aaf94153962ee4899d143429c7d53b Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Thu, 20 Mar 2025 13:10:56 +0900 Subject: [PATCH 05/27] test(asset-back): add tests for AssetCatalog --- tests/engine/CMakeLists.txt | 3 +- tests/engine/assets/AssetCatalog.test.cpp | 230 ++++++++++++++++++++++ 2 files changed, 232 insertions(+), 1 deletion(-) create mode 100644 tests/engine/assets/AssetCatalog.test.cpp diff --git a/tests/engine/CMakeLists.txt b/tests/engine/CMakeLists.txt index 04d6d3b23..6a8f31194 100644 --- a/tests/engine/CMakeLists.txt +++ b/tests/engine/CMakeLists.txt @@ -31,8 +31,9 @@ add_executable(engine_tests ${BASEDIR}/scene/Scene.test.cpp ${BASEDIR}/scene/SceneManager.test.cpp ${BASEDIR}/components/Camera.test.cpp - ${BASEDIR}/assets/AssetName.test.cpp ${BASEDIR}/assets/AssetLocation.test.cpp + ${BASEDIR}/assets/AssetCatalog.test.cpp + ${BASEDIR}/assets/AssetName.test.cpp ${BASEDIR}/assets/AssetRef.test.cpp # Add other engine test files here ) diff --git a/tests/engine/assets/AssetCatalog.test.cpp b/tests/engine/assets/AssetCatalog.test.cpp new file mode 100644 index 000000000..7736db86d --- /dev/null +++ b/tests/engine/assets/AssetCatalog.test.cpp @@ -0,0 +1,230 @@ +//// AssetCatalog.test.cpp //////////////////////////////////////////////////// +// +// zzzzz zzz zzzzzzzzzzzzz zzzz zzzz zzzzzz zzzzz +// zzzzzzz zzz zzzz zzzz zzzz zzzz +// zzz zzz zzz zzzzzzzzzzzzz zzzz zzzz zzz +// zzz zzz zzz z zzzz zzzz zzzz zzzz +// zzz zzz zzzzzzzzzzzzz zzzz zzz zzzzzzz zzzzz +// +// Author: Guillaume HEIN +// Date: 20/03/2025 +// Description: Unit tests for the AssetCatalog class +// +/////////////////////////////////////////////////////////////////////////////// + +#include +#include "assets/AssetCatalog.hpp" +#include "assets/Asset.hpp" +#include "assets/Assets/Texture/Texture.hpp" +#include "assets/Assets/Model/Model.hpp" + +namespace nexo::assets { + + class AssetCatalogTest : public ::testing::Test { + protected: + void SetUp() override { + } + + void TearDown() override { + } + AssetCatalog assetCatalog; + }; + + TEST_F(AssetCatalogTest, RegisterAndRetrieveAssetById) { + // Register an asset + AssetLocation location("text@test/texture"); + const auto textureAsset = new Texture(); + const auto ref = assetCatalog.registerAsset(location, textureAsset); + ASSERT_TRUE(ref.isValid()); + + const auto id = ref.lock()->getID(); + + // Retrieve by ID + const auto retrievedRef = assetCatalog.getAsset(id); + + EXPECT_TRUE(retrievedRef.isValid()); + EXPECT_EQ(retrievedRef.lock()->getID(), id); + } + + TEST_F(AssetCatalogTest, RegisterAndRetrieveAssetByLocation) { + // Register an asset + AssetLocation location("text@test/texture"); + const auto textureAsset = new Texture(); + const auto ref = assetCatalog.registerAsset(location, textureAsset); + ASSERT_TRUE(ref.isValid()); + ASSERT_TRUE(ref); + + // Retrieve by location + const auto retrievedRef = assetCatalog.getAsset(location); + + EXPECT_TRUE(retrievedRef.isValid()); + EXPECT_TRUE(retrievedRef); + EXPECT_EQ(retrievedRef.lock()->getID(), ref.lock()->getID()); + } + + TEST_F(AssetCatalogTest, DeleteAssetById) { + AssetLocation location("text@test/texture"); + const auto textureAsset = new Texture(); + auto ref = assetCatalog.registerAsset(location, textureAsset); + auto id = ref.lock()->getID(); + + // Delete by ID + assetCatalog.deleteAsset(id); + + // Asset should no longer be retrievable + auto retrievedRef = assetCatalog.getAsset(id); + EXPECT_FALSE(retrievedRef.isValid()); + EXPECT_FALSE(retrievedRef); + EXPECT_FALSE(retrievedRef.lock()); + + EXPECT_FALSE(ref.isValid()); + EXPECT_FALSE(ref); + EXPECT_FALSE(ref.lock()); + } + + TEST_F(AssetCatalogTest, DeleteAssetByReference) { + AssetLocation location("text@test/texture"); + const auto textureAsset = new Texture(); + auto ref = assetCatalog.registerAsset(location, textureAsset); + + // Delete by reference + assetCatalog.deleteAsset(ref); + + // Asset should no longer be retrievable + auto retrievedRef = assetCatalog.getAsset(location); + EXPECT_FALSE(retrievedRef.isValid()); + EXPECT_FALSE(retrievedRef); + EXPECT_FALSE(retrievedRef.lock()); + + EXPECT_FALSE(ref.isValid()); + EXPECT_FALSE(ref); + EXPECT_FALSE(ref.lock()); + } + + TEST_F(AssetCatalogTest, GetAssetsReturnsAllAssets) { + const auto textureAsset = new Texture(); + const auto modelAsset = new Model(); + + // Register multiple assets + assetCatalog.registerAsset(AssetLocation("text@test/texture"), textureAsset); + assetCatalog.registerAsset(AssetLocation("model@test/model"), modelAsset); + + // Get all assets + auto assets = assetCatalog.getAssets(); + + // Should have 2 assets + EXPECT_EQ(assets.size(), 2); + for (const auto& assetRef : assets) { + EXPECT_TRUE(assetRef); + EXPECT_TRUE(assetRef.isValid()); + EXPECT_TRUE(assetRef.lock()); + } + } + + TEST_F(AssetCatalogTest, GetAssetsReturnsAllAssetsViews) { + const auto textureAsset = new Texture(); + const auto modelAsset = new Model(); + + // Register multiple assets + const auto textRef = assetCatalog.registerAsset(AssetLocation("text@test/texture"), textureAsset); + const auto modelRef = assetCatalog.registerAsset(AssetLocation("model@test/model"), modelAsset); + + // Get all assets as a view + auto assetsView = assetCatalog.getAssetsView(); + + // Should have 2 assets + EXPECT_EQ(assetsView.size(), 2); + for (const GenericAssetRef& assetRef : assetsView) { + EXPECT_TRUE(assetRef); + EXPECT_TRUE(assetRef.isValid()); + EXPECT_TRUE(assetRef.lock()); + } + + // Delete all assets + assetCatalog.deleteAsset(textRef); + assetCatalog.deleteAsset(modelRef); + + assetsView = assetCatalog.getAssetsView(); + // Should have 0 assets + EXPECT_EQ(assetsView.size(), 0); + } + + TEST_F(AssetCatalogTest, MultipleAssetsDeleteOne) { + const auto textureAsset = new Texture(); + const auto modelAsset = new Model(); + + // Register multiple assets + const auto textRef = assetCatalog.registerAsset(AssetLocation("text@test/texture"), textureAsset); + const auto modelRef = assetCatalog.registerAsset(AssetLocation("model@test/model"), modelAsset); + const auto modelId = modelRef.lock()->getID(); + + // Get all assets + auto assets = assetCatalog.getAssets(); + + // Should have 2 assets + EXPECT_EQ(assets.size(), 2); + for (const auto& assetRef : assets) { + EXPECT_TRUE(assetRef); + EXPECT_TRUE(assetRef.isValid()); + EXPECT_TRUE(assetRef.lock()); + } + + // Delete model + assetCatalog.deleteAsset(modelRef); + EXPECT_FALSE(modelRef.isValid()); + EXPECT_FALSE(modelRef); + EXPECT_FALSE(modelRef.lock()); + + assets = assetCatalog.getAssets(); + // Should have 1 asset + EXPECT_EQ(assets.size(), 1); + EXPECT_EQ(assets[0].lock()->getID(), textRef.lock()->getID()); + + // Check that the model asset is no longer retrievable + auto retrievedRef = assetCatalog.getAsset(modelId); + EXPECT_FALSE(retrievedRef.isValid()); + EXPECT_FALSE(retrievedRef); + EXPECT_FALSE(retrievedRef.lock()); + + // Check that the texture asset is still retrievable + retrievedRef = assetCatalog.getAsset(textRef.lock()->getID()); + EXPECT_TRUE(retrievedRef.isValid()); + EXPECT_TRUE(retrievedRef); + EXPECT_TRUE(retrievedRef.lock()); + } + + TEST_F(AssetCatalogTest, GetNonExistentAssetReturnsInvalidRef) { + // Try to get asset with non-existent ID + AssetID nonExistentId; // Default-constructed UUID should be nil + auto ref = assetCatalog.getAsset(nonExistentId); + + EXPECT_FALSE(ref.isValid()); + EXPECT_FALSE(ref); + EXPECT_FALSE(ref.lock()); + + // Try to get asset with non-existent location + AssetLocation nonExistentLocation("test@does/not/exist"); + ref = assetCatalog.getAsset(nonExistentLocation); + + EXPECT_FALSE(ref.isValid()); + EXPECT_FALSE(ref); + EXPECT_FALSE(ref.lock()); + } + + TEST_F(AssetCatalogTest, GetNoAssets) + { + AssetLocation nonExistentLocation("test@does/not/exist"); + assetCatalog.registerAsset(nonExistentLocation, nullptr); + + assetCatalog.deleteAsset(AssetID{}); + auto assets = assetCatalog.getAssets(); + EXPECT_EQ(assets.size(), 0); + + auto assetsView = assetCatalog.getAssetsView(); + EXPECT_EQ(assetsView.size(), 0); + } + +// Note: Tests for getAssetsOfType and getAssetsOfTypeView would need to be added +// once the static_assert in these methods is resolved + +} // namespace nexo::assets From bed35115260e20c06793ee94436525426f80c08b Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Thu, 20 Mar 2025 13:14:43 +0900 Subject: [PATCH 06/27] ci(asset-back): use Ninja as generator for cmake presets --- CMakePresets.json | 1 + 1 file changed, 1 insertion(+) diff --git a/CMakePresets.json b/CMakePresets.json index 9d3d4d54b..81a154bbb 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -10,6 +10,7 @@ "description": "Default configuration with vcpkg", "hidden": false, "binaryDir": "${sourceDir}/build", + "generator": "Ninja", "cacheVariables": { "CMAKE_TOOLCHAIN_FILE": { "type": "FILEPATH", From fabc85e8823319fce5aeffbe0df06c44c65ad153 Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Thu, 20 Mar 2025 13:36:29 +0900 Subject: [PATCH 07/27] refactor(asset-back): set AssetCatalog constructor as default --- engine/src/assets/AssetCatalog.cpp | 3 --- engine/src/assets/AssetCatalog.hpp | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/engine/src/assets/AssetCatalog.cpp b/engine/src/assets/AssetCatalog.cpp index 4399c9c1b..62b861e6d 100644 --- a/engine/src/assets/AssetCatalog.cpp +++ b/engine/src/assets/AssetCatalog.cpp @@ -17,9 +17,6 @@ #include namespace nexo::assets { - AssetCatalog::AssetCatalog() - { - } void AssetCatalog::deleteAsset(AssetID id) { diff --git a/engine/src/assets/AssetCatalog.hpp b/engine/src/assets/AssetCatalog.hpp index 50cffc2a6..cb523aee3 100644 --- a/engine/src/assets/AssetCatalog.hpp +++ b/engine/src/assets/AssetCatalog.hpp @@ -39,7 +39,7 @@ namespace nexo::assets { private: // Singleton: private constructor and destructor - AssetCatalog(); + AssetCatalog() = default; ~AssetCatalog() = default; public: From 66cc9e485d5a3fa21d9fc93a5d027a5d2e4d6a19 Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Thu, 20 Mar 2025 13:36:52 +0900 Subject: [PATCH 08/27] test(asset-back): add AssetCatalogTest constructor and destructor --- tests/engine/assets/AssetCatalog.test.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/engine/assets/AssetCatalog.test.cpp b/tests/engine/assets/AssetCatalog.test.cpp index 7736db86d..746fe6749 100644 --- a/tests/engine/assets/AssetCatalog.test.cpp +++ b/tests/engine/assets/AssetCatalog.test.cpp @@ -22,11 +22,18 @@ namespace nexo::assets { class AssetCatalogTest : public ::testing::Test { protected: + AssetCatalogTest() : assetCatalog() { + } + + ~AssetCatalogTest() override { + } + void SetUp() override { } void TearDown() override { } + AssetCatalog assetCatalog; }; From 92a6e3e335a6a1a8df9f8182d55effe4a34c8c1f Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Thu, 20 Mar 2025 13:46:09 +0900 Subject: [PATCH 09/27] fix(asset-back): global macro not set on MSVC --- engine/src/assets/AssetCatalog.hpp | 4 ++-- tests/CMakeLists.txt | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/engine/src/assets/AssetCatalog.hpp b/engine/src/assets/AssetCatalog.hpp index cb523aee3..fff033fbc 100644 --- a/engine/src/assets/AssetCatalog.hpp +++ b/engine/src/assets/AssetCatalog.hpp @@ -21,7 +21,7 @@ #include #include -#if NEXO_TESTING +#ifdef NEXO_TESTING class AssetCatalogTest; #endif @@ -33,7 +33,7 @@ namespace nexo::assets { * @brief Singleton class that holds all the assets in the engine. */ class AssetCatalog { -#if NEXO_TESTING +#ifdef NEXO_TESTING friend class AssetCatalogTest; #endif diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index fd4ad88cd..0ccab91db 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -31,7 +31,7 @@ set(TEST_MAIN_FILES ) # Add global macro to set NEXO_TESTING to 1 -add_definitions(-DNEXO_TESTING=1) +add_compile_definitions(NEXO_TESTING) message(STATUS "Set global MACRO: NEXO_TESTING = 1") include(${CMAKE_CURRENT_LIST_DIR}/engine/CMakeLists.txt) From 8d7c7534985ced5e0b6f3cbd1915977fc49aba27 Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Thu, 20 Mar 2025 21:37:34 +0900 Subject: [PATCH 10/27] refactor(asset-back): remove NEXO_TESTING macro, difficult to compile The use of NEXO_TESTING could be useful for test specific code, such as friends. However, it is complicated to recompile all of our libs for tests only. Moreover it can lead to bad practice, tests might not test the same code as production. --- engine/src/assets/AssetCatalog.hpp | 10 +--------- tests/CMakeLists.txt | 4 ---- 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/engine/src/assets/AssetCatalog.hpp b/engine/src/assets/AssetCatalog.hpp index fff033fbc..fb89ad187 100644 --- a/engine/src/assets/AssetCatalog.hpp +++ b/engine/src/assets/AssetCatalog.hpp @@ -21,10 +21,6 @@ #include #include -#ifdef NEXO_TESTING -class AssetCatalogTest; -#endif - namespace nexo::assets { /** @@ -33,11 +29,7 @@ namespace nexo::assets { * @brief Singleton class that holds all the assets in the engine. */ class AssetCatalog { -#ifdef NEXO_TESTING - friend class AssetCatalogTest; -#endif - - private: + protected: // Singleton: private constructor and destructor AssetCatalog() = default; ~AssetCatalog() = default; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 0ccab91db..485fa9a11 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -30,10 +30,6 @@ set(TEST_MAIN_FILES ${CMAKE_CURRENT_LIST_DIR}/test_main.cpp ) -# Add global macro to set NEXO_TESTING to 1 -add_compile_definitions(NEXO_TESTING) -message(STATUS "Set global MACRO: NEXO_TESTING = 1") - include(${CMAKE_CURRENT_LIST_DIR}/engine/CMakeLists.txt) include(${CMAKE_CURRENT_LIST_DIR}/common/CMakeLists.txt) include(${CMAKE_CURRENT_LIST_DIR}/renderer/CMakeLists.txt) From 1df38e58610d14dc7428a0073704a1015353fd0b Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Thu, 20 Mar 2025 21:52:26 +0900 Subject: [PATCH 11/27] feat(asset-back): share some CLion config --- .gitignore | 5 +++-- .idea/cmake.xml | 10 ++++++++++ .idea/sonarlint.xml | 8 ++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) create mode 100644 .idea/cmake.xml create mode 100644 .idea/sonarlint.xml diff --git a/.gitignore b/.gitignore index 04494421c..d9ac4b3ed 100644 --- a/.gitignore +++ b/.gitignore @@ -60,7 +60,9 @@ src/client/main.cpp # Covers JetBrains IDEs: IntelliJ, RubyMine, PhpStorm, AppCode, PyCharm, CLion, Android Studio, WebStorm and Rider # Reference: https://intellij-support.jetbrains.com/hc/en-us/articles/206544839 -**/.idea/ +.idea/* +!cmake.xml +!sonarlint.xml # User-specific stuff .idea/**/workspace.xml @@ -150,7 +152,6 @@ B-CPP-500_rtype.pdf *.log # Jetbrains IDEs -.idea/ build/ vcpkg*/ diff --git a/.idea/cmake.xml b/.idea/cmake.xml new file mode 100644 index 000000000..a75305231 --- /dev/null +++ b/.idea/cmake.xml @@ -0,0 +1,10 @@ + + + + + + + + + + \ No newline at end of file diff --git a/.idea/sonarlint.xml b/.idea/sonarlint.xml new file mode 100644 index 000000000..8ab94a851 --- /dev/null +++ b/.idea/sonarlint.xml @@ -0,0 +1,8 @@ + + + + + \ No newline at end of file From e3712157fc58af17681c2719c52b8814d2b24fc0 Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Thu, 20 Mar 2025 21:57:55 +0900 Subject: [PATCH 12/27] feat(asset-back): use Mock class for AssetCatalog --- tests/engine/assets/AssetCatalog.test.cpp | 33 ++++++++++++++--------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/tests/engine/assets/AssetCatalog.test.cpp b/tests/engine/assets/AssetCatalog.test.cpp index 746fe6749..b6165de37 100644 --- a/tests/engine/assets/AssetCatalog.test.cpp +++ b/tests/engine/assets/AssetCatalog.test.cpp @@ -20,6 +20,14 @@ namespace nexo::assets { + class MockAssetCatalog : public AssetCatalog { + public: + MockAssetCatalog() = default; + ~MockAssetCatalog() = default; + + // Mock methods if needed + }; + class AssetCatalogTest : public ::testing::Test { protected: AssetCatalogTest() : assetCatalog() { @@ -34,12 +42,12 @@ namespace nexo::assets { void TearDown() override { } - AssetCatalog assetCatalog; + MockAssetCatalog assetCatalog; }; TEST_F(AssetCatalogTest, RegisterAndRetrieveAssetById) { // Register an asset - AssetLocation location("text@test/texture"); + const AssetLocation location("text@test/texture"); const auto textureAsset = new Texture(); const auto ref = assetCatalog.registerAsset(location, textureAsset); ASSERT_TRUE(ref.isValid()); @@ -55,7 +63,7 @@ namespace nexo::assets { TEST_F(AssetCatalogTest, RegisterAndRetrieveAssetByLocation) { // Register an asset - AssetLocation location("text@test/texture"); + const AssetLocation location("text@test/texture"); const auto textureAsset = new Texture(); const auto ref = assetCatalog.registerAsset(location, textureAsset); ASSERT_TRUE(ref.isValid()); @@ -72,14 +80,14 @@ namespace nexo::assets { TEST_F(AssetCatalogTest, DeleteAssetById) { AssetLocation location("text@test/texture"); const auto textureAsset = new Texture(); - auto ref = assetCatalog.registerAsset(location, textureAsset); - auto id = ref.lock()->getID(); + const auto ref = assetCatalog.registerAsset(location, textureAsset); + const auto id = ref.lock()->getID(); // Delete by ID assetCatalog.deleteAsset(id); // Asset should no longer be retrievable - auto retrievedRef = assetCatalog.getAsset(id); + const auto retrievedRef = assetCatalog.getAsset(id); EXPECT_FALSE(retrievedRef.isValid()); EXPECT_FALSE(retrievedRef); EXPECT_FALSE(retrievedRef.lock()); @@ -92,13 +100,13 @@ namespace nexo::assets { TEST_F(AssetCatalogTest, DeleteAssetByReference) { AssetLocation location("text@test/texture"); const auto textureAsset = new Texture(); - auto ref = assetCatalog.registerAsset(location, textureAsset); + const auto ref = assetCatalog.registerAsset(location, textureAsset); // Delete by reference assetCatalog.deleteAsset(ref); // Asset should no longer be retrievable - auto retrievedRef = assetCatalog.getAsset(location); + const auto retrievedRef = assetCatalog.getAsset(location); EXPECT_FALSE(retrievedRef.isValid()); EXPECT_FALSE(retrievedRef); EXPECT_FALSE(retrievedRef.lock()); @@ -202,7 +210,7 @@ namespace nexo::assets { TEST_F(AssetCatalogTest, GetNonExistentAssetReturnsInvalidRef) { // Try to get asset with non-existent ID - AssetID nonExistentId; // Default-constructed UUID should be nil + constexpr AssetID nonExistentId; // Default-constructed UUID should be nil auto ref = assetCatalog.getAsset(nonExistentId); EXPECT_FALSE(ref.isValid()); @@ -220,18 +228,17 @@ namespace nexo::assets { TEST_F(AssetCatalogTest, GetNoAssets) { - AssetLocation nonExistentLocation("test@does/not/exist"); + const AssetLocation nonExistentLocation("test@does/not/exist"); assetCatalog.registerAsset(nonExistentLocation, nullptr); assetCatalog.deleteAsset(AssetID{}); - auto assets = assetCatalog.getAssets(); + const auto assets = assetCatalog.getAssets(); EXPECT_EQ(assets.size(), 0); auto assetsView = assetCatalog.getAssetsView(); EXPECT_EQ(assetsView.size(), 0); } -// Note: Tests for getAssetsOfType and getAssetsOfTypeView would need to be added -// once the static_assert in these methods is resolved + // TODO: Tests for getAssetsOfType and getAssetsOfTypeView would need to be added once the static_assert in these methods is resolved } // namespace nexo::assets From e71924551f88894f854eb169e1106aaede5bbd91 Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Thu, 20 Mar 2025 22:17:34 +0900 Subject: [PATCH 13/27] test(asset-back): add tests for AssetCatalog singleton --- tests/engine/assets/AssetCatalog.test.cpp | 127 ++++++++++++++++++++-- 1 file changed, 116 insertions(+), 11 deletions(-) diff --git a/tests/engine/assets/AssetCatalog.test.cpp b/tests/engine/assets/AssetCatalog.test.cpp index b6165de37..51df76063 100644 --- a/tests/engine/assets/AssetCatalog.test.cpp +++ b/tests/engine/assets/AssetCatalog.test.cpp @@ -29,20 +29,20 @@ namespace nexo::assets { }; class AssetCatalogTest : public ::testing::Test { - protected: - AssetCatalogTest() : assetCatalog() { - } + protected: + AssetCatalogTest() : assetCatalog() { + } - ~AssetCatalogTest() override { - } + ~AssetCatalogTest() override { + } - void SetUp() override { - } + void SetUp() override { + } - void TearDown() override { - } + void TearDown() override { + } - MockAssetCatalog assetCatalog; + MockAssetCatalog assetCatalog; }; TEST_F(AssetCatalogTest, RegisterAndRetrieveAssetById) { @@ -239,6 +239,111 @@ namespace nexo::assets { EXPECT_EQ(assetsView.size(), 0); } - // TODO: Tests for getAssetsOfType and getAssetsOfTypeView would need to be added once the static_assert in these methods is resolved + class AssetCatalogSingletonTest : public ::testing::Test { + protected: + AssetCatalogSingletonTest() { + } + + ~AssetCatalogSingletonTest() override { + } + + // Do not call AssetCatalog::getInstance() before tests + // to avoid creating the singleton instance before the test SingletonCreationMultithreaded + + void TearDown() override { + // Clean up the singleton instance + const auto assets = AssetCatalog::getInstance().getAssets(); + for (auto& asset : assets) { + AssetCatalog::getInstance().deleteAsset(asset); + } + } + }; + + TEST_F(AssetCatalogSingletonTest, SingletonCreationMultithreaded) + { + // Get instance at the same time on multiple threads, verify that they are the same + std::vector threads; + + std::vector instances; + std::mutex instancesMutex; + constexpr int numThreads = 5; + + for (int i = 0; i < numThreads; ++i) { + threads.emplace_back([&instances, &instancesMutex]() { + auto& instance = AssetCatalog::getInstance(); + EXPECT_TRUE(instance.getAssetsView().empty()); + + // Store the instance in a thread-safe manner + instancesMutex.lock(); + instances.push_back(&instance); + instancesMutex.unlock(); + }); + } + for (auto& thread : threads) { + thread.join(); + } + // Check that all instances are the same + for (const auto& instance : instances) { + EXPECT_EQ(instance, &AssetCatalog::getInstance()); + } + } + + TEST_F(AssetCatalogSingletonTest, SingletonInstance) + { + auto& instance1 = AssetCatalog::getInstance(); + auto& instance2 = AssetCatalog::getInstance(); + + EXPECT_EQ(&instance1, &instance2); + EXPECT_TRUE(AssetCatalog::getInstance().getAssetsView().empty()); + } + + TEST_F(AssetCatalogSingletonTest, SingletonRegisterAndRetrieve) + { + auto& instance = AssetCatalog::getInstance(); + + const AssetLocation location("text@test/texture"); + const auto textureAsset = new Texture(); + const auto ref = instance.registerAsset(location, textureAsset); + ASSERT_TRUE(ref.isValid()); + + const auto id = ref.lock()->getID(); + + // Retrieve by ID + const auto retrievedRef = instance.getAsset(id); + + EXPECT_TRUE(retrievedRef.isValid()); + EXPECT_EQ(retrievedRef.lock()->getID(), id); + } + + TEST_F(AssetCatalogSingletonTest, SingletonShouldBeEmpty) + { + EXPECT_TRUE(AssetCatalog::getInstance().getAssetsView().empty()); + } + + TEST_F(AssetCatalogSingletonTest, SingletonDeleteAsset) + { + auto& instance = AssetCatalog::getInstance(); + + const AssetLocation location("text@test/texture"); + const auto textureAsset = new Texture(); + const auto ref = instance.registerAsset(location, textureAsset); + ASSERT_TRUE(ref.isValid()); + + const auto id = ref.lock()->getID(); + + // Delete by ID + instance.deleteAsset(id); + + // Asset should no longer be retrievable + const auto retrievedRef = instance.getAsset(id); + EXPECT_FALSE(retrievedRef.isValid()); + EXPECT_FALSE(retrievedRef); + EXPECT_FALSE(retrievedRef.lock()); + EXPECT_FALSE(ref.isValid()); + EXPECT_FALSE(ref); + } + + + // TODO: Tests for getAssetsOfType and getAssetsOfTypeView would need to be added once the static_assert in these methods is resolved } // namespace nexo::assets From ffbd8f2166c579418aa5869997cf7d972b499cc5 Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Fri, 21 Mar 2025 01:30:45 +0900 Subject: [PATCH 14/27] feat(asset-back): add JSON serialization for AssetType enum --- engine/src/assets/Asset.hpp | 46 +++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/engine/src/assets/Asset.hpp b/engine/src/assets/Asset.hpp index c7f1e3151..0867ab157 100644 --- a/engine/src/assets/Asset.hpp +++ b/engine/src/assets/Asset.hpp @@ -24,6 +24,7 @@ #include "AssetLocation.hpp" #include "AssetRef.hpp" +#include "json.hpp" namespace nexo::assets { @@ -46,21 +47,46 @@ namespace nexo::assets { _COUNT }; + + /** * @brief Array of asset type names * @note The order of the array must match the order of the AssetType enum. */ - const std::array(AssetType::_COUNT)> AssetTypeNames = { - - "Texture", - "Model", - "Sound", - "Music", - "Font", - "Shader", - "Script" + constexpr const char *AssetTypeNames[] = { + "UNKNOWN", + "TEXTURE", + "MODEL", + "SOUND", + "MUSIC", + "FONT", + "SHADER", + "SCRIPT" }; + static_assert( + static_cast(AssetType::_COUNT) == std::size(AssetTypeNames), + "AssetTypeNames array size must match AssetType enum size" + ); + + constexpr const char *getAssetTypeName(AssetType type) { + return AssetTypeNames[static_cast(type)]; + } + + inline void to_json(nlohmann::json& j, AssetType type) { + j = getAssetTypeName(type); + } + + inline void from_json(const nlohmann::json& j, AssetType& type) { + for (int i = 0; i < static_cast(AssetType::_COUNT); ++i) { + if (j == AssetTypeNames[i]) { + type = static_cast(i); + return; + } + } + type = AssetType::UNKNOWN; + } + /** * @brief Asset ID type * @note This is a UUID that uniquely identifies an asset. Alias of boost::uuids::uuid. @@ -142,6 +168,8 @@ namespace nexo::assets { friend class AssetRef; public: + static constexpr AssetType TYPE = TAssetType; + virtual ~Asset() override { delete data; From 6bedba18fd0d9e6d3f97b77772dc1a0d028972a8 Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Fri, 21 Mar 2025 01:31:11 +0900 Subject: [PATCH 15/27] feat(asset-back): rename genUniqueDependencyName to genUniqueDependencyLocation and add formatUniqueName method --- engine/src/assets/AssetImporterContext.hpp | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/engine/src/assets/AssetImporterContext.hpp b/engine/src/assets/AssetImporterContext.hpp index ad7a12474..88fd75140 100644 --- a/engine/src/assets/AssetImporterContext.hpp +++ b/engine/src/assets/AssetImporterContext.hpp @@ -84,7 +84,12 @@ namespace nexo::assets { template requires std::derived_from - AssetLocation genUniqueDependencyName(); + AssetLocation genUniqueDependencyLocation(); + + static AssetName formatUniqueName(const std::string& name, const AssetType type, unsigned int id) + { + return AssetName(std::format("{}_{}{}", name, getAssetTypeName(type), id)); + } private: @@ -99,19 +104,17 @@ namespace nexo::assets { template requires std::derived_from - AssetLocation AssetImporterContext::genUniqueDependencyName() + AssetLocation AssetImporterContext::genUniqueDependencyLocation() { - auto depLoc = AssetLocation( - std::format("{}_{}{}", location.getFullLocation(), AssetTypeNames[AssetType::getType()], ++m_depUniqueId) - ); + auto depLoc = AssetLocation(location.getFullLocation()); + depLoc.setName(formatUniqueName(location.getName().data(), AssetType::TYPE, ++m_depUniqueId)); + if (!AssetCatalog::getInstance().getAsset(depLoc)) return depLoc; // If the location already exists, we need to generate a new one - auto name = std::string(location.getName()); while (AssetCatalog::getInstance().getAsset(depLoc)) { - std::string newName = name + std::to_string(++m_depUniqueId); - depLoc.setName(newName); + depLoc.setName(formatUniqueName(location.getName().data(), AssetType::TYPE, ++m_depUniqueId)); if (m_depUniqueId > ASSET_MAX_DEPENDENCIES) { // Prevent infinite loop LOG(NEXO_ERROR, "Failed to generate unique name for asset: {}: couldn't find unique id", depLoc.getFullLocation()); From 9e3fc4448db00dd90a014d4199dc7ba27a961c8f Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Fri, 21 Mar 2025 01:33:21 +0900 Subject: [PATCH 16/27] test(asset-back): add AssetImporterContext tests --- tests/engine/CMakeLists.txt | 1 + .../assets/AssetImporterContext.test.cpp | 228 ++++++++++++++++++ 2 files changed, 229 insertions(+) create mode 100644 tests/engine/assets/AssetImporterContext.test.cpp diff --git a/tests/engine/CMakeLists.txt b/tests/engine/CMakeLists.txt index 6a8f31194..cab4ee97c 100644 --- a/tests/engine/CMakeLists.txt +++ b/tests/engine/CMakeLists.txt @@ -35,6 +35,7 @@ add_executable(engine_tests ${BASEDIR}/assets/AssetCatalog.test.cpp ${BASEDIR}/assets/AssetName.test.cpp ${BASEDIR}/assets/AssetRef.test.cpp + ${BASEDIR}/assets/AssetImporterContext.test.cpp # Add other engine test files here ) diff --git a/tests/engine/assets/AssetImporterContext.test.cpp b/tests/engine/assets/AssetImporterContext.test.cpp new file mode 100644 index 000000000..ca0e68621 --- /dev/null +++ b/tests/engine/assets/AssetImporterContext.test.cpp @@ -0,0 +1,228 @@ +//// AssetImporterContext.test.cpp //////////////////////////////////////////// +// +// zzzzz zzz zzzzzzzzzzzzz zzzz zzzz zzzzzz zzzzz +// zzzzzzz zzz zzzz zzzz zzzz zzzz +// zzz zzz zzz zzzzzzzzzzzzz zzzz zzzz zzz +// zzz zzz zzz z zzzz zzzz zzzz zzzz +// zzz zzz zzzzzzzzzzzzz zzzz zzz zzzzzzz zzzzz +// +// Author: Guillaume HEIN +// Date: 04/04/2025 +// Description: Unit tests for the AssetImporterContext class +// +/////////////////////////////////////////////////////////////////////////////// + +#include +#include "assets/AssetImporterContext.hpp" +#include "assets/Asset.hpp" +#include "assets/AssetCatalog.hpp" +#include "assets/Assets/Texture/Texture.hpp" +#include "assets/Assets/Model/Model.hpp" +#include "json.hpp" + +namespace nexo::assets { + + struct TestParams { + int intValue{}; + float floatValue{}; + std::string stringValue; + + NLOHMANN_DEFINE_TYPE_INTRUSIVE(TestParams, + intValue, + floatValue, + stringValue + ) + }; + + class AssetImporterContextTest : public ::testing::Test { + protected: + void SetUp() override + { + // Clean up the catalog before each test + auto& catalog = AssetCatalog::getInstance(); + for (auto& asset : catalog.getAssets()) { + catalog.deleteAsset(asset); + } + } + + void TearDown() override + { + // Clean up the catalog after each test + auto& catalog = AssetCatalog::getInstance(); + for (auto& asset : catalog.getAssets()) { + catalog.deleteAsset(asset); + } + } + + AssetImporterContext context; + }; + + TEST_F(AssetImporterContextTest, GetMainAssetEmptyOnCreation) + { + EXPECT_EQ(context.getMainAsset(), nullptr); + } + + TEST_F(AssetImporterContextTest, SetAndGetMainAsset) + { + auto* asset = new Texture(); + context.setMainAsset(asset); + EXPECT_EQ(context.getMainAsset(), asset); + } + + TEST_F(AssetImporterContextTest, GetDependenciesEmptyOnCreation) + { + EXPECT_TRUE(context.getDependencies().empty()); + } + + TEST_F(AssetImporterContextTest, AddAndGetDependency) + { + // Register an asset first to get a valid reference + auto& catalog = AssetCatalog::getInstance(); + auto* asset = new Texture(); + auto ref = catalog.registerAsset(AssetLocation("test@texture/dependency"), asset); + EXPECT_TRUE(ref); + + // Add as dependency + context.addDependency(ref); + + // Check dependency was added + auto dependencies = context.getDependencies(); + ASSERT_EQ(dependencies.size(), 1); + EXPECT_EQ(dependencies[0].lock()->getID(), ref.lock()->getID()); + } + + TEST_F(AssetImporterContextTest, AddMultipleDependencies) + { + auto& catalog = AssetCatalog::getInstance(); + + // Create and register multiple assets + auto* texture = new Texture(); + auto* model = new Model(); + auto textureRef = catalog.registerAsset(AssetLocation("text@path"), texture); + auto modelRef = catalog.registerAsset(AssetLocation("model@path"), model); + EXPECT_TRUE(textureRef); + EXPECT_TRUE(modelRef); + + // Add dependencies + context.addDependency(textureRef); + context.addDependency(modelRef); + + // Check dependencies were added + auto dependencies = context.getDependencies(); + ASSERT_EQ(dependencies.size(), 2); + EXPECT_EQ(dependencies[0].lock()->getID(), textureRef.lock()->getID()); + EXPECT_EQ(dependencies[1].lock()->getID(), modelRef.lock()->getID()); + } + + TEST_F(AssetImporterContextTest, SetAndGetJsonParameters) + { + json params = { + {"name", "test"}, + {"value", 42}, + {"enabled", true} + }; + + context.setParameters(params); + auto retrievedParams = context.getParameters(); + + EXPECT_EQ(retrievedParams["name"], "test"); + EXPECT_EQ(retrievedParams["value"], 42); + EXPECT_EQ(retrievedParams["enabled"], true); + } + + TEST_F(AssetImporterContextTest, SetAndGetTypedParameters) + { + const TestParams params { + .intValue = 123, + .floatValue = 3.14f, + .stringValue = "test" + }; + + context.setParameters(params); + auto [intValue, floatValue, stringValue] = context.getParameters(); + + EXPECT_EQ(intValue, 123); + EXPECT_FLOAT_EQ(floatValue, 3.14f); + EXPECT_EQ(stringValue, "test"); + } + + TEST_F(AssetImporterContextTest, GenUniqueDependencyLocation) + { + context.location = AssetLocation("test@folder/main"); + + // Generate a unique name for a texture dependency + const auto depName1 = context.genUniqueDependencyLocation(); + EXPECT_EQ(depName1.getFullLocation(), "test_TEXTURE1@folder/main"); + + // Generate another unique name + const auto depName2 = context.genUniqueDependencyLocation(); + EXPECT_EQ(depName2.getFullLocation(), "test_TEXTURE2@folder/main"); + + // Names should be different + EXPECT_NE(depName1.getFullLocation(), depName2.getFullLocation()); + } + + TEST_F(AssetImporterContextTest, GenUniqueDependencyLocationWithExistingAsset) + { + context.location = AssetLocation("test@folder/main"); + + // Generate a name + auto depName1 = context.genUniqueDependencyLocation(); + EXPECT_EQ(depName1.getFullLocation(), "test_TEXTURE1@folder/main"); + + // Register an asset with that name + auto& catalog = AssetCatalog::getInstance(); + auto* asset = new Texture(); + EXPECT_TRUE(catalog.registerAsset(depName1, asset)); + + // Generate another name - should be different + const auto depName2 = context.genUniqueDependencyLocation(); + EXPECT_NE(depName1.getFullLocation(), depName2.getFullLocation()); + EXPECT_EQ(depName2.getFullLocation(), "test_TEXTURE2@folder/main"); + + // The new name should not exist in catalog + EXPECT_FALSE(catalog.getAsset(depName2).isValid()); + } + + TEST_F(AssetImporterContextTest, GenUniqueDependencyLocationWithCollidingName) + { + context.location = AssetLocation("test@folder/main"); + + // Generate a name + auto depName1 = context.genUniqueDependencyLocation(); + + // Register an asset with that name + auto& catalog = AssetCatalog::getInstance(); + auto* asset = new Model(); + EXPECT_TRUE(catalog.registerAsset(depName1, asset)); + EXPECT_EQ(depName1.getFullLocation(), "test_MODEL1@folder/main"); + + // Let's register an asset with the same name as a future dependency + auto* asset2 = new Model(); + EXPECT_TRUE(catalog.registerAsset(AssetLocation("test_MODEL2@folder/main"), asset2)); + + // Generate another dep name: should prevent collision + const auto depName2 = context.genUniqueDependencyLocation(); + EXPECT_EQ(depName2.getFullLocation(), "test_MODEL3@folder/main"); + } + + TEST_F(AssetImporterContextTest, DefaultContextValues) + { + EXPECT_EQ(context.location.getFullLocation(), "default"); + EXPECT_EQ(context.getMainAsset(), nullptr); + EXPECT_TRUE(context.getDependencies().empty()); + EXPECT_TRUE(context.getParameters().is_null()); + } + + TEST_F(AssetImporterContextTest, InputPropertyExists) + { + // Just test that the input property exists and can be assigned + const ImporterInputVariant input = ImporterFileInput{.filePath = std::filesystem::path("test.png")}; + context.input = input; + + // Check if we can access the input + ASSERT_TRUE(std::holds_alternative(context.input)); + EXPECT_EQ(std::get(context.input).filePath, "test.png"); + } + +} From 9b09e77a34de4c87d693408151b9d985e283df67 Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Fri, 21 Mar 2025 15:43:32 +0900 Subject: [PATCH 17/27] test(asset-back): use const for references in AssetImporterContext tests --- tests/engine/assets/AssetImporterContext.test.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/engine/assets/AssetImporterContext.test.cpp b/tests/engine/assets/AssetImporterContext.test.cpp index ca0e68621..e29f26886 100644 --- a/tests/engine/assets/AssetImporterContext.test.cpp +++ b/tests/engine/assets/AssetImporterContext.test.cpp @@ -79,14 +79,14 @@ namespace nexo::assets { // Register an asset first to get a valid reference auto& catalog = AssetCatalog::getInstance(); auto* asset = new Texture(); - auto ref = catalog.registerAsset(AssetLocation("test@texture/dependency"), asset); + const auto ref = catalog.registerAsset(AssetLocation("test@texture/dependency"), asset); EXPECT_TRUE(ref); // Add as dependency context.addDependency(ref); // Check dependency was added - auto dependencies = context.getDependencies(); + const auto dependencies = context.getDependencies(); ASSERT_EQ(dependencies.size(), 1); EXPECT_EQ(dependencies[0].lock()->getID(), ref.lock()->getID()); } @@ -98,8 +98,8 @@ namespace nexo::assets { // Create and register multiple assets auto* texture = new Texture(); auto* model = new Model(); - auto textureRef = catalog.registerAsset(AssetLocation("text@path"), texture); - auto modelRef = catalog.registerAsset(AssetLocation("model@path"), model); + const auto textureRef = catalog.registerAsset(AssetLocation("text@path"), texture); + const auto modelRef = catalog.registerAsset(AssetLocation("model@path"), model); EXPECT_TRUE(textureRef); EXPECT_TRUE(modelRef); @@ -108,7 +108,7 @@ namespace nexo::assets { context.addDependency(modelRef); // Check dependencies were added - auto dependencies = context.getDependencies(); + const auto dependencies = context.getDependencies(); ASSERT_EQ(dependencies.size(), 2); EXPECT_EQ(dependencies[0].lock()->getID(), textureRef.lock()->getID()); EXPECT_EQ(dependencies[1].lock()->getID(), modelRef.lock()->getID()); @@ -167,7 +167,7 @@ namespace nexo::assets { context.location = AssetLocation("test@folder/main"); // Generate a name - auto depName1 = context.genUniqueDependencyLocation(); + const auto depName1 = context.genUniqueDependencyLocation(); EXPECT_EQ(depName1.getFullLocation(), "test_TEXTURE1@folder/main"); // Register an asset with that name @@ -189,7 +189,7 @@ namespace nexo::assets { context.location = AssetLocation("test@folder/main"); // Generate a name - auto depName1 = context.genUniqueDependencyLocation(); + const auto depName1 = context.genUniqueDependencyLocation(); // Register an asset with that name auto& catalog = AssetCatalog::getInstance(); From 7fb3190c8b4933ff81a2a941b204313693ac9585 Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Fri, 21 Mar 2025 15:44:07 +0900 Subject: [PATCH 18/27] test(asset-back): fix leak in AssetImporterContext.test.cpp --- tests/engine/assets/AssetImporterContext.test.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/engine/assets/AssetImporterContext.test.cpp b/tests/engine/assets/AssetImporterContext.test.cpp index e29f26886..da2d80111 100644 --- a/tests/engine/assets/AssetImporterContext.test.cpp +++ b/tests/engine/assets/AssetImporterContext.test.cpp @@ -64,9 +64,9 @@ namespace nexo::assets { TEST_F(AssetImporterContextTest, SetAndGetMainAsset) { - auto* asset = new Texture(); - context.setMainAsset(asset); - EXPECT_EQ(context.getMainAsset(), asset); + Texture asset; + context.setMainAsset(&asset); + EXPECT_EQ(context.getMainAsset(), &asset); } TEST_F(AssetImporterContextTest, GetDependenciesEmptyOnCreation) From 19e7dd36e4c8e9a0efd1d77dc00f4497094a0edd Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Sun, 23 Mar 2025 05:53:28 +0900 Subject: [PATCH 19/27] fix(asset-back): mark importAssetUsingImporter and importAssetTryImporters as const --- engine/src/assets/AssetImporter.cpp | 4 ++-- engine/src/assets/AssetImporter.hpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/engine/src/assets/AssetImporter.cpp b/engine/src/assets/AssetImporter.cpp index 3704b8fe5..9b3ee6c99 100644 --- a/engine/src/assets/AssetImporter.cpp +++ b/engine/src/assets/AssetImporter.cpp @@ -47,7 +47,7 @@ namespace nexo::assets { } GenericAssetRef AssetImporter::importAssetUsingImporter(const AssetLocation& location, - const ImporterInputVariant& inputVariant, AssetImporterBase* importer) + const ImporterInputVariant& inputVariant, AssetImporterBase* importer) const { AssetImporterContext* ctx = m_customCtx; AssetImporterContext ctxOnStack; @@ -71,7 +71,7 @@ namespace nexo::assets { } GenericAssetRef AssetImporter::importAssetTryImporters(const AssetLocation& location, - const ImporterInputVariant& inputVariant, const std::vector& importers) + const ImporterInputVariant& inputVariant, const std::vector& importers) const { std::vector untriedImporters; for (const auto& importer : importers) { diff --git a/engine/src/assets/AssetImporter.hpp b/engine/src/assets/AssetImporter.hpp index 155b0b794..e91a6a92f 100644 --- a/engine/src/assets/AssetImporter.hpp +++ b/engine/src/assets/AssetImporter.hpp @@ -46,9 +46,9 @@ namespace nexo::assets { requires std::derived_from AssetRef importAsset(const AssetLocation& location, const ImporterInputVariant& inputVariant); GenericAssetRef importAssetAuto(const AssetLocation& location, const ImporterInputVariant& inputVariant); - GenericAssetRef importAssetUsingImporter(const AssetLocation& location, const ImporterInputVariant& inputVariant, AssetImporterBase *importer); + GenericAssetRef importAssetUsingImporter(const AssetLocation& location, const ImporterInputVariant& inputVariant, AssetImporterBase *importer) const; GenericAssetRef importAssetTryImporters(const AssetLocation& location, const ImporterInputVariant& inputVariant, const std::vector& - importers); + importers) const; /** * @brief Get all registered importers for an asset type From 5f412555fd65b63c188bd0966998b44da7aa8cbf Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Sun, 23 Mar 2025 05:56:22 +0900 Subject: [PATCH 20/27] refactor(asset-back): enhance AssetImporter interface and improve parameter handling --- engine/src/assets/AssetImporter.hpp | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/engine/src/assets/AssetImporter.hpp b/engine/src/assets/AssetImporter.hpp index e91a6a92f..81dadd076 100644 --- a/engine/src/assets/AssetImporter.hpp +++ b/engine/src/assets/AssetImporter.hpp @@ -97,12 +97,20 @@ namespace nexo::assets { void clearCustomContext() { m_customCtx = nullptr; } - AssetImporterContext *getCustomContext() const { return m_customCtx; } + [[nodiscard]] AssetImporterContext *getCustomContext() const { return m_customCtx; } void setParameters(const json& params); - private: + protected: + + /** + * @brief Protected constructor for custom importers + * @note Used currently by unit tests + */ + explicit AssetImporter(AssetImporterContext *ctx) : m_customCtx(ctx) + { + } /** * @brief Register an importer for a specific asset type @@ -122,7 +130,7 @@ namespace nexo::assets { * * @tparam AssetType The type of asset the importer can handle * @param importer The importer instance to register - * @param priority Optional priority value (higher values = higher priority) + * @param priority Optional priority value (higher values = higher priority, if equal then insertion order) */ template requires std::derived_from @@ -166,7 +174,7 @@ namespace nexo::assets { } template requires std::derived_from - void AssetImporter::registerImporter(AssetImporterBase *importer, int priority) + void AssetImporter::registerImporter(AssetImporterBase *importer, const int priority) { const auto typeIdx = std::type_index(typeid(AssetType)); @@ -181,7 +189,7 @@ namespace nexo::assets { auto& importersDetailsVec = m_importersDetails[typeIdx]; size_t i = 0; - for (; i < importersVec.size() && importersDetailsVec[i].priority < priority; ++i); + for (; i < importersVec.size() && priority <= importersDetailsVec[i].priority ; ++i); importersVec.insert(importersVec.begin() + static_cast(i), importer); importersDetailsVec.insert(importersDetailsVec.begin() + static_cast(i), {priority}); } From 128fc69841629aaa36a2acfdea6b42931ff7d78d Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Sun, 23 Mar 2025 05:56:53 +0900 Subject: [PATCH 21/27] refactor(asset-back): replace gtest_add_tests with gtest_discover_tests for improved test discovery --- tests/CMakeLists.txt | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 485fa9a11..fb09470c4 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -36,20 +36,20 @@ include(${CMAKE_CURRENT_LIST_DIR}/renderer/CMakeLists.txt) include(${CMAKE_CURRENT_LIST_DIR}/ecs/CMakeLists.txt) # Add tests -gtest_add_tests(TARGET engine_tests - TEST_LIST engineTestsList +gtest_discover_tests(engine_tests + TEST_LIST engineTestsList ) -gtest_add_tests(TARGET common_tests - TEST_LIST commonTestsList +gtest_discover_tests(common_tests + TEST_LIST commonTestsList ) -gtest_add_tests(TARGET renderer_tests - TEST_LIST rendererTestsList +gtest_discover_tests(renderer_tests + TEST_LIST rendererTestsList ) -gtest_add_tests(TARGET ecs_tests - TEST_LIST ecsTestsList +gtest_discover_tests(ecs_tests + TEST_LIST ecsTestsList ) -# Core engine tests + # Core engine tests set_tests_properties(${engineTestsList} PROPERTIES LABELS "engine") # Common tests set_tests_properties(${commonTestsList} PROPERTIES LABELS "common") From 7712ae14d28449a21ef4eeb6bedc15a1daf245c3 Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Sun, 23 Mar 2025 05:57:25 +0900 Subject: [PATCH 22/27] test(asset-back): add AssetImporter.test.cpp and change link visibility to PRIVATE --- tests/engine/CMakeLists.txt | 3 +- tests/engine/assets/AssetImporter.test.cpp | 186 +++++++++++++++++++++ 2 files changed, 188 insertions(+), 1 deletion(-) create mode 100644 tests/engine/assets/AssetImporter.test.cpp diff --git a/tests/engine/CMakeLists.txt b/tests/engine/CMakeLists.txt index cab4ee97c..5b9bac150 100644 --- a/tests/engine/CMakeLists.txt +++ b/tests/engine/CMakeLists.txt @@ -36,8 +36,9 @@ add_executable(engine_tests ${BASEDIR}/assets/AssetName.test.cpp ${BASEDIR}/assets/AssetRef.test.cpp ${BASEDIR}/assets/AssetImporterContext.test.cpp + ${BASEDIR}/assets/AssetImporter.test.cpp # Add other engine test files here ) # Link gtest and engine (renderer) libraries -target_link_libraries(engine_tests GTest::gtest GTest::gmock nexoRenderer) +target_link_libraries(engine_tests PRIVATE GTest::gtest GTest::gmock nexoRenderer) diff --git a/tests/engine/assets/AssetImporter.test.cpp b/tests/engine/assets/AssetImporter.test.cpp new file mode 100644 index 000000000..e0abacff0 --- /dev/null +++ b/tests/engine/assets/AssetImporter.test.cpp @@ -0,0 +1,186 @@ +//// AssetImporter.test.cpp /////////////////////////////////////////////////// +// +// zzzzz zzz zzzzzzzzzzzzz zzzz zzzz zzzzzz zzzzz +// zzzzzzz zzz zzzz zzzz zzzz zzzz +// zzz zzz zzz zzzzzzzzzzzzz zzzz zzzz zzz +// zzz zzz zzz z zzzz zzzz zzzz zzzz +// zzz zzz zzzzzzzzzzzzz zzzz zzz zzzzzzz zzzzz +// +// Author: Guillaume HEIN +// Date: 21/03/2025 +// Description: Unit tests for the AssetImporter class +// +/////////////////////////////////////////////////////////////////////////////// + +#include +#include +#include "assets/AssetImporter.hpp" +#include "assets/AssetImporterBase.hpp" +#include "assets/AssetImporterContext.hpp" +#include "assets/Asset.hpp" +#include "assets/AssetCatalog.hpp" +#include "assets/Assets/Texture/Texture.hpp" +#include "assets/Assets/Model/Model.hpp" + +namespace nexo::assets { + + // Mock AssetImporterBase class + class MockImporter final : public AssetImporterBase { + public: + MOCK_METHOD(bool, canRead, (const ImporterInputVariant& inputVariant), (override)); + MOCK_METHOD(void, importImpl, (AssetImporterContext& ctx), (override)); + }; + + + + class AssetImporterTest : public ::testing::Test { + protected: + void SetUp() override + { + // Clean up the catalog before each test + auto& catalog = AssetCatalog::getInstance(); + for (auto& asset : catalog.getAssets()) { + catalog.deleteAsset(asset); + } + } + + void TearDown() override + { + // Clean up the catalog after each test + auto& catalog = AssetCatalog::getInstance(); + for (auto& asset : catalog.getAssets()) { + catalog.deleteAsset(asset); + } + } + }; + + class MockAssetImporter : public AssetImporter { + FRIEND_TEST(AssetImporterTest, ImporterAssetUsingImporterSucess); + FRIEND_TEST(AssetImporterTest, ImporterAssetAutoSucess); + FRIEND_TEST(AssetImporterTest, ImporterPriorityOrder); + public: + MockAssetImporter() : AssetImporter(nullptr) + { + } + + ~MockAssetImporter() = default; + }; + + + /** + * @brief Test the import of an asset using a specific importer + * + */ + TEST_F(AssetImporterTest, ImporterAssetUsingImporterSucess) + { + MockAssetImporter importer; + const auto mockImporter = new MockImporter(); + const AssetLocation location("test::myAsset@path"); + ImporterFileInput input; + const auto expectedAsset = new Texture(); + + // Simulate successful import + EXPECT_CALL(*mockImporter, canRead(testing::_)).Times(0); // Never called + EXPECT_CALL(*mockImporter, importImpl(testing::_)) + .WillOnce(testing::Invoke([&](AssetImporterContext& ctx) { + ctx.setMainAsset(expectedAsset); + })); + + importer.registerImporter(mockImporter, 100); + + const auto assetRef = importer.importAssetUsingImporter(location, input, mockImporter); + + ASSERT_TRUE(assetRef.isValid()); + ASSERT_EQ(assetRef.lock().get(), expectedAsset); + ASSERT_EQ(assetRef.lock()->getMetadata().location.getFullLocation(), location.getFullLocation()); + } + + TEST_F(AssetImporterTest, ImporterAssetAutoSucess) + { + MockAssetImporter importer; + const auto mockImporter = new MockImporter(); + const AssetLocation location("test::myAsset@path"); + ImporterFileInput input; + const auto expectedAsset = new Texture(); + + // Setup expectations + EXPECT_CALL(*mockImporter, canRead(testing::_)).WillOnce(testing::Return(true)); + EXPECT_CALL(*mockImporter, importImpl(testing::_)) + .WillOnce(testing::Invoke([&](AssetImporterContext& ctx) { + ctx.setMainAsset(expectedAsset); + })); + + // Register the mock importer + importer.registerImporter(mockImporter, 100); + + // Call the method + const auto assetRef = importer.importAssetAuto(location, input); + + // Assertions + ASSERT_TRUE(assetRef.isValid()); + ASSERT_EQ(assetRef.lock().get(), expectedAsset); + ASSERT_EQ(assetRef.lock()->getMetadata().location.getFullLocation(), location.getFullLocation()); + + // Clean up + importer.unregisterAllImportersForType(); + // Delete the mock importer + delete mockImporter; + } + + TEST_F(AssetImporterTest, ImportAssetAutoFailureNoImporters) + { + MockAssetImporter importer; + const AssetLocation location("test::myAsset@path"); + ImporterFileInput input; + + // Call the method + const auto assetRef = importer.importAssetAuto(location, input); + + // Assertions + ASSERT_FALSE(assetRef.isValid()); + } + + TEST_F(AssetImporterTest, ImporterPriorityOrder) { + MockAssetImporter importer; + const auto bestImporter = new MockImporter(); + const auto wrongImporter = new MockImporter(); + const AssetLocation location("test::myAsset@path"); + ImporterFileInput input; + const auto expectedAsset = new Texture(); + + // Setup expectations + EXPECT_CALL(*bestImporter, canRead(testing::_)).WillOnce(testing::Return(true)); + EXPECT_CALL(*bestImporter, importImpl(testing::_)) + .WillOnce(testing::Invoke([&](AssetImporterContext& ctx) { + ctx.setMainAsset(expectedAsset); + })); + + EXPECT_CALL(*wrongImporter, canRead(testing::_)).Times(0); + EXPECT_CALL(*wrongImporter, importImpl(testing::_)).Times(0); + + // Register the mock importers with different priorities + importer.registerImporter(wrongImporter, 50); + importer.registerImporter(bestImporter, 100); + + // Call the method + const auto assetRef = importer.importAssetAuto(location, input); + + // Assertions + ASSERT_TRUE(assetRef.isValid()); + ASSERT_EQ(assetRef.lock().get(), expectedAsset); + } + + TEST_F(AssetImporterTest, SetAndGetCustomContext) { + MockAssetImporter importer; + AssetImporterContext customContext; + + // Set custom context + importer.setCustomContext(&customContext); + ASSERT_EQ(importer.getCustomContext(), &customContext); + + // Clear custom context + importer.clearCustomContext(); + ASSERT_EQ(importer.getCustomContext(), nullptr); + } + +} // namespace nexo::assets From 853c06f4939d06e7a67d1c5534f89f1120ed0d8c Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Tue, 25 Mar 2025 12:55:54 +0900 Subject: [PATCH 23/27] refactor(asset-back): improve const correctness and simplify unregister logic in AssetImporter --- engine/src/assets/AssetImporter.cpp | 2 +- engine/src/assets/AssetImporter.hpp | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/engine/src/assets/AssetImporter.cpp b/engine/src/assets/AssetImporter.cpp index 9b3ee6c99..9c067d3bb 100644 --- a/engine/src/assets/AssetImporter.cpp +++ b/engine/src/assets/AssetImporter.cpp @@ -59,7 +59,7 @@ namespace nexo::assets { importer->import(*ctx); - auto asset = ctx->getMainAsset(); + const auto asset = ctx->getMainAsset(); if (!asset) return GenericAssetRef::null(); if (asset->getID().is_nil()) diff --git a/engine/src/assets/AssetImporter.hpp b/engine/src/assets/AssetImporter.hpp index 81dadd076..8e6cf9fba 100644 --- a/engine/src/assets/AssetImporter.hpp +++ b/engine/src/assets/AssetImporter.hpp @@ -225,7 +225,6 @@ namespace nexo::assets { void AssetImporter::unregisterAllImportersForType() { const auto typeIdx = std::type_index(typeid(AssetType)); - m_importers.erase(typeIdx); - m_importersDetails.erase(typeIdx); + unregisterAllImportersForType(typeIdx); } } // namespace nexo::assets From e6375feae03409c7e24bc82e7d5d73c06ac79c07 Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Tue, 25 Mar 2025 12:56:02 +0900 Subject: [PATCH 24/27] test(asset-back): add tests for multiple importer types and incompatible importers --- tests/engine/assets/AssetImporter.test.cpp | 138 ++++++++++++++++++++- 1 file changed, 135 insertions(+), 3 deletions(-) diff --git a/tests/engine/assets/AssetImporter.test.cpp b/tests/engine/assets/AssetImporter.test.cpp index e0abacff0..1cda16834 100644 --- a/tests/engine/assets/AssetImporter.test.cpp +++ b/tests/engine/assets/AssetImporter.test.cpp @@ -16,9 +16,6 @@ #include #include "assets/AssetImporter.hpp" #include "assets/AssetImporterBase.hpp" -#include "assets/AssetImporterContext.hpp" -#include "assets/Asset.hpp" -#include "assets/AssetCatalog.hpp" #include "assets/Assets/Texture/Texture.hpp" #include "assets/Assets/Model/Model.hpp" @@ -58,6 +55,9 @@ namespace nexo::assets { FRIEND_TEST(AssetImporterTest, ImporterAssetUsingImporterSucess); FRIEND_TEST(AssetImporterTest, ImporterAssetAutoSucess); FRIEND_TEST(AssetImporterTest, ImporterPriorityOrder); + FRIEND_TEST(AssetImporterTest, MultipleImportersType); + FRIEND_TEST(AssetImporterTest, TryIncompatibleImporters); + FRIEND_TEST(AssetImporterTest, ImportersTriedNoValidFound); public: MockAssetImporter() : AssetImporter(nullptr) { @@ -183,4 +183,136 @@ namespace nexo::assets { ASSERT_EQ(importer.getCustomContext(), nullptr); } + class MockShaderAsset final : public Asset { + + }; + + TEST_F(AssetImporterTest, MultipleImportersType) + { + MockAssetImporter importer; + const auto textureImporter = new MockImporter(); + const auto textureImporter2 = new MockImporter(); + const auto validModelImporter = new MockImporter(); + const auto cannotReadModelImporter = new MockImporter(); + + importer.registerImporter(textureImporter, 100); + importer.registerImporter(validModelImporter, 90); + importer.registerImporter(textureImporter2, 50); + importer.registerImporter(cannotReadModelImporter, 110); + importer.registerImporter(120); + + const auto textureImporters = importer.getImportersForType(); + const auto modelImporters = importer.getImportersForType(); + + // Verify that importers are registered correctly + EXPECT_EQ(textureImporters.size(), 2); + EXPECT_EQ(modelImporters.size(), 3); + + EXPECT_TRUE(importer.hasImportersForType()); + EXPECT_TRUE(importer.hasImportersForType()); + + EXPECT_EQ(textureImporters[0], textureImporter); + EXPECT_EQ(textureImporters[1], textureImporter2); + + const auto cannotReadModelImporter2 = dynamic_cast(modelImporters[0]); + EXPECT_NE(cannotReadModelImporter2, nullptr); + EXPECT_EQ(modelImporters[1], cannotReadModelImporter); + EXPECT_EQ(modelImporters[2], validModelImporter); + + // Setup expectations for each importer + // Texture importers should NEVER be called + EXPECT_CALL(*textureImporter, canRead(testing::_)).Times(0); + EXPECT_CALL(*textureImporter, importImpl(testing::_)).Times(0); + EXPECT_CALL(*textureImporter2, canRead(testing::_)).Times(0); + EXPECT_CALL(*textureImporter2, importImpl(testing::_)).Times(0); + + // Only the validModelImporter should be called + EXPECT_CALL(*validModelImporter, canRead(testing::_)).WillOnce(testing::Return(true)); + EXPECT_CALL(*validModelImporter, importImpl(testing::_)) + .WillOnce(testing::Invoke([](AssetImporterContext& ctx) { + ctx.setMainAsset(new Model()); + })); + // The other model importers should return false + EXPECT_CALL(*cannotReadModelImporter, canRead(testing::_)).WillOnce(testing::Return(false)); + EXPECT_CALL(*cannotReadModelImporter, importImpl(testing::_)).Times(0); + EXPECT_CALL(*cannotReadModelImporter2, canRead(testing::_)).WillOnce(testing::Return(false)); + EXPECT_CALL(*cannotReadModelImporter2, importImpl(testing::_)).Times(0); + + const AssetLocation location("test::myAsset@path"); + const ImporterFileInput input; + // Should import the model using the validModelImporter, + // even though cannotReadModelImporter is registered as higher priority + const auto assetRef = importer.importAsset(location, input); + EXPECT_TRUE(assetRef); + + const AssetLocation location2("test::myAsset@path2"); + const auto invalidShaderAssetRef = importer.importAsset(location2, input); + EXPECT_FALSE(invalidShaderAssetRef); + + } + + /** + * @brief Test the import and the feature that tries incompatible importers. + * In the code, as a last resort, importers that previously return canRead -> false + * are called again to try to import the asset. + */ + TEST_F(AssetImporterTest, TryIncompatibleImporters) { + MockAssetImporter importer; + const auto bestImporter = new MockImporter(); + const auto wrongImporter = new MockImporter(); + const AssetLocation location("test::myAsset@path"); + ImporterFileInput input; + const auto expectedAsset = new Texture(); + + // bestImporter is checked for canRead, then ultimately it is still used for importImpl + EXPECT_CALL(*bestImporter, canRead(testing::_)) + .WillOnce(testing::Return(false)); + EXPECT_CALL(*bestImporter, importImpl(testing::_)) + .WillOnce(testing::Invoke([&](AssetImporterContext& ctx) { + ctx.setMainAsset(expectedAsset); + })); + + EXPECT_CALL(*wrongImporter, canRead(testing::_)).WillOnce(testing::Return(false)); + EXPECT_CALL(*wrongImporter, importImpl(testing::_)).Times(0); + + // Register the mock importers with different priorities + importer.registerImporter(wrongImporter, 50); + importer.registerImporter(bestImporter, 100); + + // Call the method + const auto assetRef = importer.importAssetAuto(location, input); + + // Assertions + ASSERT_TRUE(assetRef.isValid()); + ASSERT_EQ(assetRef.lock().get(), expectedAsset); + } + + TEST_F(AssetImporterTest, ImportersTriedNoValidFound) { + MockAssetImporter importer; + const auto wrongImporter1 = new MockImporter(); + const auto wrongImporter2 = new MockImporter(); + const AssetLocation location("test::myAsset@path"); + ImporterFileInput input; + + // wrongImporter1 is checked for canRead, then ultimately it is still used for importImpl + // and don't set a mainAsset + EXPECT_CALL(*wrongImporter1, canRead(testing::_)) + .WillOnce(testing::Return(false)); + EXPECT_CALL(*wrongImporter1, importImpl(testing::_)) + .WillOnce(testing::Return()); + + EXPECT_CALL(*wrongImporter2, canRead(testing::_)).WillOnce(testing::Return(false)); + EXPECT_CALL(*wrongImporter2, importImpl(testing::_)).WillOnce(testing::Return()); + + // Register the mock importers with different priorities + importer.registerImporter(wrongImporter2, 50); + importer.registerImporter(wrongImporter1, 100); + + // Call the method + const auto assetRef = importer.importAssetAuto(location, input); + + // Assertions + ASSERT_FALSE(assetRef.isValid()); + } + } // namespace nexo::assets From 0f012191ec09976164ec3b2df57abfca119c206d Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Tue, 25 Mar 2025 13:11:38 +0900 Subject: [PATCH 25/27] test(asset-back): enhance test expectations with proper ordering in AssetImporter tests --- tests/engine/assets/AssetImporter.test.cpp | 97 +++++++++++++++------- 1 file changed, 67 insertions(+), 30 deletions(-) diff --git a/tests/engine/assets/AssetImporter.test.cpp b/tests/engine/assets/AssetImporter.test.cpp index 1cda16834..50ba868b8 100644 --- a/tests/engine/assets/AssetImporter.test.cpp +++ b/tests/engine/assets/AssetImporter.test.cpp @@ -21,6 +21,10 @@ namespace nexo::assets { + using testing::Expectation; + using testing::Invoke; + using testing::Return; + // Mock AssetImporterBase class class MockImporter final : public AssetImporterBase { public: @@ -82,7 +86,7 @@ namespace nexo::assets { // Simulate successful import EXPECT_CALL(*mockImporter, canRead(testing::_)).Times(0); // Never called EXPECT_CALL(*mockImporter, importImpl(testing::_)) - .WillOnce(testing::Invoke([&](AssetImporterContext& ctx) { + .WillOnce(Invoke([&](AssetImporterContext& ctx) { ctx.setMainAsset(expectedAsset); })); @@ -103,10 +107,13 @@ namespace nexo::assets { ImporterFileInput input; const auto expectedAsset = new Texture(); - // Setup expectations - EXPECT_CALL(*mockImporter, canRead(testing::_)).WillOnce(testing::Return(true)); + // Setup expectations with ordering + Expectation canReadCall = EXPECT_CALL(*mockImporter, canRead(testing::_)) + .WillOnce(Return(true)); + EXPECT_CALL(*mockImporter, importImpl(testing::_)) - .WillOnce(testing::Invoke([&](AssetImporterContext& ctx) { + .After(canReadCall) + .WillOnce(Invoke([&](AssetImporterContext& ctx) { ctx.setMainAsset(expectedAsset); })); @@ -123,8 +130,6 @@ namespace nexo::assets { // Clean up importer.unregisterAllImportersForType(); - // Delete the mock importer - delete mockImporter; } TEST_F(AssetImporterTest, ImportAssetAutoFailureNoImporters) @@ -148,10 +153,13 @@ namespace nexo::assets { ImporterFileInput input; const auto expectedAsset = new Texture(); - // Setup expectations - EXPECT_CALL(*bestImporter, canRead(testing::_)).WillOnce(testing::Return(true)); + // Setup expectations with ordering + Expectation canReadCall = EXPECT_CALL(*bestImporter, canRead(testing::_)) + .WillOnce(Return(true)); + EXPECT_CALL(*bestImporter, importImpl(testing::_)) - .WillOnce(testing::Invoke([&](AssetImporterContext& ctx) { + .After(canReadCall) + .WillOnce(Invoke([&](AssetImporterContext& ctx) { ctx.setMainAsset(expectedAsset); })); @@ -226,16 +234,28 @@ namespace nexo::assets { EXPECT_CALL(*textureImporter2, canRead(testing::_)).Times(0); EXPECT_CALL(*textureImporter2, importImpl(testing::_)).Times(0); - // Only the validModelImporter should be called - EXPECT_CALL(*validModelImporter, canRead(testing::_)).WillOnce(testing::Return(true)); + // Setup call expectations with proper ordering + // First the templated MockImporter will be checked + Expectation canReadCall2 = EXPECT_CALL(*cannotReadModelImporter2, canRead(testing::_)) + .WillOnce(Return(false)); + + // Then the cannotReadModelImporter will be checked + Expectation canReadCall = EXPECT_CALL(*cannotReadModelImporter, canRead(testing::_)) + .After(canReadCall2) + .WillOnce(Return(false)); + + // Finally the validModelImporter will be checked and used + Expectation validCanReadCall = EXPECT_CALL(*validModelImporter, canRead(testing::_)) + .After(canReadCall) + .WillOnce(Return(true)); + EXPECT_CALL(*validModelImporter, importImpl(testing::_)) - .WillOnce(testing::Invoke([](AssetImporterContext& ctx) { + .After(validCanReadCall) + .WillOnce(Invoke([](AssetImporterContext& ctx) { ctx.setMainAsset(new Model()); })); - // The other model importers should return false - EXPECT_CALL(*cannotReadModelImporter, canRead(testing::_)).WillOnce(testing::Return(false)); + EXPECT_CALL(*cannotReadModelImporter, importImpl(testing::_)).Times(0); - EXPECT_CALL(*cannotReadModelImporter2, canRead(testing::_)).WillOnce(testing::Return(false)); EXPECT_CALL(*cannotReadModelImporter2, importImpl(testing::_)).Times(0); const AssetLocation location("test::myAsset@path"); @@ -248,7 +268,6 @@ namespace nexo::assets { const AssetLocation location2("test::myAsset@path2"); const auto invalidShaderAssetRef = importer.importAsset(location2, input); EXPECT_FALSE(invalidShaderAssetRef); - } /** @@ -264,15 +283,23 @@ namespace nexo::assets { ImporterFileInput input; const auto expectedAsset = new Texture(); - // bestImporter is checked for canRead, then ultimately it is still used for importImpl - EXPECT_CALL(*bestImporter, canRead(testing::_)) - .WillOnce(testing::Return(false)); + // Setup expectations with ordering + // First the bestImporter canRead is checked + Expectation bestCanReadCall = EXPECT_CALL(*bestImporter, canRead(testing::_)) + .WillOnce(Return(false)); + + // Then wrongImporter canRead is checked + Expectation wrongCanReadCall = EXPECT_CALL(*wrongImporter, canRead(testing::_)) + .After(bestCanReadCall) + .WillOnce(Return(false)); + + // Finally bestImporter importImpl is called as last resort EXPECT_CALL(*bestImporter, importImpl(testing::_)) - .WillOnce(testing::Invoke([&](AssetImporterContext& ctx) { + .After(wrongCanReadCall) + .WillOnce(Invoke([&](AssetImporterContext& ctx) { ctx.setMainAsset(expectedAsset); })); - EXPECT_CALL(*wrongImporter, canRead(testing::_)).WillOnce(testing::Return(false)); EXPECT_CALL(*wrongImporter, importImpl(testing::_)).Times(0); // Register the mock importers with different priorities @@ -294,15 +321,25 @@ namespace nexo::assets { const AssetLocation location("test::myAsset@path"); ImporterFileInput input; - // wrongImporter1 is checked for canRead, then ultimately it is still used for importImpl - // and don't set a mainAsset - EXPECT_CALL(*wrongImporter1, canRead(testing::_)) - .WillOnce(testing::Return(false)); - EXPECT_CALL(*wrongImporter1, importImpl(testing::_)) - .WillOnce(testing::Return()); - - EXPECT_CALL(*wrongImporter2, canRead(testing::_)).WillOnce(testing::Return(false)); - EXPECT_CALL(*wrongImporter2, importImpl(testing::_)).WillOnce(testing::Return()); + // Setup expectations with ordering + // First wrongImporter1 canRead is checked + Expectation wrong1CanReadCall = EXPECT_CALL(*wrongImporter1, canRead(testing::_)) + .WillOnce(Return(false)); + + // Then wrongImporter2 canRead is checked + Expectation wrong2CanReadCall = EXPECT_CALL(*wrongImporter2, canRead(testing::_)) + .After(wrong1CanReadCall) + .WillOnce(Return(false)); + + // Then wrongImporter1 importImpl is attempted (highest priority first) + Expectation wrong1ImportCall = EXPECT_CALL(*wrongImporter1, importImpl(testing::_)) + .After(wrong2CanReadCall) + .WillOnce(Return()); + + // Finally wrongImporter2 importImpl is attempted + EXPECT_CALL(*wrongImporter2, importImpl(testing::_)) + .After(wrong1ImportCall) + .WillOnce(Return()); // Register the mock importers with different priorities importer.registerImporter(wrongImporter2, 50); From 9cdd0c2c21c79970f036ed0c5de15aa6aebf9cff Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Tue, 25 Mar 2025 15:49:10 +0900 Subject: [PATCH 26/27] test(asset-back): ensure proper cleanup by deleting mock importer in AssetImporter tests --- tests/engine/assets/AssetImporter.test.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/engine/assets/AssetImporter.test.cpp b/tests/engine/assets/AssetImporter.test.cpp index 50ba868b8..41b3fa0c3 100644 --- a/tests/engine/assets/AssetImporter.test.cpp +++ b/tests/engine/assets/AssetImporter.test.cpp @@ -130,6 +130,7 @@ namespace nexo::assets { // Clean up importer.unregisterAllImportersForType(); + delete mockImporter; } TEST_F(AssetImporterTest, ImportAssetAutoFailureNoImporters) From 75f5d4ccb3bfa4c2ad31f0cac936f58817b6ba28 Mon Sep 17 00:00:00 2001 From: Guillaume HEIN Date: Thu, 27 Mar 2025 17:48:54 +0900 Subject: [PATCH 27/27] test(asset-back): add EXPECT for invalidating assetRef --- tests/engine/assets/AssetRef.test.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/engine/assets/AssetRef.test.cpp b/tests/engine/assets/AssetRef.test.cpp index 9cbc79675..67bc612bf 100644 --- a/tests/engine/assets/AssetRef.test.cpp +++ b/tests/engine/assets/AssetRef.test.cpp @@ -55,16 +55,18 @@ namespace nexo::assets { } TEST_F(AssetRefTest, IsValidReturnsCorrectValue) { - GenericAssetRef ref(genericAsset); + const GenericAssetRef ref(genericAsset); EXPECT_TRUE(ref.isValid()); // Simulate expired pointer + GenericAssetRef tempRef; { - auto tempAsset = std::make_shared(); - GenericAssetRef tempRef(tempAsset); + const auto tempAsset = std::make_shared(); + tempRef = GenericAssetRef(tempAsset); EXPECT_TRUE(tempRef.isValid()); } // tempAsset goes out of scope here, should expire + EXPECT_FALSE(tempRef.isValid()); } TEST_F(AssetRefTest, LockReturnsCorrectPointer) {