Fix LLGLTFMaterial hashing

This PR fixes the non-working material hashing for LLGLTFMaterial instances.

There are several issues in the current code, stemming to the fact that the hashing is performed on the block of the member variables:

1.- There are padding bytes between member variables, even after rearranging them to avoid most of the padding; in particular, the std::array's size is not a multiple of 4 bytes (64 bits), and most compilers will pad them to the next 4-byte aligment as a result. Note that C++ standards do not impose the zeroing of padding bytes on construction of a class instance, with only a couple exceptions (such as explicit zero-initialization). Those bytes MUST therefore be zeroed by us on construction.
2.- The TextureTransform strutcure getPacked() method did not touch some of the packed bytes, and as a result could *potentially* cause an issue for hashing when applied to a transform of another material instance.
3.- With the recent addition of the local textures tracking map, the said map cannot be hashed as a block of memory (map pairs will typically be allocated on the heap or on the stack, not in the memory block used by member variables).

This PR solves all these issues and offers proper hashing of LLGLTFMaterial instances.

1 files changed, 90 insertions, 19 deletions

diff --git a/indra/llprimitive/llgltfmaterial.cpp b/indra/llprimitive/llgltfmaterial.cpp
index ae165f7fa4..e9d3350ee9 100644
--- a/indra/llprimitive/llgltfmaterial.cpp
+++ b/indra/llprimitive/llgltfmaterial.cpp
@@ -47,16 +47,49 @@ const char* const LLGLTFMaterial::GLTF_FILE_EXTENSION_TRANSFORM_ROTATION = "rota
 // special UUID that indicates a null UUID in override data
 const LLUUID LLGLTFMaterial::GLTF_OVERRIDE_NULL_UUID = LLUUID("ffffffff-ffff-ffff-ffff-ffffffffffff");
 
+LLGLTFMaterial::LLGLTFMaterial()
+{
+    // IMPORTANT: since we use the hash of the member variables memory block of
+    // this class to detect changes, we must ensure that all its padding bytes
+    // have been zeroed out. But of course, we must leave the LLRefCount member
+    // variable untouched (and skip it when hashing), and we cannot either
+    // touch the local texture overrides map (else we destroy pointers, and
+    // sundry private data, which would lead to a crash when using that map).
+    // The variable members have therefore been arranged so that anything,
+    // starting at mLocalTexDataDigest and up to the end of the members, can be
+    // safely zeroed. HB
+    const size_t offset = intptr_t(&mLocalTexDataDigest) - intptr_t(this);
+    memset((void*)((const char*)this + offset), 0, sizeof(*this) - offset);
+
+    // Now that we zeroed out our member variables, we can set the ones that
+    // should not be zero to their default value. HB
+    mBaseColor.set(1.f, 1.f, 1.f, 1.f);
+    mMetallicFactor = mRoughnessFactor = 1.f;
+    mAlphaCutoff = 0.5f;
+    for (U32 i = 0; i < GLTF_TEXTURE_INFO_COUNT; ++i)
+    {
+        mTextureTransform[i].mScale.set(1.f, 1.f);
+#if 0
+        mTextureTransform[i].mOffset.clear();
+        mTextureTransform[i].mRotation = 0.f;
+#endif    
+    }
+#if 0
+    mLocalTexDataDigest = 0;
+    mAlphaMode = ALPHA_MODE_OPAQUE;    // This is 0
+    mOverrideDoubleSided = mOverrideAlphaMode = false;
+#endif
+}
+
 void LLGLTFMaterial::TextureTransform::getPacked(F32 (&packed)[8]) const
 {
     packed[0] = mScale.mV[VX];
     packed[1] = mScale.mV[VY];
     packed[2] = mRotation;
-    // packed[3] = unused
     packed[4] = mOffset.mV[VX];
     packed[5] = mOffset.mV[VY];
-    // packed[6] = unused
-    // packed[7] = unused
+    // Not used but nonetheless zeroed for proper hashing. HB
+    packed[3] = packed[6] = packed[7] = 0.f;
 }
 
 bool LLGLTFMaterial::TextureTransform::operator==(const TextureTransform& other) const
@@ -89,13 +122,37 @@ LLGLTFMaterial& LLGLTFMaterial::operator=(const LLGLTFMaterial& rhs)
     mOverrideDoubleSided = rhs.mOverrideDoubleSided;
     mOverrideAlphaMode = rhs.mOverrideAlphaMode;
 
-    mTrackingIdToLocalTexture = rhs.mTrackingIdToLocalTexture;
-
-    updateTextureTracking();
+    if (rhs.mTrackingIdToLocalTexture.empty())
+    {
+        mTrackingIdToLocalTexture.clear();
+        mLocalTexDataDigest = 0;
+    }
+    else
+    {
+        mTrackingIdToLocalTexture = rhs.mTrackingIdToLocalTexture;
+        updateLocalTexDataDigest();
+        updateTextureTracking();
+    }
 
     return *this;
 }
 
