Repository navigation
Conversation
|
Warning Review limit reachedYou'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 59 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Simplifier contract and input preparation indra/llmath/almeshsimplifier.h, indra/llmath/almeshsimplifier.cpp, indra/llmath/CMakeLists.txt |
Adds seam modes and simplification targets. Prepares mesh attributes, calculates target indices, and handles seam constraints and distant-part pruning. |
Simplifier outputs and validation indra/llmath/almeshsimplifier.cpp, indra/llmath/tests/almeshsimplifier_test.cpp, indra/llmath/CMakeLists.txt |
Adds vertex compaction and model- and face-level simplification. Tests cover target ratios, seam and border handling, detached parts, compaction, and mesh validity. |
Normal generation and model integration indra/llmath/llvolume.h, indra/llmath/llvolume.cpp, indra/llprimitive/llmodel.cpp, indra/llprimitive/tests/llmodel_test.cpp, indra/llmath/tests/almeshsimplifier_test.cpp, indra/llprimitive/CMakeLists.txt |
Adds crease-angle normal generation to LLVolumeFace and calls it from LLModel. Tests check generated normals and retention of rigged-model weights. |
Preview LOD integration indra/newview/llmodelpreview.cpp, indra/newview/llmodelpreview.h |
Replaces preview-local meshoptimizer routines with ALMeshSimplifier calls for precise, sloppy, and automatic LOD generation. Retains the existing drift checks and retry limits. |
Priority:
Estimated code review effort: 4 (Complex) | ~60 minutes
Change: Bug fix
Merge Risk: 
d8fbf
-
The change swaps the uploader's LOD and normal generation onto a new simplifier. The checks did not find a concrete merge-blocking problem, and the build dependencies are satisfied. Viewer testing is still reported as pending.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 48.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 60 functions across 9 files. (2 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title names meshoptimizer's attribute-aware simplifier as the source of the uploader's LODs and normals. The changeset adds ALMeshSimplifier, built on meshopt_simplifyWithAttributes, and routes LO… Description check The description covers the change, its motivation, the stack order, and detailed results with before and after tables. It also lists testing done and states that viewer testing is not yet complete. Th… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names meshoptimizer's attribute-aware simplifier as the source of the uploader's LODs and normals. The changeset adds ALMeshSimplifier, built on meshopt_simplifyWithAttributes, and routes LOD generation and Generate Normals through it. The title is specific, but it names the library rather than the new ALMeshSimplifier class or the replaced per-model and per-face paths. It is also a long single sentence of 10 words and 84 characters.
Full details: Description check
-
Explanation
The description covers the change, its motivation, the stack order, and detailed results with before and after tables. It also lists testing done and states that viewer testing is not yet complete. The template's Related Issues section is missing: no issue link is provided, and the 'Issue Link' line is still a placeholder. The checklist is not marked, and the Additional Notes section is absent. Most required content is present, so the check passes. The author should add the related issue link or state that none exists, and tick or explain the checklist items.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 48.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 60 functions across 9 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
- 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.
A rabbit checks each mesh and seam,
Then trims the triangles with care.
New normals follow every crease,
LODs take shape through tested steps,
And every weighted vertex stays there.
Comment @coderabbitai help to get the list of available commands.
e2db787 to
33d4d46
Compare
…e simplifier ALMeshSimplifier makes a model's levels of detail from its faces, all of them together or one at a time, and compacts what it leaves: - meshopt_simplifyWithAttributes weighs normals (0.25) and UVs (0.1, scaled by each face's UV density) in its error, so a low LOD keeps its shading and texture placement rather than its positions alone. Over 1,343 cached meshes at the uploader's default targets, the median normal error at its three LODs falls from 6.9 / 20.2 / 35.6 degrees to 2.7 / 11.3 / 25.4, and UV drift from .017 / .098 / .169 to .006 / .076 / .106, for a median deviation of .0094 / .0415 / .115 against .0081 / .0333 / .110. - meshopt_SimplifyPermissive, with the seams that must hold protected: face borders always, texture seams unless UVs may go. It replaces welding through shadow index buffers, falls back to the sloppy simplifier about a third as often, and lands nearer the target. - meshopt_SimplifyPrune drops small detached parts at the two lowest LODs, never one larger than 5% of the model. - meshopt_SimplifyRegularizeLight at the two lowest LODs: a fifth fewer slivers for about 2% more deviation. - One face at a time locks the borders of faces that touch another, so material boundaries stay closed: 392 of 820 meshes cracked before, 1 after. - Compaction is meshopt_optimizeVertexFetchRemap over every vertex stream, skin weights and tangents included. Error and normals are worked in the model's own proportions, positions times mNormalizedScale, so a crease angle and an error bound mean what they say on a stretched model. LLVolumeFace::generateNormals makes a face's normals with meshopt_generateNormals and welds them back; a corner of a triangle with no area gets a real normal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LLModel::generateNormals made a face's normals in six passes over std::maps: a faceted copy, an angle-cutoff weld, smoothed normals over the welded copy, a second faceted copy and a nearest-normal match. It now asks each face for generateNormals at the crease angle given. Against the assets' own authored normals, the mean error falls from 7.44 to 7.05 degrees and the vertices added from 2.9% to 0.9%. Every vertex keeps its exact position, so the skin weights the model keeps by position are all found again; a test holds that for generated normals and for every LOD. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LLModelPreview's own meshoptimizer passes, genMeshOptimizerPerModel and genMeshOptimizerPerFace, with their shadow index buffers, their std::map weld and their simplification modes, are gone. The modes the uploader offers now ask ALMeshSimplifier: - Auto simplifies the model's faces together, keeping every seam, then letting normal seams move, then texture seams, and falls back to the sloppy simplifier only when none of those reaches the target. - Precise simplifies each face on its own, keeping every seam, with the borders it shares with other faces locked so materials do not part. - Sloppy simplifies each face with the sloppy simplifier, then keeping every seam where that leaves nothing. In error-threshold mode the error now counts normal and UV error, so at 1% and 3% a LOD keeps 42% and 28% of its triangles where it kept 34% and 23%, and its normal error falls from 12 and 22 degrees to 4 and 10. Precise measures its error against the whole model rather than each face. Low LODs land nearer their triangle targets than they did, which can raise the upload estimate slightly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
33d4d46 to
df8fae4
Compare
…the collapses The case simplifying a sphere to an error of 0.02 measured its deviation at 1.37 times the error as MSVC builds it and at 1.65 under clang-cl with fused multiply-adds, against a bound of 1.5. The error is meshoptimizer's estimate, each vertex's distance from the planes of the triangles it has taken over, and how far a left-out vertex of a curved region lies past it depends on the collapses rounding leads to. The bound is now twice the error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Holding every seam, meshoptimizer runs out of edges it may collapse near the target, and the rounding of its errors decides which are left: the 64 by 32 UV sphere's 27th, a target of 147 triangles, ends at 146 as MSVC builds it and at 150 as GCC does on x64. The case lets that mode come within 5% over its target; free to move a seam, a simplification still reaches it, and every mode is still held to what the uploader's Auto mode takes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The mesh uploader made LODs with two meshoptimizer passes of its own,
genMeshOptimizerPerModelandgenMeshOptimizerPerFace. They welded through shadow index buffers and astd::map, and simplified by position alone. Generate Normals took six passes overstd::maps.With this PR, both go through one class,
ALMeshSimplifier, built onmeshopt_simplifyWithAttributes. A low LOD now keeps its shading and texture placement, and material boundaries stay closed.This is part 7 of a stack of 7. Merge them in order: each PR is based on the one before it and shows only its own commits.
Changes
ALMeshSimplifier(llmath)meshopt_SimplifyPermissive: protects the seams that must hold: face borders always, and texture seams unless UVs are allowed to move. This replaces welding through shadow index buffers.meshopt_SimplifyPrune: drops small detached parts at the two lowest LODs, never one larger than 5% of the model.meshopt_SimplifyRegularizeLight: applies at the two lowest LODs. It leaves a fifth fewer slivers, for about 2% more deviation.meshopt_optimizeVertexFetchRemapover every vertex stream, skin weights and tangents included.mNormalizedScale), so a crease angle and an error bound mean what they say on a stretched model.LLVolumeFace::generateNormals: usesmeshopt_generateNormalsand welds back. A corner of a triangle with no area gets a real normal.Uploader
LLModel::generateNormalscallsLLVolumeFace::generateNormalsat the given crease angle. Every vertex keeps its exact position, so every skin weight the model keeps by position is found again.LLMeshOptimizer's shadow-index and simplify wrappers no longer have callers.Results
LODs at the uploader's default targets
Medians over 1,343 cached meshes. Each cell is LOD 2 / LOD 1 / LOD 0.
Error-threshold mode
The error now counts normal and UV error:
Generate Normals
Against the assets' own authored normals:
Testing
llmath_tests, shuffled: 446 cases. 444 pass, 1 is skipped, and 1 fails as expected. The expected failure is the[!shouldfail]case the planar texgen PR adds for tapered round sides.llmath_tests:almeshsimplifier_test.cpp.llprimitive_tests: all 49 cases pass. The newllmodel_test.cppchecks that every skin weight is found again after Generate Normals and at every LOD.🤖 Generated with Claude Code