From 98fe059d970f8e502e06f091662e473615be19f7 Mon Sep 17 00:00:00 2001 From: "Anthony J. Thibault" Date: Thu, 16 May 2019 13:12:16 -0700 Subject: [PATCH] Fix blender::run crash Pass a shared_ptr of the HFMModel to the Blender. This will prevent the HFMModel from being destroyed on the main thread if an Avatar changes their skeletonModelURL. Also the _blendshapeOffset hash in Model has been eliminated, it was not necessary and was also a source of data races. The body of Blender::run has been updated slightly to reduce the number of allocation necessary for temporary QVectors. --- libraries/hfm/src/hfm/HFM.h | 1 + .../src/model-networking/ModelCache.h | 3 +- .../render-utils/src/CauterizedModel.cpp | 5 +- libraries/render-utils/src/Model.cpp | 153 ++++++++---------- libraries/render-utils/src/Model.h | 5 - .../render-utils/src/SoftAttachmentModel.cpp | 2 +- 6 files changed, 70 insertions(+), 99 deletions(-) diff --git a/libraries/hfm/src/hfm/HFM.h b/libraries/hfm/src/hfm/HFM.h index 8c60169289..484a10aa3b 100644 --- a/libraries/hfm/src/hfm/HFM.h +++ b/libraries/hfm/src/hfm/HFM.h @@ -291,6 +291,7 @@ public: class Model { public: using Pointer = std::shared_ptr; + using ConstPointer = std::shared_ptr; QString originalURL; QString author; diff --git a/libraries/model-networking/src/model-networking/ModelCache.h b/libraries/model-networking/src/model-networking/ModelCache.h index f9ae2dccd6..5b78c18184 100644 --- a/libraries/model-networking/src/model-networking/ModelCache.h +++ b/libraries/model-networking/src/model-networking/ModelCache.h @@ -46,6 +46,7 @@ public: bool isHFMModelLoaded() const { return (bool)_hfmModel; } const HFMModel& getHFMModel() const { return *_hfmModel; } + const HFMModel::ConstPointer& getConstHFMModelPointer() const { return _hfmModel; } const MaterialMapping& getMaterialMapping() const { return _materialMapping; } const GeometryMeshes& getMeshes() const { return *_meshes; } const std::shared_ptr getShapeMaterial(int shapeID) const; @@ -59,7 +60,7 @@ public: protected: // Shared across all geometries, constant throughout lifetime - std::shared_ptr _hfmModel; + HFMModel::ConstPointer _hfmModel; MaterialMapping _materialMapping; std::shared_ptr _meshes; std::shared_ptr _meshParts; diff --git a/libraries/render-utils/src/CauterizedModel.cpp b/libraries/render-utils/src/CauterizedModel.cpp index c700f1ad3f..2cc3f49259 100644 --- a/libraries/render-utils/src/CauterizedModel.cpp +++ b/libraries/render-utils/src/CauterizedModel.cpp @@ -86,8 +86,6 @@ void CauterizedModel::createRenderItemSet() { // Create the render payloads int numParts = (int)mesh->getNumParts(); for (int partIndex = 0; partIndex < numParts; partIndex++) { - initializeBlendshapes(hfmModel.meshes[i], i); - auto ptr = std::make_shared(shared_from_this(), i, partIndex, shapeID, transform, offset); _modelMeshRenderItems << std::static_pointer_cast(ptr); auto material = getGeometry()->getShapeMaterial(shapeID); @@ -96,7 +94,6 @@ void CauterizedModel::createRenderItemSet() { shapeID++; } } - _blendshapeOffsetsInitialized = true; } else { Model::createRenderItemSet(); } @@ -181,7 +178,7 @@ void CauterizedModel::updateClusterMatrices() { // post the blender if we're not currently waiting for one to finish auto modelBlender = DependencyManager::get(); - if (_blendshapeOffsetsInitialized && modelBlender->shouldComputeBlendshapes() && hfmModel.hasBlendedMeshes() && _blendshapeCoefficients != _blendedBlendshapeCoefficients) { + if (modelBlender->shouldComputeBlendshapes() && hfmModel.hasBlendedMeshes() && _blendshapeCoefficients != _blendedBlendshapeCoefficients) { _blendedBlendshapeCoefficients = _blendshapeCoefficients; modelBlender->noteRequiresBlend(getThisPointer()); } diff --git a/libraries/render-utils/src/Model.cpp b/libraries/render-utils/src/Model.cpp index 96979a23a9..7cb20de526 100644 --- a/libraries/render-utils/src/Model.cpp +++ b/libraries/render-utils/src/Model.cpp @@ -314,10 +314,8 @@ bool Model::updateGeometry() { state.clusterDualQuaternions.resize(mesh.clusters.size()); state.clusterMatrices.resize(mesh.clusters.size()); _meshStates.push_back(state); - initializeBlendshapes(mesh, i); i++; } - _blendshapeOffsetsInitialized = true; needFullUpdate = true; emit rigReady(); } @@ -1022,9 +1020,6 @@ void Model::removeFromScene(const render::ScenePointer& scene, render::Transacti _modelMeshRenderItemShapes.clear(); _priorityMap.clear(); - _blendshapeOffsets.clear(); - _blendshapeOffsetsInitialized = false; - _addedToScene = false; _renderInfoVertexCount = 0; @@ -1415,7 +1410,7 @@ void Model::updateClusterMatrices() { // post the blender if we're not currently waiting for one to finish auto modelBlender = DependencyManager::get(); - if (_blendshapeOffsetsInitialized && modelBlender->shouldComputeBlendshapes() && hfmModel.hasBlendedMeshes() && _blendshapeCoefficients != _blendedBlendshapeCoefficients) { + if (modelBlender->shouldComputeBlendshapes() && hfmModel.hasBlendedMeshes() && _blendshapeCoefficients != _blendedBlendshapeCoefficients) { _blendedBlendshapeCoefficients = _blendshapeCoefficients; modelBlender->noteRequiresBlend(getThisPointer()); } @@ -1423,8 +1418,6 @@ void Model::updateClusterMatrices() { void Model::deleteGeometry() { _deleteGeometryCounter++; - _blendshapeOffsets.clear(); - _blendshapeOffsetsInitialized = false; _meshStates.clear(); _rig.destroyAnimGraph(); _blendedBlendshapeCoefficients.clear(); @@ -1494,7 +1487,6 @@ void Model::createRenderItemSet() { // Create the render payloads int numParts = (int)mesh->getNumParts(); for (int partIndex = 0; partIndex < numParts; partIndex++) { - initializeBlendshapes(hfmModel.meshes[i], i); _modelMeshRenderItems << std::make_shared(shared_from_this(), i, partIndex, shapeID, transform, offset); auto material = getGeometry()->getShapeMaterial(shapeID); _modelMeshMaterialNames.push_back(material ? material->getName() : ""); @@ -1502,7 +1494,6 @@ void Model::createRenderItemSet() { shapeID++; } } - _blendshapeOffsetsInitialized = true; } bool Model::isRenderable() const { @@ -1722,119 +1713,105 @@ void packBlendshapeOffsetTo_Pos_F32_3xSN10_Nor_3xSN10_Tan_3xSN10(glm::uvec4& pac class Blender : public QRunnable { public: - Blender(ModelPointer model, int blendNumber, const Geometry::WeakPointer& geometry, const QVector& blendshapeCoefficients); + Blender(ModelPointer model, HFMModel::ConstPointer hfmModel, int blendNumber, const QVector& blendshapeCoefficients); virtual void run() override; private: - ModelPointer _model; + HFMModel::ConstPointer _hfmModel; int _blendNumber; - Geometry::WeakPointer _geometry; QVector _blendshapeCoefficients; }; -Blender::Blender(ModelPointer model, int blendNumber, const Geometry::WeakPointer& geometry, const QVector& blendshapeCoefficients) : +Blender::Blender(ModelPointer model, HFMModel::ConstPointer hfmModel, int blendNumber, const QVector& blendshapeCoefficients) : _model(model), + _hfmModel(hfmModel), _blendNumber(blendNumber), - _geometry(geometry), _blendshapeCoefficients(blendshapeCoefficients) { } void Blender::run() { - QVector blendshapeOffsets; + DETAILED_PROFILE_RANGE_EX(simulation_animation, __FUNCTION__, 0xFFFF0000, 0, { { "url", _model->getURL().toString() } }); + int numBlendshapeOffsets = 0; // number of offsets required for all meshes. + int numMeshes = 0; // number of meshes in this model. + int maxVertsInMesh = 0; // number of vertices in the largest mesh. + for (auto meshIter = _hfmModel->meshes.cbegin(); meshIter != _hfmModel->meshes.cend(); ++meshIter) { + numMeshes++; + int numVertsInMesh = meshIter->vertices.size(); + numBlendshapeOffsets += numVertsInMesh; + } + + // all elements are default constructed to zero offsets. + QVector packedBlendshapeOffsets(numBlendshapeOffsets); + QVector unpackedBlendshapeOffsets(numBlendshapeOffsets); + + // allocate the required size QVector blendedMeshSizes; - if (_model && _model->isLoaded()) { - DETAILED_PROFILE_RANGE_EX(simulation_animation, __FUNCTION__, 0xFFFF0000, 0, { { "url", _model->getURL().toString() } }); - int offset = 0; - const auto& meshes = _model->getHFMModel().meshes; - int meshIndex = 0; - for(const HFMMesh& mesh : meshes) { - auto modelMeshBlendshapeOffsets = _model->_blendshapeOffsets.find(meshIndex++); - if (mesh.blendshapes.isEmpty() || modelMeshBlendshapeOffsets == _model->_blendshapeOffsets.end()) { - // Not blendshaped or not initialized - blendedMeshSizes.push_back(0); + blendedMeshSizes.reserve(numMeshes); + + int offset = 0; + for (auto meshIter = _hfmModel->meshes.cbegin(); meshIter != _hfmModel->meshes.cend(); ++meshIter) { + if (meshIter->blendshapes.isEmpty()) { + blendedMeshSizes.push_back(0); + continue; + } + int numVertsInMesh = meshIter->vertices.size(); + blendedMeshSizes.push_back(numVertsInMesh); + + // for each blendshape in this mesh, accumulate the offsets into unpackedBlendshapeOffsets. + const float NORMAL_COEFFICIENT_SCALE = 0.01f; + for (int i = 0, n = qMin(_blendshapeCoefficients.size(), meshIter->blendshapes.size()); i < n; i++) { + float vertexCoefficient = _blendshapeCoefficients.at(i); + const float EPSILON = 0.0001f; + if (vertexCoefficient < EPSILON) { continue; } - if (mesh.vertices.size() != modelMeshBlendshapeOffsets->second.size()) { - // Mesh sizes don't match. Something has gone wrong - blendedMeshSizes.push_back(0); - continue; - } + float normalCoefficient = vertexCoefficient * NORMAL_COEFFICIENT_SCALE; + const HFMBlendshape& blendshape = meshIter->blendshapes.at(i); + for (int j = 0; j < blendshape.indices.size(); ++j) { + int index = blendshape.indices.at(j); - blendshapeOffsets += modelMeshBlendshapeOffsets->second; - BlendshapeOffset* meshBlendshapeOffsets = blendshapeOffsets.data() + offset; - int numVertices = modelMeshBlendshapeOffsets->second.size(); - blendedMeshSizes.push_back(numVertices); - offset += numVertices; - std::vector unpackedBlendshapeOffsets(modelMeshBlendshapeOffsets->second.size()); - - const float NORMAL_COEFFICIENT_SCALE = 0.01f; - for (int i = 0, n = qMin(_blendshapeCoefficients.size(), mesh.blendshapes.size()); i < n; i++) { - float vertexCoefficient = _blendshapeCoefficients.at(i); - const float EPSILON = 0.0001f; - if (vertexCoefficient < EPSILON) { - continue; - } - - float normalCoefficient = vertexCoefficient * NORMAL_COEFFICIENT_SCALE; - const HFMBlendshape& blendshape = mesh.blendshapes.at(i); - for (int j = 0; j < blendshape.indices.size(); ++j) { - int index = blendshape.indices.at(j); - - auto& currentBlendshapeOffset = unpackedBlendshapeOffsets[index]; - currentBlendshapeOffset.positionOffset += blendshape.vertices.at(j) * vertexCoefficient; - - currentBlendshapeOffset.normalOffset += blendshape.normals.at(j) * normalCoefficient; - if (j < blendshape.tangents.size()) { - currentBlendshapeOffset.tangentOffset += blendshape.tangents.at(j) * normalCoefficient; - } - } - } - - // Blendshape offsets are generrated, now let's pack it on its way to gpu - // FIXME it feels like we could be more effectively using SIMD here - { - auto unpacked = unpackedBlendshapeOffsets.data(); - auto packed = meshBlendshapeOffsets; - for (int j = 0; j < (int)unpackedBlendshapeOffsets.size(); ++j) { - packBlendshapeOffsetTo_Pos_F32_3xSN10_Nor_3xSN10_Tan_3xSN10((*packed).packedPosNorTan, (*unpacked)); - ++unpacked; - ++packed; + auto& currentBlendshapeOffset = unpackedBlendshapeOffsets[offset + index]; + currentBlendshapeOffset.positionOffset += blendshape.vertices.at(j) * vertexCoefficient; + currentBlendshapeOffset.normalOffset += blendshape.normals.at(j) * normalCoefficient; + if (j < blendshape.tangents.size()) { + currentBlendshapeOffset.tangentOffset += blendshape.tangents.at(j) * normalCoefficient; } } } + offset += numVertsInMesh; } + + // convert unpackedBlendshapeOffsets into packedBlendshapeOffsets for the gpu. + // FIXME it feels like we could be more effectively using SIMD here + { + auto unpacked = unpackedBlendshapeOffsets.data(); + auto packed = packedBlendshapeOffsets.data(); + for (int i = 0; i < unpackedBlendshapeOffsets.size(); ++i) { + packBlendshapeOffsetTo_Pos_F32_3xSN10_Nor_3xSN10_Tan_3xSN10((*packed).packedPosNorTan, (*unpacked)); + ++unpacked; + ++packed; + } + } + // post the result to the ModelBlender, which will dispatch to the model if still alive QMetaObject::invokeMethod(DependencyManager::get().data(), "setBlendedVertices", - Q_ARG(ModelPointer, _model), Q_ARG(int, _blendNumber), Q_ARG(QVector, blendshapeOffsets), Q_ARG(QVector, blendedMeshSizes)); + Q_ARG(ModelPointer, _model), Q_ARG(int, _blendNumber), + Q_ARG(QVector, packedBlendshapeOffsets), + Q_ARG(QVector, blendedMeshSizes)); } bool Model::maybeStartBlender() { if (isLoaded()) { - QThreadPool::globalInstance()->start(new Blender(getThisPointer(), ++_blendNumber, _renderGeometry, _blendshapeCoefficients)); + QThreadPool::globalInstance()->start(new Blender(getThisPointer(), getGeometry()->getConstHFMModelPointer(), + ++_blendNumber, _blendshapeCoefficients)); return true; } return false; } -void Model::initializeBlendshapes(const HFMMesh& mesh, int index) { - if (mesh.blendshapes.empty()) { - // mesh doesn't have blendshape, did we allocate one though ? - if (_blendshapeOffsets.find(index) != _blendshapeOffsets.end()) { - qWarning() << "Mesh does not have Blendshape yet the blendshapeOffsets are allocated ?"; - } - return; - } - // Mesh has blendshape, let s allocate the local buffer if not done yet - if (_blendshapeOffsets.find(index) == _blendshapeOffsets.end()) { - QVector blendshapeOffset; - blendshapeOffset.fill(BlendshapeOffset(), mesh.vertices.size()); - _blendshapeOffsets[index] = blendshapeOffset; - } -} - ModelBlender::ModelBlender() : _pendingBlenders(0) { } diff --git a/libraries/render-utils/src/Model.h b/libraries/render-utils/src/Model.h index 7c861a3bc1..2ea1f87fae 100644 --- a/libraries/render-utils/src/Model.h +++ b/libraries/render-utils/src/Model.h @@ -363,8 +363,6 @@ public: void addMaterial(graphics::MaterialLayer material, const std::string& parentMaterialName); void removeMaterial(graphics::MaterialPointer material, const std::string& parentMaterialName); - std::unordered_map> _blendshapeOffsets; - public slots: void loadURLFinished(bool success); @@ -446,7 +444,6 @@ protected: QVector _blendshapeCoefficients; QVector _blendedBlendshapeCoefficients; int _blendNumber { 0 }; - bool _blendshapeOffsetsInitialized { false }; mutable QMutex _mutex{ QMutex::Recursive }; @@ -509,8 +506,6 @@ protected: bool shouldInvalidatePayloadShapeKey(int meshIndex); - void initializeBlendshapes(const HFMMesh& mesh, int index); - private: float _loadingPriority { 0.0f }; diff --git a/libraries/render-utils/src/SoftAttachmentModel.cpp b/libraries/render-utils/src/SoftAttachmentModel.cpp index a6f82e1417..186f9e682a 100644 --- a/libraries/render-utils/src/SoftAttachmentModel.cpp +++ b/libraries/render-utils/src/SoftAttachmentModel.cpp @@ -71,7 +71,7 @@ void SoftAttachmentModel::updateClusterMatrices() { // post the blender if we're not currently waiting for one to finish auto modelBlender = DependencyManager::get(); - if (_blendshapeOffsetsInitialized && modelBlender->shouldComputeBlendshapes() && hfmModel.hasBlendedMeshes() && _blendshapeCoefficients != _blendedBlendshapeCoefficients) { + if (modelBlender->shouldComputeBlendshapes() && hfmModel.hasBlendedMeshes() && _blendshapeCoefficients != _blendedBlendshapeCoefficients) { _blendedBlendshapeCoefficients = _blendshapeCoefficients; modelBlender->noteRequiresBlend(getThisPointer()); }