+void LLGLTFMaterial::updateLocalTexDataDigest()
+{
+    mLocalTexDataDigest = 0;
+    if (!mTrackingIdToLocalTexture.empty())
+    {
+        for (local_tex_map_t::const_iterator
+                it = mTrackingIdToLocalTexture.begin(),
+                end = mTrackingIdToLocalTexture.end();
+             it != end; ++it)
+        {
+            mLocalTexDataDigest ^= it->first.getDigest64() ^
+                                   it->second.getDigest64();
+        }
+    }
+}
+
 bool LLGLTFMaterial::operator==(const LLGLTFMaterial& rhs) const
 {
     return mTextureId == rhs.mTextureId &&
@@ -547,7 +604,7 @@ void LLGLTFMaterial::applyOverride(const LLGLTFMaterial& override_mat)
 {
     LL_PROFILE_ZONE_SCOPED;
 
-    for (int i = 0; i < GLTF_TEXTURE_INFO_COUNT; ++i)
+    for (U32 i = 0; i < GLTF_TEXTURE_INFO_COUNT; ++i)
     {
         LLUUID& texture_id = mTextureId[i];
         const LLUUID& override_texture_id = override_mat.mTextureId[i];
@@ -588,7 +645,7 @@ void LLGLTFMaterial::applyOverride(const LLGLTFMaterial& override_mat)
         mDoubleSided = override_mat.mDoubleSided;
     }
 
-    for (int i = 0; i < GLTF_TEXTURE_INFO_COUNT; ++i)
+    for (U32 i = 0; i < GLTF_TEXTURE_INFO_COUNT; ++i)
     {
         if (override_mat.mTextureTransform[i].mOffset != getDefaultTextureOffset())
         {
@@ -606,9 +663,13 @@ void LLGLTFMaterial::applyOverride(const LLGLTFMaterial& override_mat)
         }
     }
 
-    mTrackingIdToLocalTexture.insert(override_mat.mTrackingIdToLocalTexture.begin(), override_mat.mTrackingIdToLocalTexture.begin());
-
-    updateTextureTracking();
+    if (!override_mat.mTrackingIdToLocalTexture.empty())
+    {
+        auto it = override_mat.mTrackingIdToLocalTexture.begin();
+        mTrackingIdToLocalTexture.insert(it, it);
+        updateLocalTexDataDigest();
+        updateTextureTracking();
+    }
 }
 
 void LLGLTFMaterial::getOverrideLLSD(const LLGLTFMaterial& override_mat, LLSD& data)
@@ -618,7 +679,7 @@ void LLGLTFMaterial::getOverrideLLSD(const LLGLTFMaterial& override_mat, LLSD& d
 
     // make every effort to shave bytes here
 
-    for (int i = 0; i < GLTF_TEXTURE_INFO_COUNT; ++i)
+    for (U32 i = 0; i < GLTF_TEXTURE_INFO_COUNT; ++i)
     {
         LLUUID& texture_id = mTextureId[i];
         const LLUUID& override_texture_id = override_mat.mTextureId[i];
@@ -663,7 +724,7 @@ void LLGLTFMaterial::getOverrideLLSD(const LLGLTFMaterial& override_mat, LLSD& d
         data["ds"] = override_mat.mDoubleSided;
     }
 
-    for (int i = 0; i < GLTF_TEXTURE_INFO_COUNT; ++i)
+    for (U32 i = 0; i < GLTF_TEXTURE_INFO_COUNT; ++i)
     {
         if (override_mat.mTextureTransform[i].mOffset != getDefaultTextureOffset())
         {
@@ -742,7 +803,7 @@ void LLGLTFMaterial::applyOverrideLLSD(const LLSD& data)
     const LLSD& ti = data["ti"];
     if (ti.isArray())
     {
-        for (int i = 0; i < GLTF_TEXTURE_INFO_COUNT; ++i)
+        for (U32 i = 0; i < GLTF_TEXTURE_INFO_COUNT; ++i)
         {
             const LLSD& o = ti[i]["o"];
             if (o.isDefined())
@@ -768,27 +829,36 @@ void LLGLTFMaterial::applyOverrideLLSD(const LLSD& data)
 LLUUID LLGLTFMaterial::getHash() const
 {
     LL_PROFILE_ZONE_SCOPED_CATEGORY_TEXTURE;
-    // HACK - hash the bytes of this object but don't include the ref count
-    LLUUID hash;
-    HBXXH128::digest(hash, (unsigned char*)this + sizeof(LLRefCount), sizeof(*this) - sizeof(LLRefCount));
-    return hash;
+    // *HACK: hash the bytes of this object but do not include the ref count
+    // neither the local texture overrides (which is a map, with pointers to
+    // key/value pairs that would change from one LLGLTFMaterial instance to
+    // the other, even though the key/value pairs could be the same, and stored
+    // elsewhere in the memory heap or on the stack).
+    // Note: this does work properly to compare two LLGLTFMaterial instances
+    // only because the padding bytes between their member variables have been
+    // dutifully zeroed in the constructor. HB
+    const size_t offset = intptr_t(&mLocalTexDataDigest) - intptr_t(this);
+    return HBXXH128::digest((const void*)((const char*)this + offset),
+                            sizeof(*this) - offset);
 }
 
 void LLGLTFMaterial::addLocalTextureTracking(const LLUUID& tracking_id, const LLUUID& tex_id)
 {
     mTrackingIdToLocalTexture[tracking_id] = tex_id;
+    updateLocalTexDataDigest();
 }
 
 void LLGLTFMaterial::removeLocalTextureTracking(const LLUUID& tracking_id)
 {
     mTrackingIdToLocalTexture.erase(tracking_id);
+    updateLocalTexDataDigest();
 }
 
 bool LLGLTFMaterial::replaceLocalTexture(const LLUUID& tracking_id, const LLUUID& old_id, const LLUUID& new_id)
 {
     bool res = false;
 
-    for (int i = 0; i < GLTF_TEXTURE_INFO_COUNT; ++i)
+    for (U32 i = 0; i < GLTF_TEXTURE_INFO_COUNT; ++i)
     {
         if (mTextureId[i] == old_id)
         {
@@ -809,6 +879,7 @@ bool LLGLTFMaterial::replaceLocalTexture(const LLUUID& tracking_id, const LLUUID
     {
         mTrackingIdToLocalTexture.erase(tracking_id);
     }
+    updateLocalTexDataDigest();
 
     return res;
 }