Skip to content

Prim generation phase 9: delete what meshoptimizer replaced - #573

Merged
RyeMutt merged 5 commits into
developfrom
rye/prim-cleanup
Oct 25, 2026
Merged

RyeMutt merged 5 commits into
developfrom
rye/prim-cleanup

Conversation

@RyeMutt

@RyeMutt RyeMutt commented Oct 25, 2026

Copy link
Copy Markdown
Member

Phase 9 of the prim generation plan: delete what meshoptimizer replaced. One commit per removal. Nothing in the tree calls any of it, tests and benchmarks included, and neither do #570, #571 or #572.

Changes

  • The Forsyth vertex cache remnant goes: LLVCacheVertexData, LLVCacheTriangleData, find_vertex_score, LLVCacheFIFO and LLVCacheLRU.
  • LLModel::optimizeVolumeFaces goes. It was the only caller of LLVolumeFace::optimize.
  • LLVolumeFace::optimize goes, and with it the members only it used: getVertexData, VertexData::compareNormal, both pushVertex overloads, pushIndex and swapData. VertexMapData stays for the DAE loader's weld.
  • LLVolumeFace::remap calls meshoptimizer itself, through its 32-bit entry points, with its scratch from the thread's arena. Its table now has one entry a vertex, as meshoptimizer documents it.
  • LLMeshOptimizer goes. LLVector4a::initClass, the llmath startup the viewer already calls, now installs ALScratchArena as meshoptimizer's allocator, at the same point in LLAppViewer::init. Tests that put meshoptimizer on the arena for a scope install the same pair themselves. llmeshoptimizer_test's case moves to llvector4a_test as a test of initClass, on a thread of its own so the floating-point mode initClass sets stays off the test thread.

One behaviour change: out-of-range upload indices

remap runs on uploads, so its indices are untrusted, and it keeps its check: an index past the face's vertices is taken as vertex 0, with the same warning. On develop, though, only the weld saw the checked indices. The index buffer was remapped through the unchecked ones, which is why the table was sized by the larger of the index and vertex counts.

  • An index past both read beyond the table. The new test, with an index of 65535, crashes on develop's remap.
  • An index below the index count took the first vertex the weld numbered, not vertex 0.

Every corner now goes through the checked indices. Valid faces weld exactly as before.

Testing

Windows, RelWithDebInfo, against a build of develop:

  • llmath_tests: 460 cases, develop's 458 plus the two new remap tests. 458 pass, with develop's one skip and one expected failure.
    • Every case both builds run has the same outcome.
    • [almeshsimplifier] is 18 cases and 509 assertions on both.
    • [llvolume], which holds the cacheOptimize tests, goes from 71 to 73 cases. Its assertions grow by 35, exactly the two new tests.
  • llprimitive_tests (49), llprimitive_test (11), and simd_tests_baseline, simd_tests_v2 and simd_tests_v4 (32 each): the same as develop.
  • ctest over those binaries: 49/49. Develop's 50 included llmeshoptimizer_test's own entry.
  • llvolume_bench: all 53 arena and heap allocation rows are identical. The cacheOptimize timings stay within run-to-run noise: both builds swing about 20% from run to run.
  • Each new test fails without its change. The out-of-range test crashes against develop's remap, and the initClass test fails with the allocator line removed.
  • llappviewer.cpp compiles against the viewer PCH.
  • Not checked in the viewer. The viewer wasn't linked, and no upload was tried in it.

🤖 Generated with Claude Code

RyeMutt and others added 5 commits October 25, 2026 09:37
LLVCacheVertexData, LLVCacheTriangleData, find_vertex_score, LLVCacheFIFO
and LLVCacheLRU are what is left of the vertex cache optimizer
meshoptimizer replaced; nothing calls them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Nothing calls it. It was the only caller of LLVolumeFace::optimize.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Its one caller, LLModel::optimizeVolumeFaces, is gone, and the uploader
compacts its levels of detail through meshoptimizer since the simplifier
change. The members only it used go with it: getVertexData,
VertexData::compareNormal, both pushVertex overloads, pushIndex and
swapData. VertexMapData stays for the DAE loader's weld.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
remap welds an uploaded face through meshoptimizer's 32-bit entry
points, with its scratch from the thread's arena, in place of
LLMeshOptimizer's U16 wrappers. The weld is the same for every valid
face: the same streams, the same unique count, and each corner given
back its vertex.

An upload's indices are untrusted, so the check stays: an index past the
face's vertices is taken as vertex 0, with the same warning. The index
buffer is now remapped through the checked indices too. Before, only the
weld saw them, and the remap went through the unchecked ones, which is
why its table was sized by the larger of the index and vertex counts.
An index past both read beyond the table: the new test, with an index of
65535, crashed there against the old code. An index below the index
count took the first vertex the weld numbered instead of vertex 0. The
table is now one entry a vertex, as meshoptimizer documents it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Its last callers were LLVolumeFace::remap, now on meshoptimizer's own
entry points, and the arena setup. Everything else in llmath and the
viewer already calls meshoptimizer directly, at 32 bits.

LLVector4a::initClass, the llmath startup the viewer already makes,
installs ALScratchArena as meshoptimizer's allocator, at the same point
in LLAppViewer::init where LLMeshOptimizer::initClass did. The tests and
the benchmark that put meshoptimizer on the arena for a scope install it
the same way. llmeshoptimizer_test's case moves to llvector4a_test as a
test of initClass, run on a thread of its own so the floating-point mode
initClass sets stays off the test thread.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 9 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 650a2f89-00e8-4ddc-ac79-61f96a57891c
Commits

Reviewing files that changed from the base of the PR and between 2a64878 and 0f979fe.

Files selected for processing: 15
  • indra/llmath/CMakeLists.txt
  • indra/llmath/llmeshoptimizer.cpp
  • indra/llmath/llmeshoptimizer.h
  • indra/llmath/llvector4a.cpp
  • indra/llmath/llvector4a.h
  • indra/llmath/llvolume.cpp
  • indra/llmath/llvolume.h
  • indra/llmath/tests/almeshsimplifier_test.cpp
  • indra/llmath/tests/llmeshoptimizer_test.cpp
  • indra/llmath/tests/llvector4a_test.cpp
  • indra/llmath/tests/llvolume_bench.cpp
  • indra/llmath/tests/llvolume_test.cpp
  • indra/llprimitive/llmodel.cpp
  • indra/llprimitive/llmodel.h
  • indra/newview/llappviewer.cpp
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@RyeMutt
RyeMutt merged commit 4928b35 into develop Oct 25, 2026
23 checks passed
@RyeMutt
RyeMutt deleted the rye/prim-cleanup branch October 25, 2026 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant