resolve mesh face_count discrepancy with server (#853 #1901) by AndrewMeadows · Pull Request #5413 · secondlife/viewer

AndrewMeadows · 2026-02-12T00:17:20Z

This provides fixes necessary for some bugs (I think #853 and #1901) where texture changes to some mesh faces would be ignored by the server because the particular face being changed was "unknown" server-side. This client-side change doesn't fix everything, but allows the client to coordinate with similar server-side changes.

AndrewMeadows · 2026-02-12T15:28:03Z

indra/llmath/llvolume.cpp

    }

-    for (S32 i=0;i<(S32)mFaces.size();i++)
+    for (U8 i=0;i<mFaces.size();i++)


I'm changing almost all of the "face count" variables to use U8 instead of a mix of types (S32, U32, size_t). There are a few places I didn't change because in context the logic was expecting invalid values (-1).

AndrewMeadows · 2026-02-12T15:35:55Z

indra/llprimitive/llprimtexturelist.cpp

 // copies LLTextureEntry 'te'
 // returns TEM_CHANGE_TEXTURE if successful, otherwise TEM_CHANGE_NONE
-S32 LLPrimTextureList::copyTexture(const U8 index, const LLTextureEntry& te)
+S32 LLPrimTextureList::copyTexture(const U8 index, const LLTextureEntry* te)


This change was necessary because I was doing the development work on Linux and g++-11.0.4 considered it an error to cast a pointer to const reference and then pass it to a function that expected a real const reference.

After making it work on Linux I ported the changes to a branch off of develop.

AndrewMeadows · 2026-02-12T15:37:13Z

indra/newview/llface.cpp

        {
            const LLVolumeFace& vol_face = volume->getVolumeFace(face->getTEOffset());
            LLVertexBuffer::drawElements(LLRender::TRIANGLES, vol_face.mPositions, NULL, vol_face.mNumIndices, vol_face.mIndices);
        }


This change because during my development I started crashing here for indexing out of bounds.

AndrewMeadows · 2026-02-12T15:43:32Z

indra/newview/llvovolume.cpp

-        mMediaImplList.resize(num_tes) ;
-
-        LLViewerObject::setNumTEs(num_tes) ;
    }


What I'm doing here changing if (a && b) else if (c) to be if (a) { if (b) else if (c) } . The reason being the old_num_tes == num_tes case was falling into the final else case but now it falls into the non-existent else case of the embedded if/else_if and is no longer handled.

AndrewMeadows · 2026-02-12T15:44:10Z

indra/llprimitive/llprimitive.cpp

 //===============================================================
 LLPrimitive::LLPrimitive()
 :   mTextureList(),
-    mNumTEs(0),


This data member wasn't being used.

Copilot

Pull request overview

This PR addresses a long-standing issue where the client and server disagreed on the number of faces for mesh objects, causing PBR materials and other texture changes to be silently dropped. The fix involves both refactoring the face count tracking mechanism and implementing a workaround protocol to communicate face counts between client and server.

Changes:

Removed the mNumTEs member variable and replaced it with direct queries to mTextureList.size() to maintain a single source of truth
Changed face count types from S32/U32 to U8 throughout the codebase, consistent with MAX_TES = 45
Refactored TextureEntry packing/unpacking to encode face count information in the wire protocol for mesh objects
Added bounds checking in renderFace() to prevent crashes when accessing volume faces

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
indra/newview/llvovolume.cpp	Reorganized setNumTEs logic for clarity without changing behavior
indra/newview/llface.cpp	Added bounds check to prevent out-of-bounds access to volume faces
indra/llprimitive/tests/llprimitive_test.cpp	Updated test stubs to match new U8 signatures
indra/llprimitive/llprimtexturelist.h	Changed size() and setSize() signatures from S32 to U8, uncommented copyTexture overload
indra/llprimitive/llprimtexturelist.cpp	Implemented U8-based iteration, added copyTexture overload, fixed typo in comment
indra/llprimitive/llprimitive.h	Removed mNumTEs field, changed MAX_TES to U8, added helper methods for TE packing/unpacking
indra/llprimitive/llprimitive.cpp	Major refactoring: extracted packing logic, implemented face count workaround for mesh, updated validTE to use mTextureList.size()
indra/llmath/llvolume.h	Changed getNumFaces() and related constants to return/use U8
indra/llmath/llvolume.cpp	Updated getNumFaces() implementation and loop variables to use U8

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-13T15:17:15Z

indra/llprimitive/llprimtexturelist.cpp

+    U8 this_size = static_cast<U8>(mEntryList.size());
+    U8 other_size = static_cast<U8>(other_list.mEntryList.size());


Potential data loss when mEntryList.size() exceeds 255. The vector size is being cast to U8, which can only hold values 0-255, but there's no check to ensure the size doesn't exceed this limit before the cast. If mEntryList.size() is greater than 255, the cast will silently truncate the value, leading to incorrect behavior.

