From 5dd88ba05c628e8feb18a3f37e295cb61729c178 Mon Sep 17 00:00:00 2001 From: amzn-mike <80125227+amzn-mike@users.noreply.github.com> Date: Mon, 14 Feb 2022 11:09:39 -0600 Subject: [PATCH] =?UTF-8?q?[LYN-9953]=20Asset=20Processor:=20Fix=20file=20?= =?UTF-8?q?watcher=20event=20handlers=20calling=20APM=20methods=20directly?= =?UTF-8?q?=20instead=20=E2=80=A6=20(#7289)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix file watcher event handlers calling APM methods directly instead of queuing them to run on the APM thread Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com> * Add unit test Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com> * Fix compile errors Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com> * Fix compile errors Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com> * Fix compile errors Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com> * Fix compile errors Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com> * Change ASSERT_ checks to EXPECT_ checks Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com> * Refactored Test Removed leftover direct call Changed invokeMethod to use lambda form Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com> * Fix compile error Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com> --- .../assetprocessor_test_files.cmake | 8 ++ .../AssetManager/assetProcessorManager.h | 46 ++++----- .../native/FileProcessor/FileProcessor.h | 10 +- .../native/FileWatcher/FileWatcher.cpp | 1 - .../native/tests/ApplicationManagerTests.cpp | 91 ++++++++++++++++++ .../native/tests/ApplicationManagerTests.h | 64 +++++++++++++ .../tests/AssetProcessorMessagesTests.cpp | 10 +- .../FileStateCache/FileStateCacheTests.cpp | 10 +- .../FileStateCache/FileStateCacheTests.h | 3 +- .../MockAssetProcessorManager.cpp | 27 ++++++ .../assetmanager/MockAssetProcessorManager.h | 32 +++++++ .../tests/assetmanager/MockFileProcessor.cpp | 22 +++++ .../tests/assetmanager/MockFileProcessor.h | 30 ++++++ .../tests/assetmanager/TestEventSignal.cpp | 37 ++++++++ .../tests/assetmanager/TestEventSignal.h | 33 +++++++ .../utilities/ApplicationManagerBase.cpp | 95 +++++++++++++------ .../native/utilities/ApplicationManagerBase.h | 4 +- .../utilities/GUIApplicationManager.cpp | 2 +- 18 files changed, 452 insertions(+), 73 deletions(-) create mode 100644 Code/Tools/AssetProcessor/native/tests/ApplicationManagerTests.cpp create mode 100644 Code/Tools/AssetProcessor/native/tests/ApplicationManagerTests.h create mode 100644 Code/Tools/AssetProcessor/native/tests/assetmanager/MockAssetProcessorManager.cpp create mode 100644 Code/Tools/AssetProcessor/native/tests/assetmanager/MockAssetProcessorManager.h create mode 100644 Code/Tools/AssetProcessor/native/tests/assetmanager/MockFileProcessor.cpp create mode 100644 Code/Tools/AssetProcessor/native/tests/assetmanager/MockFileProcessor.h create mode 100644 Code/Tools/AssetProcessor/native/tests/assetmanager/TestEventSignal.cpp create mode 100644 Code/Tools/AssetProcessor/native/tests/assetmanager/TestEventSignal.h diff --git a/Code/Tools/AssetProcessor/assetprocessor_test_files.cmake b/Code/Tools/AssetProcessor/assetprocessor_test_files.cmake index bc7b16cc79..20bfc4221c 100644 --- a/Code/Tools/AssetProcessor/assetprocessor_test_files.cmake +++ b/Code/Tools/AssetProcessor/assetprocessor_test_files.cmake @@ -32,6 +32,12 @@ set(FILES native/tests/assetmanager/AssetProcessorManagerTest.h native/tests/assetmanager/ModtimeScanningTests.cpp native/tests/assetmanager/ModtimeScanningTests.h + native/tests/assetmanager/MockAssetProcessorManager.cpp + native/tests/assetmanager/MockAssetProcessorManager.h + native/tests/assetmanager/MockFileProcessor.h + native/tests/assetmanager/MockFileProcessor.cpp + native/tests/assetmanager/TestEventSignal.cpp + native/tests/assetmanager/TestEventSignal.h native/tests/utilities/assetUtilsTest.cpp native/tests/platformconfiguration/platformconfigurationtests.cpp native/tests/platformconfiguration/platformconfigurationtests.h @@ -51,6 +57,8 @@ set(FILES native/tests/SourceFileRelocatorTests.cpp native/tests/PathDependencyManagerTests.cpp native/tests/AssetProcessorMessagesTests.cpp + native/tests/ApplicationManagerTests.cpp + native/tests/ApplicationManagerTests.h native/unittests/AssetProcessingStateDataUnitTests.cpp native/unittests/AssetProcessingStateDataUnitTests.h native/unittests/AssetProcessorManagerUnitTests.cpp diff --git a/Code/Tools/AssetProcessor/native/AssetManager/assetProcessorManager.h b/Code/Tools/AssetProcessor/native/AssetManager/assetProcessorManager.h index eb40b476b1..a4035b3951 100644 --- a/Code/Tools/AssetProcessor/native/AssetManager/assetProcessorManager.h +++ b/Code/Tools/AssetProcessor/native/AssetManager/assetProcessorManager.h @@ -205,7 +205,7 @@ namespace AssetProcessor void EmitResolvedDependency(const AZ::Data::AssetId& assetId, const AzToolsFramework::AssetDatabase::ProductDependencyDatabaseEntry& entry); //! Internal structure that will hold all the necessary information to process jobs later. - //! We need to hold these jobs because they have declared either source dependency on other sources + //! We need to hold these jobs because they have declared either source dependency on other sources //! or a job dependency and we can only resolve these dependencies once all the create jobs are completed. struct JobToProcessEntry { @@ -229,14 +229,14 @@ namespace AssetProcessor //! Emit whenever a new asset is found or an existing asset is updated void AssetMessage(AzFramework::AssetSystem::AssetNotificationMessage message); - + // InputAssetProcessed - uses absolute asset path of input file void InputAssetProcessed(QString fullAssetPath, QString platform); void RequestInputAssetStatus(QString inputAssetPath, QString platform, QString jobDescription); void RequestPriorityAssetCompile(QString inputAssetPath, QString platform, QString jobDescription); - //! AssetProcessorManagerIdleState is emitted when APM idle state changes, we emit true when + //! AssetProcessorManagerIdleState is emitted when APM idle state changes, we emit true when //! APM is waiting for outside stimulus i.e its has eaten through all of its queues and is only waiting for //! responses back from other systems (like its waiting for responses back from the compiler) void AssetProcessorManagerIdleState(bool state); @@ -272,12 +272,12 @@ namespace AssetProcessor void AssessFilesFromScanner(QSet filePaths); - void AssessModifiedFile(QString filePath); - void AssessAddedFile(QString filePath); - void AssessDeletedFile(QString filePath); + virtual void AssessModifiedFile(QString filePath); + virtual void AssessAddedFile(QString filePath); + virtual void AssessDeletedFile(QString filePath); void OnAssetScannerStatusChange(AssetProcessor::AssetScanningStatus status); void OnJobStatusChanged(JobEntry jobEntry, JobStatus status); - + void CheckAssetProcessorIdleState(); void QuitRequested(); @@ -290,7 +290,7 @@ namespace AssetProcessor //! A network request came in asking for asset database location GetAbsoluteAssetDatabaseLocationResponse ProcessGetAbsoluteAssetDatabaseLocationRequest(MessageData messageData); - + //! This request comes in and is expected to do whatever heuristic is required in order to determine if an asset actually exists in the database. void OnRequestAssetExists(NetworkRequestID requestId, QString platform, QString searchTerm, AZ::Data::AssetId assetId); @@ -329,7 +329,7 @@ namespace AssetProcessor bool InitializeCacheRoot(); void PopulateJobStateCache(); void AutoFailJob(const AZStd::string& consoleMsg, const AZStd::string& autoFailReason, const AZStd::vector::iterator& assetIter); - + using ProductInfoList = AZStd::vector>; void WriteProductTableInfo(AZStd::pair& pair, AZStd::vector& subIds, AZStd::unordered_set& dependencyContainer, const AZStd::string& platform); @@ -362,7 +362,7 @@ namespace AssetProcessor //! Search the database and the the source dependency maps for the the sourceUuid. if found returns the cached info bool SearchSourceInfoBySourceUUID(const AZ::Uuid& sourceUuid, AssetProcessorManager::SourceInfo& result); - + //! Adds the source to the database and returns the corresponding sourceDatabase Entry void AddSourceToDatabase(AzToolsFramework::AssetDatabase::SourceDatabaseEntry& sourceDatabaseEntry, const ScanFolderInfo* scanFolder, QString relativeSourceFilePath); @@ -378,8 +378,8 @@ namespace AssetProcessor // Load the old scan folders and match them up with new scan folders. Make sure they're bool MigrateScanFolders(); - //! Checks whether the AP is aware of any source file that has indicated the inputted - //! source file as its dependency, and if found do we need to put that file back in the asset pipeline queue again + //! Checks whether the AP is aware of any source file that has indicated the inputted + //! source file as its dependency, and if found do we need to put that file back in the asset pipeline queue again QStringList GetSourceFilesWhichDependOnSourceFile(const QString& sourcePath); /** Given a BuilderSDK SourceFileDependency, try to find out what actual database source name is. @@ -399,9 +399,9 @@ namespace AssetProcessor void UpdateWildcardDependencies(JobDetails& job, size_t jobDependencySlot, QStringList& resolvedDependencyList); //! Check whether the job can be analyzed by APM, - //! A job cannot be analyzed if any of its dependent job hasn't been fingerprinted + //! A job cannot be analyzed if any of its dependent job hasn't been fingerprinted bool CanAnalyzeJob(const JobDetails& jobDetails); - + //! Analyzes and forward the job to the RCController if the job requires processing void ProcessJob(JobDetails& jobDetails); @@ -417,7 +417,7 @@ namespace AssetProcessor ThreadController* m_assetCatalog; typedef QHash FileExamineContainer; FileExamineContainer m_filesToExamine; // order does not actually matter in this (yet) - + // this map contains a list of source files that were discovered in the database before asset scanning began. // (so files from a previous run). // as asset scanning encounters files, it will remove them from this map, and when its done, @@ -455,20 +455,20 @@ namespace AssetProcessor AZ::s64 m_highestJobRunKeySoFar = 0; AZStd::vector m_jobEntries; AZStd::unordered_set m_jobsToProcess; - //! This map is required to prevent multiple sourceFile modified events been send by the APM + //! This map is required to prevent multiple sourceFile modified events been send by the APM AZStd::unordered_map m_sourceFileModTimeMap; - AZStd::unordered_map m_jobFingerprintMap; + AZStd::unordered_map m_jobFingerprintMap; AZStd::unordered_map> m_jobDescToBuilderUuidMap; - + AZStd::unique_ptr m_pathDependencyManager; AZStd::unique_ptr m_sourceFileRelocator; JobDiagnosticTracker m_jobDiagnosticTracker{}; - - QSet m_checkFoldersToRemove; //!< List of folders that needs to be checked for removal later by AP + + QSet m_checkFoldersToRemove; //!< List of folders that needs to be checked for removal later by AP //! List of all scanfolders that are present in the database but not currently watched by AP AZStd::unordered_map m_scanFoldersInDatabase; - + int m_numOfJobsToAnalyze = 0; bool m_alreadyQueuedCheckForIdle = false; @@ -502,7 +502,7 @@ namespace AssetProcessor * full absolute paths. */ void QueryAbsolutePathDependenciesRecursive(QString inputDatabasePath, SourceFilesForFingerprintingContainer& finalDependencyList, AzToolsFramework::AssetDatabase::SourceFileDependencyEntry::TypeOfDependency dependencyType, bool reverseQuery); - + // we can't write a job to the database as not needing analysis the next time around, // until all jobs related to it are finished. This is becuase the jobs themselves are not written to the database // so until all jobs are finished, we need to re-analyze the source file next time. @@ -529,7 +529,7 @@ namespace AssetProcessor JobStarted, JobFinished, }; - + // ideally you would already have the absolute path to the file, and call this function with it: void UpdateAnalysisTrackerForFile(const char* fullPathToFile, AnalysisTrackerUpdateType updateType); diff --git a/Code/Tools/AssetProcessor/native/FileProcessor/FileProcessor.h b/Code/Tools/AssetProcessor/native/FileProcessor/FileProcessor.h index e455fe91cf..f09d424a47 100644 --- a/Code/Tools/AssetProcessor/native/FileProcessor/FileProcessor.h +++ b/Code/Tools/AssetProcessor/native/FileProcessor/FileProcessor.h @@ -19,7 +19,7 @@ namespace AzToolsFramework { - namespace AssetDatabase + namespace AssetDatabase { class FileDatabaseEntry; } @@ -40,17 +40,17 @@ namespace AssetProcessor public Q_SLOTS: //! AssetScanner changed its status void OnAssetScannerStatusChange(AssetScanningStatus status); - + //! AssetScanner found a file void AssessFilesFromScanner(QSet files); - + //! AssetScanner found a folder void AssessFoldersFromScanner(QSet folders); //! FileWatcher detected added file - void AssessAddedFile(QString fileName); + virtual void AssessAddedFile(QString fileName); //! FileWatcher detected removed file - void AssessDeletedFile(QString fileName); + virtual void AssessDeletedFile(QString fileName); //! Synchronize AssetScanner data with Files table void Sync(); diff --git a/Code/Tools/AssetProcessor/native/FileWatcher/FileWatcher.cpp b/Code/Tools/AssetProcessor/native/FileWatcher/FileWatcher.cpp index 993e7f2760..2eb56487d9 100644 --- a/Code/Tools/AssetProcessor/native/FileWatcher/FileWatcher.cpp +++ b/Code/Tools/AssetProcessor/native/FileWatcher/FileWatcher.cpp @@ -148,7 +148,6 @@ void FileWatcher::StopWatching() { if (!m_startedWatching) { - AZ_Warning("FileWatcher", false, "StopWatching() called when is not watching for file changes."); return; } diff --git a/Code/Tools/AssetProcessor/native/tests/ApplicationManagerTests.cpp b/Code/Tools/AssetProcessor/native/tests/ApplicationManagerTests.cpp new file mode 100644 index 0000000000..37717897ef --- /dev/null +++ b/Code/Tools/AssetProcessor/native/tests/ApplicationManagerTests.cpp @@ -0,0 +1,91 @@ +/* + * Copyright (c) Contributors to the Open 3D Engine Project. + * For complete copyright and license terms please see the LICENSE at the root of this distribution. + * + * SPDX-License-Identifier: Apache-2.0 OR MIT + * + */ + +#include "ApplicationManagerTests.h" + +#include +#include +#include + +namespace UnitTests +{ + bool DatabaseLocationListener::GetAssetDatabaseLocation(AZStd::string& location) + { + location = m_databaseLocation; + return true; + } + + void ApplicationManagerTest::SetUp() + { + ScopedAllocatorSetupFixture::SetUp(); + + AZ::IO::Path tempDir(m_tempDir.GetDirectory()); + m_databaseLocationListener.m_databaseLocation = (tempDir / "test_database.sqlite").Native(); + + // We need a QCoreApplication to run the event loop + int argc = 0; + m_coreApplication = AZStd::make_unique(argc, nullptr); + + m_applicationManager = AZStd::make_unique(&argc, nullptr); + m_applicationManager->m_platformConfiguration = new AssetProcessor::PlatformConfiguration{ nullptr }; + m_applicationManager->m_fileStateCache = AZStd::make_unique(); + + m_mockAPM = AZStd::make_unique(m_applicationManager->m_platformConfiguration, nullptr); + m_applicationManager->m_assetProcessorManager = m_mockAPM.get(); + + AZStd::vector platforms; + m_applicationManager->m_platformConfiguration->EnablePlatform(AssetBuilderSDK::PlatformInfo{ "pc", { "tag" } }); + m_applicationManager->m_platformConfiguration->PopulatePlatformsForScanFolder(platforms); + m_applicationManager->m_platformConfiguration->AddScanFolder( + AssetProcessor::ScanFolderInfo{ tempDir.c_str(), "test", "test", true, true, platforms }); + + m_apmThread = AZStd::make_unique(nullptr); + m_apmThread->setObjectName("APM Thread"); + m_applicationManager->m_assetProcessorManager->moveToThread(m_apmThread.get()); + m_apmThread->start(); + + m_fileProcessorThread = AZStd::make_unique(nullptr); + m_fileProcessorThread->setObjectName("File Processor Thread"); + auto fileProcessor = AZStd::make_unique(m_applicationManager->m_platformConfiguration); + fileProcessor->moveToThread(m_fileProcessorThread.get()); + m_mockFileProcessor = fileProcessor.get(); + m_applicationManager->m_fileProcessor = AZStd::move(fileProcessor); // The manager is taking ownership + m_fileProcessorThread->start(); + + auto fileWatcher = AZStd::make_unique(); + m_fileWatcher = fileWatcher.get(); + + // This is what we're testing, it will set up connections between the fileWatcher and the 2 QObject handlers we'll check + m_applicationManager->InitFileMonitor(AZStd::move(fileWatcher)); // The manager is going to take ownership of the file watcher + } + + void ApplicationManagerTest::TearDown() + { + m_apmThread->exit(); + m_fileProcessorThread->exit(); + m_mockAPM = nullptr; + + ScopedAllocatorSetupFixture::TearDown(); + } + + TEST_F(ApplicationManagerTest, FileWatcherEventsTriggered_ProperlySignalledOnCorrectThread) + { + AZ::IO::Path tempDir(m_tempDir.GetDirectory()); + + Q_EMIT m_fileWatcher->fileAdded((tempDir / "test").c_str()); + Q_EMIT m_fileWatcher->fileModified((tempDir / "test2").c_str()); + Q_EMIT m_fileWatcher->fileRemoved((tempDir / "test3").c_str()); + + EXPECT_TRUE(m_mockAPM->m_events[Added].WaitAndCheck()) << "APM Added event failed"; + EXPECT_TRUE(m_mockAPM->m_events[Modified].WaitAndCheck()) << "APM Modified event failed"; + EXPECT_TRUE(m_mockAPM->m_events[Deleted].WaitAndCheck()) << "APM Deleted event failed"; + + EXPECT_TRUE(m_mockFileProcessor->m_events[Added].WaitAndCheck()) << "File Processor Added event failed"; + EXPECT_TRUE(m_mockFileProcessor->m_events[Deleted].WaitAndCheck()) << "File Processor Deleted event failed"; + } +} diff --git a/Code/Tools/AssetProcessor/native/tests/ApplicationManagerTests.h b/Code/Tools/AssetProcessor/native/tests/ApplicationManagerTests.h new file mode 100644 index 0000000000..fea10b224b --- /dev/null +++ b/Code/Tools/AssetProcessor/native/tests/ApplicationManagerTests.h @@ -0,0 +1,64 @@ +/* + * Copyright (c) Contributors to the Open 3D Engine Project. + * For complete copyright and license terms please see the LICENSE at the root of this distribution. + * + * SPDX-License-Identifier: Apache-2.0 OR MIT + * + */ + +#pragma once + +#include +#include +#include "assetmanager/MockAssetProcessorManager.h" +#include "assetmanager/MockFileProcessor.h" + +namespace UnitTests +{ + struct MockBatchApplicationManager : BatchApplicationManager + { + using ApplicationManagerBase::InitFileMonitor; + using ApplicationManagerBase::m_assetProcessorManager; + using ApplicationManagerBase::m_fileProcessor; + using ApplicationManagerBase::m_fileStateCache; + using ApplicationManagerBase::m_platformConfiguration; + using BatchApplicationManager::BatchApplicationManager; + }; + + class DatabaseLocationListener : public AzToolsFramework::AssetDatabase::AssetDatabaseRequests::Bus::Handler + { + public: + DatabaseLocationListener() + { + BusConnect(); + } + ~DatabaseLocationListener() override + { + BusDisconnect(); + } + + bool GetAssetDatabaseLocation(AZStd::string& location) override; + + AZStd::string m_databaseLocation; + }; + + struct ApplicationManagerTest : ::UnitTest::ScopedAllocatorSetupFixture + { + protected: + void SetUp() override; + void TearDown() override; + + AZ::Test::ScopedAutoTempDirectory m_tempDir; + DatabaseLocationListener m_databaseLocationListener; + + AZStd::unique_ptr m_coreApplication; + AZStd::unique_ptr m_applicationManager; + AZStd::unique_ptr m_apmThread; + AZStd::unique_ptr m_fileProcessorThread; + AZStd::unique_ptr m_mockAPM; + + // These are just aliases, no need to manage/delete them + FileWatcher* m_fileWatcher{}; + MockFileProcessor* m_mockFileProcessor{}; + }; +} diff --git a/Code/Tools/AssetProcessor/native/tests/AssetProcessorMessagesTests.cpp b/Code/Tools/AssetProcessor/native/tests/AssetProcessorMessagesTests.cpp index 6a0da96151..c6105142f1 100644 --- a/Code/Tools/AssetProcessor/native/tests/AssetProcessorMessagesTests.cpp +++ b/Code/Tools/AssetProcessor/native/tests/AssetProcessorMessagesTests.cpp @@ -126,18 +126,18 @@ namespace AssetProcessorMessagesTests m_batchApplicationManager->InitAssetProcessorManager(); m_assetCatalog = AZStd::make_unique(nullptr, m_batchApplicationManager->m_platformConfiguration); - + m_batchApplicationManager->m_assetCatalog = m_assetCatalog.get(); m_batchApplicationManager->InitRCController(); m_batchApplicationManager->InitFileStateCache(); - m_batchApplicationManager->InitFileMonitor(); + m_batchApplicationManager->InitFileMonitor(AZStd::make_unique()); m_batchApplicationManager->InitApplicationServer(); m_batchApplicationManager->InitConnectionManager(); // Note this must be constructed after InitConnectionManager is called since it will interact with the connection manager m_assetRequestHandler = new MockAssetRequestHandler(); m_batchApplicationManager->InitAssetRequestHandler(m_assetRequestHandler); - m_batchApplicationManager->m_fileWatcher.StartWatching(); + m_batchApplicationManager->m_fileWatcher->StartWatching(); QObject::connect(m_batchApplicationManager->m_connectionManager, &ConnectionManager::ConnectionError, [](unsigned /*connId*/, QString error) { @@ -222,7 +222,7 @@ namespace AssetProcessorMessagesTests thread.join(); } - + protected: MockAssetRequestHandler* m_assetRequestHandler{}; // Not owned, AP will delete this pointer @@ -246,7 +246,7 @@ namespace AssetProcessorMessagesTests { // Test that we can successfully send network messages and have them arrive for processing // For messages that have a response, it also verifies the response comes back - // Note that several harmless warnings will be triggered due to the messages not having any data set + // Note that several harmless warnings will be triggered due to the messages not having any data set using namespace AzFramework::AssetSystem; using namespace AzToolsFramework::AssetSystem; diff --git a/Code/Tools/AssetProcessor/native/tests/FileStateCache/FileStateCacheTests.cpp b/Code/Tools/AssetProcessor/native/tests/FileStateCache/FileStateCacheTests.cpp index c4288860c3..718f41a7b7 100644 --- a/Code/Tools/AssetProcessor/native/tests/FileStateCache/FileStateCacheTests.cpp +++ b/Code/Tools/AssetProcessor/native/tests/FileStateCache/FileStateCacheTests.cpp @@ -12,10 +12,12 @@ namespace UnitTests { + using AssetFileInfo = AssetProcessor::AssetFileInfo; + void FileStateCacheTests::SetUp() { m_temporarySourceDir = QDir(m_temporaryDir.path()); - m_fileStateCache = AZStd::make_unique(); + m_fileStateCache = AZStd::make_unique(); } void FileStateCacheTests::TearDown() @@ -26,9 +28,9 @@ namespace UnitTests void FileStateCacheTests::CheckForFile(QString path, bool shouldExist) { bool exists = false; - FileStateInfo fileInfo; + AssetProcessor::FileStateInfo fileInfo; - auto* fileStateInterface = AZ::Interface::Get(); + auto* fileStateInterface = AZ::Interface::Get(); ASSERT_NE(fileStateInterface, nullptr); exists = fileStateInterface->Exists(path); @@ -142,7 +144,7 @@ namespace UnitTests TEST_F(FileStateCacheTests, PassthroughTest) { m_fileStateCache = nullptr; // Need to release the existing one first since only one handler can exist for the ebus - m_fileStateCache = AZStd::make_unique(); + m_fileStateCache = AZStd::make_unique(); QString testPath = m_temporarySourceDir.absoluteFilePath("test.txt"); CheckForFile(testPath, false); diff --git a/Code/Tools/AssetProcessor/native/tests/FileStateCache/FileStateCacheTests.h b/Code/Tools/AssetProcessor/native/tests/FileStateCache/FileStateCacheTests.h index 78ddc6aaed..14e4763d29 100644 --- a/Code/Tools/AssetProcessor/native/tests/FileStateCache/FileStateCacheTests.h +++ b/Code/Tools/AssetProcessor/native/tests/FileStateCache/FileStateCacheTests.h @@ -16,7 +16,6 @@ namespace UnitTests { using namespace testing; using ::testing::NiceMock; - using namespace AssetProcessor; class FileStateCacheTests : public ::testing::Test { @@ -29,6 +28,6 @@ namespace UnitTests protected: QTemporaryDir m_temporaryDir; QDir m_temporarySourceDir; - AZStd::unique_ptr m_fileStateCache; + AZStd::unique_ptr m_fileStateCache; }; } diff --git a/Code/Tools/AssetProcessor/native/tests/assetmanager/MockAssetProcessorManager.cpp b/Code/Tools/AssetProcessor/native/tests/assetmanager/MockAssetProcessorManager.cpp new file mode 100644 index 0000000000..8b14ef0915 --- /dev/null +++ b/Code/Tools/AssetProcessor/native/tests/assetmanager/MockAssetProcessorManager.cpp @@ -0,0 +1,27 @@ +/* + * Copyright (c) Contributors to the Open 3D Engine Project. + * For complete copyright and license terms please see the LICENSE at the root of this distribution. + * + * SPDX-License-Identifier: Apache-2.0 OR MIT + * + */ + +#include + +namespace UnitTests +{ + void MockAssetProcessorManager::AssessAddedFile(QString filePath) + { + m_events[TestEvents::Added].Signal(); + } + + void MockAssetProcessorManager::AssessModifiedFile(QString filePath) + { + m_events[TestEvents::Modified].Signal(); + } + + void MockAssetProcessorManager::AssessDeletedFile(QString filePath) + { + m_events[TestEvents::Deleted].Signal(); + } +} diff --git a/Code/Tools/AssetProcessor/native/tests/assetmanager/MockAssetProcessorManager.h b/Code/Tools/AssetProcessor/native/tests/assetmanager/MockAssetProcessorManager.h new file mode 100644 index 0000000000..dadb62d18d --- /dev/null +++ b/Code/Tools/AssetProcessor/native/tests/assetmanager/MockAssetProcessorManager.h @@ -0,0 +1,32 @@ +/* + * Copyright (c) Contributors to the Open 3D Engine Project. + * For complete copyright and license terms please see the LICENSE at the root of this distribution. + * + * SPDX-License-Identifier: Apache-2.0 OR MIT + * + */ + +#pragma once + +#include +#include +#include + +namespace UnitTests +{ + struct MockAssetProcessorManager : ::AssetProcessor::AssetProcessorManager + { + Q_OBJECT + + using AssetProcessorManager::AssetProcessorManager; + + public Q_SLOTS: + void AssessAddedFile(QString filePath) override; + void AssessModifiedFile(QString filePath) override; + void AssessDeletedFile(QString filePath) override; + + public: + + TestEventPair m_events[TestEvents::NumEvents]; + }; +} diff --git a/Code/Tools/AssetProcessor/native/tests/assetmanager/MockFileProcessor.cpp b/Code/Tools/AssetProcessor/native/tests/assetmanager/MockFileProcessor.cpp new file mode 100644 index 0000000000..ed16a4cf7f --- /dev/null +++ b/Code/Tools/AssetProcessor/native/tests/assetmanager/MockFileProcessor.cpp @@ -0,0 +1,22 @@ +/* + * Copyright (c) Contributors to the Open 3D Engine Project. + * For complete copyright and license terms please see the LICENSE at the root of this distribution. + * + * SPDX-License-Identifier: Apache-2.0 OR MIT + * + */ + +#include "MockFileProcessor.h" + +namespace UnitTests +{ + void MockFileProcessor::AssessAddedFile(QString fileName) + { + m_events[TestEvents::Added].Signal(); + } + + void MockFileProcessor::AssessDeletedFile(QString fileName) + { + m_events[TestEvents::Deleted].Signal(); + } +} diff --git a/Code/Tools/AssetProcessor/native/tests/assetmanager/MockFileProcessor.h b/Code/Tools/AssetProcessor/native/tests/assetmanager/MockFileProcessor.h new file mode 100644 index 0000000000..604b5d84cd --- /dev/null +++ b/Code/Tools/AssetProcessor/native/tests/assetmanager/MockFileProcessor.h @@ -0,0 +1,30 @@ +/* + * Copyright (c) Contributors to the Open 3D Engine Project. + * For complete copyright and license terms please see the LICENSE at the root of this distribution. + * + * SPDX-License-Identifier: Apache-2.0 OR MIT + * + */ + +#pragma once + +#include +#include +#include + +namespace UnitTests +{ + struct MockFileProcessor : ::AssetProcessor::FileProcessor + { + Q_OBJECT + + using FileProcessor::FileProcessor; + + public Q_SLOTS: + void AssessAddedFile(QString fileName) override; + void AssessDeletedFile(QString fileName) override; + + public: + TestEventPair m_events[TestEvents::NumEvents]; + }; +} diff --git a/Code/Tools/AssetProcessor/native/tests/assetmanager/TestEventSignal.cpp b/Code/Tools/AssetProcessor/native/tests/assetmanager/TestEventSignal.cpp new file mode 100644 index 0000000000..df5c676a66 --- /dev/null +++ b/Code/Tools/AssetProcessor/native/tests/assetmanager/TestEventSignal.cpp @@ -0,0 +1,37 @@ +/* + * Copyright (c) Contributors to the Open 3D Engine Project. + * For complete copyright and license terms please see the LICENSE at the root of this distribution. + * + * SPDX-License-Identifier: Apache-2.0 OR MIT + * + */ + +#include +#include +#include + +namespace UnitTests +{ + void TestEventPair::Signal() + { + bool expected = false; + ASSERT_TRUE(m_signaled.compare_exchange_strong(expected, true)); + ASSERT_EQ(m_threadId, AZStd::thread_id{}); + m_threadId = AZStd::this_thread::get_id(); + m_event.release(); + } + + bool TestEventPair::WaitAndCheck() + { + constexpr int MaxWaitTimeMilliseconds = 100; + + auto thisThreadId = AZStd::this_thread::get_id(); + bool acquireSuccess = m_event.try_acquire_for(AZStd::chrono::milliseconds(MaxWaitTimeMilliseconds)); + + EXPECT_TRUE(acquireSuccess); + EXPECT_NE(m_threadId, AZStd::thread_id{}); + EXPECT_TRUE(m_threadId != thisThreadId); + + return acquireSuccess && m_threadId != AZStd::thread_id{} && m_threadId != thisThreadId; + } +} diff --git a/Code/Tools/AssetProcessor/native/tests/assetmanager/TestEventSignal.h b/Code/Tools/AssetProcessor/native/tests/assetmanager/TestEventSignal.h new file mode 100644 index 0000000000..5251be5b3f --- /dev/null +++ b/Code/Tools/AssetProcessor/native/tests/assetmanager/TestEventSignal.h @@ -0,0 +1,33 @@ +/* + * Copyright (c) Contributors to the Open 3D Engine Project. + * For complete copyright and license terms please see the LICENSE at the root of this distribution. + * + * SPDX-License-Identifier: Apache-2.0 OR MIT + * + */ + +#pragma once + +#include + +namespace UnitTests +{ + class TestEventPair + { + AZStd::atomic_bool m_signaled{ false }; + AZStd::thread_id m_threadId; + AZStd::binary_semaphore m_event; + + public: + void Signal(); + bool WaitAndCheck(); + }; + + enum TestEvents : int + { + Added = 0, + Modified, + Deleted, + NumEvents + }; +} diff --git a/Code/Tools/AssetProcessor/native/utilities/ApplicationManagerBase.cpp b/Code/Tools/AssetProcessor/native/utilities/ApplicationManagerBase.cpp index 634fdbd3bb..93cc533702 100644 --- a/Code/Tools/AssetProcessor/native/utilities/ApplicationManagerBase.cpp +++ b/Code/Tools/AssetProcessor/native/utilities/ApplicationManagerBase.cpp @@ -431,79 +431,114 @@ void ApplicationManagerBase::DestroyPlatformConfiguration() } } -void ApplicationManagerBase::InitFileMonitor() +void ApplicationManagerBase::InitFileMonitor(AZStd::unique_ptr fileWatcher) { + m_fileWatcher = AZStd::move(fileWatcher); + for (int folderIdx = 0; folderIdx < m_platformConfiguration->GetScanFolderCount(); ++folderIdx) { const AssetProcessor::ScanFolderInfo& info = m_platformConfiguration->GetScanFolderAt(folderIdx); - m_fileWatcher.AddFolderWatch(info.ScanPath(), info.RecurseSubFolders()); + m_fileWatcher->AddFolderWatch(info.ScanPath(), info.RecurseSubFolders()); } QDir cacheRoot; if (AssetUtilities::ComputeProjectCacheRoot(cacheRoot)) { - m_fileWatcher.AddFolderWatch(cacheRoot.absolutePath(), true); + m_fileWatcher->AddFolderWatch(cacheRoot.absolutePath(), true); } if (m_platformConfiguration->GetScanFolderCount() || !cacheRoot.path().isEmpty()) { const auto cachePath = QDir::toNativeSeparators(cacheRoot.absolutePath()); + // For the handlers below, we need to make sure to use invokeMethod on any QObjects so Qt can queue + // the callback to run on the QObject's thread if needed. The APM methods for example are not thread-safe. + const auto OnFileAdded = [this, cachePath](QString path) { const bool isCacheRoot = path.startsWith(cachePath); - if (isCacheRoot) + if (!isCacheRoot) { - m_fileStateCache->AddFile(path); - } - else - { - m_assetProcessorManager->AssessAddedFile(path); - m_fileStateCache->AddFile(path); - AZ::Interface::Get()->FileAdded(path); - m_fileProcessor->AssetProcessor::FileProcessor::AssessAddedFile(path); + [[maybe_unused]] bool result = QMetaObject::invokeMethod(m_assetProcessorManager, [this, path]() + { + m_assetProcessorManager->AssessAddedFile(path); + }, Qt::QueuedConnection); + AZ_Assert(result, "Failed to invoke m_assetProcessorManager::AssessAddedFile"); + + result = QMetaObject::invokeMethod(m_fileProcessor.get(), [this, path]() + { + m_fileProcessor->AssessAddedFile(path); + }, Qt::QueuedConnection); + AZ_Assert(result, "Failed to invoke m_fileProcessor::AssessAddedFile"); + + auto cache = AZ::Interface::Get(); + + if(cache) + { + cache->FileAdded(path); + } + else + { + AZ_Error("AssetProcessor", false, "ExcludedFolderCacheInterface not found"); + } } + + m_fileStateCache->AddFile(path); }; const auto OnFileModified = [this, cachePath](QString path) { const bool isCacheRoot = path.startsWith(cachePath); - if (isCacheRoot) - { - m_assetProcessorManager->AssessModifiedFile(path); - } - else + if (!isCacheRoot) { - m_assetProcessorManager->AssessModifiedFile(path); m_fileStateCache->UpdateFile(path); } + + [[maybe_unused]] bool result = QMetaObject::invokeMethod( + m_assetProcessorManager, + [this, path] + { + m_assetProcessorManager->AssessModifiedFile(path); + }, Qt::QueuedConnection); + + AZ_Assert(result, "Failed to invoke m_assetProcessorManager::AssessModifiedFile"); }; const auto OnFileRemoved = [this, cachePath](QString path) { + [[maybe_unused]] bool result = false; const bool isCacheRoot = path.startsWith(cachePath); - if (isCacheRoot) + if (!isCacheRoot) { - m_fileStateCache->RemoveFile(path); - m_assetProcessorManager->AssessDeletedFile(path); + result = QMetaObject::invokeMethod(m_fileProcessor.get(), [this, path]() + { + m_fileProcessor->AssessDeletedFile(path); + }, Qt::QueuedConnection); + AZ_Assert(result, "Failed to invoke m_fileProcessor::AssessDeletedFile"); } - else + + result = QMetaObject::invokeMethod(m_assetProcessorManager, [this, path]() { m_assetProcessorManager->AssessDeletedFile(path); - m_fileStateCache->RemoveFile(path); - m_fileProcessor->AssessDeletedFile(path); - } + }, Qt::QueuedConnection); + AZ_Assert(result, "Failed to invoke m_assetProcessorManager::AssessDeletedFile"); + + m_fileStateCache->RemoveFile(path); }; - connect(&m_fileWatcher, &FileWatcher::fileAdded, OnFileAdded); - connect(&m_fileWatcher, &FileWatcher::fileModified, OnFileModified); - connect(&m_fileWatcher, &FileWatcher::fileRemoved, OnFileRemoved); + connect(m_fileWatcher.get(), &FileWatcher::fileAdded, OnFileAdded); + connect(m_fileWatcher.get(), &FileWatcher::fileModified, OnFileModified); + connect(m_fileWatcher.get(), &FileWatcher::fileRemoved, OnFileRemoved); } } void ApplicationManagerBase::DestroyFileMonitor() { - m_fileWatcher.ClearFolderWatches(); + if(m_fileWatcher) + { + m_fileWatcher->ClearFolderWatches(); + m_fileWatcher = nullptr; + } } void ApplicationManagerBase::DestroyApplicationServer() @@ -1357,7 +1392,7 @@ bool ApplicationManagerBase::Activate() InitFileProcessor(); InitAssetCatalog(); - InitFileMonitor(); + InitFileMonitor(AZStd::make_unique()); InitAssetScanner(); InitAssetServerHandler(); InitRCController(); diff --git a/Code/Tools/AssetProcessor/native/utilities/ApplicationManagerBase.h b/Code/Tools/AssetProcessor/native/utilities/ApplicationManagerBase.h index 8591228375..23600963f6 100644 --- a/Code/Tools/AssetProcessor/native/utilities/ApplicationManagerBase.h +++ b/Code/Tools/AssetProcessor/native/utilities/ApplicationManagerBase.h @@ -134,7 +134,7 @@ protected: virtual void DestroyAssetScanner(); virtual bool InitPlatformConfiguration(); virtual void DestroyPlatformConfiguration(); - virtual void InitFileMonitor(); + virtual void InitFileMonitor(AZStd::unique_ptr fileWatcher); virtual void DestroyFileMonitor(); virtual bool InitBuilderConfiguration(); virtual void InitControlRequestHandler(); @@ -191,7 +191,7 @@ protected: bool m_sourceControlReady = false; bool m_fullIdle = false; - FileWatcher m_fileWatcher; + AZStd::unique_ptr m_fileWatcher; AssetProcessor::PlatformConfiguration* m_platformConfiguration = nullptr; AssetProcessor::AssetProcessorManager* m_assetProcessorManager = nullptr; AssetProcessor::AssetCatalog* m_assetCatalog = nullptr; diff --git a/Code/Tools/AssetProcessor/native/utilities/GUIApplicationManager.cpp b/Code/Tools/AssetProcessor/native/utilities/GUIApplicationManager.cpp index 13a6b0699a..6a918b3a89 100644 --- a/Code/Tools/AssetProcessor/native/utilities/GUIApplicationManager.cpp +++ b/Code/Tools/AssetProcessor/native/utilities/GUIApplicationManager.cpp @@ -479,7 +479,7 @@ bool GUIApplicationManager::PostActivate() return false; } - m_fileWatcher.StartWatching(); + m_fileWatcher->StartWatching(); return true; }