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.

2 files changed, 128 insertions, 43 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;
 }
diff --git a/indra/llprimitive/llgltfmaterial.h b/indra/llprimitive/llgltfmaterial.h
index 02f62fb08c..9f817d6a19 100644
--- a/indra/llprimitive/llgltfmaterial.h
+++ b/indra/llprimitive/llgltfmaterial.h
@@ -81,7 +81,7 @@ public:
         ALPHA_MODE_MASK
     };
 
-    LLGLTFMaterial() {}
+    LLGLTFMaterial();
     LLGLTFMaterial(const LLGLTFMaterial& rhs);
 
     LLGLTFMaterial& operator=(const LLGLTFMaterial& rhs);
@@ -110,25 +110,6 @@ public:
     static const char* const GLTF_FILE_EXTENSION_TRANSFORM_ROTATION;
     static const LLUUID GLTF_OVERRIDE_NULL_UUID;
 
-    std::array<LLUUID, GLTF_TEXTURE_INFO_COUNT> mTextureId;
-    std::array<TextureTransform, GLTF_TEXTURE_INFO_COUNT> mTextureTransform;
-
-    // NOTE: initialize values to defaults according to the GLTF spec
-    // NOTE: these values should be in linear color space
-    LLColor4 mBaseColor = LLColor4(1, 1, 1, 1);
-    LLColor3 mEmissiveColor = LLColor3(0, 0, 0);
-
-    F32 mMetallicFactor = 1.f;
-    F32 mRoughnessFactor = 1.f;
-    F32 mAlphaCutoff = 0.5f;
-
-    bool mDoubleSided = false;
-    AlphaMode mAlphaMode = ALPHA_MODE_OPAQUE;
-
-    // override specific flags for state that can't use off-by-epsilon or UUID hack
-    bool mOverrideDoubleSided = false;
-    bool mOverrideAlphaMode = false;
-
     // get a UUID based on a hash of this LLGLTFMaterial
     LLUUID getHash() const;
 
@@ -229,10 +210,6 @@ public:
     virtual bool replaceLocalTexture(const LLUUID& tracking_id, const LLUUID &old_id, const LLUUID& new_id);
     virtual void updateTextureTracking();
 
-    // These fields are local to viewer and are a part of local bitmap support
-    typedef std::map<LLUUID, LLUUID> local_tex_map_t;
-    local_tex_map_t mTrackingIdToLocalTexture;
-
 protected:
     static LLVector2 vec2FromJson(const std::map<std::string, tinygltf::Value>& object, const char* key, const LLVector2& default_value);
     static F32 floatFromJson(const std::map<std::string, tinygltf::Value>& object, const char* key, const F32 default_value);
@@ -249,4 +226,41 @@ protected:
     void writeToTexture(tinygltf::Model& model, T& texture_info, TextureInfo texture_info_id, bool force_write = false) const;
     template<typename T>
     static void writeToTexture(tinygltf::Model& model, T& texture_info, const LLUUID& texture_id, const TextureTransform& transform, bool force_write = false);
+
+    // Used to update the digest of the mTrackingIdToLocalTexture map each time
+    // it is changed; this way, that digest can be used by the fast getHash()
+    // method intsead of having to hash all individual keys and values. HB
+    void updateLocalTexDataDigest();
+
+public:
+    // These fields are local to viewer and are a part of local bitmap support
+    // IMPORTANT: do not move this member down (and do not move
+    // mLocalTexDataDigest either): the getHash() method does rely on the
+    // current ordering. HB
+    typedef std::map<LLUUID, LLUUID> local_tex_map_t;
+    local_tex_map_t mTrackingIdToLocalTexture;
+
+    // Used to store a digest of mTrackingIdToLocalTexture when the latter is
+    // not empty, or zero otherwise. HB
+    U64 mLocalTexDataDigest;
+
+    std::array<LLUUID, GLTF_TEXTURE_INFO_COUNT> mTextureId;
+    std::array<TextureTransform, GLTF_TEXTURE_INFO_COUNT> mTextureTransform;
+
+    // NOTE: initialize values to defaults according to the GLTF spec
+    // NOTE: these values should be in linear color space
+    LLColor4 mBaseColor;
+    LLColor3 mEmissiveColor;
+
+    F32 mMetallicFactor;
+    F32 mRoughnessFactor;
+    F32 mAlphaCutoff;
+
+    AlphaMode mAlphaMode;
+    bool mDoubleSided;
+
+    // Override specific flags for state that can't use off-by-epsilon or UUID
+    // hack
+    bool mOverrideDoubleSided;
+    bool mOverrideAlphaMode;
 };