cleanup threading and result handling for DomainBaker

libraries/model-baking/src/Baker.cpp

@@ -18,3 +18,15 @@ void Baker::handleError(const QString& error) {
     _errorList.append(error);
     emit finished();
 }
+
+void Baker::appendErrors(const QStringList& errors) {
+    // we're appending errors, presumably from a baking operation we called
+    // add those to our list and emit that we are finished
+    _errorList.append(errors);
+    emit finished();
+}
+
+void Baker::handleWarning(const QString& warning) {
+    qCWarning(model_baking).noquote() << warning;
+    _warningList.append(warning);
+}

libraries/model-baking/src/Baker.h

@@ -23,13 +23,21 @@ public:
     bool hasErrors() const { return !_errorList.isEmpty(); }
     QStringList getErrors() const { return _errorList; }
 
+    bool hasWarnings() const { return !_warningList.isEmpty(); }
+    QStringList getWarnings() const { return _warningList; }
+
 signals:
     void finished();
 
 protected:
     void handleError(const QString& error);
+    void handleWarning(const QString& warning);
+
+    void appendErrors(const QStringList& errors);
+    void appendWarnings(const QStringList& warnings) { _warningList << warnings; }
 
     QStringList _errorList;
+    QStringList _warningList;
 };
 
 #endif // hifi_Baker_h

libraries/model-baking/src/FBXBaker.cpp

@@ -18,6 +18,8 @@
 #include <QtCore/QFileInfo>
 #include <QtCore/QThread>
 
+#include <mutex>
+
 #include <NetworkAccessManager.h>
 
 #include "ModelBakingLoggingCategory.h"
@@ -25,24 +27,26 @@
 
 #include "FBXBaker.h"
 
+std::once_flag onceFlag;
+FBXSDKManagerUniquePointer FBXBaker::_sdkManager { nullptr };
 
 FBXBaker::FBXBaker(const QUrl& fbxURL, const QString& baseOutputPath, bool copyOriginals) :
     _fbxURL(fbxURL),
     _baseOutputPath(baseOutputPath),
     _copyOriginals(copyOriginals)
 {
-    // create an FBX SDK manager
+    std::call_once(onceFlag, [](){
-    _sdkManager = FbxManager::Create();
+        // create the static FBX SDK manager
+        _sdkManager = FBXSDKManagerUniquePointer(FbxManager::Create(), [](FbxManager* manager){
+            manager->Destroy();
+        });
+    });
 
     // grab the name of the FBX from the URL, this is used for folder output names
     auto fileName = fbxURL.fileName();
     _fbxName = fileName.left(fileName.indexOf('.'));
 }
 
-FBXBaker::~FBXBaker() {
-    _sdkManager->Destroy();
-}
-
 static const QString BAKED_OUTPUT_SUBFOLDER = "baked/";
 static const QString ORIGINAL_OUTPUT_SUBFOLDER = "original/";
 
@@ -88,15 +92,8 @@ void FBXBaker::bakeSourceCopy() {
         return;
     }
 