The good news is: for other reasons not evident here we don't expect mEntryList.size() to ever report more than 45.

Copilot · 2026-02-13T15:17:16Z

indra/llprimitive/llprimtexturelist.cpp

+        mEntryList[index] = other_list.getTexture(index)->newCopy();
    }

    // add new entires if needed


Typo in comment: 'entires' should be 'entries'.

Suggested change

// add new entires if needed

// add new entries if needed

Copilot · 2026-02-13T15:17:16Z

indra/llprimitive/llprimitive.cpp

+                if (flags && ! highest_flagged_index)
+                {
+                    // initialize highest_flagged_index
+                    while (flags > 1)
+                    {
+                        highest_flagged_index++;
+                        flags >>= 1;
+                    }
+                }


Bug in highest_flagged_index calculation for multi-byte bitfields. When the first non-zero flags byte is not the first byte of the bitfield, the initialization logic (lines 1218-1226) finds the highest bit within that byte but doesn't account for the bit offset from previous bytes. For example, if only index 20 is flagged (requiring 3 bytes where bytes 1-2 are 0x00 and byte 3 is non-zero), the code would initialize highest_flagged_index to the bit position within byte 3 (e.g., 6) but fail to add the 14-bit offset from the previous two bytes. The increment at line 1210 only happens when highest_flagged_index is already non-zero, so it won't correct this. The initialization should account for how many bytes have already been processed.

Unfortunately Copilot does not realize: the variable-length bitfield is only as long as necessary to pack the non-zero bits. Zero-value bytes at the front of the bitfield are never packed. This probably means this code at least deserves a comment about that fact make the code a little bit more understandable.

Copilot · 2026-02-13T15:17:16Z

indra/llprimitive/llprimitive.cpp

+        // Decreases in the LLVolume's notion of "num_faces" happens for LOD reasons
+        // which do not reflect actual change of faces of the object, so we ignore them.


Comment appears to be incorrect or misleading. The comment states "Decreases in the LLVolume's notion of 'num_faces' happens for LOD reasons which do not reflect actual change of faces of the object, so we ignore them." However, the actual check (getNumTEs() < mVolumep->getNumFaces()) handles the case when the volume has MORE faces than the texture entries, not fewer. If the intent is to ignore decreases, the condition should be inverted, or if the condition is correct, the comment should explain that we're updating texture entries when the volume has more faces.

Suggested change

// Decreases in the LLVolume's notion of "num_faces" happens for LOD reasons

// which do not reflect actual change of faces of the object, so we ignore them.

// If the LLVolume's notion of "num_faces" increases, update the number of

// texture entries to match. Decreases in "num_faces" happen for LOD reasons

// which do not reflect an actual change in the object's faces, so we ignore them.

I appreciate the effort Copilot but you misunderstand how the comment relates to the logic flow. The "ignore" statement is trying to explain why there is no "else" case. Nevertheless, I will modify the comment to be less confusing to friendly AI assistants.

Copilot · 2026-02-13T15:17:16Z

indra/llmath/llvolume.cpp

