Refresh import fixes

- Fixed various issues caused by refresh not being done inside
  transaction.
- Added a confirmation prompt to refresh import to warn user about
  the operation not being undoable and resetting the undo stack.
- Fixed issue where material class update was attempted even if just
  materialdef file was changed. This should only be done when shader
  is updated.
- Fixed refreshing import that is not referenced currently.
- Synced import images and meshes dir names given to ImportImpl
  constructor and saved to .import file, as the latter are used
  when refreshing import. "Meshes" vs "meshes" caused breakage
  of referred highlights and mesh visually updating on editor
  in some cases.
- Fixed the material getting lost when refreshing import where the
  model name changes but the material stays the same.

Task-number: QT3DS-3876
Task-number: QT3DS-3879
Task-number: QT3DS-3880
Change-Id: I65e59a570b01b58d3252457f590b9f4425758865
Reviewed-by: Mahmoud Badri <mahmoud.badri@qt.io>
Reviewed-by: Tomi Korpipää <tomi.korpipaa@qt.io>

8 files changed, 128 insertions, 75 deletions

diff --git a/src/Authoring/Client/Code/Core/Doc/ComposerEditorInterface.cpp b/src/Authoring/Client/Code/Core/Doc/ComposerEditorInterface.cpp
index 2391952f..9d477e0c 100644
--- a/src/Authoring/Client/Code/Core/Doc/ComposerEditorInterface.cpp
+++ b/src/Authoring/Client/Code/Core/Doc/ComposerEditorInterface.cpp
@@ -319,6 +319,11 @@ struct SComposerImportInterface : public SComposerImportBase, public IComposerEd
         m_Editor.setMaterialReferenceByPath(instance, materialName.toQString());
         m_Editor.SetName(instance, materialName);
         m_Editor.setMaterialSourcePath(instance, sourcePath);