-    // remove the embedded media folder that the FBX SDK produces when reading the original
+    // check if we're already done with textures (in case we had none to re-write)
-    removeEmbeddedMediaFolder();
+    checkIfTexturesFinished();
-
-    if (hasErrors()) {
-        return;
-    }
-
-    // cleanup the originals if we weren't asked to keep them around
-    possiblyCleanupOriginals();
 }
 
 void FBXBaker::setupOutputFolder() {
@@ -186,7 +183,7 @@ void FBXBaker::handleFBXNetworkReply() {
 
 void FBXBaker::importScene() {
     // create an FBX SDK importer
-    FbxImporter* importer = FbxImporter::Create(_sdkManager, "");
+    FbxImporter* importer = FbxImporter::Create(_sdkManager.get(), "");
 
     // import the copy of the original FBX file
     QString originalCopyPath = pathToCopyOfOriginal();
@@ -201,7 +198,7 @@ void FBXBaker::importScene() {
     }
 
     // setup a new scene to hold the imported file
-    _scene = FbxScene::Create(_sdkManager, "bakeScene");
+    _scene = FbxScene::Create(_sdkManager.get(), "bakeScene");
 
     // import the file to the created scene
     importer->Import(_scene);
@@ -397,13 +394,15 @@ void FBXBaker::rewriteAndBakeSceneTextures() {
 
                                 // figure out the URL to this texture, embedded or external
                                 auto urlToTexture = getTextureURL(textureFileInfo, fileTexture);
-                                
+
-                                // add the deduced url to the texture, associated with the resulting baked texture file name,
+                                if (!_unbakedTextures.contains(urlToTexture)) {
-                                // to our hash of textures needing to be baked
+                                    // add the deduced url to the texture, associated with the resulting baked texture file name,
-                                _unbakedTextures.insert(urlToTexture, bakedTextureFileName);
+                                    // to our hash of textures needing to be baked
-                                
+                                    _unbakedTextures.insert(urlToTexture, bakedTextureFileName);
-                                // bake this texture asynchronously
+
-                                bakeTexture(urlToTexture);
+                                    // bake this texture asynchronously
+                                    bakeTexture(urlToTexture);
+                                }
                             }
                         }
                     }
@@ -417,63 +416,68 @@ void FBXBaker::rewriteAndBakeSceneTextures() {
 
 void FBXBaker::bakeTexture(const QUrl& textureURL) {
     // start a bake for this texture and add it to our list to keep track of
-    auto bakingTexture = new TextureBaker(textureURL);
+    QSharedPointer<TextureBaker> bakingTexture { new TextureBaker(textureURL), &TextureBaker::deleteLater };
 
-    connect(bakingTexture, &TextureBaker::finished, this, &FBXBaker::handleBakedTexture);
+    connect(bakingTexture.data(), &Baker::finished, this, &FBXBaker::handleBakedTexture);
 
-    QtConcurrent::run(bakingTexture, &TextureBaker::bake);
+    QtConcurrent::run(bakingTexture.data(), &TextureBaker::bake);
 
-    _bakingTextures.emplace_back(bakingTexture);
+    _bakingTextures.insert(bakingTexture);
 }
 
 void FBXBaker::handleBakedTexture() {
-    auto bakedTexture = qobject_cast<TextureBaker*>(sender());
+    TextureBaker* bakedTexture = qobject_cast<TextureBaker*>(sender());
 
-    // use the path to the texture being baked to determine if this was an embedded or a linked texture
+    // make sure we haven't already run into errors, and that this is a valid texture
+    if (!hasErrors() && bakedTexture) {
+        if (!bakedTexture->hasErrors()) {
+            // use the path to the texture being baked to determine if this was an embedded or a linked texture
 
-    // it is embeddded if the texure being baked was inside the original output folder
+            // it is embeddded if the texure being baked was inside the original output folder
-    // since that is where the FBX SDK places the .fbm folder it generates when importing the FBX
+            // since that is where the FBX SDK places the .fbm folder it generates when importing the FBX
 
-    auto originalOutputFolder = QUrl::fromLocalFile(_uniqueOutputPath + ORIGINAL_OUTPUT_SUBFOLDER);
+            auto originalOutputFolder = QUrl::fromLocalFile(_uniqueOutputPath + ORIGINAL_OUTPUT_SUBFOLDER);
 
-    if (!originalOutputFolder.isParentOf(bakedTexture->getTextureURL())) {
+            if (!originalOutputFolder.isParentOf(bakedTexture->getTextureURL())) {
-        // for linked textures we want to save a copy of original texture beside the original FBX
+                // for linked textures we want to save a copy of original texture beside the original FBX
 
-        qCDebug(model_baking) << "Saving original texture for" << bakedTexture->getTextureURL();
+                qCDebug(model_baking) << "Saving original texture for" << bakedTexture->getTextureURL();
 
-        // check if we have a relative path to use for the texture
+                // check if we have a relative path to use for the texture
-        auto relativeTexturePath = texturePathRelativeToFBX(_fbxURL, bakedTexture->getTextureURL());
+                auto relativeTexturePath = texturePathRelativeToFBX(_fbxURL, bakedTexture->getTextureURL());
 
-        QFile originalTextureFile {
+                QFile originalTextureFile {
-            _uniqueOutputPath + ORIGINAL_OUTPUT_SUBFOLDER + relativeTexturePath + bakedTexture->getTextureURL().fileName()
+                    _uniqueOutputPath + ORIGINAL_OUTPUT_SUBFOLDER + relativeTexturePath + bakedTexture->getTextureURL().fileName()
-        };
+                };
 
-        if (relativeTexturePath.length() > 0) {
+                if (relativeTexturePath.length() > 0) {
-            // make the folders needed by the relative path
+                    // make the folders needed by the relative path
+                }
+
+                if (originalTextureFile.open(QIODevice::WriteOnly) && originalTextureFile.write(bakedTexture->getOriginalTexture()) != -1) {
+                    qCDebug(model_baking) << "Saved original texture file" << originalTextureFile.fileName()
+                    << "for" << _fbxURL;
+                } else {
+                    handleError("Could not save original external texture " + originalTextureFile.fileName()
+                                + " for " + _fbxURL.toString());
+                    return;
+                }
+            }
+
+            // now that this texture has been baked and handled, we can remove that TextureBaker from our list
+            _unbakedTextures.remove(bakedTexture->getTextureURL());
+
+            checkIfTexturesFinished();
+        } else {            
+            // there was an error baking this texture - add it to our list of errors and stop processing this FBX
+            appendErrors(bakedTexture->getErrors());
         }
-
-        if (originalTextureFile.open(QIODevice::WriteOnly) && originalTextureFile.write(bakedTexture->getOriginalTexture()) != -1) {
-            qCDebug(model_baking) << "Saved original texture file" << originalTextureFile.fileName()
-                << "for" << _fbxURL;
-        } else {
-            handleError("Could not save original external texture " + originalTextureFile.fileName()
-                       + " for " + _fbxURL.toString());
-            return;
-        }
-    }
-
-    // now that this texture has been baked and handled, we can remove that TextureBaker from our list
-    _unbakedTextures.remove(bakedTexture->getTextureURL());
-
-    // check if we're done everything we need to do for this FBX
-    if (_unbakedTextures.isEmpty()) {
-        emit finished();
     }
 }
 
 void FBXBaker::exportScene() {
     // setup the exporter
-    FbxExporter* exporter = FbxExporter::Create(_sdkManager, "");
+    FbxExporter* exporter = FbxExporter::Create(_sdkManager.get(), "");
 
     auto rewrittenFBXPath = _uniqueOutputPath + BAKED_OUTPUT_SUBFOLDER + _fbxName + BAKED_FBX_EXTENSION;
 
@@ -508,3 +512,27 @@ void FBXBaker::possiblyCleanupOriginals() {
         QDir(_uniqueOutputPath + ORIGINAL_OUTPUT_SUBFOLDER).removeRecursively();
     }
 }
+
+void FBXBaker::checkIfTexturesFinished() {
+    // check if we're done everything we need to do for this FBX
+    // and emit our finished signal if we're done
+    if (_unbakedTextures.isEmpty()) {
+        // remove the embedded media folder that the FBX SDK produces when reading the original
+        removeEmbeddedMediaFolder();
+
+        if (hasErrors()) {
+            return;
+        }
+
+        // cleanup the originals if we weren't asked to keep them around
+        possiblyCleanupOriginals();
+
+        if (hasErrors()) {
+            return;
+        }
+
+        qCDebug(model_baking) << "Finished baking" << _fbxURL;
+        
+        emit finished();
+    }
+}

libraries/model-baking/src/FBXBaker.h

@@ -18,6 +18,7 @@
 #include <QtNetwork/QNetworkReply>
 
 #include "Baker.h"
+#include "TextureBaker.h"
 
 namespace fbxsdk {
     class FbxManager;
@@ -45,23 +46,22 @@ enum TextureType {
     UNUSED_TEXTURE = -1
 };
 
-class TextureBaker;
-
 static const QString BAKED_FBX_EXTENSION = ".baked.fbx";
+using FBXSDKManagerUniquePointer = std::unique_ptr<fbxsdk::FbxManager, std::function<void (fbxsdk::FbxManager *)>>;
 
 class FBXBaker : public Baker {
     Q_OBJECT
 public:
     FBXBaker(const QUrl& fbxURL, const QString& baseOutputPath, bool copyOriginals = true);
-    ~FBXBaker();
 
+    // all calls to bake must be from the same thread, because the Autodesk SDK will cause
+    // a crash if it is called from multiple threads
     Q_INVOKABLE virtual void bake() override;
 
     QUrl getFBXUrl() const { return _fbxURL; }
     QString getBakedFBXRelativePath() const { return _bakedFBXRelativePath; }
 
 signals:
-    void finished();
     void allTexturesBaked();
 
     void sourceCopyReadyToLoad();
@@ -84,6 +84,8 @@ private:
     void removeEmbeddedMediaFolder();
     void possiblyCleanupOriginals();
 
+    void checkIfTexturesFinished();
+
     QString createBakedTextureFileName(const QFileInfo& textureFileInfo);
     QUrl getTextureURL(const QFileInfo& textureFileInfo, fbxsdk::FbxFileTexture* fileTexture);
 
@@ -98,7 +100,7 @@ private:
     QString _uniqueOutputPath;
     QString _bakedFBXRelativePath;
 
-    fbxsdk::FbxManager* _sdkManager;
+    static FBXSDKManagerUniquePointer _sdkManager;
     fbxsdk::FbxScene* _scene { nullptr };
 
     QStringList _errorList;
@@ -107,7 +109,7 @@ private:
     QHash<QString, int> _textureNameMatchCount;
     QHash<uint64_t, TextureType> _textureTypes;
 
-    std::list<std::unique_ptr<TextureBaker>> _bakingTextures;
+    QSet<QSharedPointer<TextureBaker>> _bakingTextures;
     QFutureSynchronizer<void> _textureBakeSynchronizer;
 
     bool _copyOriginals { true };

libraries/model-baking/src/TextureBaker.cpp

@@ -29,6 +29,10 @@ void TextureBaker::bake() {
     // first load the texture (either locally or remotely)
     loadTexture();
 
+    if (hasErrors()) {
+        return;
+    }
+
     qCDebug(model_baking) << "Baking texture at" << _textureURL;
 
     emit finished();
@@ -41,9 +45,7 @@ void TextureBaker::loadTexture() {
         QFile localTexture { _textureURL.toLocalFile() };
 
         if (!localTexture.open(QIODevice::ReadOnly)) {
-            qCWarning(model_baking) << "Unable to open local texture at" << _textureURL << "for baking";
+            handleError("Unable to open texture " + _textureURL.toString());
-
-            emit finished();
             return;
         }
 

libraries/model-baking/src/TextureBaker.h

@@ -22,16 +22,13 @@ class TextureBaker : public Baker {
 
 public:
     TextureBaker(const QUrl& textureURL);
-
+    
     void bake();
 
     const QByteArray& getOriginalTexture() const { return _originalTexture; }
 
     const QUrl& getTextureURL() const { return _textureURL; }
 
-signals:
-    void finished();
-
 private:
     void loadTexture();
     void handleTextureNetworkReply(QNetworkReply* requestReply);

tools/oven/src/DomainBaker.cpp

@@ -36,10 +36,29 @@ DomainBaker::DomainBaker(const QUrl& localModelFileURL, const QString& domainNam
 
 void DomainBaker::bake() {
     setupOutputFolder();
+
+    if (hasErrors()) {
+        return;
+    }
+
     loadLocalFile();
+
+    if (hasErrors()) {
+        return;
+    }
+
     setupBakerThread();
+
+    if (hasErrors()) {
+        return;
+    }
+
     enumerateEntities();
 
+    if (hasErrors()) {
+        return;
+    }
+
     if (!_entitiesNeedingRewrite.isEmpty()) {
         // use a QEventLoop to wait for all entity rewrites to be completed before writing the final models file
         QEventLoop eventLoop;
@@ -47,8 +66,16 @@ void DomainBaker::bake() {
         eventLoop.exec();
     }
 
+    if (hasErrors()) {
+        return;
+    }
+
     writeNewEntitiesFile();
 
+    if (hasErrors()) {
+        return;
+    }
+
     // stop the FBX baker thread now that all our bakes have completed
     _fbxBakerThread->quit();
 
@@ -70,8 +97,8 @@ void DomainBaker::setupOutputFolder() {
     QDir outputDir { _baseOutputPath };
 
     if (!outputDir.mkpath(outputDirectoryName)) {
-
         // add an error to specify that the output directory could not be created
+        handleError("Could not create output folder");
 
         return;
     }
@@ -84,7 +111,7 @@ void DomainBaker::setupOutputFolder() {
     static const QString CONTENT_OUTPUT_FOLDER_NAME = "content";
     if (!outputDir.mkpath(CONTENT_OUTPUT_FOLDER_NAME)) {
         // add an error to specify that the content output directory could not be created
-
+        handleError("Could not create content folder");
         return;
     }
 
@@ -95,17 +122,18 @@ const QString ENTITIES_OBJECT_KEY = "Entities";
 
 void DomainBaker::loadLocalFile() {
     // load up the local entities file
-    QFile modelsFile { _localEntitiesFileURL.toLocalFile() };
+    QFile entitiesFile { _localEntitiesFileURL.toLocalFile() };
 
-    if (!modelsFile.open(QIODevice::ReadOnly)) {
+    if (!entitiesFile.open(QIODevice::ReadOnly)) {
         // add an error to our list to specify that the file could not be read
+        handleError("Could not open entities file");
 
         // return to stop processing
         return;
     }
 
     // grab a byte array from the file
-    auto fileContents = modelsFile.readAll();
+    auto fileContents = entitiesFile.readAll();
 
     // check if we need to inflate a gzipped models file or if this was already decompressed
     static const QString GZIPPED_ENTITIES_FILE_SUFFIX = "gz";
@@ -167,10 +195,10 @@ void DomainBaker::enumerateEntities() {
 
                     // setup an FBXBaker for this URL, as long as we don't already have one
                     if (!_bakers.contains(modelURL)) {
-                        QSharedPointer<FBXBaker> baker { new FBXBaker(modelURL, _contentOutputPath) };
+                        QSharedPointer<FBXBaker> baker { new FBXBaker(modelURL, _contentOutputPath), &FBXBaker::deleteLater };
 
                         // make sure our handler is called when the baker is done
-                        connect(baker.data(), &FBXBaker::finished, this, &DomainBaker::handleFinishedBaker);
+                        connect(baker.data(), &Baker::finished, this, &DomainBaker::handleFinishedBaker);
 
                         // insert it into our bakers hash so we hold a strong pointer to it
                         _bakers.insert(modelURL, baker);
@@ -194,35 +222,44 @@ void DomainBaker::handleFinishedBaker() {
     auto baker = qobject_cast<FBXBaker*>(sender());
 
     if (baker) {
-        // this FBXBaker is done and everything went according to plan
+        if (!baker->hasErrors()) {
+            // this FBXBaker is done and everything went according to plan
 
-        // enumerate the QJsonRef values for the URL of this FBX from our multi hash of
+            // enumerate the QJsonRef values for the URL of this FBX from our multi hash of
-        // entity objects needing a URL re-write
+            // entity objects needing a URL re-write
-        for (QJsonValueRef entityValue : _entitiesNeedingRewrite.values(baker->getFBXUrl())) {
+            for (QJsonValueRef entityValue : _entitiesNeedingRewrite.values(baker->getFBXUrl())) {
 
-            // convert the entity QJsonValueRef to a QJsonObject so we can modify its URL
+                // convert the entity QJsonValueRef to a QJsonObject so we can modify its URL
-            auto entity = entityValue.toObject();
+                auto entity = entityValue.toObject();
 
-            // grab the old URL
+                // grab the old URL
-            QUrl oldModelURL { entity[ENTITY_MODEL_URL_KEY].toString() };
+                QUrl oldModelURL { entity[ENTITY_MODEL_URL_KEY].toString() };
 
-            // setup a new URL using the prefix we were passed
+                // setup a new URL using the prefix we were passed
-            QUrl newModelURL = _destinationPath.resolved(baker->getBakedFBXRelativePath().mid(1));
+                QUrl newModelURL = _destinationPath.resolved(baker->getBakedFBXRelativePath().mid(1));
 
-            // copy the fragment and query from the old model URL
+                // copy the fragment and query from the old model URL
-            newModelURL.setQuery(oldModelURL.query());
+                newModelURL.setQuery(oldModelURL.query());
-            newModelURL.setFragment(oldModelURL.fragment());
+                newModelURL.setFragment(oldModelURL.fragment());
 
-            // set the new model URL as the value in our temp QJsonObject
+                // set the new model URL as the value in our temp QJsonObject
-            entity[ENTITY_MODEL_URL_KEY] = newModelURL.toString();
+                entity[ENTITY_MODEL_URL_KEY] = newModelURL.toString();
-
+                
-            // replace our temp object with the value referenced by our QJsonValueRef
+                // replace our temp object with the value referenced by our QJsonValueRef
-            entityValue = entity;
+                entityValue = entity;
+            }
+        } else {
+            // this model failed to bake - this doesn't fail the entire bake but we need to add
+            // the errors from the model to our errors
+            appendWarnings(baker->getErrors());
         }
 
         // remove the baked URL from the multi hash of entities needing a re-write
         _entitiesNeedingRewrite.remove(baker->getFBXUrl());
 
+        // drop our shared pointer to this baker so that it gets cleaned up
+        _bakers.remove(baker->getFBXUrl());
+
         if (_entitiesNeedingRewrite.isEmpty()) {
             emit allModelsFinished();
         }
@@ -256,12 +293,14 @@ void DomainBaker::writeNewEntitiesFile() {
 
     if (!compressedEntitiesFile.open(QIODevice::WriteOnly)
         || (compressedEntitiesFile.write(compressedJson) == -1)) {
-        qWarning() << "Failed to export baked entities file to" << bakedEntitiesFilePath;
+
         // add an error to our list to state that the output models file could not be created or could not be written to
+        handleError("Failed to export baked entities file");
 
         return;
     }
 
     qDebug() << "Exported entities file with baked model URLs to" << bakedEntitiesFilePath;
+    qDebug() << "WARNINGS:" << _warningList;
 }
 

tools/oven/src/DomainBaker.h

@@ -30,7 +30,6 @@ public:
     virtual void bake() override;
 
 signals:
-    void finished();
     void allModelsFinished();
 
 private slots:

tools/oven/src/ui/DomainBakeWidget.cpp

@@ -242,6 +242,8 @@ void DomainBakeWidget::handleFinishedBaker() {
 
             if (baker->hasErrors()) {
                 resultsWindow->changeStatusForRow(resultRow, baker->getErrors().join("\n"));
+            } else if (baker->hasWarnings()) {
+                resultsWindow->changeStatusForRow(resultRow, baker->getWarnings().join("\n"));
             } else {
                 resultsWindow->changeStatusForRow(resultRow, "Success");
             }