+U8 LLVolume::getNumFaces() const
 {
-    return mIsMeshAssetLoaded ? getNumVolumeFaces() : (S32)mProfilep->mFaces.size();
+    return mIsMeshAssetLoaded ? getNumVolumeFaces() : static_cast<U8>(mProfilep->mFaces.size());


Potential data loss when mFaces.size() exceeds 255. The vector size is being cast to U8, which can only hold values 0-255. If mFaces.size() is greater than 255, the cast will silently truncate the value, leading to incorrect face count.

Suggested change

return mIsMeshAssetLoaded ? getNumVolumeFaces() : static_cast<U8>(mProfilep->mFaces.size());

// Clamp face count to maximum value representable by U8 to avoid truncation.

U32 num_faces = mIsMeshAssetLoaded

? static_cast<U32>(getNumVolumeFaces())

: static_cast<U32>(mProfilep->mFaces.size());

if (num_faces > 255u)

{

num_faces = 255u;

}

return static_cast<U8>(num_faces);

Copilot · 2026-02-13T15:17:17Z

indra/llprimitive/llprimtexturelist.cpp

    else if (new_size < current_size)
    {
-        for (size_t index = current_size-1; index >= new_size; --index)
+        for (U8 index = current_size-1; index >= new_size; --index)


Infinite loop bug when new_size is 0. The loop variable 'index' is U8, so when it reaches 0 and decrements (--index), it will underflow to 255 instead of going negative, causing the condition 'index >= new_size' to remain true indefinitely. The loop should either use a signed type for the index, iterate in the opposite direction, or explicitly check for the underflow condition.

Suggested change

for (U8 index = current_size-1; index >= new_size; --index)

for (S32 index = current_size - 1; index >= new_size; --index)

Copilot · 2026-02-13T15:17:17Z

indra/llprimitive/llprimitive.cpp

+    if (mVolumep && mVolumep->getParams().isMeshSculpt())
+    {
+        cur_ptr += pack_TE_field(cur_ptr, (U8 *)colors, 4 ,last_face_index, MVT_U8);
+    }
+    else
+    {
+        cur_ptr += pack_TE_field(cur_ptr, (U8 *)colors, 4 ,last_face_index, MVT_U8);
+    }


Dead code: The if and else branches contain identical code, making the conditional check unnecessary. This appears to be leftover scaffolding that should be removed. The mesh sculpt-specific workaround for face count is correctly implemented later at lines 1587-1591 for the bump field.

Suggested change

if (mVolumep && mVolumep->getParams().isMeshSculpt())

{

cur_ptr += pack_TE_field(cur_ptr, (U8 *)colors, 4 ,last_face_index, MVT_U8);

}

else

{

cur_ptr += pack_TE_field(cur_ptr, (U8 *)colors, 4 ,last_face_index, MVT_U8);

}

cur_ptr += pack_TE_field(cur_ptr, (U8 *)colors, 4 ,last_face_index, MVT_U8);

Copilot is correct! Oh I see why this happened... during debugging I had a temporary method called pack_TE_field_verbose() which I was using to see what was happening for mesh objects only. When I cleaned up my debugging code I did not evaluate this code carefully enough.

Copilot · 2026-02-13T15:17:17Z

indra/llprimitive/llprimtexturelist.cpp

+        for (U8 index = this_size; index > other_size; --index)
        {
            delete mEntryList[index-1];
        }


Potential infinite loop bug when other_size is 0. The loop variable 'index' is U8, so when it reaches 0 and decrements (--index), it will underflow to 255 instead of going negative, causing the condition 'index > other_size' to remain true indefinitely. The loop should either use a signed type for the index or explicitly check for the underflow condition.

LLPrimTextureList::copy() is only happening in the context of initializing this object as a copy of other object, which has already been initialized and should never have zero texture entries.

github-actions bot added c/cpp llmath llprimitive labels Feb 12, 2026

akleshchev approved these changes Feb 12, 2026

View reviewed changes

AndrewMeadows commented Feb 12, 2026

View reviewed changes

AndrewMeadows mentioned this pull request Feb 13, 2026

FIX - pre-jump and land animations being shortcircuited by race conditions with AGENT_CONTROL_FINISH_ANIM when chaining jumps #5409

Merged

AndrewMeadows and others added 6 commits February 12, 2026 21:37

resolve mesh face_count discrepancy with server (#853 #1901)

6623579

fix LLPrimTextureList stubbery for tests

83dbf06

fix bad comparison of U8 to negative number

ef0005c

restore getExpectedNumTEs()

bdb468c

fix comment

ed147f2

fix unit test

253d5a1

AndrewMeadows force-pushed the leviathan/ghi-1901 branch from ed129a1 to 253d5a1 Compare February 13, 2026 05:37

akleshchev requested a review from Copilot February 13, 2026 15:11

Copilot started reviewing on behalf of akleshchev February 13, 2026 15:11 View session

Copilot AI reviewed Feb 13, 2026

View reviewed changes

remove cruft, add comments and assserts

9b30663

		U8 this_size = static_cast<U8>(mEntryList.size());
		U8 other_size = static_cast<U8>(other_list.mEntryList.size());

		// Decreases in the LLVolume's notion of "num_faces" happens for LOD reasons
		// which do not reflect actual change of faces of the object, so we ignore them.

-        // Decreases in the LLVolume's notion of "num_faces" happens for LOD reasons
-        // which do not reflect actual change of faces of the object, so we ignore them.
+        // If the LLVolume's notion of "num_faces" increases, update the number of
+        // texture entries to match.  Decreases in "num_faces" happen for LOD reasons
+        // which do not reflect an actual change in the object's faces, so we ignore them.

-    return mIsMeshAssetLoaded ? getNumVolumeFaces() : static_cast<U8>(mProfilep->mFaces.size());
+    // Clamp face count to maximum value representable by U8 to avoid truncation.
+    U32 num_faces = mIsMeshAssetLoaded
+                       ? static_cast<U32>(getNumVolumeFaces())
+                       : static_cast<U32>(mProfilep->mFaces.size());
+    if (num_faces > 255u)
+    {
+        num_faces = 255u;
+    }
+    return static_cast<U8>(num_faces);

	for (U8 index = current_size-1; index >= new_size; --index)
	for (S32 index = current_size - 1; index >= new_size; --index)

Conversation

AndrewMeadows commented Feb 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants