Code review feedback

libraries/render-utils/src/CauterizedModel.cpp

@@ -275,7 +275,7 @@ void CauterizedModel::updateRenderItems() {
                     data.setEnableCauterization(enableCauterization);
                     data.updateKey(isVisible, isLayeredInFront || isLayeredInHUD, render::ItemKey::TAG_BITS_ALL);
                     data.setLayer(isLayeredInFront, isLayeredInHUD);
-                    data.setShapeKey(invalidatePayloadShapeKey, isWireframe);
+                    data.setShapeKey(invalidatePayloadShapeKey, isWireframe, useDualQuaternionSkinning);
                 });
             }
 

libraries/render-utils/src/MeshPartPayload.cpp

@@ -223,7 +223,7 @@ ModelMeshPartPayload::ModelMeshPartPayload(ModelPointer model, int meshIndex, in
 
     assert(model && model->isLoaded());
 
-    _useDualQuaternionSkinning = model->getUseDualQuaternionSkinning();
+    bool useDualQuaternionSkinning = model->getUseDualQuaternionSkinning();
 
     _blendedVertexBuffer = model->_blendedVertexBuffers[_meshIndex];
     auto& modelMesh = model->getGeometry()->getMeshes().at(_meshIndex);
@@ -231,7 +231,7 @@ ModelMeshPartPayload::ModelMeshPartPayload(ModelPointer model, int meshIndex, in
 
     updateMeshPart(modelMesh, partIndex);
 
-    if (_useDualQuaternionSkinning) {
+    if (useDualQuaternionSkinning) {
         computeAdjustedLocalBound(state.clusterDualQuaternions);
     } else {
         computeAdjustedLocalBound(state.clusterMatrices);
@@ -239,7 +239,7 @@ ModelMeshPartPayload::ModelMeshPartPayload(ModelPointer model, int meshIndex, in
 
     updateTransform(transform, offsetTransform);
     Transform renderTransform = transform;
-    if (_useDualQuaternionSkinning) {
+    if (useDualQuaternionSkinning) {
         if (state.clusterDualQuaternions.size() == 1) {
             const auto& dq = state.clusterDualQuaternions[0];
             Transform transform(dq.getRotation(),
@@ -281,6 +281,13 @@ void ModelMeshPartPayload::notifyLocationChanged() {
 }
 
 void ModelMeshPartPayload::updateClusterBuffer(const std::vector<glm::mat4>& clusterMatrices) {
+
+    // reset cluster buffer if we change the cluster buffer type
+    if (_clusterBufferType != ClusterBufferType::Matrices) {
+        _clusterBuffer.reset();
+    }
+    _clusterBufferType = ClusterBufferType::Matrices;
+
     // Once computed the cluster matrices, update the buffer(s)
     if (clusterMatrices.size() > 1) {
         if (!_clusterBuffer) {
@@ -295,6 +302,13 @@ void ModelMeshPartPayload::updateClusterBuffer(const std::vector<glm::mat4>& clu
 }
 
 void ModelMeshPartPayload::updateClusterBuffer(const std::vector<Model::TransformDualQuaternion>& clusterDualQuaternions) {
+
+    // reset cluster buffer if we change the cluster buffer type
+    if (_clusterBufferType != ClusterBufferType::DualQuaternions) {
+        _clusterBuffer.reset();
+    }
+    _clusterBufferType = ClusterBufferType::DualQuaternions;
+
     // Once computed the cluster matrices, update the buffer(s)
     if (clusterDualQuaternions.size() > 1) {
         if (!_clusterBuffer) {
@@ -360,7 +374,7 @@ int ModelMeshPartPayload::getLayer() const {
     return _layer;
 }
 
-void ModelMeshPartPayload::setShapeKey(bool invalidateShapeKey, bool isWireframe) {
+void ModelMeshPartPayload::setShapeKey(bool invalidateShapeKey, bool isWireframe, bool useDualQuaternionSkinning) {
     if (invalidateShapeKey) {
         _shapeKey = ShapeKey::Builder::invalid();
         return;
@@ -407,7 +421,7 @@ void ModelMeshPartPayload::setShapeKey(bool invalidateShapeKey, bool isWireframe
     if (isWireframe) {
         builder.withWireframe();
     }
-    if (_useDualQuaternionSkinning && isSkinned) {
+    if (isSkinned && useDualQuaternionSkinning) {
         builder.withDualQuatSkinned();
     }
 
@@ -497,10 +511,3 @@ void ModelMeshPartPayload::computeAdjustedLocalBound(const std::vector<Model::Tr
         }
     }
 }
-
-void ModelMeshPartPayload::setUseDualQuaternionSkinning(bool value) {
-    if (value != _useDualQuaternionSkinning) {
-        _clusterBuffer.reset();
-    }
-    _useDualQuaternionSkinning = value;
-}

libraries/render-utils/src/MeshPartPayload.h

@@ -108,7 +108,7 @@ public:
     void render(RenderArgs* args) override;
 
     void setLayer(bool isLayeredInFront, bool isLayeredInHUD);
-    void setShapeKey(bool invalidateShapeKey, bool isWireframe);
+    void setShapeKey(bool invalidateShapeKey, bool isWireframe, bool useDualQuaternionSkinning);
 
     // ModelMeshPartPayload functions to perform render
     void bindMesh(gpu::Batch& batch) override;
@@ -120,17 +120,17 @@ public:
     // dual quaternion skinning
     void computeAdjustedLocalBound(const std::vector<Model::TransformDualQuaternion>& clusterDualQuaternions);
 
-    void setUseDualQuaternionSkinning(bool value);
-
     gpu::BufferPointer _clusterBuffer;
 
+    enum class ClusterBufferType { Matrices, DualQuaternions };
+    ClusterBufferType _clusterBufferType { ClusterBufferType::Matrices };
+
     int _meshIndex;
     int _shapeID;
 
     bool _isSkinned{ false };
     bool _isBlendShaped { false };
     bool _hasTangents { false };
-    bool _useDualQuaternionSkinning { false };
 
 private:
     void initCache(const ModelPointer& model);

libraries/render-utils/src/Model.cpp

@@ -288,7 +288,6 @@ void Model::updateRenderItems() {
                                                                   invalidatePayloadShapeKey, isWireframe, isVisible,
                                                                   viewTagBits, isLayeredInFront,
                                                                   isLayeredInHUD, isGroupCulled](ModelMeshPartPayload& data) {
-                data.setUseDualQuaternionSkinning(useDualQuaternionSkinning);
                 if (useDualQuaternionSkinning) {
                     data.updateClusterBuffer(meshState.clusterDualQuaternions);
                 } else {
@@ -314,7 +313,7 @@ void Model::updateRenderItems() {
 
                 data.updateKey(isVisible, isLayeredInFront || isLayeredInHUD, viewTagBits, isGroupCulled);
                 data.setLayer(isLayeredInFront, isLayeredInHUD);
-                data.setShapeKey(invalidatePayloadShapeKey, isWireframe);
+                data.setShapeKey(invalidatePayloadShapeKey, isWireframe, useDualQuaternionSkinning);
             });
         }
 
@@ -1591,8 +1590,7 @@ void Model::addMaterial(graphics::MaterialLayer material, const std::string& par
                 data.addMaterial(material);
                 // if the material changed, we might need to update our item key or shape key
                 data.updateKey(visible, layeredInFront || layeredInHUD, viewTagBits);
-                data.setUseDualQuaternionSkinning(useDualQuaternionSkinning);
+                data.setShapeKey(invalidatePayloadShapeKey, wireframe, useDualQuaternionSkinning);
-                data.setShapeKey(invalidatePayloadShapeKey, wireframe);
             });
         }
     }
@@ -1618,8 +1616,7 @@ void Model::removeMaterial(graphics::MaterialPointer material, const std::string
                 data.removeMaterial(material);
                 // if the material changed, we might need to update our item key or shape key
                 data.updateKey(visible, layeredInFront || layeredInHUD, viewTagBits);
-                data.setUseDualQuaternionSkinning(useDualQuaternionSkinning);
+                data.setShapeKey(invalidatePayloadShapeKey, wireframe, useDualQuaternionSkinning);
-                data.setShapeKey(invalidatePayloadShapeKey, wireframe);
             });
         }
     }

libraries/shared/src/GLMHelpers.cpp

@@ -602,5 +602,5 @@ glm::vec3 randVector() {
 }
 
 bool isNonUniformScale(const glm::vec3& scale) {
-    return fabsf(scale.x - scale.y) > EPSILON || fabsf(scale.y - scale.z) > EPSILON;
+    return fabsf(scale.x - scale.y) > EPSILON || fabsf(scale.y - scale.z) > EPSILON || fabsf(scale.z - scale.x);
 }