From 8eb2d17c7715dbe72d4719d7ed3dd05f8cb4b65e Mon Sep 17 00:00:00 2001 From: Henri Beauchamp Date: Thu, 30 Nov 2023 13:23:57 +0100 Subject: 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. --- indra/llprimitive/llgltfmaterial.cpp | 109 +++++++++++++++++++++++++++++------ 1 file changed, 90 insertions(+), 19 deletions(-) (limited to 'indra/llprimitive/llgltfmaterial.cpp') 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; } -- cgit v1.2.3 From 9441608623a2692263191c254db23765eefa2cef Mon Sep 17 00:00:00 2001 From: Cosmic Linden Date: Mon, 6 May 2024 10:07:02 -0700 Subject: secondlife/viewer#907: Local PBR terrain texture transforms --- indra/llprimitive/llgltfmaterial.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) (limited to 'indra/llprimitive/llgltfmaterial.cpp') diff --git a/indra/llprimitive/llgltfmaterial.cpp b/indra/llprimitive/llgltfmaterial.cpp index 12af568b7e..94bc5ef74c 100644 --- a/indra/llprimitive/llgltfmaterial.cpp +++ b/indra/llprimitive/llgltfmaterial.cpp @@ -81,7 +81,7 @@ LLGLTFMaterial::LLGLTFMaterial() #endif } -void LLGLTFMaterial::TextureTransform::getPacked(F32 (&packed)[8]) const +void LLGLTFMaterial::TextureTransform::getPacked(Pack& packed) const { packed[0] = mScale.mV[VX]; packed[1] = mScale.mV[VY]; @@ -92,6 +92,15 @@ void LLGLTFMaterial::TextureTransform::getPacked(F32 (&packed)[8]) const packed[3] = packed[6] = packed[7] = 0.f; } +void LLGLTFMaterial::TextureTransform::getPackedTight(PackTight& packed) const +{ + packed[0] = mScale.mV[VX]; + packed[1] = mScale.mV[VY]; + packed[2] = mRotation; + packed[3] = mOffset.mV[VX]; + packed[4] = mOffset.mV[VY]; +} + bool LLGLTFMaterial::TextureTransform::operator==(const TextureTransform& other) const { return mOffset == other.mOffset && mScale == other.mScale && mRotation == other.mRotation; -- cgit v1.2.3 From b42f9d836b4c0f7fbd4bdae1734021e2a09fdbe8 Mon Sep 17 00:00:00 2001 From: Ansariel Date: Sat, 1 Jun 2024 15:49:26 +0200 Subject: Re-enable a lot of compiler warnings for MSVC and address the C4267 "possible loss of precision" warnings --- indra/llprimitive/llgltfmaterial.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/llprimitive/llgltfmaterial.cpp') diff --git a/indra/llprimitive/llgltfmaterial.cpp b/indra/llprimitive/llgltfmaterial.cpp index 12af568b7e..dd4ab82abc 100644 --- a/indra/llprimitive/llgltfmaterial.cpp +++ b/indra/llprimitive/llgltfmaterial.cpp @@ -180,7 +180,7 @@ bool LLGLTFMaterial::fromJSON(const std::string& json, std::string& warn_msg, st tinygltf::Model model_in; - if (gltf.LoadASCIIFromString(&model_in, &error_msg, &warn_msg, json.c_str(), json.length(), "")) + if (gltf.LoadASCIIFromString(&model_in, &error_msg, &warn_msg, json.c_str(), static_cast(json.length()), "")) { setFromModel(model_in, 0); -- cgit v1.2.3 From 6de0086ae9c678dafa8a26eb117279a487860f2f Mon Sep 17 00:00:00 2001 From: Cosmic Linden Date: Wed, 15 May 2024 19:27:37 -0700 Subject: secondlife/viewer#1475: Add PBR terrain repeats editing --- indra/llprimitive/llgltfmaterial.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'indra/llprimitive/llgltfmaterial.cpp') diff --git a/indra/llprimitive/llgltfmaterial.cpp b/indra/llprimitive/llgltfmaterial.cpp index 94bc5ef74c..008c72462c 100644 --- a/indra/llprimitive/llgltfmaterial.cpp +++ b/indra/llprimitive/llgltfmaterial.cpp @@ -681,7 +681,7 @@ void LLGLTFMaterial::applyOverride(const LLGLTFMaterial& override_mat) } } -void LLGLTFMaterial::getOverrideLLSD(const LLGLTFMaterial& override_mat, LLSD& data) +void LLGLTFMaterial::getOverrideLLSD(const LLGLTFMaterial& override_mat, LLSD& data) const { LL_PROFILE_ZONE_SCOPED; llassert(data.isUndefined()); @@ -690,7 +690,7 @@ void LLGLTFMaterial::getOverrideLLSD(const LLGLTFMaterial& override_mat, LLSD& d for (U32 i = 0; i < GLTF_TEXTURE_INFO_COUNT; ++i) { - LLUUID& texture_id = mTextureId[i]; + const LLUUID& texture_id = mTextureId[i]; const LLUUID& override_texture_id = override_mat.mTextureId[i]; if (override_texture_id.notNull() && override_texture_id != texture_id) { -- cgit v1.2.3 From c95b4bf3ea2b681d6d05468b07e60fedb71fa2cf Mon Sep 17 00:00:00 2001 From: Andrey Lihatskiy Date: Mon, 10 Jun 2024 20:42:42 +0300 Subject: Post-merge - trim trailing whitespace --- indra/llprimitive/llgltfmaterial.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'indra/llprimitive/llgltfmaterial.cpp') diff --git a/indra/llprimitive/llgltfmaterial.cpp b/indra/llprimitive/llgltfmaterial.cpp index dd4ab82abc..e6cc070114 100644 --- a/indra/llprimitive/llgltfmaterial.cpp +++ b/indra/llprimitive/llgltfmaterial.cpp @@ -5,21 +5,21 @@ * $LicenseInfo:firstyear=2022&license=viewerlgpl$ * Second Life Viewer Source Code * Copyright (C) 2022, Linden Research, Inc. - * + * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public * License as published by the Free Software Foundation; * version 2.1 of the License only. - * + * * This library is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * Lesser General Public License for more details. - * + * * You should have received a copy of the GNU Lesser General Public * License along with this library; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA - * + * * Linden Research, Inc., 945 Battery Street, San Francisco, CA 94111 USA * $/LicenseInfo$ */ @@ -72,7 +72,7 @@ LLGLTFMaterial::LLGLTFMaterial() #if 0 mTextureTransform[i].mOffset.clear(); mTextureTransform[i].mRotation = 0.f; -#endif +#endif } #if 0 mLocalTexDataDigest = 0; @@ -783,7 +783,7 @@ void LLGLTFMaterial::applyOverrideLLSD(const LLSD& data) { mMetallicFactor = mf.asReal(); if (mMetallicFactor == getDefaultMetallicFactor()) - { + { // HACK -- nudge by epsilon if we receive a default value (indicates override to default) mMetallicFactor -= FLT_EPSILON; } @@ -794,7 +794,7 @@ void LLGLTFMaterial::applyOverrideLLSD(const LLSD& data) { mRoughnessFactor = rf.asReal(); if (mRoughnessFactor == getDefaultRoughnessFactor()) - { + { // HACK -- nudge by epsilon if we receive a default value (indicates override to default) mRoughnessFactor -= FLT_EPSILON; } -- cgit v1.2.3