From 429e65888b38a3c86f866029b5bb28c4b4c1378c Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Sun, 16 Apr 2017 15:52:17 -0700 Subject: [PATCH] cleanup threading and result handling for DomainBaker --- libraries/model-baking/src/Baker.cpp | 12 ++ libraries/model-baking/src/Baker.h | 8 ++ libraries/model-baking/src/FBXBaker.cpp | 150 ++++++++++++-------- libraries/model-baking/src/FBXBaker.h | 14 +- libraries/model-baking/src/TextureBaker.cpp | 8 +- libraries/model-baking/src/TextureBaker.h | 5 +- tools/oven/src/DomainBaker.cpp | 91 ++++++++---- tools/oven/src/DomainBaker.h | 1 - tools/oven/src/ui/DomainBakeWidget.cpp | 2 + 9 files changed, 190 insertions(+), 101 deletions(-) diff --git a/libraries/model-baking/src/Baker.cpp b/libraries/model-baking/src/Baker.cpp index 8e118790cc..b692c1d96b 100644 --- a/libraries/model-baking/src/Baker.cpp +++ b/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); +} diff --git a/libraries/model-baking/src/Baker.h b/libraries/model-baking/src/Baker.h index 19b1486346..6853620361 100644 --- a/libraries/model-baking/src/Baker.h +++ b/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 diff --git a/libraries/model-baking/src/FBXBaker.cpp b/libraries/model-baking/src/FBXBaker.cpp index 8181932247..ff1d6659aa 100644 --- a/libraries/model-baking/src/FBXBaker.cpp +++ b/libraries/model-baking/src/FBXBaker.cpp @@ -18,6 +18,8 @@ #include #include +#include + #include #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 - _sdkManager = FbxManager::Create(); + std::call_once(onceFlag, [](){ + // 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 - removeEmbeddedMediaFolder(); - - if (hasErrors()) { - return; - } - - // cleanup the originals if we weren't asked to keep them around - possiblyCleanupOriginals(); + // check if we're already done with textures (in case we had none to re-write) + checkIfTexturesFinished(); } 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, - // to our hash of textures needing to be baked - _unbakedTextures.insert(urlToTexture, bakedTextureFileName); - - // bake this texture asynchronously - bakeTexture(urlToTexture); + + if (!_unbakedTextures.contains(urlToTexture)) { + // add the deduced url to the texture, associated with the resulting baked texture file name, + // to our hash of textures needing to be baked + _unbakedTextures.insert(urlToTexture, bakedTextureFileName); + + // 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 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(sender()); + TextureBaker* bakedTexture = qobject_cast(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 - // since that is where the FBX SDK places the .fbm folder it generates when importing the FBX + // 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 - auto originalOutputFolder = QUrl::fromLocalFile(_uniqueOutputPath + ORIGINAL_OUTPUT_SUBFOLDER); + auto originalOutputFolder = QUrl::fromLocalFile(_uniqueOutputPath + ORIGINAL_OUTPUT_SUBFOLDER); - if (!originalOutputFolder.isParentOf(bakedTexture->getTextureURL())) { - // for linked textures we want to save a copy of original texture beside the original FBX + if (!originalOutputFolder.isParentOf(bakedTexture->getTextureURL())) { + // 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 - auto relativeTexturePath = texturePathRelativeToFBX(_fbxURL, bakedTexture->getTextureURL()); + // check if we have a relative path to use for the texture + auto relativeTexturePath = texturePathRelativeToFBX(_fbxURL, bakedTexture->getTextureURL()); - QFile originalTextureFile { - _uniqueOutputPath + ORIGINAL_OUTPUT_SUBFOLDER + relativeTexturePath + bakedTexture->getTextureURL().fileName() - }; + QFile originalTextureFile { + _uniqueOutputPath + ORIGINAL_OUTPUT_SUBFOLDER + relativeTexturePath + bakedTexture->getTextureURL().fileName() + }; - if (relativeTexturePath.length() > 0) { - // make the folders needed by the relative path + if (relativeTexturePath.length() > 0) { + // 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(); + } +} diff --git a/libraries/model-baking/src/FBXBaker.h b/libraries/model-baking/src/FBXBaker.h index a44ce4d0bf..ef94de7a2e 100644 --- a/libraries/model-baking/src/FBXBaker.h +++ b/libraries/model-baking/src/FBXBaker.h @@ -18,6 +18,7 @@ #include #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>; 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 _textureNameMatchCount; QHash _textureTypes; - std::list> _bakingTextures; + QSet> _bakingTextures; QFutureSynchronizer _textureBakeSynchronizer; bool _copyOriginals { true }; diff --git a/libraries/model-baking/src/TextureBaker.cpp b/libraries/model-baking/src/TextureBaker.cpp index a717835ed7..bdd20ea270 100644 --- a/libraries/model-baking/src/TextureBaker.cpp +++ b/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"; - - emit finished(); + handleError("Unable to open texture " + _textureURL.toString()); return; } diff --git a/libraries/model-baking/src/TextureBaker.h b/libraries/model-baking/src/TextureBaker.h index 06bac0d066..4f793af37d 100644 --- a/libraries/model-baking/src/TextureBaker.h +++ b/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); diff --git a/tools/oven/src/DomainBaker.cpp b/tools/oven/src/DomainBaker.cpp index c779fd3a40..b53b74f227 100644 --- a/tools/oven/src/DomainBaker.cpp +++ b/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 baker { new FBXBaker(modelURL, _contentOutputPath) }; + QSharedPointer 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(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 - // entity objects needing a URL re-write - for (QJsonValueRef entityValue : _entitiesNeedingRewrite.values(baker->getFBXUrl())) { + // enumerate the QJsonRef values for the URL of this FBX from our multi hash of + // entity objects needing a URL re-write + for (QJsonValueRef entityValue : _entitiesNeedingRewrite.values(baker->getFBXUrl())) { - // convert the entity QJsonValueRef to a QJsonObject so we can modify its URL - auto entity = entityValue.toObject(); + // convert the entity QJsonValueRef to a QJsonObject so we can modify its URL + auto entity = entityValue.toObject(); - // grab the old URL - QUrl oldModelURL { entity[ENTITY_MODEL_URL_KEY].toString() }; + // grab the old URL + QUrl oldModelURL { entity[ENTITY_MODEL_URL_KEY].toString() }; - // setup a new URL using the prefix we were passed - QUrl newModelURL = _destinationPath.resolved(baker->getBakedFBXRelativePath().mid(1)); + // setup a new URL using the prefix we were passed + QUrl newModelURL = _destinationPath.resolved(baker->getBakedFBXRelativePath().mid(1)); - // copy the fragment and query from the old model URL - newModelURL.setQuery(oldModelURL.query()); - newModelURL.setFragment(oldModelURL.fragment()); + // copy the fragment and query from the old model URL + newModelURL.setQuery(oldModelURL.query()); + newModelURL.setFragment(oldModelURL.fragment()); - // set the new model URL as the value in our temp QJsonObject - entity[ENTITY_MODEL_URL_KEY] = newModelURL.toString(); - - // replace our temp object with the value referenced by our QJsonValueRef - entityValue = entity; + // set the new model URL as the value in our temp QJsonObject + entity[ENTITY_MODEL_URL_KEY] = newModelURL.toString(); + + // replace our temp object with the value referenced by our QJsonValueRef + 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; } diff --git a/tools/oven/src/DomainBaker.h b/tools/oven/src/DomainBaker.h index 3eae758445..6c8555cc62 100644 --- a/tools/oven/src/DomainBaker.h +++ b/tools/oven/src/DomainBaker.h @@ -30,7 +30,6 @@ public: virtual void bake() override; signals: - void finished(); void allModelsFinished(); private slots: diff --git a/tools/oven/src/ui/DomainBakeWidget.cpp b/tools/oven/src/ui/DomainBakeWidget.cpp index b1f549dd00..7c8c462cfd 100644 --- a/tools/oven/src/ui/DomainBakeWidget.cpp +++ b/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"); }