From 1a36693161424a82d2e103c96258701a22bde004 Mon Sep 17 00:00:00 2001 From: SamGondelman Date: Tue, 11 Sep 2018 16:58:38 -0700 Subject: [PATCH 01/13] I hate blendshapes --- .../render-utils/src/CauterizedModel.cpp | 3 +- libraries/render-utils/src/Model.cpp | 43 ++++++++++--------- libraries/render-utils/src/Model.h | 2 - 3 files changed, 25 insertions(+), 23 deletions(-) diff --git a/libraries/render-utils/src/CauterizedModel.cpp b/libraries/render-utils/src/CauterizedModel.cpp index c4631c3676..ffb652f923 100644 --- a/libraries/render-utils/src/CauterizedModel.cpp +++ b/libraries/render-utils/src/CauterizedModel.cpp @@ -86,7 +86,7 @@ void CauterizedModel::createRenderItemSet() { // Create the render payloads int numParts = (int)mesh->getNumParts(); for (int partIndex = 0; partIndex < numParts; partIndex++) { - if (!fbxGeometry.meshes[i].blendshapes.empty()) { + if (!fbxGeometry.meshes[i].blendshapes.empty() && _blendedVertexBuffers.find(i) == _blendedVertexBuffers.end()) { initializeBlendshapes(fbxGeometry.meshes[i], i); } auto ptr = std::make_shared(shared_from_this(), i, partIndex, shapeID, transform, offset); @@ -97,6 +97,7 @@ void CauterizedModel::createRenderItemSet() { shapeID++; } } + _blendedVertexBuffersInitialized = true; } else { Model::createRenderItemSet(); } diff --git a/libraries/render-utils/src/Model.cpp b/libraries/render-utils/src/Model.cpp index ba2bd28852..2feae1fabb 100644 --- a/libraries/render-utils/src/Model.cpp +++ b/libraries/render-utils/src/Model.cpp @@ -302,12 +302,18 @@ bool Model::updateGeometry() { assert(_meshStates.empty()); const FBXGeometry& fbxGeometry = getFBXGeometry(); + int i = 0; foreach (const FBXMesh& mesh, fbxGeometry.meshes) { MeshState state; state.clusterDualQuaternions.resize(mesh.clusters.size()); state.clusterMatrices.resize(mesh.clusters.size()); _meshStates.push_back(state); + if (!mesh.blendshapes.empty() && _blendedVertexBuffers.find(i) == _blendedVertexBuffers.end()) { + initializeBlendshapes(mesh, i); + } + i++; } + _blendedVertexBuffersInitialized = true; needFullUpdate = true; emit rigReady(); } @@ -1273,8 +1279,7 @@ QStringList Model::getJointNames() const { class Blender : public QRunnable { public: - Blender(ModelPointer model, int blendNumber, const Geometry::WeakPointer& geometry, - const QVector& meshes, const QVector& blendshapeCoefficients); + Blender(ModelPointer model, int blendNumber, const Geometry::WeakPointer& geometry, const QVector& blendshapeCoefficients); virtual void run() override; @@ -1283,16 +1288,13 @@ private: ModelPointer _model; int _blendNumber; Geometry::WeakPointer _geometry; - QVector _meshes; QVector _blendshapeCoefficients; }; -Blender::Blender(ModelPointer model, int blendNumber, const Geometry::WeakPointer& geometry, - const QVector& meshes, const QVector& blendshapeCoefficients) : +Blender::Blender(ModelPointer model, int blendNumber, const Geometry::WeakPointer& geometry, const QVector& blendshapeCoefficients) : _model(model), _blendNumber(blendNumber), _geometry(geometry), - _meshes(meshes), _blendshapeCoefficients(blendshapeCoefficients) { } @@ -1300,10 +1302,12 @@ void Blender::run() { DETAILED_PROFILE_RANGE_EX(simulation_animation, __FUNCTION__, 0xFFFF0000, 0, { { "url", _model->getURL().toString() } }); QVector vertices; QVector normalsAndTangents; - if (_model) { + auto geometry = _geometry.lock(); + if (_model && geometry) { int offset = 0; int normalsAndTangentsOffset = 0; - foreach (const FBXMesh& mesh, _meshes) { + auto meshes = geometry->getFBXGeometry().meshes; + foreach (const FBXMesh& mesh, meshes) { if (mesh.blendshapes.isEmpty()) { continue; } @@ -1526,8 +1530,7 @@ bool Model::maybeStartBlender() { if (isLoaded()) { const FBXGeometry& fbxGeometry = getFBXGeometry(); if (fbxGeometry.hasBlendedMeshes()) { - QThreadPool::globalInstance()->start(new Blender(getThisPointer(), ++_blendNumber, _renderGeometry, - fbxGeometry.meshes, _blendshapeCoefficients)); + QThreadPool::globalInstance()->start(new Blender(getThisPointer(), ++_blendNumber, _renderGeometry, _blendshapeCoefficients)); return true; } } @@ -1537,11 +1540,11 @@ bool Model::maybeStartBlender() { void Model::setBlendedVertices(int blendNumber, const Geometry::WeakPointer& geometry, const QVector& vertices, const QVector& normalsAndTangents) { auto geometryRef = geometry.lock(); - if (!geometryRef || _renderGeometry != geometryRef || blendNumber < _appliedBlendNumber) { + if (!geometryRef || _renderGeometry != geometryRef || blendNumber < _appliedBlendNumber || !_blendedVertexBuffersInitialized) { return; } _appliedBlendNumber = blendNumber; - const FBXGeometry& fbxGeometry = getFBXGeometry(); + const FBXGeometry& fbxGeometry = geometryRef->getFBXGeometry(); int index = 0; int normalAndTangentIndex = 0; for (int i = 0; i < fbxGeometry.meshes.size(); i++) { @@ -1552,11 +1555,11 @@ void Model::setBlendedVertices(int blendNumber, const Geometry::WeakPointer& geo const auto vertexCount = mesh.vertices.size(); const auto verticesSize = vertexCount * sizeof(glm::vec3); - const auto& buffer = _blendedVertexBuffers[i]; - assert(buffer && _blendedVertexBuffersInitialized); - buffer->resize(mesh.vertices.size() * sizeof(glm::vec3) + mesh.normalsAndTangents.size() * sizeof(NormalType)); - buffer->setSubData(0, verticesSize, (gpu::Byte*) vertices.constData() + index * sizeof(glm::vec3)); - buffer->setSubData(verticesSize, mesh.normalsAndTangents.size() * sizeof(NormalType), (const gpu::Byte*) normalsAndTangents.data() + normalAndTangentIndex * sizeof(NormalType)); + const auto& buffer = _blendedVertexBuffers.find(i); + assert(buffer != _blendedVertexBuffers.end()); + buffer->second->resize(mesh.vertices.size() * sizeof(glm::vec3) + mesh.normalsAndTangents.size() * sizeof(NormalType)); + buffer->second->setSubData(0, verticesSize, (gpu::Byte*) vertices.constData() + index * sizeof(glm::vec3)); + buffer->second->setSubData(verticesSize, mesh.normalsAndTangents.size() * sizeof(NormalType), (const gpu::Byte*) normalsAndTangents.data() + normalAndTangentIndex * sizeof(NormalType)); index += vertexCount; normalAndTangentIndex += mesh.normalsAndTangents.size(); @@ -1599,6 +1602,7 @@ const render::ItemIDs& Model::fetchRenderItemIDs() const { } void Model::initializeBlendshapes(const FBXMesh& mesh, int index) { + _blendedVertexBuffers[index] = std::make_shared(); QVector normalsAndTangents; normalsAndTangents.resize(2 * mesh.normals.size()); @@ -1627,12 +1631,10 @@ void Model::initializeBlendshapes(const FBXMesh& mesh, int index) { } }); const auto verticesSize = mesh.vertices.size() * sizeof(glm::vec3); - _blendedVertexBuffers[index] = std::make_shared(); _blendedVertexBuffers[index]->resize(mesh.vertices.size() * sizeof(glm::vec3) + normalsAndTangents.size() * sizeof(NormalType)); _blendedVertexBuffers[index]->setSubData(0, verticesSize, (const gpu::Byte*) mesh.vertices.constData()); _blendedVertexBuffers[index]->setSubData(verticesSize, normalsAndTangents.size() * sizeof(NormalType), (const gpu::Byte*) normalsAndTangents.data()); mesh.normalsAndTangents = normalsAndTangents; - _blendedVertexBuffersInitialized = true; } void Model::createRenderItemSet() { @@ -1673,7 +1675,7 @@ void Model::createRenderItemSet() { // Create the render payloads int numParts = (int)mesh->getNumParts(); for (int partIndex = 0; partIndex < numParts; partIndex++) { - if (!fbxGeometry.meshes[i].blendshapes.empty()) { + if (!fbxGeometry.meshes[i].blendshapes.empty() && _blendedVertexBuffers.find(i) == _blendedVertexBuffers.end()) { initializeBlendshapes(fbxGeometry.meshes[i], i); } _modelMeshRenderItems << std::make_shared(shared_from_this(), i, partIndex, shapeID, transform, offset); @@ -1683,6 +1685,7 @@ void Model::createRenderItemSet() { shapeID++; } } + _blendedVertexBuffersInitialized = true; } bool Model::isRenderable() const { diff --git a/libraries/render-utils/src/Model.h b/libraries/render-utils/src/Model.h index e7534f5b89..f35048aa2f 100644 --- a/libraries/render-utils/src/Model.h +++ b/libraries/render-utils/src/Model.h @@ -345,8 +345,6 @@ public: void addMaterial(graphics::MaterialLayer material, const std::string& parentMaterialName); void removeMaterial(graphics::MaterialPointer material, const std::string& parentMaterialName); - bool areBlendedVertexBuffersInitialized(int index) { return _blendedVertexBuffersInitialized; } - public slots: void loadURLFinished(bool success); From 4b93f9f5cda5c5233b2a5ba89319ac6975678530 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 12 Sep 2018 12:20:03 -0700 Subject: [PATCH 02/13] Revert "use different XORed instance ID for replicas" This reverts commit a37a19da1e4e7657548fb29ad6cf4e130a5eda76. --- libraries/avatars/src/AvatarHashMap.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index 284bb20f16..6fc53fcc59 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -86,8 +86,7 @@ void AvatarReplicas::processDeletedTraitInstance(const QUuid& parentID, AvatarTr if (_replicasMap.find(parentID) != _replicasMap.end()) { auto &replicas = _replicasMap[parentID]; for (auto avatar : replicas) { - avatar->processDeletedTraitInstance(traitType, - AvatarTraits::xoredInstanceID(instanceID, avatar->getTraitInstanceXORID())); + avatar->processDeletedTraitInstance(traitType, instanceID); } } } @@ -96,9 +95,7 @@ void AvatarReplicas::processTraitInstance(const QUuid& parentID, AvatarTraits::T if (_replicasMap.find(parentID) != _replicasMap.end()) { auto &replicas = _replicasMap[parentID]; for (auto avatar : replicas) { - avatar->processTraitInstance(traitType, - AvatarTraits::xoredInstanceID(instanceID, avatar->getTraitInstanceXORID()), - traitBinaryData); + avatar->processTraitInstance(traitType, instanceID, traitBinaryData); } } } @@ -364,11 +361,11 @@ void AvatarHashMap::processBulkAvatarTraits(QSharedPointer mess // in order to handle re-connections to the avatar mixer when the other if (traitBinarySize == AvatarTraits::DELETED_TRAIT_SIZE) { avatar->processDeletedTraitInstance(traitType, xoredInstanceID); - _replicas.processDeletedTraitInstance(avatarID, traitType, traitInstanceID); + _replicas.processDeletedTraitInstance(avatarID, traitType, xoredInstanceID); } else { auto traitData = message->read(traitBinarySize); avatar->processTraitInstance(traitType, xoredInstanceID, traitData); - _replicas.processTraitInstance(avatarID, traitType, traitInstanceID, traitData); + _replicas.processTraitInstance(avatarID, traitType, xoredInstanceID, traitData); } processedInstanceVersion = packetTraitVersion; } else { From 6d3e6a661f1bc5d3650c6d11e502d4072c78826e Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 12 Sep 2018 12:20:31 -0700 Subject: [PATCH 03/13] Revert "XOR incoming trait instance IDs for other avatars" This reverts commit 9b3d9dd0f319ad2964e54a72bd17934765a91c02. --- libraries/avatars/src/AvatarData.h | 5 ----- libraries/avatars/src/AvatarHashMap.cpp | 20 ++++-------------- libraries/avatars/src/AvatarTraits.h | 21 ++----------------- libraries/avatars/src/ClientTraitsHandler.cpp | 21 ++++++++++++------- libraries/avatars/src/ClientTraitsHandler.h | 4 ++++ 5 files changed, 24 insertions(+), 47 deletions(-) diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index e9f1f5f6c3..b010a9f924 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -1193,9 +1193,6 @@ public: void setReplicaIndex(int replicaIndex) { _replicaIndex = replicaIndex; } int getReplicaIndex() { return _replicaIndex; } - const AvatarTraits::TraitInstanceID getTraitInstanceXORID() const { return _traitInstanceXORID; } - void cycleTraitInstanceXORID() { _traitInstanceXORID = QUuid::createUuid(); } - signals: /**jsdoc @@ -1502,8 +1499,6 @@ private: // privatize the copy constructor and assignment operator so they cannot be called AvatarData(const AvatarData&); AvatarData& operator= (const AvatarData&); - - AvatarTraits::TraitInstanceID _traitInstanceXORID { QUuid::createUuid() }; }; Q_DECLARE_METATYPE(AvatarData*) diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index 6fc53fcc59..d205a915f8 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -343,29 +343,17 @@ void AvatarHashMap::processBulkAvatarTraits(QSharedPointer mess AvatarTraits::TraitInstanceID traitInstanceID = QUuid::fromRfc4122(message->readWithoutCopy(NUM_BYTES_RFC4122_UUID)); - // XOR the incoming trait instance ID with this avatar object's personal XOR ID - - // this ensures that we have separate entity instances in the local tree - // if we briefly end up with two Avatar objects for this node - - // (which can occur if the shared pointer for the - // previous instance of an avatar hasn't yet gone out of scope before the - // new instance is created) - - auto xoredInstanceID = AvatarTraits::xoredInstanceID(traitInstanceID, avatar->getTraitInstanceXORID()); - message->readPrimitive(&traitBinarySize); auto& processedInstanceVersion = lastProcessedVersions.getInstanceValueRef(traitType, traitInstanceID); if (packetTraitVersion > processedInstanceVersion) { - // in order to handle re-connections to the avatar mixer when the other if (traitBinarySize == AvatarTraits::DELETED_TRAIT_SIZE) { - avatar->processDeletedTraitInstance(traitType, xoredInstanceID); - _replicas.processDeletedTraitInstance(avatarID, traitType, xoredInstanceID); + avatar->processDeletedTraitInstance(traitType, traitInstanceID); + _replicas.processDeletedTraitInstance(avatarID, traitType, traitInstanceID); } else { auto traitData = message->read(traitBinarySize); - avatar->processTraitInstance(traitType, xoredInstanceID, traitData); - _replicas.processTraitInstance(avatarID, traitType, xoredInstanceID, traitData); + avatar->processTraitInstance(traitType, traitInstanceID, traitData); + _replicas.processTraitInstance(avatarID, traitType, traitInstanceID, traitData); } processedInstanceVersion = packetTraitVersion; } else { diff --git a/libraries/avatars/src/AvatarTraits.h b/libraries/avatars/src/AvatarTraits.h index 47be0d6111..5866caf234 100644 --- a/libraries/avatars/src/AvatarTraits.h +++ b/libraries/avatars/src/AvatarTraits.h @@ -41,8 +41,7 @@ namespace AvatarTraits { const TraitWireSize DELETED_TRAIT_SIZE = -1; inline qint64 packInstancedTraitDelete(TraitType traitType, TraitInstanceID instanceID, ExtendedIODevice& destination, - TraitVersion traitVersion = NULL_TRAIT_VERSION, - TraitInstanceID xoredInstanceID = TraitInstanceID()) { + TraitVersion traitVersion = NULL_TRAIT_VERSION) { qint64 bytesWritten = 0; bytesWritten += destination.writePrimitive(traitType); @@ -51,28 +50,12 @@ namespace AvatarTraits { bytesWritten += destination.writePrimitive(traitVersion); } - if (xoredInstanceID.isNull()) { - bytesWritten += destination.write(instanceID.toRfc4122()); - } else { - bytesWritten += destination.write(xoredInstanceID.toRfc4122()); - } + bytesWritten += destination.write(instanceID.toRfc4122()); bytesWritten += destination.writePrimitive(DELETED_TRAIT_SIZE); return bytesWritten; } - - inline TraitInstanceID xoredInstanceID(TraitInstanceID localInstanceID, TraitInstanceID xorKeyID) { - QByteArray xoredInstanceID { NUM_BYTES_RFC4122_UUID, 0 }; - auto xorKeyIDBytes = xorKeyID.toRfc4122(); - auto localInstanceIDBytes = localInstanceID.toRfc4122(); - - for (auto i = 0; i < localInstanceIDBytes.size(); ++i) { - xoredInstanceID[i] = localInstanceIDBytes[i] ^ xorKeyIDBytes[i]; - } - - return QUuid::fromRfc4122(xoredInstanceID); - } }; #endif // hifi_AvatarTraits_h diff --git a/libraries/avatars/src/ClientTraitsHandler.cpp b/libraries/avatars/src/ClientTraitsHandler.cpp index 479852cf9a..94823610dc 100644 --- a/libraries/avatars/src/ClientTraitsHandler.cpp +++ b/libraries/avatars/src/ClientTraitsHandler.cpp @@ -45,7 +45,7 @@ void ClientTraitsHandler::resetForNewMixer() { _owningAvatar->prepareResetTraitInstances(); // reset the trait XOR ID since we're resetting for a new avatar mixer - _owningAvatar->cycleTraitInstanceXORID(); + _sessionXORID = QUuid::createUuid().toRfc4122(); } void ClientTraitsHandler::sendChangedTraitsToMixer() { @@ -100,15 +100,11 @@ void ClientTraitsHandler::sendChangedTraitsToMixer() { // since this is going to the mixer, use the XORed instance ID (to anonymize trait instance IDs // that would typically persist across sessions) _owningAvatar->packTraitInstance(instancedIt->traitType, instanceIDValuePair.id, *traitsPacketList, - AvatarTraits::NULL_TRAIT_VERSION, - AvatarTraits::xoredInstanceID(instanceIDValuePair.id, - _owningAvatar->getTraitInstanceXORID())); + AvatarTraits::NULL_TRAIT_VERSION, xorInstanceID(instanceIDValuePair.id)); } else if (!_shouldPerformInitialSend && instanceIDValuePair.value == Deleted) { // pack delete for this trait instance AvatarTraits::packInstancedTraitDelete(instancedIt->traitType, instanceIDValuePair.id, - *traitsPacketList, AvatarTraits::NULL_TRAIT_VERSION, - AvatarTraits::xoredInstanceID(instanceIDValuePair.id, - _owningAvatar->getTraitInstanceXORID())); + *traitsPacketList); } } @@ -122,6 +118,17 @@ void ClientTraitsHandler::sendChangedTraitsToMixer() { } } +AvatarTraits::TraitInstanceID ClientTraitsHandler::xorInstanceID(AvatarTraits::TraitInstanceID localInstanceID) { + QByteArray xorInstanceID { NUM_BYTES_RFC4122_UUID, 0 }; + auto localInstanceIDBytes = localInstanceID.toRfc4122(); + + for (auto i = 0; i < localInstanceIDBytes.size(); ++i) { + xorInstanceID[i] = localInstanceIDBytes[i] ^ _sessionXORID[i]; + } + + return QUuid::fromRfc4122(xorInstanceID); +} + void ClientTraitsHandler::processTraitOverride(QSharedPointer message, SharedNodePointer sendingNode) { if (sendingNode->getType() == NodeType::AvatarMixer) { while (message->getBytesLeftToRead()) { diff --git a/libraries/avatars/src/ClientTraitsHandler.h b/libraries/avatars/src/ClientTraitsHandler.h index 6d1592ba74..2319061502 100644 --- a/libraries/avatars/src/ClientTraitsHandler.h +++ b/libraries/avatars/src/ClientTraitsHandler.h @@ -37,6 +37,8 @@ public: void resetForNewMixer(); + AvatarTraits::TraitInstanceID xorInstanceID(AvatarTraits::TraitInstanceID localInstanceID); + public slots: void processTraitOverride(QSharedPointer message, SharedNodePointer sendingNode); @@ -53,6 +55,8 @@ private: AvatarTraits::TraitVersion _currentTraitVersion { AvatarTraits::DEFAULT_TRAIT_VERSION }; AvatarTraits::TraitVersion _currentSkeletonVersion { AvatarTraits::NULL_TRAIT_VERSION }; + + QByteArray _sessionXORID { NUM_BYTES_RFC4122_UUID, 0}; bool _shouldPerformInitialSend { false }; bool _hasChangedTraits { false }; From 1789d51d6e15ab7982579f8bbc6a03104e15d0a0 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 12 Sep 2018 12:22:07 -0700 Subject: [PATCH 04/13] Revert "cycle avatar entity IDs for new avatar mixer" This reverts commit 20912349a4f0b8854bda467ee1e1bd04d9bb67ef. --- .../src/avatars-renderer/Avatar.cpp | 1 + libraries/avatars/src/AvatarData.cpp | 10 ++-------- libraries/avatars/src/AvatarData.h | 3 +-- libraries/avatars/src/AvatarTraits.h | 2 +- libraries/avatars/src/ClientTraitsHandler.cpp | 20 +------------------ libraries/avatars/src/ClientTraitsHandler.h | 8 ++------ 6 files changed, 8 insertions(+), 36 deletions(-) diff --git a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp index ac25f65576..cadc3b31e3 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp +++ b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp @@ -141,6 +141,7 @@ Avatar::~Avatar() { } }); } + auto geometryCache = DependencyManager::get(); if (geometryCache) { geometryCache->releaseID(_nameRectGeometryID); diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index e7aaa18576..ea6dbd7074 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1861,9 +1861,7 @@ qint64 AvatarData::packTrait(AvatarTraits::TraitType traitType, ExtendedIODevice } qint64 AvatarData::packTraitInstance(AvatarTraits::TraitType traitType, AvatarTraits::TraitInstanceID traitInstanceID, - ExtendedIODevice& destination, AvatarTraits::TraitVersion traitVersion, - AvatarTraits::TraitInstanceID wireInstanceID) { - + ExtendedIODevice& destination, AvatarTraits::TraitVersion traitVersion) { qint64 bytesWritten = 0; bytesWritten += destination.writePrimitive(traitType); @@ -1872,11 +1870,7 @@ qint64 AvatarData::packTraitInstance(AvatarTraits::TraitType traitType, AvatarTr bytesWritten += destination.writePrimitive(traitVersion); } - if (!wireInstanceID.isNull()) { - bytesWritten += destination.write(wireInstanceID.toRfc4122()); - } else { - bytesWritten += destination.write(traitInstanceID.toRfc4122()); - } + bytesWritten += destination.write(traitInstanceID.toRfc4122()); if (traitType == AvatarTraits::AvatarEntity) { // grab a read lock on the avatar entities and check for entity data for the given ID diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index b010a9f924..db2b82b0b7 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -962,8 +962,7 @@ public: qint64 packTrait(AvatarTraits::TraitType traitType, ExtendedIODevice& destination, AvatarTraits::TraitVersion traitVersion = AvatarTraits::NULL_TRAIT_VERSION); qint64 packTraitInstance(AvatarTraits::TraitType traitType, AvatarTraits::TraitInstanceID instanceID, - ExtendedIODevice& destination, AvatarTraits::TraitVersion traitVersion = AvatarTraits::NULL_TRAIT_VERSION, - AvatarTraits::TraitInstanceID wireInstanceID = AvatarTraits::TraitInstanceID()); + ExtendedIODevice& destination, AvatarTraits::TraitVersion traitVersion = AvatarTraits::NULL_TRAIT_VERSION); void prepareResetTraitInstances(); diff --git a/libraries/avatars/src/AvatarTraits.h b/libraries/avatars/src/AvatarTraits.h index 5866caf234..f0c807a432 100644 --- a/libraries/avatars/src/AvatarTraits.h +++ b/libraries/avatars/src/AvatarTraits.h @@ -41,7 +41,7 @@ namespace AvatarTraits { const TraitWireSize DELETED_TRAIT_SIZE = -1; inline qint64 packInstancedTraitDelete(TraitType traitType, TraitInstanceID instanceID, ExtendedIODevice& destination, - TraitVersion traitVersion = NULL_TRAIT_VERSION) { + TraitVersion traitVersion = NULL_TRAIT_VERSION) { qint64 bytesWritten = 0; bytesWritten += destination.writePrimitive(traitType); diff --git a/libraries/avatars/src/ClientTraitsHandler.cpp b/libraries/avatars/src/ClientTraitsHandler.cpp index 94823610dc..a06b53da7c 100644 --- a/libraries/avatars/src/ClientTraitsHandler.cpp +++ b/libraries/avatars/src/ClientTraitsHandler.cpp @@ -43,9 +43,6 @@ void ClientTraitsHandler::resetForNewMixer() { // pre-fill the instanced statuses that we will need to send next frame _owningAvatar->prepareResetTraitInstances(); - - // reset the trait XOR ID since we're resetting for a new avatar mixer - _sessionXORID = QUuid::createUuid().toRfc4122(); } void ClientTraitsHandler::sendChangedTraitsToMixer() { @@ -96,11 +93,7 @@ void ClientTraitsHandler::sendChangedTraitsToMixer() { || instanceIDValuePair.value == Updated) { // this is a changed trait we need to send or we haven't send out trait information yet // ask the owning avatar to pack it - - // since this is going to the mixer, use the XORed instance ID (to anonymize trait instance IDs - // that would typically persist across sessions) - _owningAvatar->packTraitInstance(instancedIt->traitType, instanceIDValuePair.id, *traitsPacketList, - AvatarTraits::NULL_TRAIT_VERSION, xorInstanceID(instanceIDValuePair.id)); + _owningAvatar->packTraitInstance(instancedIt->traitType, instanceIDValuePair.id, *traitsPacketList); } else if (!_shouldPerformInitialSend && instanceIDValuePair.value == Deleted) { // pack delete for this trait instance AvatarTraits::packInstancedTraitDelete(instancedIt->traitType, instanceIDValuePair.id, @@ -118,17 +111,6 @@ void ClientTraitsHandler::sendChangedTraitsToMixer() { } } -AvatarTraits::TraitInstanceID ClientTraitsHandler::xorInstanceID(AvatarTraits::TraitInstanceID localInstanceID) { - QByteArray xorInstanceID { NUM_BYTES_RFC4122_UUID, 0 }; - auto localInstanceIDBytes = localInstanceID.toRfc4122(); - - for (auto i = 0; i < localInstanceIDBytes.size(); ++i) { - xorInstanceID[i] = localInstanceIDBytes[i] ^ _sessionXORID[i]; - } - - return QUuid::fromRfc4122(xorInstanceID); -} - void ClientTraitsHandler::processTraitOverride(QSharedPointer message, SharedNodePointer sendingNode) { if (sendingNode->getType() == NodeType::AvatarMixer) { while (message->getBytesLeftToRead()) { diff --git a/libraries/avatars/src/ClientTraitsHandler.h b/libraries/avatars/src/ClientTraitsHandler.h index 2319061502..27ba58d46b 100644 --- a/libraries/avatars/src/ClientTraitsHandler.h +++ b/libraries/avatars/src/ClientTraitsHandler.h @@ -30,15 +30,13 @@ public: void markTraitUpdated(AvatarTraits::TraitType updatedTrait) { _traitStatuses[updatedTrait] = Updated; _hasChangedTraits = true; } - void markInstancedTraitUpdated(AvatarTraits::TraitType traitType, AvatarTraits::TraitInstanceID updatedInstanceID) + void markInstancedTraitUpdated(AvatarTraits::TraitType traitType, QUuid updatedInstanceID) { _traitStatuses.instanceInsert(traitType, updatedInstanceID, Updated); _hasChangedTraits = true; } - void markInstancedTraitDeleted(AvatarTraits::TraitType traitType, AvatarTraits::TraitInstanceID deleteInstanceID) + void markInstancedTraitDeleted(AvatarTraits::TraitType traitType, QUuid deleteInstanceID) { _traitStatuses.instanceInsert(traitType, deleteInstanceID, Deleted); _hasChangedTraits = true; } void resetForNewMixer(); - AvatarTraits::TraitInstanceID xorInstanceID(AvatarTraits::TraitInstanceID localInstanceID); - public slots: void processTraitOverride(QSharedPointer message, SharedNodePointer sendingNode); @@ -55,8 +53,6 @@ private: AvatarTraits::TraitVersion _currentTraitVersion { AvatarTraits::DEFAULT_TRAIT_VERSION }; AvatarTraits::TraitVersion _currentSkeletonVersion { AvatarTraits::NULL_TRAIT_VERSION }; - - QByteArray _sessionXORID { NUM_BYTES_RFC4122_UUID, 0}; bool _shouldPerformInitialSend { false }; bool _hasChangedTraits { false }; From b1f44d2ad7c82f085881f8e243f75cf06f272167 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 12 Sep 2018 12:30:15 -0700 Subject: [PATCH 05/13] perform avatar entity cleanup in AvatarManager::handleRemovedAvatar --- interface/src/avatar/AvatarManager.cpp | 5 ++++ .../src/avatars-renderer/Avatar.cpp | 24 ++++++++++--------- .../src/avatars-renderer/Avatar.h | 1 + 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index b9e30a38eb..443d19e473 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -443,6 +443,11 @@ void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar avatar->die(); queuePhysicsChange(avatar); + // remove this avatar's entities from the tree now, if we wait (as we did previously) for this Avatar's destructor + // it might not fire until after we create a new instance for the same remote avatar, which creates a race + // on the creation of entities for that avatar instance and the deletion of entities for this instance + avatar->removeAvatarEntitiesFromTree(); + if (removalReason == KillAvatarReason::TheirAvatarEnteredYourBubble) { emit DependencyManager::get()->enteredIgnoreRadius(); } else if (removalReason == KillAvatarReason::AvatarDisconnected) { diff --git a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp index cadc3b31e3..914a3b7c6e 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp +++ b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp @@ -131,17 +131,6 @@ Avatar::Avatar(QThread* thread) : } Avatar::~Avatar() { - auto treeRenderer = DependencyManager::get(); - EntityTreePointer entityTree = treeRenderer ? treeRenderer->getTree() : nullptr; - if (entityTree) { - entityTree->withWriteLock([&] { - AvatarEntityMap avatarEntities = getAvatarEntityData(); - for (auto entityID : avatarEntities.keys()) { - entityTree->deleteEntity(entityID, true, true); - } - }); - } - auto geometryCache = DependencyManager::get(); if (geometryCache) { geometryCache->releaseID(_nameRectGeometryID); @@ -386,6 +375,19 @@ void Avatar::updateAvatarEntities() { setAvatarEntityDataChanged(false); } +void Avatar::removeAvatarEntitiesFromTree() { + auto treeRenderer = DependencyManager::get(); + EntityTreePointer entityTree = treeRenderer ? treeRenderer->getTree() : nullptr; + if (entityTree) { + entityTree->withWriteLock([&] { + AvatarEntityMap avatarEntities = getAvatarEntityData(); + for (auto entityID : avatarEntities.keys()) { + entityTree->deleteEntity(entityID, true, true); + } + }); + } +} + void Avatar::relayJointDataToChildren() { forEachChild([&](SpatiallyNestablePointer child) { if (child->getNestableType() == NestableType::Entity) { diff --git a/libraries/avatars-renderer/src/avatars-renderer/Avatar.h b/libraries/avatars-renderer/src/avatars-renderer/Avatar.h index 3482c3c193..4f1c010d84 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/Avatar.h +++ b/libraries/avatars-renderer/src/avatars-renderer/Avatar.h @@ -73,6 +73,7 @@ public: void init(); void updateAvatarEntities(); + void removeAvatarEntitiesFromTree(); void simulate(float deltaTime, bool inView); virtual void simulateAttachments(float deltaTime); From 9a435cdca2965daf13f896c7b4aef5661497ff42 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Thu, 6 Sep 2018 15:08:46 -0700 Subject: [PATCH 06/13] Reduce needless smart pointer duplication --- libraries/gpu-gl-common/src/gpu/gl/GLBackendInput.cpp | 8 ++++---- libraries/gpu-gl-common/src/gpu/gl/GLBackendPipeline.cpp | 4 ++-- libraries/gpu-gl-common/src/gpu/gl/GLBackendTexture.cpp | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLBackendInput.cpp b/libraries/gpu-gl-common/src/gpu/gl/GLBackendInput.cpp index 8d46b4c6e3..ee094a2d2c 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLBackendInput.cpp +++ b/libraries/gpu-gl-common/src/gpu/gl/GLBackendInput.cpp @@ -17,7 +17,7 @@ using namespace gpu; using namespace gpu::gl; void GLBackend::do_setInputFormat(const Batch& batch, size_t paramOffset) { - Stream::FormatPointer format = batch._streamFormats.get(batch._params[paramOffset]._uint); + const auto& format = batch._streamFormats.get(batch._params[paramOffset]._uint); if (format != _input._format) { _input._format = format; if (format) { @@ -37,7 +37,7 @@ void GLBackend::do_setInputFormat(const Batch& batch, size_t paramOffset) { void GLBackend::do_setInputBuffer(const Batch& batch, size_t paramOffset) { Offset stride = batch._params[paramOffset + 0]._uint; Offset offset = batch._params[paramOffset + 1]._uint; - BufferPointer buffer = batch._buffers.get(batch._params[paramOffset + 2]._uint); + const auto& buffer = batch._buffers.get(batch._params[paramOffset + 2]._uint); uint32 channel = batch._params[paramOffset + 3]._uint; if (channel < getNumInputBuffers()) { @@ -119,7 +119,7 @@ void GLBackend::do_setIndexBuffer(const Batch& batch, size_t paramOffset) { _input._indexBufferType = (Type)batch._params[paramOffset + 2]._uint; _input._indexBufferOffset = batch._params[paramOffset + 0]._uint; - BufferPointer indexBuffer = batch._buffers.get(batch._params[paramOffset + 1]._uint); + const auto& indexBuffer = batch._buffers.get(batch._params[paramOffset + 1]._uint); if (indexBuffer != _input._indexBuffer) { _input._indexBuffer = indexBuffer; if (indexBuffer) { @@ -136,7 +136,7 @@ void GLBackend::do_setIndirectBuffer(const Batch& batch, size_t paramOffset) { _input._indirectBufferOffset = batch._params[paramOffset + 1]._uint; _input._indirectBufferStride = batch._params[paramOffset + 2]._uint; - BufferPointer buffer = batch._buffers.get(batch._params[paramOffset]._uint); + const auto& buffer = batch._buffers.get(batch._params[paramOffset]._uint); if (buffer != _input._indirectBuffer) { _input._indirectBuffer = buffer; if (buffer) { diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLBackendPipeline.cpp b/libraries/gpu-gl-common/src/gpu/gl/GLBackendPipeline.cpp index 05ded3eece..7e54774df6 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLBackendPipeline.cpp +++ b/libraries/gpu-gl-common/src/gpu/gl/GLBackendPipeline.cpp @@ -23,7 +23,7 @@ using namespace gpu; using namespace gpu::gl; void GLBackend::do_setPipeline(const Batch& batch, size_t paramOffset) { - PipelinePointer pipeline = batch._pipelines.get(batch._params[paramOffset + 0]._uint); + const auto& pipeline = batch._pipelines.get(batch._params[paramOffset + 0]._uint); if (_pipeline._pipeline == pipeline) { return; @@ -193,7 +193,7 @@ void GLBackend::do_setUniformBuffer(const Batch& batch, size_t paramOffset) { return; } - BufferPointer uniformBuffer = batch._buffers.get(batch._params[paramOffset + 2]._uint); + const auto& uniformBuffer = batch._buffers.get(batch._params[paramOffset + 2]._uint); GLintptr rangeStart = batch._params[paramOffset + 1]._uint; GLsizeiptr rangeSize = batch._params[paramOffset + 0]._uint; diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLBackendTexture.cpp b/libraries/gpu-gl-common/src/gpu/gl/GLBackendTexture.cpp index ca4e328612..f4fb3fcf2c 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLBackendTexture.cpp +++ b/libraries/gpu-gl-common/src/gpu/gl/GLBackendTexture.cpp @@ -66,7 +66,7 @@ GLTexture* GLBackend::syncGPUObject(const TexturePointer& texturePointer) { } void GLBackend::do_generateTextureMips(const Batch& batch, size_t paramOffset) { - TexturePointer resourceTexture = batch._textures.get(batch._params[paramOffset + 0]._uint); + const auto& resourceTexture = batch._textures.get(batch._params[paramOffset + 0]._uint); if (!resourceTexture) { return; } From cfff28ad0af99250319c7471a75f2d85c79ebdd2 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Thu, 6 Sep 2018 16:03:43 -0700 Subject: [PATCH 07/13] More shared pointer deduplication --- .../gpu-gl-common/src/gpu/gl/GLBackend.cpp | 9 +- .../gpu-gl-common/src/gpu/gl/GLBackend.h | 314 +++++++++++++----- .../src/gpu/gl/GLBackendInput.cpp | 31 +- .../src/gpu/gl/GLBackendOutput.cpp | 25 +- .../src/gpu/gl/GLBackendPipeline.cpp | 54 +-- .../gpu-gl-common/src/gpu/gl/GLPipeline.cpp | 4 +- libraries/gpu-gl/src/gpu/gl41/GL41Backend.h | 2 +- .../gpu-gl/src/gpu/gl41/GL41BackendBuffer.cpp | 13 +- .../gpu-gl/src/gpu/gl41/GL41BackendInput.cpp | 13 +- libraries/gpu-gl/src/gpu/gl45/GL45Backend.h | 2 +- .../gpu-gl/src/gpu/gl45/GL45BackendBuffer.cpp | 8 +- .../gpu-gl/src/gpu/gl45/GL45BackendInput.cpp | 9 +- libraries/gpu-gles/src/gpu/gles/GLESBackend.h | 2 +- .../src/gpu/gles/GLESBackendBuffer.cpp | 6 +- libraries/gpu/src/gpu/Batch.cpp | 2 +- libraries/gpu/src/gpu/Batch.h | 2 +- libraries/gpu/src/gpu/Framebuffer.cpp | 10 +- libraries/gpu/src/gpu/Framebuffer.h | 6 +- 18 files changed, 330 insertions(+), 182 deletions(-) diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLBackend.cpp b/libraries/gpu-gl-common/src/gpu/gl/GLBackend.cpp index c1848d99b1..30a3cf0aaf 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLBackend.cpp +++ b/libraries/gpu-gl-common/src/gpu/gl/GLBackend.cpp @@ -150,6 +150,7 @@ void GLBackend::init() { }); } + GLBackend::GLBackend(bool syncCache) { _pipeline._cameraCorrectionBuffer._buffer->flush(); initShaderBinaryCache(); @@ -201,9 +202,10 @@ void GLBackend::renderPassTransfer(const Batch& batch) { { Vec2u outputSize{ 1,1 }; - if (_output._framebuffer) { - outputSize.x = _output._framebuffer->getWidth(); - outputSize.y = _output._framebuffer->getHeight(); + auto framebuffer = acquire(_output._framebuffer); + if (framebuffer) { + outputSize.x = framebuffer->getWidth(); + outputSize.y = framebuffer->getHeight(); } else if (glm::dot(_transform._projectionJitter, _transform._projectionJitter)>0.0f) { qCWarning(gpugllogging) << "Jittering needs to have a frame buffer to be set"; } @@ -220,6 +222,7 @@ void GLBackend::renderPassTransfer(const Batch& batch) { _stereo._contextDisable = false; break; + case Batch::COMMAND_setFramebuffer: case Batch::COMMAND_setViewportTransform: case Batch::COMMAND_setViewTransform: case Batch::COMMAND_setProjectionTransform: diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLBackend.h b/libraries/gpu-gl-common/src/gpu/gl/GLBackend.h index 9b3a28e6fd..7927734256 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLBackend.h +++ b/libraries/gpu-gl-common/src/gpu/gl/GLBackend.h @@ -30,7 +30,6 @@ #include "GLShared.h" - // Different versions for the stereo drawcall // Current preferred is "instanced" which draw the shape twice but instanced and rely on clipping plane to draw left/right side only #if defined(USE_GLES) @@ -40,7 +39,6 @@ #define GPU_STEREO_TECHNIQUE_INSTANCED #endif - // Let these be configured by the one define picked above #ifdef GPU_STEREO_TECHNIQUE_DOUBLED_SIMPLE #define GPU_STEREO_DRAWCALL_DOUBLED @@ -56,8 +54,151 @@ #define GPU_STEREO_CAMERA_BUFFER #endif +// +// GL Backend pointer storage mechanism +// One of the following three defines must be uncommented. + +// Equivalent to current state of affairs in master, +// Works pretty well, but ends up creating a lot of needless smart pointer duplication +// which means there's a high aggregate cost of manipulating std::shared_ptr counters +//#define GPU_POINTER_STORAGE_SHARED + +// The platonic ideal, use references to smart pointers. +// However, this produces artifacts because there are too many places in the code right now that +// create temporary values (undesirable smart pointer duplications) and then those temp variables +// get passed on and have their reference taken, and then invalidated +//#define GPU_POINTER_STORAGE_REF + +// Raw pointer manipulation. Seems more dangerous than the reference wrappers, +// but in practice, the danger of grabbing a reference to a temporary variable +// is causing issues +#define GPU_POINTER_STORAGE_RAW + namespace gpu { namespace gl { +#if defined(GPU_POINTER_STORAGE_SHARED) +template +static inline bool compare(const std::shared_ptr& a, const std::shared_ptr& b) { + return a == b; +} + +template +static inline T* acquire(const std::shared_ptr& pointer) { + return pointer.get(); +} + +template +static inline void reset(std::shared_ptr& pointer) { + return pointer.reset(); +} + +template +static inline bool valid(const std::shared_ptr& pointer) { + return pointer.operator bool(); +} + +template +static inline void assign(std::shared_ptr&pointer, const std::shared_ptr& source) { + pointer = source; +} + +using BufferReference = BufferPointer; +using TextureReference = TexturePointer; +using FramebufferReference = FramebufferPointer; +using FormatReference = Stream::FormatPointer; +using PipelineReference = PipelinePointer; + +#define GPU_REFERENCE_INIT_VALUE nullptr + +#elif defined(GPU_POINTER_STORAGE_REF) + +template +class PointerReferenceWrapper : public std::reference_wrapper> { + using Parent = std::reference_wrapper>; + +public: + using Pointer = std::shared_ptr; + PointerReferenceWrapper() : Parent(EMPTY()) {} + PointerReferenceWrapper(const Pointer& pointer) : Parent(pointer) {} + void clear() { *this = EMPTY(); } + +private: + static const Pointer& EMPTY() { + static const Pointer EMPTY_VALUE; + return EMPTY_VALUE; + }; +}; + +template +static bool compare(const PointerReferenceWrapper& reference, const std::shared_ptr& pointer) { + return reference.get() == pointer; +} + +template +static inline T* acquire(const PointerReferenceWrapper& reference) { + return reference.get().get(); +} + +template +static void assign(PointerReferenceWrapper& reference, const std::shared_ptr& pointer) { + reference = pointer; +} + +template +static bool valid(const PointerReferenceWrapper& reference) { + return reference.get().operator bool(); +} + +template +static inline void reset(PointerReferenceWrapper& reference) { + return reference.clear(); +} + +using BufferReference = PointerReferenceWrapper; +using TextureReference = PointerReferenceWrapper; +using FramebufferReference = PointerReferenceWrapper; +using FormatReference = PointerReferenceWrapper; +using PipelineReference = PointerReferenceWrapper; + +#define GPU_REFERENCE_INIT_VALUE + +#elif defined(GPU_POINTER_STORAGE_RAW) + +template +static bool compare(const T*const& rawPointer, const std::shared_ptr& pointer) { + return rawPointer == pointer.get(); +} + +template +static inline T* acquire(T*& rawPointer) { + return rawPointer; +} + +template +static inline bool valid(const T*const& rawPointer) { + return rawPointer; +} + +template +static inline void reset(T*& rawPointer) { + rawPointer = nullptr; +} + +template +static inline void assign(T*& rawPointer, const std::shared_ptr& pointer) { + rawPointer = pointer.get(); +} + +using BufferReference = Buffer*; +using TextureReference = Texture*; +using FramebufferReference = Framebuffer*; +using FormatReference = Stream::Format*; +using PipelineReference = Pipeline*; + +#define GPU_REFERENCE_INIT_VALUE nullptr + +#endif + class GLBackend : public Backend, public std::enable_shared_from_this { // Context Backend static interface required friend class gpu::Context; @@ -67,8 +208,9 @@ class GLBackend : public Backend, public std::enable_shared_from_this protected: explicit GLBackend(bool syncCache); GLBackend(); -public: + +public: #if defined(USE_GLES) // https://www.khronos.org/registry/OpenGL-Refpages/es3/html/glGet.xhtml static const GLint MIN_REQUIRED_TEXTURE_IMAGE_UNITS = 16; @@ -109,8 +251,8 @@ public: // This is the ugly "download the pixels to sysmem for taking a snapshot" // Just avoid using it, it's ugly and will break performances virtual void downloadFramebuffer(const FramebufferPointer& srcFramebuffer, - const Vec4i& region, QImage& destImage) final override; - + const Vec4i& region, + QImage& destImage) final override; // this is the maximum numeber of available input buffers size_t getNumInputBuffers() const { return _input._invalidBuffers.size(); } @@ -131,7 +273,6 @@ public: static const int MAX_NUM_RESOURCE_TABLE_TEXTURES = 2; size_t getMaxNumResourceTextureTables() const { return MAX_NUM_RESOURCE_TABLE_TEXTURES; } - // Draw Stage virtual void do_draw(const Batch& batch, size_t paramOffset) = 0; virtual void do_drawIndexed(const Batch& batch, size_t paramOffset) = 0; @@ -183,7 +324,6 @@ public: // Reset stages virtual void do_resetStages(const Batch& batch, size_t paramOffset) final; - virtual void do_disableContextViewCorrection(const Batch& batch, size_t paramOffset) final; virtual void do_restoreContextViewCorrection(const Batch& batch, size_t paramOffset) final; @@ -203,7 +343,7 @@ public: virtual void do_popProfileRange(const Batch& batch, size_t paramOffset) final; // TODO: As long as we have gl calls explicitely issued from interface - // code, we need to be able to record and batch these calls. THe long + // code, we need to be able to record and batch these calls. THe long // term strategy is to get rid of any GL calls in favor of the HIFI GPU API virtual void do_glUniform1i(const Batch& batch, size_t paramOffset) final; virtual void do_glUniform1f(const Batch& batch, size_t paramOffset) final; @@ -228,7 +368,9 @@ public: virtual void do_setStateAntialiasedLineEnable(bool enable) final; virtual void do_setStateDepthBias(Vec2 bias) final; virtual void do_setStateDepthTest(State::DepthTest test) final; - virtual void do_setStateStencil(State::StencilActivation activation, State::StencilTest frontTest, State::StencilTest backTest) final; + virtual void do_setStateStencil(State::StencilActivation activation, + State::StencilTest frontTest, + State::StencilTest backTest) final; virtual void do_setStateAlphaToCoverageEnable(bool enable) final; virtual void do_setStateSampleMask(uint32 mask) final; virtual void do_setStateBlend(State::BlendFunction blendFunction) final; @@ -257,7 +399,9 @@ public: virtual void releaseQuery(GLuint id) const; virtual void queueLambda(const std::function lambda) const; - bool isTextureManagementSparseEnabled() const override { return (_textureManagement._sparseCapable && Texture::getEnableSparseTextures()); } + bool isTextureManagementSparseEnabled() const override { + return (_textureManagement._sparseCapable && Texture::getEnableSparseTextures()); + } protected: virtual GLint getRealUniformLocation(GLint location) const; @@ -266,11 +410,11 @@ protected: // FIXME instead of a single flag, create a features struct similar to // https://www.khronos.org/registry/vulkan/specs/1.0/man/html/VkPhysicalDeviceFeatures.html - virtual bool supportsBindless() const { return false; } + virtual bool supportsBindless() const { return false; } static const size_t INVALID_OFFSET = (size_t)-1; - bool _inRenderTransferPass { false }; - int _currentDraw { -1 }; + bool _inRenderTransferPass{ false }; + int _currentDraw{ -1 }; std::list profileRanges; mutable Mutex _trashMutex; @@ -299,46 +443,42 @@ protected: virtual void updateInput() = 0; struct InputStageState { - bool _invalidFormat { true }; - bool _lastUpdateStereoState{ false }; + bool _invalidFormat{ true }; + bool _lastUpdateStereoState{ false }; bool _hadColorAttribute{ true }; - Stream::FormatPointer _format; + FormatReference _format{ GPU_REFERENCE_INIT_VALUE }; std::string _formatKey; typedef std::bitset ActivationCache; - ActivationCache _attributeActivation { 0 }; + ActivationCache _attributeActivation{ 0 }; typedef std::bitset BuffersState; BuffersState _invalidBuffers{ 0 }; BuffersState _attribBindingBuffers{ 0 }; - Buffers _buffers; - Offsets _bufferOffsets; - Offsets _bufferStrides; - std::vector _bufferVBOs; + std::vector _buffers{ MAX_NUM_INPUT_BUFFERS, GPU_REFERENCE_INIT_VALUE }; + Offsets _bufferOffsets; + Offsets _bufferStrides; + std::vector _bufferVBOs; glm::vec4 _colorAttribute{ 0.0f }; - BufferPointer _indexBuffer; - Offset _indexBufferOffset { 0 }; - Type _indexBufferType { UINT32 }; - - BufferPointer _indirectBuffer; + BufferReference _indexBuffer{ GPU_REFERENCE_INIT_VALUE }; + Offset _indexBufferOffset{ 0 }; + Type _indexBufferType{ UINT32 }; + + BufferReference _indirectBuffer{ GPU_REFERENCE_INIT_VALUE }; Offset _indirectBufferOffset{ 0 }; Offset _indirectBufferStride{ 0 }; - GLuint _defaultVAO { 0 }; + GLuint _defaultVAO{ 0 }; - InputStageState() : - _invalidFormat(true), - _format(0), - _formatKey(), - _attributeActivation(0), - _buffers(_invalidBuffers.size(), BufferPointer(0)), - _bufferOffsets(_invalidBuffers.size(), 0), - _bufferStrides(_invalidBuffers.size(), 0), - _bufferVBOs(_invalidBuffers.size(), 0) {} + InputStageState() { + _bufferOffsets.resize(MAX_NUM_INPUT_BUFFERS, 0); + _bufferStrides.resize(MAX_NUM_INPUT_BUFFERS, 0); + _bufferVBOs.resize(MAX_NUM_INPUT_BUFFERS, 0); + } } _input; virtual void initTransform() = 0; @@ -349,7 +489,7 @@ protected: virtual void resetTransformStage(); // Allows for correction of the camera pose to account for changes - // between the time when a was recorded and the time(s) when it is + // between the time when a was recorded and the time(s) when it is // executed // Prev is the previous correction used at previous frame struct CameraCorrection { @@ -364,9 +504,12 @@ protected: struct Cameras { TransformCamera _cams[2]; - Cameras() {}; + Cameras(){}; Cameras(const TransformCamera& cam) { memcpy(_cams, &cam, sizeof(TransformCamera)); }; - Cameras(const TransformCamera& camL, const TransformCamera& camR) { memcpy(_cams, &camL, sizeof(TransformCamera)); memcpy(_cams + 1, &camR, sizeof(TransformCamera)); }; + Cameras(const TransformCamera& camL, const TransformCamera& camR) { + memcpy(_cams, &camL, sizeof(TransformCamera)); + memcpy(_cams + 1, &camR, sizeof(TransformCamera)); + }; }; using CameraBufferElement = Cameras; @@ -380,25 +523,24 @@ protected: mutable std::map _drawCallInfoOffsets; - GLuint _objectBuffer { 0 }; - GLuint _cameraBuffer { 0 }; - GLuint _drawCallInfoBuffer { 0 }; - GLuint _objectBufferTexture { 0 }; - size_t _cameraUboSize { 0 }; + GLuint _objectBuffer{ 0 }; + GLuint _cameraBuffer{ 0 }; + GLuint _drawCallInfoBuffer{ 0 }; + GLuint _objectBufferTexture{ 0 }; + size_t _cameraUboSize{ 0 }; bool _viewIsCamera{ false }; - bool _skybox { false }; + bool _skybox{ false }; Transform _view; CameraCorrection _correction; bool _viewCorrectionEnabled{ true }; - Mat4 _projection; - Vec4i _viewport { 0, 0, 1, 1 }; - Vec2 _depthRange { 0.0f, 1.0f }; + Vec4i _viewport{ 0, 0, 1, 1 }; + Vec2 _depthRange{ 0.0f, 1.0f }; Vec2 _projectionJitter{ 0.0f, 0.0f }; - bool _invalidView { false }; - bool _invalidProj { false }; - bool _invalidViewport { false }; + bool _invalidView{ false }; + bool _invalidProj{ false }; + bool _invalidViewport{ false }; bool _enabledDrawcallInfoBuffer{ false }; @@ -417,44 +559,47 @@ protected: struct UniformStageState { struct BufferState { - BufferPointer buffer; + BufferReference buffer{ GPU_REFERENCE_INIT_VALUE }; GLintptr offset{ 0 }; GLsizeiptr size{ 0 }; - BufferState(const BufferPointer& buffer = nullptr, GLintptr offset = 0, GLsizeiptr size = 0); - bool operator ==(BufferState& other) const { - return offset == other.offset && size == other.size && buffer == other.buffer; + //BufferState(const BufferPointer& buffer = nullptr, GLintptr offset = 0, GLsizeiptr size = 0); + + BufferState& operator=(const BufferState& other) = delete; + void reset() { gpu::gl::reset(buffer); offset = 0; size = 0; } + bool compare(const BufferPointer& buffer, GLintptr offset, GLsizeiptr size) { + const auto& self = *this; + return (self.offset == offset && self.size == size && gpu::gl::compare(self.buffer, buffer)); } }; // MAX_NUM_UNIFORM_BUFFERS-1 is the max uniform index BATCHES are allowed to set, but - // MIN_REQUIRED_UNIFORM_BUFFER_BINDINGS is used here because the backend sets some - // internal UBOs for things like camera correction + // MIN_REQUIRED_UNIFORM_BUFFER_BINDINGS is used here because the backend sets some + // internal UBOs for things like camera correction std::array _buffers; } _uniform; - // Helper function that provides common code + // Helper function that provides common code void bindUniformBuffer(uint32_t slot, const BufferPointer& buffer, GLintptr offset = 0, GLsizeiptr size = 0); void releaseUniformBuffer(uint32_t slot); void resetUniformStage(); // update resource cache and do the gl bind/unbind call with the current gpu::Buffer cached at slot s // This is using different gl object depending on the gl version - virtual bool bindResourceBuffer(uint32_t slot, BufferPointer& buffer) = 0; + virtual bool bindResourceBuffer(uint32_t slot, const BufferPointer& buffer) = 0; virtual void releaseResourceBuffer(uint32_t slot) = 0; - // Helper function that provides common code used by do_setResourceTexture and + // Helper function that provides common code used by do_setResourceTexture and // do_setResourceTextureTable (in non-bindless mode) void bindResourceTexture(uint32_t slot, const TexturePointer& texture); - // update resource cache and do the gl unbind call with the current gpu::Texture cached at slot s void releaseResourceTexture(uint32_t slot); void resetResourceStage(); struct ResourceStageState { - std::array _buffers; - std::array _textures; + std::vector _buffers{ MAX_NUM_RESOURCE_BUFFERS, BufferReference() }; + std::vector _textures{ MAX_NUM_RESOURCE_TEXTURES, GPU_REFERENCE_INIT_VALUE }; //Textures _textures { { MAX_NUM_RESOURCE_TEXTURES } }; int findEmptyTextureSlot() const; } _resource; @@ -470,21 +615,22 @@ protected: void resetPipelineStage(); struct PipelineStageState { - PipelinePointer _pipeline; + PipelineReference _pipeline{ GPU_REFERENCE_INIT_VALUE }; - GLuint _program { 0 }; - bool _cameraCorrection { false }; - GLShader* _programShader { nullptr }; - bool _invalidProgram { false }; + GLuint _program{ 0 }; + bool _cameraCorrection{ false }; + GLShader* _programShader{ nullptr }; + bool _invalidProgram{ false }; - BufferView _cameraCorrectionBuffer { gpu::BufferView(std::make_shared(sizeof(CameraCorrection), nullptr )) }; - BufferView _cameraCorrectionBufferIdentity { gpu::BufferView(std::make_shared(sizeof(CameraCorrection), nullptr )) }; + BufferView _cameraCorrectionBuffer{ gpu::BufferView(std::make_shared(sizeof(CameraCorrection), nullptr)) }; + BufferView _cameraCorrectionBufferIdentity{ gpu::BufferView( + std::make_shared(sizeof(CameraCorrection), nullptr)) }; State::Data _stateCache{ State::DEFAULT }; - State::Signature _stateSignatureCache { 0 }; + State::Signature _stateSignatureCache{ 0 }; - GLState* _state { nullptr }; - bool _invalidState { false }; + GLState* _state{ nullptr }; + bool _invalidState{ false }; PipelineStageState() { _cameraCorrectionBuffer.edit() = CameraCorrection(); @@ -498,9 +644,9 @@ protected: virtual GLShader* compileBackendProgram(const Shader& program, const Shader::CompilationHandler& handler); virtual GLShader* compileBackendShader(const Shader& shader, const Shader::CompilationHandler& handler); virtual std::string getBackendShaderHeader() const = 0; - // For a program, this will return a string containing all the source files (without any - // backend headers or defines). For a vertex, fragment or geometry shader, this will - // return the fully customized shader with all the version and backend specific + // For a program, this will return a string containing all the source files (without any + // backend headers or defines). For a vertex, fragment or geometry shader, this will + // return the fully customized shader with all the version and backend specific // preprocessor directives // The program string returned can be used as a key for a cache of shader binaries // The shader strings can be reliably sent to the low level `compileShader` functions @@ -516,22 +662,22 @@ protected: // Synchronize the state cache of this Backend with the actual real state of the GL Context void syncOutputStateCache(); void resetOutputStage(); - + struct OutputStageState { - FramebufferPointer _framebuffer { nullptr }; - GLuint _drawFBO { 0 }; + FramebufferReference _framebuffer{ GPU_REFERENCE_INIT_VALUE }; + GLuint _drawFBO{ 0 }; } _output; void resetQueryStage(); struct QueryStageState { - uint32_t _rangeQueryDepth { 0 }; + uint32_t _rangeQueryDepth{ 0 }; } _queryStage; void resetStages(); // Stores cached binary versions of the shaders for quicker startup on subsequent runs - // Note that shaders in the cache can still fail to load due to hardware or driver - // changes that invalidate the cached binary, in which case we fall back on compiling + // Note that shaders in the cache can still fail to load due to hardware or driver + // changes that invalidate the cached binary, in which case we fall back on compiling // the source again struct ShaderBinaryCache { std::mutex _mutex; @@ -543,7 +689,7 @@ protected: virtual void killShaderBinaryCache(); struct TextureManagementStageState { - bool _sparseCapable { false }; + bool _sparseCapable{ false }; GLTextureTransferEnginePointer _transferEngine; } _textureManagement; virtual void initTextureManagementStage(); @@ -556,6 +702,6 @@ protected: friend class GLShader; }; -} } +}} // namespace gpu::gl #endif diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLBackendInput.cpp b/libraries/gpu-gl-common/src/gpu/gl/GLBackendInput.cpp index ee094a2d2c..219efae866 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLBackendInput.cpp +++ b/libraries/gpu-gl-common/src/gpu/gl/GLBackendInput.cpp @@ -18,8 +18,8 @@ using namespace gpu::gl; void GLBackend::do_setInputFormat(const Batch& batch, size_t paramOffset) { const auto& format = batch._streamFormats.get(batch._params[paramOffset]._uint); - if (format != _input._format) { - _input._format = format; + if (!compare(_input._format, format)) { + assign(_input._format, format); if (format) { auto inputFormat = GLInputFormat::sync((*format)); assert(inputFormat); @@ -42,8 +42,8 @@ void GLBackend::do_setInputBuffer(const Batch& batch, size_t paramOffset) { if (channel < getNumInputBuffers()) { bool isModified = false; - if (_input._buffers[channel] != buffer) { - _input._buffers[channel] = buffer; + if (!compare(_input._buffers[channel], buffer)) { + assign(_input._buffers[channel], buffer); _input._bufferVBOs[channel] = getBufferIDUnsynced((*buffer)); isModified = true; } @@ -94,18 +94,18 @@ void GLBackend::resetInputStage() { // Reset index buffer _input._indexBufferType = UINT32; _input._indexBufferOffset = 0; - _input._indexBuffer.reset(); + reset(_input._indexBuffer); glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0); (void) CHECK_GL_ERROR(); // Reset vertex buffer and format - _input._format.reset(); + reset(_input._format); _input._formatKey.clear(); _input._invalidFormat = false; _input._attributeActivation.reset(); for (uint32_t i = 0; i < _input._buffers.size(); i++) { - _input._buffers[i].reset(); + reset(_input._buffers[i]); _input._bufferOffsets[i] = 0; _input._bufferStrides[i] = 0; _input._bufferVBOs[i] = 0; @@ -120,8 +120,8 @@ void GLBackend::do_setIndexBuffer(const Batch& batch, size_t paramOffset) { _input._indexBufferOffset = batch._params[paramOffset + 0]._uint; const auto& indexBuffer = batch._buffers.get(batch._params[paramOffset + 1]._uint); - if (indexBuffer != _input._indexBuffer) { - _input._indexBuffer = indexBuffer; + if (!compare(_input._indexBuffer, indexBuffer)) { + assign(_input._indexBuffer, indexBuffer); if (indexBuffer) { glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, getBufferIDUnsynced(*indexBuffer)); } else { @@ -137,8 +137,8 @@ void GLBackend::do_setIndirectBuffer(const Batch& batch, size_t paramOffset) { _input._indirectBufferStride = batch._params[paramOffset + 2]._uint; const auto& buffer = batch._buffers.get(batch._params[paramOffset]._uint); - if (buffer != _input._indirectBuffer) { - _input._indirectBuffer = buffer; + if (!compare(_input._indirectBuffer, buffer)) { + assign(_input._indirectBuffer, buffer); if (buffer) { glBindBuffer(GL_DRAW_INDIRECT_BUFFER, getBufferIDUnsynced(*buffer)); } else { @@ -152,7 +152,7 @@ void GLBackend::do_setIndirectBuffer(const Batch& batch, size_t paramOffset) { void GLBackend::updateInput() { bool isStereoNow = isStereo(); - // track stereo state change potentially happening wihtout changing the input format + // track stereo state change potentially happening without changing the input format // this is a rare case requesting to invalid the format #ifdef GPU_STEREO_DRAWCALL_INSTANCED _input._invalidFormat |= (isStereoNow != _input._lastUpdateStereoState); @@ -163,13 +163,14 @@ void GLBackend::updateInput() { InputStageState::ActivationCache newActivation; // Assign the vertex format required - if (_input._format) { + auto format = acquire(_input._format); + if (format) { bool hasColorAttribute{ false }; _input._attribBindingBuffers.reset(); - const Stream::Format::AttributeMap& attributes = _input._format->getAttributes(); - auto& inputChannels = _input._format->getChannels(); + const auto& attributes = format->getAttributes(); + const auto& inputChannels = format->getChannels(); for (auto& channelIt : inputChannels) { auto bufferChannelNum = (channelIt).first; const Stream::Format::ChannelMap::value_type::second_type& channel = (channelIt).second; diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLBackendOutput.cpp b/libraries/gpu-gl-common/src/gpu/gl/GLBackendOutput.cpp index d1ab34da90..370e50592d 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLBackendOutput.cpp +++ b/libraries/gpu-gl-common/src/gpu/gl/GLBackendOutput.cpp @@ -25,23 +25,19 @@ using namespace gpu::gl; void GLBackend::syncOutputStateCache() { GLint currentFBO; glGetIntegerv(GL_DRAW_FRAMEBUFFER_BINDING, ¤tFBO); - _output._drawFBO = currentFBO; - _output._framebuffer.reset(); + reset(_output._framebuffer); } void GLBackend::resetOutputStage() { - if (_output._framebuffer) { - _output._framebuffer.reset(); - _output._drawFBO = 0; - glBindFramebuffer(GL_DRAW_FRAMEBUFFER, 0); - } - + _output._drawFBO = 0; + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, 0); glEnable(GL_FRAMEBUFFER_SRGB); + reset(_output._framebuffer); } void GLBackend::do_setFramebuffer(const Batch& batch, size_t paramOffset) { - auto framebuffer = batch._framebuffers.get(batch._params[paramOffset]._uint); + const auto& framebuffer = batch._framebuffers.get(batch._params[paramOffset]._uint); setFramebuffer(framebuffer); } @@ -55,13 +51,13 @@ void GLBackend::do_setFramebufferSwapChain(const Batch& batch, size_t paramOffse } void GLBackend::setFramebuffer(const FramebufferPointer& framebuffer) { - if (_output._framebuffer != framebuffer) { + if (!compare(_output._framebuffer, framebuffer)) { auto newFBO = getFramebufferID(framebuffer); if (_output._drawFBO != newFBO) { _output._drawFBO = newFBO; glBindFramebuffer(GL_DRAW_FRAMEBUFFER, newFBO); } - _output._framebuffer = framebuffer; + assign(_output._framebuffer, framebuffer); } } @@ -114,8 +110,9 @@ void GLBackend::do_clearFramebuffer(const Batch& batch, size_t paramOffset) { } std::vector drawBuffers; + auto framebuffer = acquire(_output._framebuffer); if (masks & Framebuffer::BUFFER_COLORS) { - if (_output._framebuffer) { + if (framebuffer) { for (unsigned int i = 0; i < Framebuffer::MAX_NUM_RENDER_BUFFERS; i++) { if (masks & (1 << i)) { drawBuffers.push_back(GL_COLOR_ATTACHMENT0 + i); @@ -163,8 +160,8 @@ void GLBackend::do_clearFramebuffer(const Batch& batch, size_t paramOffset) { } // Restore the color draw buffers only if a frmaebuffer is bound - if (_output._framebuffer && !drawBuffers.empty()) { - auto glFramebuffer = syncGPUObject(*_output._framebuffer); + if (framebuffer && !drawBuffers.empty()) { + auto glFramebuffer = syncGPUObject(*framebuffer); if (glFramebuffer) { glDrawBuffers((GLsizei)glFramebuffer->_colorBuffers.size(), glFramebuffer->_colorBuffers.data()); } diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLBackendPipeline.cpp b/libraries/gpu-gl-common/src/gpu/gl/GLBackendPipeline.cpp index 7e54774df6..9b28fa8114 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLBackendPipeline.cpp +++ b/libraries/gpu-gl-common/src/gpu/gl/GLBackendPipeline.cpp @@ -25,7 +25,7 @@ using namespace gpu::gl; void GLBackend::do_setPipeline(const Batch& batch, size_t paramOffset) { const auto& pipeline = batch._pipelines.get(batch._params[paramOffset + 0]._uint); - if (_pipeline._pipeline == pipeline) { + if (compare(_pipeline._pipeline, pipeline)) { return; } @@ -34,7 +34,7 @@ void GLBackend::do_setPipeline(const Batch& batch, size_t paramOffset) { // null pipeline == reset if (!pipeline) { - _pipeline._pipeline.reset(); + reset(_pipeline._pipeline); _pipeline._program = 0; _pipeline._cameraCorrection = false; @@ -73,7 +73,7 @@ void GLBackend::do_setPipeline(const Batch& batch, size_t paramOffset) { } // Remember the new pipeline - _pipeline._pipeline = pipeline; + assign(_pipeline._pipeline, pipeline); } // THis should be done on Pipeline::update... @@ -81,7 +81,7 @@ void GLBackend::do_setPipeline(const Batch& batch, size_t paramOffset) { glUseProgram(_pipeline._program); if (_pipeline._cameraCorrection) { // Invalidate uniform buffer cache slot - _uniform._buffers[gpu::slot::buffer::CameraCorrection] = {}; + _uniform._buffers[gpu::slot::buffer::CameraCorrection].reset(); auto& cameraCorrectionBuffer = _transform._viewCorrectionEnabled ? _pipeline._cameraCorrectionBuffer._buffer : _pipeline._cameraCorrectionBufferIdentity._buffer; @@ -112,7 +112,7 @@ void GLBackend::updatePipeline() { _pipeline._stateSignatureCache |= _pipeline._state->_signature; // And perform - for (auto command : _pipeline._state->_commands) { + for (const auto& command : _pipeline._state->_commands) { command->run(this); } } else { @@ -134,23 +134,21 @@ void GLBackend::resetPipelineStage() { _pipeline._invalidProgram = false; _pipeline._program = 0; _pipeline._programShader = nullptr; - _pipeline._pipeline.reset(); + reset(_pipeline._pipeline); glUseProgram(0); } -GLBackend::UniformStageState::BufferState::BufferState(const BufferPointer& buffer, GLintptr offset, GLsizeiptr size) - : buffer(buffer), offset(offset), size(size) {} - void GLBackend::releaseUniformBuffer(uint32_t slot) { - auto& buf = _uniform._buffers[slot]; - if (buf.buffer) { - auto* object = Backend::getGPUObject(*buf.buffer); + auto& bufferState = _uniform._buffers[slot]; + auto buffer = acquire(bufferState.buffer); + if (buffer) { + auto* object = Backend::getGPUObject(*buffer); if (object) { glBindBufferBase(GL_UNIFORM_BUFFER, slot, 0); // RELEASE (void)CHECK_GL_ERROR(); } - buf = UniformStageState::BufferState(); } + bufferState.reset(); } void GLBackend::resetUniformStage() { @@ -165,18 +163,20 @@ void GLBackend::bindUniformBuffer(uint32_t slot, const BufferPointer& buffer, GL return; } - UniformStageState::BufferState bufferState{ buffer, offset, size }; + auto& currentBufferState = _uniform._buffers[slot]; // check cache before thinking - if (_uniform._buffers[slot] == bufferState) { + if (currentBufferState.compare(buffer, offset, size)) { return; } // Grab the true gl Buffer object auto glBO = getBufferIDUnsynced(*buffer); if (glBO) { - glBindBufferRange(GL_UNIFORM_BUFFER, slot, glBO, bufferState.offset, bufferState.size); - _uniform._buffers[slot] = bufferState; + glBindBufferRange(GL_UNIFORM_BUFFER, slot, glBO, offset, size); + assign(currentBufferState.buffer, buffer); + currentBufferState.offset = offset; + currentBufferState.size = size; (void)CHECK_GL_ERROR(); } else { releaseUniformBuffer(slot); @@ -201,7 +201,7 @@ void GLBackend::do_setUniformBuffer(const Batch& batch, size_t paramOffset) { } void GLBackend::releaseResourceTexture(uint32_t slot) { - auto& tex = _resource._textures[slot]; + auto tex = acquire(_resource._textures[slot]); if (tex) { auto* object = Backend::getGPUObject(*tex); if (object) { @@ -210,8 +210,8 @@ void GLBackend::releaseResourceTexture(uint32_t slot) { glBindTexture(target, 0); // RELEASE (void)CHECK_GL_ERROR(); } - tex.reset(); } + reset(_resource._textures[slot]); } void GLBackend::resetResourceStage() { @@ -232,14 +232,14 @@ void GLBackend::do_setResourceBuffer(const Batch& batch, size_t paramOffset) { return; } - auto resourceBuffer = batch._buffers.get(batch._params[paramOffset + 0]._uint); + const auto& resourceBuffer = batch._buffers.get(batch._params[paramOffset + 0]._uint); if (!resourceBuffer) { releaseResourceBuffer(slot); return; } // check cache before thinking - if (_resource._buffers[slot] == resourceBuffer) { + if (compare(_resource._buffers[slot], resourceBuffer)) { return; } @@ -248,7 +248,7 @@ void GLBackend::do_setResourceBuffer(const Batch& batch, size_t paramOffset) { // If successful bind then cache it if (bindResourceBuffer(slot, resourceBuffer)) { - _resource._buffers[slot] = resourceBuffer; + assign(_resource._buffers[slot], resourceBuffer); } else { // else clear slot and cache releaseResourceBuffer(slot); return; @@ -293,14 +293,14 @@ void GLBackend::do_setResourceFramebufferSwapChainTexture(const Batch& batch, si } auto index = batch._params[paramOffset + 2]._uint; auto renderBufferSlot = batch._params[paramOffset + 3]._uint; - auto resourceFramebuffer = swapChain->get(index); - auto resourceTexture = resourceFramebuffer->getRenderBuffer(renderBufferSlot); + const auto& resourceFramebuffer = swapChain->get(index); + const auto& resourceTexture = resourceFramebuffer->getRenderBuffer(renderBufferSlot); setResourceTexture(slot, resourceTexture); } void GLBackend::setResourceTexture(unsigned int slot, const TexturePointer& resourceTexture) { // check cache before thinking - if (_resource._textures[slot] == resourceTexture) { + if (compare(_resource._textures[slot], resourceTexture)) { return; } @@ -317,7 +317,7 @@ void GLBackend::setResourceTexture(unsigned int slot, const TexturePointer& reso (void)CHECK_GL_ERROR(); - _resource._textures[slot] = resourceTexture; + assign(_resource._textures[slot], resourceTexture); _stats._RSAmountTextureMemoryBounded += (int)object->size(); @@ -343,7 +343,7 @@ void GLBackend::do_setResourceTextureTable(const Batch& batch, size_t paramOffse int GLBackend::ResourceStageState::findEmptyTextureSlot() const { // start from the end of the slots, try to find an empty one that can be used for (auto i = MAX_NUM_RESOURCE_TEXTURES - 1; i > 0; i--) { - if (!_textures[i]) { + if (!valid(_textures[i])) { return i; } } diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLPipeline.cpp b/libraries/gpu-gl-common/src/gpu/gl/GLPipeline.cpp index 1b479dceb8..a099e6e66a 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLPipeline.cpp +++ b/libraries/gpu-gl-common/src/gpu/gl/GLPipeline.cpp @@ -24,7 +24,7 @@ GLPipeline* GLPipeline::sync(GLBackend& backend, const Pipeline& pipeline) { } // No object allocated yet, let's see if it's worth it... - ShaderPointer shader = pipeline.getProgram(); + const auto& shader = pipeline.getProgram(); // If this pipeline's shader has already failed to compile, don't try again if (shader->compilationHasFailed()) { @@ -37,7 +37,7 @@ GLPipeline* GLPipeline::sync(GLBackend& backend, const Pipeline& pipeline) { return nullptr; } - StatePointer state = pipeline.getState(); + const auto& state = pipeline.getState(); GLState* stateObject = GLState::sync(*state); if (stateObject == nullptr) { return nullptr; diff --git a/libraries/gpu-gl/src/gpu/gl41/GL41Backend.h b/libraries/gpu-gl/src/gpu/gl41/GL41Backend.h index f4078f5479..e5f7415107 100644 --- a/libraries/gpu-gl/src/gpu/gl41/GL41Backend.h +++ b/libraries/gpu-gl/src/gpu/gl41/GL41Backend.h @@ -161,7 +161,7 @@ protected: void updateTransform(const Batch& batch) override; // Resource Stage - bool bindResourceBuffer(uint32_t slot, BufferPointer& buffer) override; + bool bindResourceBuffer(uint32_t slot, const BufferPointer& buffer) override; void releaseResourceBuffer(uint32_t slot) override; // Output stage diff --git a/libraries/gpu-gl/src/gpu/gl41/GL41BackendBuffer.cpp b/libraries/gpu-gl/src/gpu/gl41/GL41BackendBuffer.cpp index ac5d5ee0c9..80fd214515 100644 --- a/libraries/gpu-gl/src/gpu/gl41/GL41BackendBuffer.cpp +++ b/libraries/gpu-gl/src/gpu/gl41/GL41BackendBuffer.cpp @@ -100,7 +100,7 @@ GLBuffer* GL41Backend::syncGPUObject(const Buffer& buffer) { return GL41Buffer::sync(*this, buffer); } -bool GL41Backend::bindResourceBuffer(uint32_t slot, BufferPointer& buffer) { +bool GL41Backend::bindResourceBuffer(uint32_t slot, const BufferPointer& buffer) { GLuint texBuffer = GL41Backend::getResourceBufferID((*buffer)); if (texBuffer) { glActiveTexture(GL_TEXTURE0 + GL41Backend::RESOURCE_BUFFER_SLOT0_TEX_UNIT + slot); @@ -108,7 +108,7 @@ bool GL41Backend::bindResourceBuffer(uint32_t slot, BufferPointer& buffer) { (void)CHECK_GL_ERROR(); - _resource._buffers[slot] = buffer; + assign(_resource._buffers[slot], buffer); return true; } @@ -117,10 +117,7 @@ bool GL41Backend::bindResourceBuffer(uint32_t slot, BufferPointer& buffer) { } void GL41Backend::releaseResourceBuffer(uint32_t slot) { - auto& buf = _resource._buffers[slot]; - if (buf) { - glActiveTexture(GL_TEXTURE0 + GL41Backend::RESOURCE_BUFFER_SLOT0_TEX_UNIT + slot); - glBindTexture(GL_TEXTURE_BUFFER, 0); - buf.reset(); - } + reset(_resource._buffers[slot]); + glActiveTexture(GL_TEXTURE0 + GL41Backend::RESOURCE_BUFFER_SLOT0_TEX_UNIT + slot); + glBindTexture(GL_TEXTURE_BUFFER, 0); } diff --git a/libraries/gpu-gl/src/gpu/gl41/GL41BackendInput.cpp b/libraries/gpu-gl/src/gpu/gl41/GL41BackendInput.cpp index c61ffb09e5..bd88be0f0d 100644 --- a/libraries/gpu-gl/src/gpu/gl41/GL41BackendInput.cpp +++ b/libraries/gpu-gl/src/gpu/gl41/GL41BackendInput.cpp @@ -35,14 +35,15 @@ void GL41Backend::updateInput() { if (_input._invalidFormat || _input._invalidBuffers.any()) { + auto format = acquire(_input._format); if (_input._invalidFormat) { InputStageState::ActivationCache newActivation; _stats._ISNumFormatChanges++; // Check expected activation - if (_input._format) { - for (auto& it : _input._format->getAttributes()) { + if (format) { + for (auto& it : format->getAttributes()) { const Stream::Attribute& attrib = (it).second; uint8_t locationCount = attrib._element.getLocationCount(); for (int i = 0; i < locationCount; ++i) { @@ -69,15 +70,15 @@ void GL41Backend::updateInput() { } // now we need to bind the buffers and assign the attrib pointers - if (_input._format) { + if (format) { bool hasColorAttribute{ false }; - const Buffers& buffers = _input._buffers; + const auto& buffers = _input._buffers; const Offsets& offsets = _input._bufferOffsets; const Offsets& strides = _input._bufferStrides; - const Stream::Format::AttributeMap& attributes = _input._format->getAttributes(); - auto& inputChannels = _input._format->getChannels(); + const auto& attributes = format->getAttributes(); + const auto& inputChannels = format->getChannels(); int numInvalids = (int)_input._invalidBuffers.count(); _stats._ISNumInputBufferChanges += numInvalids; diff --git a/libraries/gpu-gl/src/gpu/gl45/GL45Backend.h b/libraries/gpu-gl/src/gpu/gl45/GL45Backend.h index a100faf432..30656b47c7 100644 --- a/libraries/gpu-gl/src/gpu/gl45/GL45Backend.h +++ b/libraries/gpu-gl/src/gpu/gl45/GL45Backend.h @@ -262,7 +262,7 @@ protected: void updateTransform(const Batch& batch) override; // Resource Stage - bool bindResourceBuffer(uint32_t slot, BufferPointer& buffer) override; + bool bindResourceBuffer(uint32_t slot, const BufferPointer& buffer) override; void releaseResourceBuffer(uint32_t slot) override; // Output stage diff --git a/libraries/gpu-gl/src/gpu/gl45/GL45BackendBuffer.cpp b/libraries/gpu-gl/src/gpu/gl45/GL45BackendBuffer.cpp index 6d17923ebd..da8f7059bf 100644 --- a/libraries/gpu-gl/src/gpu/gl45/GL45BackendBuffer.cpp +++ b/libraries/gpu-gl/src/gpu/gl45/GL45BackendBuffer.cpp @@ -60,14 +60,14 @@ GLBuffer* GL45Backend::syncGPUObject(const Buffer& buffer) { } -bool GL45Backend::bindResourceBuffer(uint32_t slot, BufferPointer& buffer) { +bool GL45Backend::bindResourceBuffer(uint32_t slot, const BufferPointer& buffer) { GLBuffer* object = syncGPUObject((*buffer)); if (object) { glBindBufferBase(GL_SHADER_STORAGE_BUFFER, slot, object->_id); (void)CHECK_GL_ERROR(); - _resource._buffers[slot] = buffer; + assign(_resource._buffers[slot], buffer); return true; } @@ -76,10 +76,10 @@ bool GL45Backend::bindResourceBuffer(uint32_t slot, BufferPointer& buffer) { } void GL45Backend::releaseResourceBuffer(uint32_t slot) { - auto& buf = _resource._buffers[slot]; + auto buf = acquire(_resource._buffers[slot]); if (buf) { glBindBufferBase(GL_SHADER_STORAGE_BUFFER, slot, 0); - buf.reset(); + reset(_resource._buffers[slot]); } } diff --git a/libraries/gpu-gl/src/gpu/gl45/GL45BackendInput.cpp b/libraries/gpu-gl/src/gpu/gl45/GL45BackendInput.cpp index 7cd8756ead..5285e62d3e 100644 --- a/libraries/gpu-gl/src/gpu/gl45/GL45BackendInput.cpp +++ b/libraries/gpu-gl/src/gpu/gl45/GL45BackendInput.cpp @@ -28,7 +28,7 @@ void GL45Backend::resetInputStage() { void GL45Backend::updateInput() { bool isStereoNow = isStereo(); - // track stereo state change potentially happening wihtout changing the input format + // track stereo state change potentially happening without changing the input format // this is a rare case requesting to invalid the format #ifdef GPU_STEREO_DRAWCALL_INSTANCED _input._invalidFormat |= (isStereoNow != _input._lastUpdateStereoState); @@ -39,13 +39,14 @@ void GL45Backend::updateInput() { InputStageState::ActivationCache newActivation; // Assign the vertex format required - if (_input._format) { + auto format = acquire(_input._format); + if (format) { bool hasColorAttribute{ false }; _input._attribBindingBuffers.reset(); - const Stream::Format::AttributeMap& attributes = _input._format->getAttributes(); - auto& inputChannels = _input._format->getChannels(); + const auto& attributes = format->getAttributes(); + const auto& inputChannels = format->getChannels(); for (auto& channelIt : inputChannels) { auto bufferChannelNum = (channelIt).first; const Stream::Format::ChannelMap::value_type::second_type& channel = (channelIt).second; diff --git a/libraries/gpu-gles/src/gpu/gles/GLESBackend.h b/libraries/gpu-gles/src/gpu/gles/GLESBackend.h index c757de0a72..56ae41da31 100644 --- a/libraries/gpu-gles/src/gpu/gles/GLESBackend.h +++ b/libraries/gpu-gles/src/gpu/gles/GLESBackend.h @@ -157,7 +157,7 @@ protected: void updateTransform(const Batch& batch) override; // Resource Stage - bool bindResourceBuffer(uint32_t slot, BufferPointer& buffer) override; + bool bindResourceBuffer(uint32_t slot, const BufferPointer& buffer) override; void releaseResourceBuffer(uint32_t slot) override; // Output stage diff --git a/libraries/gpu-gles/src/gpu/gles/GLESBackendBuffer.cpp b/libraries/gpu-gles/src/gpu/gles/GLESBackendBuffer.cpp index 7dd08df409..04f8628db3 100644 --- a/libraries/gpu-gles/src/gpu/gles/GLESBackendBuffer.cpp +++ b/libraries/gpu-gles/src/gpu/gles/GLESBackendBuffer.cpp @@ -72,14 +72,14 @@ GLBuffer* GLESBackend::syncGPUObject(const Buffer& buffer) { return GLESBuffer::sync(*this, buffer); } -bool GLESBackend::bindResourceBuffer(uint32_t slot, BufferPointer& buffer) { +bool GLESBackend::bindResourceBuffer(uint32_t slot, const BufferPointer& buffer) { GLBuffer* object = syncGPUObject((*buffer)); if (object) { glBindBufferBase(GL_SHADER_STORAGE_BUFFER, slot, object->_id); (void)CHECK_GL_ERROR(); - _resource._buffers[slot] = buffer; + assign(_resource._buffers[slot], buffer); return true; } @@ -91,7 +91,7 @@ void GLESBackend::releaseResourceBuffer(uint32_t slot) { auto& buf = _resource._buffers[slot]; if (buf) { glBindBufferBase(GL_SHADER_STORAGE_BUFFER, slot, 0); - buf.reset(); + reset(buf); } } diff --git a/libraries/gpu/src/gpu/Batch.cpp b/libraries/gpu/src/gpu/Batch.cpp index b6714e2f1a..745f1d1845 100644 --- a/libraries/gpu/src/gpu/Batch.cpp +++ b/libraries/gpu/src/gpu/Batch.cpp @@ -498,7 +498,7 @@ void Batch::setupNamedCalls(const std::string& instanceName, NamedBatchData::Fun captureNamedDrawCallInfo(instanceName); } -BufferPointer Batch::getNamedBuffer(const std::string& instanceName, uint8_t index) { +const BufferPointer& Batch::getNamedBuffer(const std::string& instanceName, uint8_t index) { NamedBatchData& instance = _namedData[instanceName]; if (instance.buffers.size() <= index) { instance.buffers.resize(index + 1); diff --git a/libraries/gpu/src/gpu/Batch.h b/libraries/gpu/src/gpu/Batch.h index b49d14e5a1..8e607a189e 100644 --- a/libraries/gpu/src/gpu/Batch.h +++ b/libraries/gpu/src/gpu/Batch.h @@ -119,7 +119,7 @@ public: void multiDrawIndexedIndirect(uint32 numCommands, Primitive primitiveType); void setupNamedCalls(const std::string& instanceName, NamedBatchData::Function function); - BufferPointer getNamedBuffer(const std::string& instanceName, uint8_t index = 0); + const BufferPointer& getNamedBuffer(const std::string& instanceName, uint8_t index = 0); // Input Stage // InputFormat diff --git a/libraries/gpu/src/gpu/Framebuffer.cpp b/libraries/gpu/src/gpu/Framebuffer.cpp index 8bb9be4a76..e88d986da6 100755 --- a/libraries/gpu/src/gpu/Framebuffer.cpp +++ b/libraries/gpu/src/gpu/Framebuffer.cpp @@ -203,11 +203,12 @@ uint32 Framebuffer::getNumRenderBuffers() const { return nb; } -TexturePointer Framebuffer::getRenderBuffer(uint32 slot) const { +const TexturePointer& Framebuffer::getRenderBuffer(uint32 slot) const { + static const TexturePointer EMPTY; if (!isSwapchain() && (slot < getMaxNumRenderBuffers())) { return _renderBuffers[slot]._texture; } else { - return TexturePointer(); + return EMPTY; } } @@ -297,9 +298,10 @@ bool Framebuffer::setDepthStencilBuffer(const TexturePointer& texture, const For return false; } -TexturePointer Framebuffer::getDepthStencilBuffer() const { +const TexturePointer& Framebuffer::getDepthStencilBuffer() const { + static const TexturePointer EMPTY; if (isSwapchain()) { - return TexturePointer(); + return EMPTY; } else { return _depthStencilBuffer._texture; } diff --git a/libraries/gpu/src/gpu/Framebuffer.h b/libraries/gpu/src/gpu/Framebuffer.h index fbbec50a28..44e945883f 100755 --- a/libraries/gpu/src/gpu/Framebuffer.h +++ b/libraries/gpu/src/gpu/Framebuffer.h @@ -95,7 +95,7 @@ public: static Framebuffer* createShadowmap(uint16 width); bool isSwapchain() const; - SwapchainPointer getSwapchain() const { return _swapchain; } + const SwapchainPointer& getSwapchain() const { return _swapchain; } uint32 getFrameCount() const; @@ -105,13 +105,13 @@ public: const TextureViews& getRenderBuffers() const { return _renderBuffers; } int32 setRenderBuffer(uint32 slot, const TexturePointer& texture, uint32 subresource = 0); - TexturePointer getRenderBuffer(uint32 slot) const; + const TexturePointer& getRenderBuffer(uint32 slot) const; uint32 getRenderBufferSubresource(uint32 slot) const; bool setDepthBuffer(const TexturePointer& texture, const Format& format, uint32 subresource = 0); bool setStencilBuffer(const TexturePointer& texture, const Format& format, uint32 subresource = 0); bool setDepthStencilBuffer(const TexturePointer& texture, const Format& format, uint32 subresource = 0); - TexturePointer getDepthStencilBuffer() const; + const TexturePointer& getDepthStencilBuffer() const; uint32 getDepthStencilBufferSubresource() const; Format getDepthStencilBufferFormat() const; From 37cf37e3e16498c74cfd285c38a1c0a34bfdeb47 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Wed, 12 Sep 2018 11:47:32 -0700 Subject: [PATCH 08/13] PR feedback --- .../gpu-gl-common/src/gpu/gl/GLBackend.cpp | 1 - .../gpu-gl-common/src/gpu/gl/GLBackend.h | 150 +++++++++--------- .../src/gpu/gl/GLBackendInput.cpp | 3 +- .../src/gpu/gl/GLBackendOutput.cpp | 10 +- .../src/gpu/gl/GLBackendPipeline.cpp | 40 ++--- .../gpu-gl/src/gpu/gl41/GL41BackendBuffer.cpp | 10 +- .../gpu-gl/src/gpu/gl41/GL41BackendInput.cpp | 4 +- .../gpu-gl/src/gpu/gl45/GL45BackendBuffer.cpp | 9 +- 8 files changed, 108 insertions(+), 119 deletions(-) diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLBackend.cpp b/libraries/gpu-gl-common/src/gpu/gl/GLBackend.cpp index 30a3cf0aaf..4fea4f2dc5 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLBackend.cpp +++ b/libraries/gpu-gl-common/src/gpu/gl/GLBackend.cpp @@ -150,7 +150,6 @@ void GLBackend::init() { }); } - GLBackend::GLBackend(bool syncCache) { _pipeline._cameraCorrectionBuffer._buffer->flush(); initShaderBinaryCache(); diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLBackend.h b/libraries/gpu-gl-common/src/gpu/gl/GLBackend.h index 7927734256..fecf1a5798 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLBackend.h +++ b/libraries/gpu-gl-common/src/gpu/gl/GLBackend.h @@ -39,6 +39,7 @@ #define GPU_STEREO_TECHNIQUE_INSTANCED #endif + // Let these be configured by the one define picked above #ifdef GPU_STEREO_TECHNIQUE_DOUBLED_SIMPLE #define GPU_STEREO_DRAWCALL_DOUBLED @@ -208,9 +209,8 @@ class GLBackend : public Backend, public std::enable_shared_from_this protected: explicit GLBackend(bool syncCache); GLBackend(); - - public: + #if defined(USE_GLES) // https://www.khronos.org/registry/OpenGL-Refpages/es3/html/glGet.xhtml static const GLint MIN_REQUIRED_TEXTURE_IMAGE_UNITS = 16; @@ -251,8 +251,8 @@ public: // This is the ugly "download the pixels to sysmem for taking a snapshot" // Just avoid using it, it's ugly and will break performances virtual void downloadFramebuffer(const FramebufferPointer& srcFramebuffer, - const Vec4i& region, - QImage& destImage) final override; + const Vec4i& region, QImage& destImage) final override; + // this is the maximum numeber of available input buffers size_t getNumInputBuffers() const { return _input._invalidBuffers.size(); } @@ -273,6 +273,7 @@ public: static const int MAX_NUM_RESOURCE_TABLE_TEXTURES = 2; size_t getMaxNumResourceTextureTables() const { return MAX_NUM_RESOURCE_TABLE_TEXTURES; } + // Draw Stage virtual void do_draw(const Batch& batch, size_t paramOffset) = 0; virtual void do_drawIndexed(const Batch& batch, size_t paramOffset) = 0; @@ -324,6 +325,7 @@ public: // Reset stages virtual void do_resetStages(const Batch& batch, size_t paramOffset) final; + virtual void do_disableContextViewCorrection(const Batch& batch, size_t paramOffset) final; virtual void do_restoreContextViewCorrection(const Batch& batch, size_t paramOffset) final; @@ -343,7 +345,7 @@ public: virtual void do_popProfileRange(const Batch& batch, size_t paramOffset) final; // TODO: As long as we have gl calls explicitely issued from interface - // code, we need to be able to record and batch these calls. THe long + // code, we need to be able to record and batch these calls. THe long // term strategy is to get rid of any GL calls in favor of the HIFI GPU API virtual void do_glUniform1i(const Batch& batch, size_t paramOffset) final; virtual void do_glUniform1f(const Batch& batch, size_t paramOffset) final; @@ -368,9 +370,7 @@ public: virtual void do_setStateAntialiasedLineEnable(bool enable) final; virtual void do_setStateDepthBias(Vec2 bias) final; virtual void do_setStateDepthTest(State::DepthTest test) final; - virtual void do_setStateStencil(State::StencilActivation activation, - State::StencilTest frontTest, - State::StencilTest backTest) final; + virtual void do_setStateStencil(State::StencilActivation activation, State::StencilTest frontTest, State::StencilTest backTest) final; virtual void do_setStateAlphaToCoverageEnable(bool enable) final; virtual void do_setStateSampleMask(uint32 mask) final; virtual void do_setStateBlend(State::BlendFunction blendFunction) final; @@ -399,9 +399,7 @@ public: virtual void releaseQuery(GLuint id) const; virtual void queueLambda(const std::function lambda) const; - bool isTextureManagementSparseEnabled() const override { - return (_textureManagement._sparseCapable && Texture::getEnableSparseTextures()); - } + bool isTextureManagementSparseEnabled() const override { return (_textureManagement._sparseCapable && Texture::getEnableSparseTextures()); } protected: virtual GLint getRealUniformLocation(GLint location) const; @@ -410,11 +408,11 @@ protected: // FIXME instead of a single flag, create a features struct similar to // https://www.khronos.org/registry/vulkan/specs/1.0/man/html/VkPhysicalDeviceFeatures.html - virtual bool supportsBindless() const { return false; } + virtual bool supportsBindless() const { return false; } static const size_t INVALID_OFFSET = (size_t)-1; - bool _inRenderTransferPass{ false }; - int _currentDraw{ -1 }; + bool _inRenderTransferPass { false }; + int _currentDraw { -1 }; std::list profileRanges; mutable Mutex _trashMutex; @@ -443,42 +441,36 @@ protected: virtual void updateInput() = 0; struct InputStageState { - bool _invalidFormat{ true }; - bool _lastUpdateStereoState{ false }; + bool _invalidFormat { true }; + bool _lastUpdateStereoState{ false }; bool _hadColorAttribute{ true }; FormatReference _format{ GPU_REFERENCE_INIT_VALUE }; std::string _formatKey; typedef std::bitset ActivationCache; - ActivationCache _attributeActivation{ 0 }; + ActivationCache _attributeActivation { 0 }; typedef std::bitset BuffersState; BuffersState _invalidBuffers{ 0 }; BuffersState _attribBindingBuffers{ 0 }; - std::vector _buffers{ MAX_NUM_INPUT_BUFFERS, GPU_REFERENCE_INIT_VALUE }; - Offsets _bufferOffsets; - Offsets _bufferStrides; - std::vector _bufferVBOs; + std::array _buffers{}; + std::array _bufferOffsets{}; + std::array _bufferStrides{}; + std::array _bufferVBOs{}; glm::vec4 _colorAttribute{ 0.0f }; - BufferReference _indexBuffer{ GPU_REFERENCE_INIT_VALUE }; - Offset _indexBufferOffset{ 0 }; - Type _indexBufferType{ UINT32 }; + BufferReference _indexBuffer{}; + Offset _indexBufferOffset { 0 }; + Type _indexBufferType { UINT32 }; - BufferReference _indirectBuffer{ GPU_REFERENCE_INIT_VALUE }; + BufferReference _indirectBuffer{}; Offset _indirectBufferOffset{ 0 }; Offset _indirectBufferStride{ 0 }; - GLuint _defaultVAO{ 0 }; - - InputStageState() { - _bufferOffsets.resize(MAX_NUM_INPUT_BUFFERS, 0); - _bufferStrides.resize(MAX_NUM_INPUT_BUFFERS, 0); - _bufferVBOs.resize(MAX_NUM_INPUT_BUFFERS, 0); - } + GLuint _defaultVAO { 0 }; } _input; virtual void initTransform() = 0; @@ -489,7 +481,7 @@ protected: virtual void resetTransformStage(); // Allows for correction of the camera pose to account for changes - // between the time when a was recorded and the time(s) when it is + // between the time when a was recorded and the time(s) when it is // executed // Prev is the previous correction used at previous frame struct CameraCorrection { @@ -504,12 +496,9 @@ protected: struct Cameras { TransformCamera _cams[2]; - Cameras(){}; + Cameras() {}; Cameras(const TransformCamera& cam) { memcpy(_cams, &cam, sizeof(TransformCamera)); }; - Cameras(const TransformCamera& camL, const TransformCamera& camR) { - memcpy(_cams, &camL, sizeof(TransformCamera)); - memcpy(_cams + 1, &camR, sizeof(TransformCamera)); - }; + Cameras(const TransformCamera& camL, const TransformCamera& camR) { memcpy(_cams, &camL, sizeof(TransformCamera)); memcpy(_cams + 1, &camR, sizeof(TransformCamera)); }; }; using CameraBufferElement = Cameras; @@ -523,24 +512,25 @@ protected: mutable std::map _drawCallInfoOffsets; - GLuint _objectBuffer{ 0 }; - GLuint _cameraBuffer{ 0 }; - GLuint _drawCallInfoBuffer{ 0 }; - GLuint _objectBufferTexture{ 0 }; - size_t _cameraUboSize{ 0 }; + GLuint _objectBuffer { 0 }; + GLuint _cameraBuffer { 0 }; + GLuint _drawCallInfoBuffer { 0 }; + GLuint _objectBufferTexture { 0 }; + size_t _cameraUboSize { 0 }; bool _viewIsCamera{ false }; - bool _skybox{ false }; + bool _skybox { false }; Transform _view; CameraCorrection _correction; bool _viewCorrectionEnabled{ true }; + Mat4 _projection; - Vec4i _viewport{ 0, 0, 1, 1 }; - Vec2 _depthRange{ 0.0f, 1.0f }; + Vec4i _viewport { 0, 0, 1, 1 }; + Vec2 _depthRange { 0.0f, 1.0f }; Vec2 _projectionJitter{ 0.0f, 0.0f }; - bool _invalidView{ false }; - bool _invalidProj{ false }; - bool _invalidViewport{ false }; + bool _invalidView { false }; + bool _invalidProj { false }; + bool _invalidViewport { false }; bool _enabledDrawcallInfoBuffer{ false }; @@ -559,10 +549,9 @@ protected: struct UniformStageState { struct BufferState { - BufferReference buffer{ GPU_REFERENCE_INIT_VALUE }; + BufferReference buffer{}; GLintptr offset{ 0 }; GLsizeiptr size{ 0 }; - //BufferState(const BufferPointer& buffer = nullptr, GLintptr offset = 0, GLsizeiptr size = 0); BufferState& operator=(const BufferState& other) = delete; void reset() { gpu::gl::reset(buffer); offset = 0; size = 0; } @@ -573,12 +562,12 @@ protected: }; // MAX_NUM_UNIFORM_BUFFERS-1 is the max uniform index BATCHES are allowed to set, but - // MIN_REQUIRED_UNIFORM_BUFFER_BINDINGS is used here because the backend sets some - // internal UBOs for things like camera correction + // MIN_REQUIRED_UNIFORM_BUFFER_BINDINGS is used here because the backend sets some + // internal UBOs for things like camera correction std::array _buffers; } _uniform; - // Helper function that provides common code + // Helper function that provides common code void bindUniformBuffer(uint32_t slot, const BufferPointer& buffer, GLintptr offset = 0, GLsizeiptr size = 0); void releaseUniformBuffer(uint32_t slot); void resetUniformStage(); @@ -588,19 +577,23 @@ protected: virtual bool bindResourceBuffer(uint32_t slot, const BufferPointer& buffer) = 0; virtual void releaseResourceBuffer(uint32_t slot) = 0; - // Helper function that provides common code used by do_setResourceTexture and + // Helper function that provides common code used by do_setResourceTexture and // do_setResourceTextureTable (in non-bindless mode) void bindResourceTexture(uint32_t slot, const TexturePointer& texture); + // update resource cache and do the gl unbind call with the current gpu::Texture cached at slot s void releaseResourceTexture(uint32_t slot); void resetResourceStage(); struct ResourceStageState { - std::vector _buffers{ MAX_NUM_RESOURCE_BUFFERS, BufferReference() }; - std::vector _textures{ MAX_NUM_RESOURCE_TEXTURES, GPU_REFERENCE_INIT_VALUE }; - //Textures _textures { { MAX_NUM_RESOURCE_TEXTURES } }; + struct TextureState { + TextureReference _texture{}; + GLenum _target; + }; + std::array _buffers{}; + std::array _textures{}; int findEmptyTextureSlot() const; } _resource; @@ -615,22 +608,21 @@ protected: void resetPipelineStage(); struct PipelineStageState { - PipelineReference _pipeline{ GPU_REFERENCE_INIT_VALUE }; + PipelineReference _pipeline{}; - GLuint _program{ 0 }; - bool _cameraCorrection{ false }; - GLShader* _programShader{ nullptr }; - bool _invalidProgram{ false }; + GLuint _program { 0 }; + bool _cameraCorrection { false }; + GLShader* _programShader { nullptr }; + bool _invalidProgram { false }; - BufferView _cameraCorrectionBuffer{ gpu::BufferView(std::make_shared(sizeof(CameraCorrection), nullptr)) }; - BufferView _cameraCorrectionBufferIdentity{ gpu::BufferView( - std::make_shared(sizeof(CameraCorrection), nullptr)) }; + BufferView _cameraCorrectionBuffer { gpu::BufferView(std::make_shared(sizeof(CameraCorrection), nullptr )) }; + BufferView _cameraCorrectionBufferIdentity { gpu::BufferView(std::make_shared(sizeof(CameraCorrection), nullptr )) }; State::Data _stateCache{ State::DEFAULT }; - State::Signature _stateSignatureCache{ 0 }; + State::Signature _stateSignatureCache { 0 }; - GLState* _state{ nullptr }; - bool _invalidState{ false }; + GLState* _state { nullptr }; + bool _invalidState { false }; PipelineStageState() { _cameraCorrectionBuffer.edit() = CameraCorrection(); @@ -644,9 +636,9 @@ protected: virtual GLShader* compileBackendProgram(const Shader& program, const Shader::CompilationHandler& handler); virtual GLShader* compileBackendShader(const Shader& shader, const Shader::CompilationHandler& handler); virtual std::string getBackendShaderHeader() const = 0; - // For a program, this will return a string containing all the source files (without any - // backend headers or defines). For a vertex, fragment or geometry shader, this will - // return the fully customized shader with all the version and backend specific + // For a program, this will return a string containing all the source files (without any + // backend headers or defines). For a vertex, fragment or geometry shader, this will + // return the fully customized shader with all the version and backend specific // preprocessor directives // The program string returned can be used as a key for a cache of shader binaries // The shader strings can be reliably sent to the low level `compileShader` functions @@ -662,22 +654,22 @@ protected: // Synchronize the state cache of this Backend with the actual real state of the GL Context void syncOutputStateCache(); void resetOutputStage(); - + struct OutputStageState { - FramebufferReference _framebuffer{ GPU_REFERENCE_INIT_VALUE }; - GLuint _drawFBO{ 0 }; + FramebufferReference _framebuffer{}; + GLuint _drawFBO { 0 }; } _output; void resetQueryStage(); struct QueryStageState { - uint32_t _rangeQueryDepth{ 0 }; + uint32_t _rangeQueryDepth { 0 }; } _queryStage; void resetStages(); // Stores cached binary versions of the shaders for quicker startup on subsequent runs - // Note that shaders in the cache can still fail to load due to hardware or driver - // changes that invalidate the cached binary, in which case we fall back on compiling + // Note that shaders in the cache can still fail to load due to hardware or driver + // changes that invalidate the cached binary, in which case we fall back on compiling // the source again struct ShaderBinaryCache { std::mutex _mutex; @@ -689,7 +681,7 @@ protected: virtual void killShaderBinaryCache(); struct TextureManagementStageState { - bool _sparseCapable{ false }; + bool _sparseCapable { false }; GLTextureTransferEnginePointer _transferEngine; } _textureManagement; virtual void initTextureManagementStage(); diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLBackendInput.cpp b/libraries/gpu-gl-common/src/gpu/gl/GLBackendInput.cpp index 219efae866..85e6ba5382 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLBackendInput.cpp +++ b/libraries/gpu-gl-common/src/gpu/gl/GLBackendInput.cpp @@ -19,8 +19,8 @@ using namespace gpu::gl; void GLBackend::do_setInputFormat(const Batch& batch, size_t paramOffset) { const auto& format = batch._streamFormats.get(batch._params[paramOffset]._uint); if (!compare(_input._format, format)) { - assign(_input._format, format); if (format) { + assign(_input._format, format); auto inputFormat = GLInputFormat::sync((*format)); assert(inputFormat); if (_input._formatKey != inputFormat->key) { @@ -28,6 +28,7 @@ void GLBackend::do_setInputFormat(const Batch& batch, size_t paramOffset) { _input._invalidFormat = true; } } else { + reset(_input._format); _input._formatKey.clear(); _input._invalidFormat = true; } diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLBackendOutput.cpp b/libraries/gpu-gl-common/src/gpu/gl/GLBackendOutput.cpp index 370e50592d..411e16b18e 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLBackendOutput.cpp +++ b/libraries/gpu-gl-common/src/gpu/gl/GLBackendOutput.cpp @@ -25,15 +25,19 @@ using namespace gpu::gl; void GLBackend::syncOutputStateCache() { GLint currentFBO; glGetIntegerv(GL_DRAW_FRAMEBUFFER_BINDING, ¤tFBO); + _output._drawFBO = currentFBO; reset(_output._framebuffer); } void GLBackend::resetOutputStage() { - _output._drawFBO = 0; - glBindFramebuffer(GL_DRAW_FRAMEBUFFER, 0); + if (valid(_output._framebuffer)) { + reset(_output._framebuffer); + _output._drawFBO = 0; + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, 0); + } + glEnable(GL_FRAMEBUFFER_SRGB); - reset(_output._framebuffer); } void GLBackend::do_setFramebuffer(const Batch& batch, size_t paramOffset) { diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLBackendPipeline.cpp b/libraries/gpu-gl-common/src/gpu/gl/GLBackendPipeline.cpp index 9b28fa8114..7a06b3af86 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLBackendPipeline.cpp +++ b/libraries/gpu-gl-common/src/gpu/gl/GLBackendPipeline.cpp @@ -140,13 +140,9 @@ void GLBackend::resetPipelineStage() { void GLBackend::releaseUniformBuffer(uint32_t slot) { auto& bufferState = _uniform._buffers[slot]; - auto buffer = acquire(bufferState.buffer); - if (buffer) { - auto* object = Backend::getGPUObject(*buffer); - if (object) { - glBindBufferBase(GL_UNIFORM_BUFFER, slot, 0); // RELEASE - (void)CHECK_GL_ERROR(); - } + if (valid(bufferState.buffer)) { + glBindBufferBase(GL_UNIFORM_BUFFER, slot, 0); // RELEASE + (void)CHECK_GL_ERROR(); } bufferState.reset(); } @@ -201,17 +197,13 @@ void GLBackend::do_setUniformBuffer(const Batch& batch, size_t paramOffset) { } void GLBackend::releaseResourceTexture(uint32_t slot) { - auto tex = acquire(_resource._textures[slot]); - if (tex) { - auto* object = Backend::getGPUObject(*tex); - if (object) { - GLuint target = object->_target; - glActiveTexture(GL_TEXTURE0 + slot); - glBindTexture(target, 0); // RELEASE - (void)CHECK_GL_ERROR(); - } + auto& textureState = _resource._textures[slot]; + if (valid(textureState._texture)) { + glActiveTexture(GL_TEXTURE0 + slot); + glBindTexture(textureState._target, 0); // RELEASE + (void)CHECK_GL_ERROR(); + reset(textureState._texture); } - reset(_resource._textures[slot]); } void GLBackend::resetResourceStage() { @@ -299,8 +291,9 @@ void GLBackend::do_setResourceFramebufferSwapChainTexture(const Batch& batch, si } void GLBackend::setResourceTexture(unsigned int slot, const TexturePointer& resourceTexture) { + auto& textureState = _resource._textures[slot]; // check cache before thinking - if (compare(_resource._textures[slot], resourceTexture)) { + if (compare(textureState._texture, resourceTexture)) { return; } @@ -310,15 +303,12 @@ void GLBackend::setResourceTexture(unsigned int slot, const TexturePointer& reso // Always make sure the GLObject is in sync GLTexture* object = syncGPUObject(resourceTexture); if (object) { + assign(textureState._texture, resourceTexture); GLuint to = object->_texture; - GLuint target = object->_target; + textureState._target = object->_target; glActiveTexture(GL_TEXTURE0 + slot); - glBindTexture(target, to); - + glBindTexture(textureState._target, to); (void)CHECK_GL_ERROR(); - - assign(_resource._textures[slot], resourceTexture); - _stats._RSAmountTextureMemoryBounded += (int)object->size(); } else { @@ -343,7 +333,7 @@ void GLBackend::do_setResourceTextureTable(const Batch& batch, size_t paramOffse int GLBackend::ResourceStageState::findEmptyTextureSlot() const { // start from the end of the slots, try to find an empty one that can be used for (auto i = MAX_NUM_RESOURCE_TEXTURES - 1; i > 0; i--) { - if (!valid(_textures[i])) { + if (!valid(_textures[i]._texture)) { return i; } } diff --git a/libraries/gpu-gl/src/gpu/gl41/GL41BackendBuffer.cpp b/libraries/gpu-gl/src/gpu/gl41/GL41BackendBuffer.cpp index 80fd214515..d5193f892a 100644 --- a/libraries/gpu-gl/src/gpu/gl41/GL41BackendBuffer.cpp +++ b/libraries/gpu-gl/src/gpu/gl41/GL41BackendBuffer.cpp @@ -117,7 +117,11 @@ bool GL41Backend::bindResourceBuffer(uint32_t slot, const BufferPointer& buffer) } void GL41Backend::releaseResourceBuffer(uint32_t slot) { - reset(_resource._buffers[slot]); - glActiveTexture(GL_TEXTURE0 + GL41Backend::RESOURCE_BUFFER_SLOT0_TEX_UNIT + slot); - glBindTexture(GL_TEXTURE_BUFFER, 0); + auto& bufferReference = _resource._buffers[slot]; + auto buffer = acquire(bufferReference); + if (buffer) { + glActiveTexture(GL_TEXTURE0 + GL41Backend::RESOURCE_BUFFER_SLOT0_TEX_UNIT + slot); + glBindTexture(GL_TEXTURE_BUFFER, 0); + reset(bufferReference); + } } diff --git a/libraries/gpu-gl/src/gpu/gl41/GL41BackendInput.cpp b/libraries/gpu-gl/src/gpu/gl41/GL41BackendInput.cpp index bd88be0f0d..2b985c122e 100644 --- a/libraries/gpu-gl/src/gpu/gl41/GL41BackendInput.cpp +++ b/libraries/gpu-gl/src/gpu/gl41/GL41BackendInput.cpp @@ -74,8 +74,8 @@ void GL41Backend::updateInput() { bool hasColorAttribute{ false }; const auto& buffers = _input._buffers; - const Offsets& offsets = _input._bufferOffsets; - const Offsets& strides = _input._bufferStrides; + const auto& offsets = _input._bufferOffsets; + const auto& strides = _input._bufferStrides; const auto& attributes = format->getAttributes(); const auto& inputChannels = format->getChannels(); diff --git a/libraries/gpu-gl/src/gpu/gl45/GL45BackendBuffer.cpp b/libraries/gpu-gl/src/gpu/gl45/GL45BackendBuffer.cpp index da8f7059bf..cb0591c31c 100644 --- a/libraries/gpu-gl/src/gpu/gl45/GL45BackendBuffer.cpp +++ b/libraries/gpu-gl/src/gpu/gl45/GL45BackendBuffer.cpp @@ -76,11 +76,10 @@ bool GL45Backend::bindResourceBuffer(uint32_t slot, const BufferPointer& buffer) } void GL45Backend::releaseResourceBuffer(uint32_t slot) { - auto buf = acquire(_resource._buffers[slot]); - if (buf) { + auto& bufferReference = _resource._buffers[slot]; + auto buffer = acquire(bufferReference); + if (buffer) { glBindBufferBase(GL_SHADER_STORAGE_BUFFER, slot, 0); - reset(_resource._buffers[slot]); + reset(bufferReference); } } - - From 700c6519bf23ec5c343e44106130c57016dc39f9 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Wed, 12 Sep 2018 11:55:07 -0700 Subject: [PATCH 09/13] More PR comments --- libraries/gpu-gles/src/gpu/gles/GLESBackendBuffer.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libraries/gpu-gles/src/gpu/gles/GLESBackendBuffer.cpp b/libraries/gpu-gles/src/gpu/gles/GLESBackendBuffer.cpp index 04f8628db3..5e4da4d1fe 100644 --- a/libraries/gpu-gles/src/gpu/gles/GLESBackendBuffer.cpp +++ b/libraries/gpu-gles/src/gpu/gles/GLESBackendBuffer.cpp @@ -88,10 +88,10 @@ bool GLESBackend::bindResourceBuffer(uint32_t slot, const BufferPointer& buffer) } void GLESBackend::releaseResourceBuffer(uint32_t slot) { - auto& buf = _resource._buffers[slot]; - if (buf) { + auto& bufferReference = _resource._buffers[slot]; + if (valid(bufferReference)) { glBindBufferBase(GL_SHADER_STORAGE_BUFFER, slot, 0); - reset(buf); + reset(bufferReference); } } From d92c62c7423684533d6ba4d9602696538f8e73b2 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Wed, 12 Sep 2018 15:04:27 -0700 Subject: [PATCH 10/13] Make android use the existing shared pointers --- .../gpu-gl-common/src/gpu/gl/GLBackend.h | 159 +++++++++--------- 1 file changed, 84 insertions(+), 75 deletions(-) diff --git a/libraries/gpu-gl-common/src/gpu/gl/GLBackend.h b/libraries/gpu-gl-common/src/gpu/gl/GLBackend.h index fecf1a5798..0b76ef17de 100644 --- a/libraries/gpu-gl-common/src/gpu/gl/GLBackend.h +++ b/libraries/gpu-gl-common/src/gpu/gl/GLBackend.h @@ -39,7 +39,6 @@ #define GPU_STEREO_TECHNIQUE_INSTANCED #endif - // Let these be configured by the one define picked above #ifdef GPU_STEREO_TECHNIQUE_DOUBLED_SIMPLE #define GPU_STEREO_DRAWCALL_DOUBLED @@ -57,23 +56,25 @@ // // GL Backend pointer storage mechanism -// One of the following three defines must be uncommented. +// One of the following three defines must be defined. +// GPU_POINTER_STORAGE_SHARED -// Equivalent to current state of affairs in master, -// Works pretty well, but ends up creating a lot of needless smart pointer duplication -// which means there's a high aggregate cost of manipulating std::shared_ptr counters -//#define GPU_POINTER_STORAGE_SHARED - -// The platonic ideal, use references to smart pointers. -// However, this produces artifacts because there are too many places in the code right now that +// The platonic ideal, use references to smart pointers. +// However, this produces artifacts because there are too many places in the code right now that // create temporary values (undesirable smart pointer duplications) and then those temp variables // get passed on and have their reference taken, and then invalidated -//#define GPU_POINTER_STORAGE_REF +// GPU_POINTER_STORAGE_REF // Raw pointer manipulation. Seems more dangerous than the reference wrappers, -// but in practice, the danger of grabbing a reference to a temporary variable +// but in practice, the danger of grabbing a reference to a temporary variable // is causing issues +// GPU_POINTER_STORAGE_RAW + +#if defined(USE_GLES) +#define GPU_POINTER_STORAGE_SHARED +#else #define GPU_POINTER_STORAGE_RAW +#endif namespace gpu { namespace gl { @@ -99,7 +100,7 @@ static inline bool valid(const std::shared_ptr& pointer) { } template -static inline void assign(std::shared_ptr&pointer, const std::shared_ptr& source) { +static inline void assign(std::shared_ptr& pointer, const std::shared_ptr& source) { pointer = source; } @@ -130,7 +131,7 @@ private: }; }; -template +template static bool compare(const PointerReferenceWrapper& reference, const std::shared_ptr& pointer) { return reference.get() == pointer; } @@ -140,12 +141,12 @@ static inline T* acquire(const PointerReferenceWrapper& reference) { return reference.get().get(); } -template +template static void assign(PointerReferenceWrapper& reference, const std::shared_ptr& pointer) { reference = pointer; } -template +template static bool valid(const PointerReferenceWrapper& reference) { return reference.get().operator bool(); } @@ -161,12 +162,12 @@ using FramebufferReference = PointerReferenceWrapper; using FormatReference = PointerReferenceWrapper; using PipelineReference = PointerReferenceWrapper; -#define GPU_REFERENCE_INIT_VALUE +#define GPU_REFERENCE_INIT_VALUE #elif defined(GPU_POINTER_STORAGE_RAW) -template -static bool compare(const T*const& rawPointer, const std::shared_ptr& pointer) { +template +static bool compare(const T* const& rawPointer, const std::shared_ptr& pointer) { return rawPointer == pointer.get(); } @@ -176,7 +177,7 @@ static inline T* acquire(T*& rawPointer) { } template -static inline bool valid(const T*const& rawPointer) { +static inline bool valid(const T* const& rawPointer) { return rawPointer; } @@ -209,8 +210,8 @@ class GLBackend : public Backend, public std::enable_shared_from_this protected: explicit GLBackend(bool syncCache); GLBackend(); -public: +public: #if defined(USE_GLES) // https://www.khronos.org/registry/OpenGL-Refpages/es3/html/glGet.xhtml static const GLint MIN_REQUIRED_TEXTURE_IMAGE_UNITS = 16; @@ -251,8 +252,8 @@ public: // This is the ugly "download the pixels to sysmem for taking a snapshot" // Just avoid using it, it's ugly and will break performances virtual void downloadFramebuffer(const FramebufferPointer& srcFramebuffer, - const Vec4i& region, QImage& destImage) final override; - + const Vec4i& region, + QImage& destImage) final override; // this is the maximum numeber of available input buffers size_t getNumInputBuffers() const { return _input._invalidBuffers.size(); } @@ -273,7 +274,6 @@ public: static const int MAX_NUM_RESOURCE_TABLE_TEXTURES = 2; size_t getMaxNumResourceTextureTables() const { return MAX_NUM_RESOURCE_TABLE_TEXTURES; } - // Draw Stage virtual void do_draw(const Batch& batch, size_t paramOffset) = 0; virtual void do_drawIndexed(const Batch& batch, size_t paramOffset) = 0; @@ -325,7 +325,6 @@ public: // Reset stages virtual void do_resetStages(const Batch& batch, size_t paramOffset) final; - virtual void do_disableContextViewCorrection(const Batch& batch, size_t paramOffset) final; virtual void do_restoreContextViewCorrection(const Batch& batch, size_t paramOffset) final; @@ -345,7 +344,7 @@ public: virtual void do_popProfileRange(const Batch& batch, size_t paramOffset) final; // TODO: As long as we have gl calls explicitely issued from interface - // code, we need to be able to record and batch these calls. THe long + // code, we need to be able to record and batch these calls. THe long // term strategy is to get rid of any GL calls in favor of the HIFI GPU API virtual void do_glUniform1i(const Batch& batch, size_t paramOffset) final; virtual void do_glUniform1f(const Batch& batch, size_t paramOffset) final; @@ -370,7 +369,9 @@ public: virtual void do_setStateAntialiasedLineEnable(bool enable) final; virtual void do_setStateDepthBias(Vec2 bias) final; virtual void do_setStateDepthTest(State::DepthTest test) final; - virtual void do_setStateStencil(State::StencilActivation activation, State::StencilTest frontTest, State::StencilTest backTest) final; + virtual void do_setStateStencil(State::StencilActivation activation, + State::StencilTest frontTest, + State::StencilTest backTest) final; virtual void do_setStateAlphaToCoverageEnable(bool enable) final; virtual void do_setStateSampleMask(uint32 mask) final; virtual void do_setStateBlend(State::BlendFunction blendFunction) final; @@ -399,7 +400,9 @@ public: virtual void releaseQuery(GLuint id) const; virtual void queueLambda(const std::function lambda) const; - bool isTextureManagementSparseEnabled() const override { return (_textureManagement._sparseCapable && Texture::getEnableSparseTextures()); } + bool isTextureManagementSparseEnabled() const override { + return (_textureManagement._sparseCapable && Texture::getEnableSparseTextures()); + } protected: virtual GLint getRealUniformLocation(GLint location) const; @@ -408,11 +411,11 @@ protected: // FIXME instead of a single flag, create a features struct similar to // https://www.khronos.org/registry/vulkan/specs/1.0/man/html/VkPhysicalDeviceFeatures.html - virtual bool supportsBindless() const { return false; } + virtual bool supportsBindless() const { return false; } static const size_t INVALID_OFFSET = (size_t)-1; - bool _inRenderTransferPass { false }; - int _currentDraw { -1 }; + bool _inRenderTransferPass{ false }; + int _currentDraw{ -1 }; std::list profileRanges; mutable Mutex _trashMutex; @@ -441,14 +444,14 @@ protected: virtual void updateInput() = 0; struct InputStageState { - bool _invalidFormat { true }; - bool _lastUpdateStereoState{ false }; + bool _invalidFormat{ true }; + bool _lastUpdateStereoState{ false }; bool _hadColorAttribute{ true }; FormatReference _format{ GPU_REFERENCE_INIT_VALUE }; std::string _formatKey; typedef std::bitset ActivationCache; - ActivationCache _attributeActivation { 0 }; + ActivationCache _attributeActivation{ 0 }; typedef std::bitset BuffersState; @@ -463,14 +466,14 @@ protected: glm::vec4 _colorAttribute{ 0.0f }; BufferReference _indexBuffer{}; - Offset _indexBufferOffset { 0 }; - Type _indexBufferType { UINT32 }; + Offset _indexBufferOffset{ 0 }; + Type _indexBufferType{ UINT32 }; BufferReference _indirectBuffer{}; Offset _indirectBufferOffset{ 0 }; Offset _indirectBufferStride{ 0 }; - GLuint _defaultVAO { 0 }; + GLuint _defaultVAO{ 0 }; } _input; virtual void initTransform() = 0; @@ -481,7 +484,7 @@ protected: virtual void resetTransformStage(); // Allows for correction of the camera pose to account for changes - // between the time when a was recorded and the time(s) when it is + // between the time when a was recorded and the time(s) when it is // executed // Prev is the previous correction used at previous frame struct CameraCorrection { @@ -496,9 +499,12 @@ protected: struct Cameras { TransformCamera _cams[2]; - Cameras() {}; + Cameras(){}; Cameras(const TransformCamera& cam) { memcpy(_cams, &cam, sizeof(TransformCamera)); }; - Cameras(const TransformCamera& camL, const TransformCamera& camR) { memcpy(_cams, &camL, sizeof(TransformCamera)); memcpy(_cams + 1, &camR, sizeof(TransformCamera)); }; + Cameras(const TransformCamera& camL, const TransformCamera& camR) { + memcpy(_cams, &camL, sizeof(TransformCamera)); + memcpy(_cams + 1, &camR, sizeof(TransformCamera)); + }; }; using CameraBufferElement = Cameras; @@ -512,25 +518,24 @@ protected: mutable std::map _drawCallInfoOffsets; - GLuint _objectBuffer { 0 }; - GLuint _cameraBuffer { 0 }; - GLuint _drawCallInfoBuffer { 0 }; - GLuint _objectBufferTexture { 0 }; - size_t _cameraUboSize { 0 }; + GLuint _objectBuffer{ 0 }; + GLuint _cameraBuffer{ 0 }; + GLuint _drawCallInfoBuffer{ 0 }; + GLuint _objectBufferTexture{ 0 }; + size_t _cameraUboSize{ 0 }; bool _viewIsCamera{ false }; - bool _skybox { false }; + bool _skybox{ false }; Transform _view; CameraCorrection _correction; bool _viewCorrectionEnabled{ true }; - Mat4 _projection; - Vec4i _viewport { 0, 0, 1, 1 }; - Vec2 _depthRange { 0.0f, 1.0f }; + Vec4i _viewport{ 0, 0, 1, 1 }; + Vec2 _depthRange{ 0.0f, 1.0f }; Vec2 _projectionJitter{ 0.0f, 0.0f }; - bool _invalidView { false }; - bool _invalidProj { false }; - bool _invalidViewport { false }; + bool _invalidView{ false }; + bool _invalidProj{ false }; + bool _invalidViewport{ false }; bool _enabledDrawcallInfoBuffer{ false }; @@ -554,7 +559,11 @@ protected: GLsizeiptr size{ 0 }; BufferState& operator=(const BufferState& other) = delete; - void reset() { gpu::gl::reset(buffer); offset = 0; size = 0; } + void reset() { + gpu::gl::reset(buffer); + offset = 0; + size = 0; + } bool compare(const BufferPointer& buffer, GLintptr offset, GLsizeiptr size) { const auto& self = *this; return (self.offset == offset && self.size == size && gpu::gl::compare(self.buffer, buffer)); @@ -562,12 +571,12 @@ protected: }; // MAX_NUM_UNIFORM_BUFFERS-1 is the max uniform index BATCHES are allowed to set, but - // MIN_REQUIRED_UNIFORM_BUFFER_BINDINGS is used here because the backend sets some - // internal UBOs for things like camera correction + // MIN_REQUIRED_UNIFORM_BUFFER_BINDINGS is used here because the backend sets some + // internal UBOs for things like camera correction std::array _buffers; } _uniform; - // Helper function that provides common code + // Helper function that provides common code void bindUniformBuffer(uint32_t slot, const BufferPointer& buffer, GLintptr offset = 0, GLsizeiptr size = 0); void releaseUniformBuffer(uint32_t slot); void resetUniformStage(); @@ -577,11 +586,10 @@ protected: virtual bool bindResourceBuffer(uint32_t slot, const BufferPointer& buffer) = 0; virtual void releaseResourceBuffer(uint32_t slot) = 0; - // Helper function that provides common code used by do_setResourceTexture and + // Helper function that provides common code used by do_setResourceTexture and // do_setResourceTextureTable (in non-bindless mode) void bindResourceTexture(uint32_t slot, const TexturePointer& texture); - // update resource cache and do the gl unbind call with the current gpu::Texture cached at slot s void releaseResourceTexture(uint32_t slot); @@ -610,19 +618,20 @@ protected: struct PipelineStageState { PipelineReference _pipeline{}; - GLuint _program { 0 }; - bool _cameraCorrection { false }; - GLShader* _programShader { nullptr }; - bool _invalidProgram { false }; + GLuint _program{ 0 }; + bool _cameraCorrection{ false }; + GLShader* _programShader{ nullptr }; + bool _invalidProgram{ false }; - BufferView _cameraCorrectionBuffer { gpu::BufferView(std::make_shared(sizeof(CameraCorrection), nullptr )) }; - BufferView _cameraCorrectionBufferIdentity { gpu::BufferView(std::make_shared(sizeof(CameraCorrection), nullptr )) }; + BufferView _cameraCorrectionBuffer{ gpu::BufferView(std::make_shared(sizeof(CameraCorrection), nullptr)) }; + BufferView _cameraCorrectionBufferIdentity{ gpu::BufferView( + std::make_shared(sizeof(CameraCorrection), nullptr)) }; State::Data _stateCache{ State::DEFAULT }; - State::Signature _stateSignatureCache { 0 }; + State::Signature _stateSignatureCache{ 0 }; - GLState* _state { nullptr }; - bool _invalidState { false }; + GLState* _state{ nullptr }; + bool _invalidState{ false }; PipelineStageState() { _cameraCorrectionBuffer.edit() = CameraCorrection(); @@ -636,9 +645,9 @@ protected: virtual GLShader* compileBackendProgram(const Shader& program, const Shader::CompilationHandler& handler); virtual GLShader* compileBackendShader(const Shader& shader, const Shader::CompilationHandler& handler); virtual std::string getBackendShaderHeader() const = 0; - // For a program, this will return a string containing all the source files (without any - // backend headers or defines). For a vertex, fragment or geometry shader, this will - // return the fully customized shader with all the version and backend specific + // For a program, this will return a string containing all the source files (without any + // backend headers or defines). For a vertex, fragment or geometry shader, this will + // return the fully customized shader with all the version and backend specific // preprocessor directives // The program string returned can be used as a key for a cache of shader binaries // The shader strings can be reliably sent to the low level `compileShader` functions @@ -654,22 +663,22 @@ protected: // Synchronize the state cache of this Backend with the actual real state of the GL Context void syncOutputStateCache(); void resetOutputStage(); - + struct OutputStageState { FramebufferReference _framebuffer{}; - GLuint _drawFBO { 0 }; + GLuint _drawFBO{ 0 }; } _output; void resetQueryStage(); struct QueryStageState { - uint32_t _rangeQueryDepth { 0 }; + uint32_t _rangeQueryDepth{ 0 }; } _queryStage; void resetStages(); // Stores cached binary versions of the shaders for quicker startup on subsequent runs - // Note that shaders in the cache can still fail to load due to hardware or driver - // changes that invalidate the cached binary, in which case we fall back on compiling + // Note that shaders in the cache can still fail to load due to hardware or driver + // changes that invalidate the cached binary, in which case we fall back on compiling // the source again struct ShaderBinaryCache { std::mutex _mutex; @@ -681,7 +690,7 @@ protected: virtual void killShaderBinaryCache(); struct TextureManagementStageState { - bool _sparseCapable { false }; + bool _sparseCapable{ false }; GLTextureTransferEnginePointer _transferEngine; } _textureManagement; virtual void initTextureManagementStage(); From 2b2091290eab1ef6ae3906c1b4f8ad1a09928826 Mon Sep 17 00:00:00 2001 From: SamGondelman Date: Wed, 12 Sep 2018 14:28:39 -0700 Subject: [PATCH 11/13] fix everything --- libraries/fbx/src/FBX.h | 1 - libraries/render-utils/src/Model.cpp | 72 +++++++++++++++------------- libraries/render-utils/src/Model.h | 8 ++-- 3 files changed, 44 insertions(+), 37 deletions(-) diff --git a/libraries/fbx/src/FBX.h b/libraries/fbx/src/FBX.h index 7bd87a4554..fdebb16bc8 100644 --- a/libraries/fbx/src/FBX.h +++ b/libraries/fbx/src/FBX.h @@ -240,7 +240,6 @@ public: QVector vertices; QVector normals; QVector tangents; - mutable QVector normalsAndTangents; // Populated later if needed for blendshapes QVector colors; QVector texCoords; QVector texCoords1; diff --git a/libraries/render-utils/src/Model.cpp b/libraries/render-utils/src/Model.cpp index 2feae1fabb..b9ed43c339 100644 --- a/libraries/render-utils/src/Model.cpp +++ b/libraries/render-utils/src/Model.cpp @@ -317,6 +317,7 @@ bool Model::updateGeometry() { needFullUpdate = true; emit rigReady(); } + return needFullUpdate; } @@ -1033,6 +1034,10 @@ void Model::removeFromScene(const render::ScenePointer& scene, render::Transacti _modelMeshMaterialNames.clear(); _modelMeshRenderItemShapes.clear(); + _blendedVertexBuffers.clear(); + _normalsAndTangents.clear(); + _blendedVertexBuffersInitialized = false; + _addedToScene = false; _renderInfoVertexCount = 0; @@ -1299,25 +1304,26 @@ Blender::Blender(ModelPointer model, int blendNumber, const Geometry::WeakPointe } void Blender::run() { - DETAILED_PROFILE_RANGE_EX(simulation_animation, __FUNCTION__, 0xFFFF0000, 0, { { "url", _model->getURL().toString() } }); QVector vertices; QVector normalsAndTangents; - auto geometry = _geometry.lock(); - if (_model && geometry) { + if (_model && _model->isLoaded()) { + DETAILED_PROFILE_RANGE_EX(simulation_animation, __FUNCTION__, 0xFFFF0000, 0, { { "url", _model->getURL().toString() } }); int offset = 0; int normalsAndTangentsOffset = 0; - auto meshes = geometry->getFBXGeometry().meshes; + auto meshes = _model->getFBXGeometry().meshes; + int meshIndex = 0; foreach (const FBXMesh& mesh, meshes) { - if (mesh.blendshapes.isEmpty()) { + auto modelMeshNormalsAndTangents = _model->_normalsAndTangents.find(meshIndex++); + if (mesh.blendshapes.isEmpty() || modelMeshNormalsAndTangents == _model->_normalsAndTangents.end()) { continue; } vertices += mesh.vertices; - normalsAndTangents += mesh.normalsAndTangents; + normalsAndTangents += modelMeshNormalsAndTangents->second; glm::vec3* meshVertices = vertices.data() + offset; NormalType* meshNormalsAndTangents = normalsAndTangents.data() + normalsAndTangentsOffset; offset += mesh.vertices.size(); - normalsAndTangentsOffset += mesh.normalsAndTangents.size(); + normalsAndTangentsOffset += modelMeshNormalsAndTangents->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); @@ -1357,9 +1363,8 @@ void Blender::run() { } // 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(const Geometry::WeakPointer&, _geometry), Q_ARG(const QVector&, vertices), - Q_ARG(const QVector&, normalsAndTangents)); + Q_ARG(ModelPointer, _model), Q_ARG(int, _blendNumber), Q_ARG(QVector, vertices), + Q_ARG(QVector, normalsAndTangents)); } void Model::setScaleToFit(bool scaleToFit, const glm::vec3& dimensions, bool forceRescale) { @@ -1537,19 +1542,18 @@ bool Model::maybeStartBlender() { return false; } -void Model::setBlendedVertices(int blendNumber, const Geometry::WeakPointer& geometry, - const QVector& vertices, const QVector& normalsAndTangents) { - auto geometryRef = geometry.lock(); - if (!geometryRef || _renderGeometry != geometryRef || blendNumber < _appliedBlendNumber || !_blendedVertexBuffersInitialized) { +void Model::setBlendedVertices(int blendNumber, const QVector& vertices, const QVector& normalsAndTangents) { + if (!isLoaded() || blendNumber < _appliedBlendNumber || !_blendedVertexBuffersInitialized) { return; } _appliedBlendNumber = blendNumber; - const FBXGeometry& fbxGeometry = geometryRef->getFBXGeometry(); + const FBXGeometry& fbxGeometry = getFBXGeometry(); int index = 0; int normalAndTangentIndex = 0; for (int i = 0; i < fbxGeometry.meshes.size(); i++) { const FBXMesh& mesh = fbxGeometry.meshes.at(i); - if (mesh.blendshapes.isEmpty()) { + auto meshNormalsAndTangents = _normalsAndTangents.find(i); + if (mesh.blendshapes.isEmpty() || meshNormalsAndTangents == _normalsAndTangents.end()) { continue; } @@ -1557,18 +1561,19 @@ void Model::setBlendedVertices(int blendNumber, const Geometry::WeakPointer& geo const auto verticesSize = vertexCount * sizeof(glm::vec3); const auto& buffer = _blendedVertexBuffers.find(i); assert(buffer != _blendedVertexBuffers.end()); - buffer->second->resize(mesh.vertices.size() * sizeof(glm::vec3) + mesh.normalsAndTangents.size() * sizeof(NormalType)); + buffer->second->resize(mesh.vertices.size() * sizeof(glm::vec3) + meshNormalsAndTangents->second.size() * sizeof(NormalType)); buffer->second->setSubData(0, verticesSize, (gpu::Byte*) vertices.constData() + index * sizeof(glm::vec3)); - buffer->second->setSubData(verticesSize, mesh.normalsAndTangents.size() * sizeof(NormalType), (const gpu::Byte*) normalsAndTangents.data() + normalAndTangentIndex * sizeof(NormalType)); + buffer->second->setSubData(verticesSize, meshNormalsAndTangents->second.size() * sizeof(NormalType), (const gpu::Byte*) normalsAndTangents.data() + normalAndTangentIndex * sizeof(NormalType)); index += vertexCount; - normalAndTangentIndex += mesh.normalsAndTangents.size(); + normalAndTangentIndex += meshNormalsAndTangents->second.size(); } } void Model::deleteGeometry() { _deleteGeometryCounter++; _blendedVertexBuffers.clear(); + _normalsAndTangents.clear(); _blendedVertexBuffersInitialized = false; _meshStates.clear(); _rig.destroyAnimGraph(); @@ -1634,7 +1639,7 @@ void Model::initializeBlendshapes(const FBXMesh& mesh, int index) { _blendedVertexBuffers[index]->resize(mesh.vertices.size() * sizeof(glm::vec3) + normalsAndTangents.size() * sizeof(NormalType)); _blendedVertexBuffers[index]->setSubData(0, verticesSize, (const gpu::Byte*) mesh.vertices.constData()); _blendedVertexBuffers[index]->setSubData(verticesSize, normalsAndTangents.size() * sizeof(NormalType), (const gpu::Byte*) normalsAndTangents.data()); - mesh.normalsAndTangents = normalsAndTangents; + _normalsAndTangents[index] = normalsAndTangents; } void Model::createRenderItemSet() { @@ -1778,35 +1783,38 @@ ModelBlender::~ModelBlender() { } void ModelBlender::noteRequiresBlend(ModelPointer model) { + Lock lock(_mutex); if (_pendingBlenders < QThread::idealThreadCount()) { if (model->maybeStartBlender()) { _pendingBlenders++; + return; } - return; } - { - Lock lock(_mutex); - _modelsRequiringBlends.insert(model); - } + _modelsRequiringBlends.insert(model); } -void ModelBlender::setBlendedVertices(ModelPointer model, int blendNumber, const Geometry::WeakPointer& geometry, - const QVector& vertices, const QVector& normalsAndTangents) { +void ModelBlender::setBlendedVertices(ModelPointer model, int blendNumber, QVector vertices, QVector normalsAndTangents) { if (model) { - model->setBlendedVertices(blendNumber, geometry, vertices, normalsAndTangents); + model->setBlendedVertices(blendNumber, vertices, normalsAndTangents); } - _pendingBlenders--; { Lock lock(_mutex); - for (auto i = _modelsRequiringBlends.begin(); i != _modelsRequiringBlends.end();) { + _pendingBlenders--; + _modelsRequiringBlends.erase(model); + std::set> modelsToErase; + for (auto i = _modelsRequiringBlends.begin(); i != _modelsRequiringBlends.end(); i++) { auto weakPtr = *i; - _modelsRequiringBlends.erase(i++); // remove front of the set ModelPointer nextModel = weakPtr.lock(); if (nextModel && nextModel->maybeStartBlender()) { _pendingBlenders++; - return; + break; + } else { + modelsToErase.insert(weakPtr); } } + for (auto& weakPtr : modelsToErase) { + _modelsRequiringBlends.erase(weakPtr); + } } } diff --git a/libraries/render-utils/src/Model.h b/libraries/render-utils/src/Model.h index f35048aa2f..447f75dd9d 100644 --- a/libraries/render-utils/src/Model.h +++ b/libraries/render-utils/src/Model.h @@ -144,8 +144,7 @@ public: bool maybeStartBlender(); /// Sets blended vertices computed in a separate thread. - void setBlendedVertices(int blendNumber, const Geometry::WeakPointer& geometry, - const QVector& vertices, const QVector& normalsAndTangents); + void setBlendedVertices(int blendNumber, const QVector& vertices, const QVector& normalsAndTangents); bool isLoaded() const { return (bool)_renderGeometry && _renderGeometry->isGeometryLoaded(); } bool isAddedToScene() const { return _addedToScene; } @@ -345,6 +344,8 @@ public: void addMaterial(graphics::MaterialLayer material, const std::string& parentMaterialName); void removeMaterial(graphics::MaterialPointer material, const std::string& parentMaterialName); + std::unordered_map> _normalsAndTangents; + public slots: void loadURLFinished(bool success); @@ -519,8 +520,7 @@ public: bool shouldComputeBlendshapes() { return _computeBlendshapes; } public slots: - void setBlendedVertices(ModelPointer model, int blendNumber, const Geometry::WeakPointer& geometry, - const QVector& vertices, const QVector& normalsAndTangents); + void setBlendedVertices(ModelPointer model, int blendNumber, QVector vertices, QVector normalsAndTangents); void setComputeBlendshapes(bool computeBlendshapes) { _computeBlendshapes = computeBlendshapes; } private: From be650d5af0c9293791b315d87bfcc924c2c42d8f Mon Sep 17 00:00:00 2001 From: Wayne Chen Date: Wed, 12 Sep 2018 16:00:43 -0700 Subject: [PATCH 12/13] changing when to log - there is broken code in bodyLoader --- .../qml/LoginDialog/LinkAccountBody.qml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/interface/resources/qml/LoginDialog/LinkAccountBody.qml b/interface/resources/qml/LoginDialog/LinkAccountBody.qml index 4196b7f168..6c0a7046e6 100644 --- a/interface/resources/qml/LoginDialog/LinkAccountBody.qml +++ b/interface/resources/qml/LoginDialog/LinkAccountBody.qml @@ -347,6 +347,12 @@ Item { onHandleLoginCompleted: { console.log("Login Succeeded, linking steam account") + if (Settings.getValue("loginDialogPoppedUp", false)) { + var data = { + "action": "user logged in" + }; + UserActivityLogger.logAction("encourageLoginDialog", data); + } if (loginDialog.isSteamRunning()) { loginDialog.linkSteam() } else { @@ -354,23 +360,17 @@ Item { bodyLoader.item.width = root.pane.width bodyLoader.item.height = root.pane.height } - if (Settings.getValue("loginDialogPoppedUp", false)) { - var data = { - "action": "user logged in" - }; - UserActivityLogger.logAction("encourageLoginDialog", data); - } } onHandleLoginFailed: { console.log("Login Failed") - mainTextContainer.visible = true - toggleLoading(false) if (Settings.getValue("loginDialogPoppedUp", false)) { var data = { "action": "user failed logging in" }; UserActivityLogger.logAction("encourageLoginDialog", data); } + mainTextContainer.visible = true + toggleLoading(false) } onHandleLinkCompleted: { console.log("Link Succeeded") From fc007982e0ca606987d44c29f4de4992539306b4 Mon Sep 17 00:00:00 2001 From: Wayne Chen Date: Wed, 12 Sep 2018 21:25:41 -0700 Subject: [PATCH 13/13] muting opt out activity if login fail/success --- .../resources/qml/LoginDialog/LinkAccountBody.qml | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/interface/resources/qml/LoginDialog/LinkAccountBody.qml b/interface/resources/qml/LoginDialog/LinkAccountBody.qml index 6c0a7046e6..57293cb5e3 100644 --- a/interface/resources/qml/LoginDialog/LinkAccountBody.qml +++ b/interface/resources/qml/LoginDialog/LinkAccountBody.qml @@ -346,12 +346,14 @@ Item { target: loginDialog onHandleLoginCompleted: { console.log("Login Succeeded, linking steam account") - - if (Settings.getValue("loginDialogPoppedUp", false)) { + var poppedUp = Settings.getValue("loginDialogPoppedUp", false); + if (poppedUp) { + console.log("[ENCOURAGELOGINDIALOG]: logging in") var data = { "action": "user logged in" }; UserActivityLogger.logAction("encourageLoginDialog", data); + Settings.setValue("loginDialogPoppedUp", false); } if (loginDialog.isSteamRunning()) { loginDialog.linkSteam() @@ -363,11 +365,14 @@ Item { } onHandleLoginFailed: { console.log("Login Failed") - if (Settings.getValue("loginDialogPoppedUp", false)) { + var poppedUp = Settings.getValue("loginDialogPoppedUp", false); + if (poppedUp) { + console.log("[ENCOURAGELOGINDIALOG]: failed logging in") var data = { "action": "user failed logging in" }; UserActivityLogger.logAction("encourageLoginDialog", data); + Settings.setValue("loginDialogPoppedUp", false); } mainTextContainer.visible = true toggleLoading(false)