Fix several issues with importing actor assets (o3de#18056) (o3de#18119)

* Fix several issues with importing actor assets

* Fixes a crash when viewing the import settings of a scene.  The crash
was caused by Qt objects interacting with non-qt threads.
* Fixes a crash caused by inactive entities being constructed in a
background thread during scene settings (ie, the prefab builder stuff)
interacting with the real actual loaded entities and undo stack in the
editor.
* Fixes a crash caused by morph targets that are empty still making it
into the data.
* Fixes a crash caused by a mixture of skinned and unskinned meshes
being combined in a skinned mesh asset by default generating skin info
if they are part of a skin export but lack it.
* Adds additional debug output for future debugging of these issues,
this debug output is not enabled by default and uses the existing
"debug output" checkbox in the Asset Processor GUI to enable it.  When
active, dumps a lot more detail from the scene and export into the
log.

Signed-off-by: Nicholas Lawson <[email protected]>

Author: nick-l-o3de

Code/Editor/Plugins/EditorAssetImporter/AssetImporterWindow.cpp

@@ -255,13 +255,14 @@ void AssetImporterWindow::OpenFileInternal(const AZStd::string& filePath)
         s_browseTag,
         [this, filePath]()
         {
+            // this is invoked across threads, so ensure that nothing touches the main thread that isn't thread safe.
+            // Qt objects, in particular, should talk via timers or queued connections.
             m_assetImporterDocument->LoadScene(filePath);
-
-            QTimer::singleShot(0, [&]() { UpdateSceneDisplay({}); });
+            QMetaObject::invokeMethod(this, &AssetImporterWindow::UpdateDefaultSceneDisplay, Qt::QueuedConnection);
         },
         [this]()
         {
-            QTimer::singleShot(0, [&]() { HandleAssetLoadingCompleted();});
+            QMetaObject::invokeMethod(this, &AssetImporterWindow::HandleAssetLoadingCompleted, Qt::QueuedConnection);
         }, this);
 
     QFileInfo fileInfo(filePath.c_str());
@@ -621,6 +622,11 @@ void AssetImporterWindow::OverlayLayerRemoved()
     }
 }
 
+void AssetImporterWindow::UpdateDefaultSceneDisplay()
+{
+    UpdateSceneDisplay({});
+}
+
 void AssetImporterWindow::UpdateSceneDisplay(const AZStd::shared_ptr<AZ::SceneAPI::Containers::Scene> scene) const
 {
     QString sceneHeaderText;

Code/Editor/Plugins/EditorAssetImporter/AssetImporterWindow.h

@@ -119,6 +119,7 @@ private slots:
 
     void OverlayLayerAdded();
     void OverlayLayerRemoved();
+    void UpdateDefaultSceneDisplay();
     void UpdateSceneDisplay(const AZStd::shared_ptr<AZ::SceneAPI::Containers::Scene> scene = {}) const;
 
     void FileChanged(QString path);

Code/Framework/AzToolsFramework/AzToolsFramework/ToolsComponents/EditorComponentBase.cpp

@@ -130,10 +130,19 @@ namespace AzToolsFramework
 
         void EditorComponentBase::SetDirty()
         {
-            if (GetEntity())
+            // Don't mark things for dirty that are not active.
+            // Entities can be inactive for a number of reasons, for example, in a prefab file
+            // being constructed on another thread, and while these might still invoke SetDirty
+            // in response to properties changing, only actualized entities should interact with
+            // the undo/redo system or the prefab change tracker for overrides, which is what
+            // marking things dirty is for.
+            if (AZ::Entity* entity = GetEntity();entity)
             {
-                AzToolsFramework::ToolsApplicationRequests::Bus::Broadcast(
-                    &AzToolsFramework::ToolsApplicationRequests::Bus::Events::AddDirtyEntity, GetEntity()->GetId());
+                if (entity->GetState() == AZ::Entity::State::Active)
+                {
+                    AzToolsFramework::ToolsApplicationRequests::Bus::Broadcast(&AzToolsFramework::ToolsApplicationRequests::Bus::Events::AddDirtyEntity, entity->GetId());
+                }
+
             }
             else
             {

Code/Tools/SceneAPI/SceneCore/Utilities/DebugOutput.cpp

@@ -12,6 +12,7 @@
 #include <AzCore/IO/SystemFile.h>
 #include <AzCore/Serialization/Json/JsonUtils.h>
 #include <AzCore/Serialization/Utils.h>
+#include <AzCore/Settings/SettingsRegistry.h>
 #include <AzFramework/StringFunc/StringFunc.h>
 #include <SceneAPI/SceneCore/Containers/Scene.h>
 #include <SceneAPI/SceneCore/Containers/Views/PairIterator.h>
@@ -22,6 +23,16 @@
 
 namespace AZ::SceneAPI::Utilities
 {
+    bool IsDebugEnabled()
+    {
+        bool resultValue = false;
+        if (auto* registry = AZ::SettingsRegistry::Get())
+        {
+            registry->Get(resultValue, AZ::SceneAPI::Utilities::Key_AssetProcessorInDebugOutput);
+        }
+        return resultValue;
+    }
+
     bool SaveToJson(const AZStd::string& fileName, const DebugSceneGraph& graph);
 
     void DebugNode::Reflect(AZ::ReflectContext* context)

Code/Tools/SceneAPI/SceneCore/Utilities/DebugOutput.h

@@ -37,6 +37,9 @@ namespace AZ::SceneAPI::Utilities
 {
     constexpr int SceneGraphVersion = 1;
 
+    //! IsDebugEnabled - returns true if additional debug output is desired from scene processing.
+    SCENE_CORE_API bool IsDebugEnabled(); 
+
     struct DebugNode
     {
         AZ_TYPE_INFO(DebugNode, "{490B9D4C-1847-46EB-BEBC-49812E104626}");

Code/Tools/SceneAPI/SceneUI/Handlers/ProcessingHandlers/AsyncOperationProcessingHandler.cpp

@@ -11,6 +11,8 @@
 #include <AzToolsFramework/Debug/TraceContext.h>
 #include <SceneAPI/SceneUI/Handlers/ProcessingHandlers/AsyncOperationProcessingHandler.h>
 
+#include <QTimer>
+
 namespace AZ
 {
     namespace SceneAPI
@@ -27,22 +29,26 @@ namespace AZ
             void AsyncOperationProcessingHandler::BeginProcessing()
             {
                 emit StatusMessageUpdated("Waiting for background processes to complete...");
-                m_thread.reset(
-                    new AZStd::thread(
-                        [this]()
-                        {
-                            AZ_TraceContext("Tag", m_traceTag);
-                            m_operationToRun();
-                            EBUS_QUEUE_FUNCTION(AZ::TickBus, AZStd::bind(&AsyncOperationProcessingHandler::OnBackgroundOperationComplete, this));
-                        }
-                    )
+                // Note that the use of a QThread instead of an AZStd::thread is intentional here, as signals, slots, timers, and other parts
+                // of Qt will cause weird behavior and crashes if invoked from a non-QThread.  Qt tries its best to compensate, but without
+                // a QThread as context, it may not correctly be able to invoke cross-thread event queues, or safely store objects in QThreadStorage.
+                m_thread = QThread::create(
+                    [this]()
+                    {
+                        AZ_TraceContext("Tag", m_traceTag);
+                        m_operationToRun();
+                        QMetaObject::invokeMethod(this, &AsyncOperationProcessingHandler::OnBackgroundOperationComplete, Qt::QueuedConnection);
+                    }
                 );
+                m_thread->start();
             }
 
             void AsyncOperationProcessingHandler::OnBackgroundOperationComplete()
             {
-                m_thread->detach();
-                m_thread.reset(nullptr);
+                m_thread->quit(); // signal the thread's event pump to exit (at this point, its almost certainly already completed)
+                m_thread->wait(); // wait for the thread to clean up any state, as well as actually join (ie, exit) so that it is no longer running.
+                delete m_thread;
+                m_thread = nullptr;
 
                 emit StatusMessageUpdated("Processing complete");
                 if (m_onComplete)

Code/Tools/SceneAPI/SceneUI/Handlers/ProcessingHandlers/AsyncOperationProcessingHandler.h

@@ -13,6 +13,7 @@
 #include <AzCore/std/functional.h>
 #include <AzCore/std/smart_ptr/unique_ptr.h>
 #include <SceneAPI/SceneUI/SceneUIConfiguration.h>
+#include <QThread>
 #endif
 
 namespace AZStd
@@ -34,14 +35,14 @@ namespace AZ
                 ~AsyncOperationProcessingHandler() override = default;
                 void BeginProcessing() override;
 
-            private:
+            private Q_SLOTS:
                 void OnBackgroundOperationComplete();
 
             private:
                 AZ_PUSH_DISABLE_DLL_EXPORT_MEMBER_WARNING
                 AZStd::function<void()> m_operationToRun;
                 AZStd::function<void()> m_onComplete;
-                AZStd::unique_ptr<AZStd::thread> m_thread;
+                QThread* m_thread = nullptr;
                 AZ_POP_DISABLE_DLL_EXPORT_MEMBER_WARNING
             };
         }