+        m_Editor.SetSpecificInstancePropertyValue(
+                    0, instance, L"importid", std::make_shared<CDataStr>(desc.m_Id));
+        m_Editor.SetSpecificInstancePropertyValue(
+                    0, instance, L"importfile",
+                    std::make_shared<CDataStr>(m_Relativeimportfile.toCString()));
     }
 
     void UpdateInstanceProperties(TImportId inInstance, const PropertyValue *propertBuffer,
@@ -641,6 +646,11 @@ struct SComposerRefreshInterface : public SComposerImportBase, public IComposerE
             m_Editor.setMaterialReferenceByPath(instance, materialName.toQString());
             m_Editor.SetName(instance, materialName);
             m_Editor.setMaterialSourcePath(instance, sourcePath);
+            m_Editor.SetSpecificInstancePropertyValue(
+                        0, instance, L"importid", std::make_shared<CDataStr>(desc.m_Id));
+            m_Editor.SetSpecificInstancePropertyValue(
+                        0, instance, L"importfile",
+                        std::make_shared<CDataStr>(m_Relativeimportfile.toCString()));
             // Insert the referenced material to the map
             // so that the child structure remains the same
             insert_unique(refInserter.first->second,
@@ -657,6 +667,12 @@ struct SComposerRefreshInterface : public SComposerImportBase, public IComposerE
                                                      m_StringTable);
              theIterator.IsDone() == false; theIterator.Next()) {
             Qt3DSDMInstanceHandle hdl = theIterator.GetCurrentInstance();
+
+            // Skip property setting for reference materials, as they have the same import id
+            // as the actual materials in the container (required to make parent-child logic work)
+            if (m_Editor.GetObjectTypeName(hdl) == "ReferencedMaterial")
+                continue;
+
             for (QT3DSU32 idx = 0; idx < propertyBufferSize; ++idx) {
                 const PropertyValue &value(propertBuffer[idx]);
                 SValue theValue(value.m_Value);
diff --git a/src/Authoring/Client/Code/Core/Doc/DocumentEditor.cpp b/src/Authoring/Client/Code/Core/Doc/DocumentEditor.cpp
index e5bda07a..4705a8eb 100644
--- a/src/Authoring/Client/Code/Core/Doc/DocumentEditor.cpp
+++ b/src/Authoring/Client/Code/Core/Doc/DocumentEditor.cpp
@@ -4612,10 +4612,13 @@ public:
 
     // Precondition is that our source path to instance map
     // has all of the source-path-to-instance hooks already looked up.
-    void DoRefreshImport(const CFilePath &inOldFile, const CFilePath &inNewFile)
+    void DoRefreshImport(const CFilePath &inOldFile, const CFilePath &inNewFile,
+                         const CFilePath &importFilePath)
     {
         ScopedBoolean __ignoredDirs(m_IgnoreDirChange);
         vector<CFilePath> importFileList;
+        if (importFilePath.Exists() && importFilePath.IsFile())
+            importFileList.push_back(importFilePath.filePath());
 
         // Find which import files use this dae file.
         for (TCharPtrToSlideInstanceMap::iterator theIter = m_SourcePathInstanceMap.begin(),
@@ -4649,75 +4652,73 @@ public:
         // OK, for each import file
         // 1.  Find each group in the system using that import file as its source path.
         // 2.  for each group we find, build a map of import id->item that we will use to
-        //		 communicate the import changes to the item.
-        // 4.  Run the refresh process using a composer editor that runs off of our
-        //		mappings
+        //     communicate the import changes to the item.
+        // 4.  Run the refresh process using a composer editor that runs off of our mappings
         TIdMultiMap theGroupIdMap;
         for (size_t importIdx = 0, end = importFileList.size(); importIdx < end; ++importIdx) {
             theGroupIdMap.clear();
             CFilePath theImportFilePath = importFileList[importIdx];
             CFilePath theImportRelativePath = m_Doc.GetRelativePathToDoc(theImportFilePath);
             TCharPtrToSlideInstanceMap::iterator theIter =
-                m_SourcePathInstanceMap.find(m_StringTable.RegisterStr(theImportRelativePath.toCString()));
-            if (theIter == m_SourcePathInstanceMap.end())
-                continue;
-            // First pass just build the group id entries.  This avoids us copying hashtables which
-            // may
-            // be quite expensive
-            for (TSlideInstanceList::iterator theSlideInst = theIter->second.begin(),
-                                              theSlideInstEnd = theIter->second.end();
-                 theSlideInst != theSlideInstEnd; ++theSlideInst) {
-                TInstanceHandle theRoot = theSlideInst->second;
-                TSlideHandle theSlide = theSlideInst->first;
-
-                // For a depth first search of all children of this object *in this slide*,
-                // if they have an import id then add them to the map.
-                DepthFirstAddImportChildren(theSlide, theRoot, theGroupIdMap, theAddedInstances);
-                TIdMultiMap::iterator theGroupId =
-                    theGroupIdMap
-                        .insert(make_pair(m_StringTable.GetWideStr(GetImportId(theRoot)),
-                                          vector<pair<Qt3DSDMSlideHandle, Qt3DSDMInstanceHandle>>()))
-                        .first;
-                insert_unique(theGroupId->second, make_pair(theSlide, theRoot));
-                theAddedInstances.insert(theRoot);
-            }
-            // Since some objects may be completely free standing, we need to go through *all*
-            // objects.
-            // Unfortunately the first revision of the system didn't put import paths on objects so
-            // we need both the above loop *and* to consider every object who's import path matches
-            // out import document's relative path.
-            theIter = theImportPaths.find(m_StringTable.RegisterStr(theImportRelativePath.toCString()));
-            TSlideHandleList theAssociatedSlides;
-            if (theIter != theImportPaths.end()) {
-                vector<pair<Qt3DSDMSlideHandle, Qt3DSDMInstanceHandle>> &theInstances =
-                    theIter->second;
-                for (size_t freeInstanceIdx = 0, end = theInstances.size(); freeInstanceIdx < end;
-                     ++freeInstanceIdx) {
-                    if (theAddedInstances.find(theInstances[freeInstanceIdx].second)
-                        != theAddedInstances.end())
-                        continue;
-                    theAssociatedSlides.clear();
-                    Qt3DSDMInstanceHandle theInstance(theInstances[freeInstanceIdx].second);
-                    GetAllAssociatedSlides(theInstance, theAssociatedSlides);
-                    TIdMultiMap::iterator theInstanceId =
-                        theGroupIdMap
-                            .insert(
-                                make_pair(m_StringTable.GetWideStr(GetImportId(theInstance)),
-                                          vector<pair<Qt3DSDMSlideHandle, Qt3DSDMInstanceHandle>>()))
-                            .first;
-                    for (size_t slideIdx = 0, slideEnd = theAssociatedSlides.size();
-                         slideIdx < slideEnd; ++slideIdx)
-                        insert_unique(theInstanceId->second,
-                                      make_pair(theAssociatedSlides[slideIdx], theInstance));
-                    theAddedInstances.insert(theInstance);
+                m_SourcePathInstanceMap.find(m_StringTable.RegisterStr(theImportRelativePath
+                                                                       .toCString()));
+            if (theIter != m_SourcePathInstanceMap.end()) {
+                // First pass just build the group id entries. This avoids us copying hashtables
+                // which may be quite expensive
+                for (TSlideInstanceList::iterator theSlideInst = theIter->second.begin(),
+                                                  theSlideInstEnd = theIter->second.end();
+                     theSlideInst != theSlideInstEnd; ++theSlideInst) {
+                    TInstanceHandle theRoot = theSlideInst->second;
+                    TSlideHandle theSlide = theSlideInst->first;
+
+                    // For a depth first search of all children of this object *in this slide*,
+                    // if they have an import id then add them to the map.
+                    DepthFirstAddImportChildren(theSlide, theRoot, theGroupIdMap,
+                                                theAddedInstances);
+                    TIdMultiMap::iterator theGroupId
+                        = theGroupIdMap.insert({m_StringTable.GetWideStr(GetImportId(theRoot)),
+                                                {}}).first;
+                    insert_unique(theGroupId->second, make_pair(theSlide, theRoot));
+                    theAddedInstances.insert(theRoot);
+                }
+                // Since some objects may be completely free standing, we need to go through *all*
+                // objects.
+                // Unfortunately the first revision of the system didn't put import paths on objects
+                // so we need both the above loop *and* to consider every object who's import path
+                // matches out import document's relative path.
+                theIter = theImportPaths.find(m_StringTable.RegisterStr(theImportRelativePath
+                                                                        .toCString()));
+                TSlideHandleList theAssociatedSlides;
+                if (theIter != theImportPaths.end()) {
+                    vector<pair<Qt3DSDMSlideHandle, Qt3DSDMInstanceHandle>> &theInstances
+                        = theIter->second;
+                    for (size_t i = 0, end = theInstances.size(); i < end; ++i) {
+                        if (theAddedInstances.find(theInstances[i].second)
+                                != theAddedInstances.end()) {
+                            continue;
+                        }
+                        theAssociatedSlides.clear();
+                        Qt3DSDMInstanceHandle theInstance(theInstances[i].second);
+                        GetAllAssociatedSlides(theInstance, theAssociatedSlides);
+                        TIdMultiMap::iterator theInstanceId
+                             = theGroupIdMap
+                                .insert({m_StringTable.GetWideStr(GetImportId(theInstance)),
+                                         {}}).first;
+                        for (size_t slideIdx = 0, slideEnd = theAssociatedSlides.size();
+                             slideIdx < slideEnd; ++slideIdx) {
+                            insert_unique(theInstanceId->second,
+                                          make_pair(theAssociatedSlides[slideIdx], theInstance));
+                        }
+                        theAddedInstances.insert(theInstance);
+                    }
                 }
             }
 
-            //
             // OK, we have distinct maps sorted on a per-slide basis for all trees of children
             // of this asset.  We now need to attempt to run the refresh algorithm.
 
-            qt3dsimp::ImportPtrOrError theImportPtr = qt3dsimp::Import::Load(theImportFilePath.toCString());
+            qt3dsimp::ImportPtrOrError theImportPtr
+                    = qt3dsimp::Import::Load(theImportFilePath.toCString());
             if (!theImportPtr.m_Value) {
                 QT3DS_ASSERT(false);
                 continue;
@@ -4737,7 +4738,8 @@ public:
                 Q3DStudio::CString::ENDOFSTRING, false)
                 && oldExtension.Compare(CDialogs::GetWideDAEFileExtension(),
                     Q3DStudio::CString::ENDOFSTRING, false)) {
-                SColladaTranslator *colladaTranslator = new SColladaTranslator(inNewFile.toQString());
+                SColladaTranslator *colladaTranslator
+                        = new SColladaTranslator(inNewFile.toQString());
                 translationLog = &(colladaTranslator->m_TranslationLog);
                 translator = colladaTranslator;
 #ifdef QT_3DSTUDIO_FBX
@@ -4773,18 +4775,21 @@ public:
         updateMaterialFiles();
     }
 
-    void RefreshImport(const CFilePath &inOldFile, const CFilePath &inNewFile) override
+    void RefreshImport(const CFilePath &inOldFile, const CFilePath &inNewFile,
+                       const CFilePath &importFilePath) override
     {
         CDispatch &theDispatch(*m_Doc.GetCore()->GetDispatch());
+
         theDispatch.FireOnProgressBegin(
             QObject::tr("Refreshing Import "), QFileInfo(inNewFile.toQString()).fileName());
         ScopedBoolean __ignoredDirs(m_IgnoreDirChange);
         try {
             m_SourcePathInstanceMap.clear();
             GetSourcePathToInstanceMap(m_SourcePathInstanceMap, true, false);
-            DoRefreshImport(inOldFile, inNewFile);
+            DoRefreshImport(inOldFile, inNewFile, importFilePath);
         } catch (...) {
         }
+
         theDispatch.FireOnProgressEnd();
     }
 
@@ -5363,7 +5368,7 @@ public:
                     theDispatch.FireReloadEffectInstance(theInstances[i].second);
                     theDispatch.FireImmediateRefreshInstance(theInstances[i].second);
                 }
-            } else if (CDialogs::materialExtensions().contains(theExtension)
+            } else if (CDialogs::shaderExtensions().contains(theExtension)
                        && theRecord.m_ModificationType != FileModificationType::Created
                        && !theInstances.empty()) {
                 CString theNameStr = GetName(theInstances[0].second);
diff --git a/src/Authoring/Client/Code/Core/Doc/IDocumentEditor.h b/src/Authoring/Client/Code/Core/Doc/IDocumentEditor.h
index 2378d713..71859311 100644
--- a/src/Authoring/Client/Code/Core/Doc/IDocumentEditor.h
+++ b/src/Authoring/Client/Code/Core/Doc/IDocumentEditor.h
@@ -460,7 +460,8 @@ public:
 
     // Refresh an import or dae file
     // Absolute path to the file.
-    virtual void RefreshImport(const CFilePath &inOldFile, const CFilePath &inNewFile) = 0;
+    virtual void RefreshImport(const CFilePath &inOldFile, const CFilePath &inNewFile,
+                               const CFilePath &importFilePath) = 0;
 
     virtual bool CleanUpMeshes() = 0;
 
diff --git a/src/Authoring/QT3DSIMP/Qt3DSImportLib/Qt3DSImport.cpp b/src/Authoring/QT3DSIMP/Qt3DSImportLib/Qt3DSImport.cpp
index df6e7279..2bbab435 100644
--- a/src/Authoring/QT3DSIMP/Qt3DSImportLib/Qt3DSImport.cpp
+++ b/src/Authoring/QT3DSIMP/Qt3DSImportLib/Qt3DSImport.cpp
@@ -1460,8 +1460,8 @@ public:
 
                 CString src = theSrcFile.toCString();
                 writer.Att(L"SrcFile", src.c_str());
-                writer.Att(L"ImageDir", L"Images");
-                writer.Att(L"MeshDir", L"Meshes");
+                writer.Att(L"ImageDir", L"maps");
+                writer.Att(L"MeshDir", L"meshes");
                 Serialize(writer, L"Image", const_cast<TStrToStrMap &>(m_Images));
                 Serialize(writer, L"Mesh", const_cast<TPathToMeshMap &>(m_Meshes));
             }
diff --git a/src/Authoring/QT3DSIMP/Qt3DSImportLib/Qt3DSImportPerformImport.cpp b/src/Authoring/QT3DSIMP/Qt3DSImportLib/Qt3DSImportPerformImport.cpp
index 8072055f..6955a052 100644
--- a/src/Authoring/QT3DSIMP/Qt3DSImportLib/Qt3DSImportPerformImport.cpp
+++ b/src/Authoring/QT3DSIMP/Qt3DSImportLib/Qt3DSImportPerformImport.cpp
@@ -58,17 +58,15 @@ void DoUpdateInstances(Import &import, IComposerEditor &composer,
     QT3DSIMP_FOREACH(idx, instances.size())
     {
         // We have to re-lookup instances here because the instance data may have changed since it
-        // was put
-        // into the import report.  For instance, you get an import report, then you add instances.
-        // This adds
-        // user ids.  But the import report is already created so the instance descriptions in the
-        // report's
-        // add list won't reflect the new reality.
+        // was put into the import report. For instance, you get an import report, then you add
+        // instances. This adds user ids. But the import report is already created so the instance
+        // descriptions in the report's add list won't reflect the new reality.
         const InstanceDesc &desc(instances[idx]);
         QT3DSU32 numProps = import.GetNumProperties(desc.m_Handle);
         properties.resize(numProps);
         import.GetProperties(desc.m_Handle, properties);
-        composer.UpdateInstanceProperties(desc.m_Id, properties.data(), (QT3DSU32)properties.size());
+        composer.UpdateInstanceProperties(desc.m_Id, properties.data(),
+                                          (QT3DSU32)properties.size());
     }
 }
 }
diff --git a/src/Authoring/Qt3DStudio/Palettes/Project/ProjectView.cpp b/src/Authoring/Qt3DStudio/Palettes/Project/ProjectView.cpp
index 82115094..7946e8a2 100644
--- a/src/Authoring/Qt3DStudio/Palettes/Project/ProjectView.cpp
+++ b/src/Authoring/Qt3DStudio/Palettes/Project/ProjectView.cpp
@@ -468,9 +468,15 @@ void ProjectView::refreshImport(int row) const
         const QFileInfo newFile(g_StudioApp.GetDialogs()->ConfirmRefreshModelFile(fullSrcPath));
         if (newFile.exists() && newFile.isFile()) {
             // We don't want to create undo point of "Refresh Import", undoing this sort of
-            // thing is supposed to be done in the DCC tool.
-            g_StudioApp.GetCore()->GetDoc()->getSceneEditor()->RefreshImport(
-                        oldFile.canonicalFilePath(), newFile.canonicalFilePath());
+            // thing is supposed to be done in the DCC tool. However, we do want to do the
+            // refresh inside a transaction to make sure signaling works correctly.
+            SCOPED_DOCUMENT_EDITOR(*g_StudioApp.GetCore()->GetDoc(), QObject::tr("Refresh Import"))
+                    ->RefreshImport(oldFile.canonicalFilePath(), newFile.canonicalFilePath(), path);
+            // Clear undo/redo stack to ensure stack is not corrupted
+            // after refresh (stack may contain additions/deletions with chosen import,
+            // in which case undoing/redoing those actions can result in crashes or wrong meshes)
+            g_StudioApp.GetCore()->GetCmdStack()->Clear();
+            g_StudioApp.GetCore()->GetDoc()->SetModifiedFlag(true);
         }
     }
 }
diff --git a/src/Authoring/Qt3DStudio/Workspace/Dialogs.cpp b/src/Authoring/Qt3DStudio/Workspace/Dialogs.cpp
index d42f54c3..276d6329 100644
--- a/src/Authoring/Qt3DStudio/Workspace/Dialogs.cpp
+++ b/src/Authoring/Qt3DStudio/Workspace/Dialogs.cpp
@@ -148,6 +148,10 @@ const char *materialExts[] = {
     "material", "shader", "materialdef", nullptr,
 };
 
+const char *shaderExts[] = {
+    "material", "shader", nullptr,
+};
+
 const wchar_t *wideMaterialExts[] = {
     L"material", L"shader", L"materialdef", nullptr,
 };
@@ -441,6 +445,16 @@ bool CDialogs::DisplayUndefinedDatainputDlg(
 
 QString CDialogs::ConfirmRefreshModelFile(const QString &inFile)
 {
+    QString prompt = QObject::tr("Refreshing an import is not undoable and resets the undo stack.\n"
+                                 "Are you sure you want to refresh '%1'?")
+                     .arg(QFileInfo(inFile).fileName());
+
+    int choice = QMessageBox::warning(nullptr, QObject::tr("Refresh import"),
+                                      prompt, QMessageBox::Yes | QMessageBox::Cancel);
+
+    if (choice == QMessageBox::Cancel)
+        return {};
+
     // this produces an extension string which contains all allowed formats specified in
     // g_AllowedImportTypes
     // currently DAE and FBX
@@ -1489,6 +1503,18 @@ QStringList CDialogs::materialExtensions()
     return exts;
 }
 
+QStringList CDialogs::shaderExtensions()
+{
+    static QStringList exts;
+    if (exts.isEmpty()) {
+        for (const char *ext : shaderExts) {
+            if (ext)
+                exts << QString::fromLatin1(ext);
+        }
+    }
+    return exts;
+}
+
 QStringList CDialogs::modelExtensions()
 {
     static QStringList exts;
diff --git a/src/Authoring/Qt3DStudio/Workspace/Dialogs.h b/src/Authoring/Qt3DStudio/Workspace/Dialogs.h
index b10922cb..4d702ddf 100644
--- a/src/Authoring/Qt3DStudio/Workspace/Dialogs.h
+++ b/src/Authoring/Qt3DStudio/Workspace/Dialogs.h
@@ -68,6 +68,7 @@ public:
     static QStringList fontExtensions();
     static QStringList mapExtensions();
     static QStringList materialExtensions();
+    static QStringList shaderExtensions();
     static QStringList modelExtensions();
     static QStringList behaviorExtensions();
     static QStringList presentationExtensions();