Add benchmark recording & parameter sweep infrastructure - #105
Merged
Conversation
Introduces LightmapUvTool.UvtLog.Category flags (SymSplit, Repack, Match, Dedup, Overlap, Topology, Validation, Export, Benchmark) and overloads for Info/Warn/Error/Verbose that accept a category. Enabled categories are persisted to EditorPrefs as a bitmask (LightmapUvTool_LogCategoryMask) and silenced independently of the Level knob. Existing call sites continue to compile via legacy string-only overloads that default to Category.General. Prefix changes from "[LightmapUV] " to "[LightmapUV][<Category>] ". This is the foundation for the transfer-modes benchmark log/metric work; no call-site migration is done in this commit.
Introduces BenchmarkRecorder (IDisposable) that collects per-mesh pipeline
metrics (TransferResult + ValidationReport + volatile counters) and writes
CSV/JSON reports into <projectRoot>/BenchmarkReports/ on Dispose.
Adds public static counters so the recorder can read them:
- SymmetrySplitShells.LastFallbackCount / LastTotalSplitCount
- GroupedShellTransfer.LastTopologyIterations / LastTopologyFixed /
LastTopologyCapHit
The enclosing callers (e.g. LightmapTransferTool) will wrap pipeline
execution in `using (BenchmarkRecorder.NewRun(...))` in a follow-up commit;
this commit only adds the recorder + hooks and keeps existing behavior.
Also migrates the SymSplit fallback / topology enforcement log sites to
categorized UvtLog overloads (Category.SymSplit, Category.Topology).
- ExecFullPipeline now opens a recorder session around the whole run and records one row per MeshEntry once the core finishes. The original body moved to ExecFullPipelineCore. - ExecRepack / ExecTransferAll open nested sessions via BenchmarkRecorder .NewRun — when already inside an outer session the call returns a NoOpScope so the outer recorder continues to own the timing/record state. - Stage timers: "pipeline" wraps the full run, "repack" wraps the repack stage, "transfer" wraps TransferAll, "validate" wraps TransferValidator .Validate. - Standalone ExecTransferAll now writes a CSV/JSON on its own when it creates the session (ownsSession branch); standalone ExecRepack only records timings and skips file output because there is no per-mesh validation data yet. - BenchmarkRecorder.NewRun now returns IDisposable and silently no-ops on nested calls; WriteArtefacts bails when no mesh rows were captured.
UvCanvasView.ValidationFilterMask: when non-zero, GlFillValidation and GlFillValidationOverlay only draw triangles whose TriIssue intersects the mask. Default (None) keeps the previous behavior. LightmapTransferTool Pipeline Settings now has a "Log filters" foldout with the UvtLog.Level picker and a toggle per UvtLog.Category (General, SymSplit, Repack, Match, Dedup, Overlap, Topology, Validation, Export, Benchmark) so noisy subsystems can be silenced without lowering the global verbosity. Transfer tab now has a "Validation Overlay" foldout beneath the Quality Report: per-category toggles (Inverted/Stretched/ZeroArea/OutOfBounds/ Overlap/TexelDensity) flip bits in canvas.ValidationFilterMask and request a repaint, so the canvas can isolate a single defect class across the current mesh entries.
TestSuiteAsset is a ScriptableObject registry of benchmark cases — one per model (FBX reference, optional LODGroup path, ExpectedRange list for GO/STOP notes, free-form notes). A custom inspector exposes a Ping button per case. Create via Assets → Create → Lightmap UV Tool → Test Suite. TRANSFER_BENCHMARK.md documents the benchmark protocol, the metrics emitted by BenchmarkRecorder, the log-category table, the Validation Overlay workflow, a (yet-to-be-filled) matrix of runs, and suggested Go/Stop thresholds. Point readers to EXPERIMENTS.md for the regressions history.
TestSuiteAsset.SweepMatrix: atlasResolutions (default 256/512/2048),
shellPaddingPxVariants (default 2/4/8/32), borderPaddingPxVariants
(default 0), resetBetweenRuns. Stored per-suite alongside the test cases.
LightmapTransferTool:
- ExecFullPipeline now has a string-label overload; the old parameterless
form delegates to it with runLabel="FullPipeline".
- ExecSweep(SweepMatrix) iterates the cartesian product, sets
ctx.AtlasResolution/ShellPaddingPx/BorderPaddingPx per cell, calls
ResetWorkingCopies() (lightweight — no sidecar delete, no FBX reimport),
then ExecFullPipeline("sweep_res{R}_pad{S}_bdr{B}"). Each cell writes
its own CSV+JSON via BenchmarkRecorder; the cell id ends up in the
filename and the runLabel column.
- Original atlas/padding values are restored in finally; progress bar
with Cancel is shown.
- New UI row under Run Full Pipeline: Sweep suite ObjectField +
"Run Sweep (N)" button showing the cell count.
TRANSFER_BENCHMARK.md: documents the sweep workflow and a one-liner to
concat the produced CSVs with pandas.
Two MenuItems under "Mesh Lab":
- "Export FBX Metrics (Selected Assets)" — scans every LODGroup/Renderer
inside the selected .fbx assets.
- "Export FBX Metrics (Scene LODGroup)" — scans the LODGroup containing
the active Hierarchy selection.
Output: <projectRoot>/BenchmarkReports/FbxMetrics_{ts}/
- FbxMetrics_{ts}.csv — one row per mesh × LOD: vertex/triangle count,
submesh count, bounds size, avg world edge length, geometry-island
count, UV0 shell count, UV0 total coverage, max/mean shell area,
shell area stddev, UV0 AABB overlap pairs, UV0 OOB verts, estimated
mirror pair count, UV2 shell count/overlaps/OOB when present.
- png/<model>_<lodGroup>_LOD{N}_<renderer>_uv0.png (+ _uv2.png) — per-
shell coloured triangles with wire + 0–1 bounding box, range
[-0.1, 1.1] so OOB verts are visible. Rendered via a RenderTexture
driven by Hidden/Internal-Colored material.
Shares the UvShellExtractor.CountAabbOverlaps/Extract helpers used by
the transfer pipeline, so FBX baseline metrics stay consistent with
BenchmarkRecorder sweep output. Join keys: (model, lodGroup,
rendererName, lodIndex).
TRANSFER_BENCHMARK.md documents both menu items and what the PNG/CSV
pair lets you do at analysis time.
Extract the PNG-rendering helper from FbxMetricsExporter into a shared
UvPngWriter (Editor/UvPngWriter.cs): renders triangles per-shell, wire,
and the 0-1 bounding box into a RenderTexture, then writes PNG. View
covers [-0.1, 1.1] so OOB verts are visible. Used by both the source
FBX baseline exporter and the benchmark recorder.
BenchmarkRecorder.RecordMesh now snapshots the result UV2 channel +
triangles from the relevant mesh (repackedMesh on source LOD,
transferredMesh on target LODs, originalMesh as fallback). On Dispose,
after the CSV/JSON is written, it dumps one PNG per recorded mesh into
a sibling "{fileBase}_png/" folder:
<rendererName>_LOD{N}_uv2.png
So for each sweep cell the BenchmarkReports/ folder ends up with:
{ts}_{lodGroup}_sweep_res256_pad2_bdr0_LegacyFixed.csv
{ts}_{lodGroup}_sweep_res256_pad2_bdr0_LegacyFixed.json
{ts}_{lodGroup}_sweep_res256_pad2_bdr0_LegacyFixed_png/
<renderer>_LOD0_uv2.png
<renderer>_LOD1_uv2.png
...
This makes cross-cell visual comparison immediate: diff two folders,
look at the same renderer across (res, pad) combinations. UV0 stays
in FbxMetricsExporter because it doesn't change between runs — one
snapshot per FBX is enough.
FbxMetricsExporter delegates PNG writing to UvPngWriter; local copy of
the renderer helpers removed.
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9736fd5609
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…enum P1 — Per-target topology metrics: GroupedShellTransfer.Transfer now snapshots LastTopologyIterations / LastTopologyFixed / LastTopologyCapHit into TransferResult immediately after EnforceShellTopologyOnUv2, so each target mesh carries its own values. BenchmarkRecorder.RunRecord reads from TransferResult instead of the global static fields; previously a multi-mesh run copied the last processed target's topology numbers into every CSV row. P2 — Sweep cancel now breaks all three loops: ExecSweep wrapped the triple foreach directly, so "break" on DisplayCancelableProgressBar only exited the innermost borderPad loop and subsequent cells still ran. Added "if (cancelled) break" guards around the outer atlasRes and shellPad loops so cancellation halts the whole sweep. P2 — Cache Log filters enum values: Log filters UI called Enum.GetValues(typeof(UvtLog.Category)) on every repaint, allocating each frame. Moved to a static readonly array (s_logCategories) built once at type init, with the composite "All" flag filtered out. OnGUI now iterates the cached array.
Summary of the 60-cell LegacyFixed sweep on Playground, Gazebo,
Carousel, Wooden_Box_Long across res∈{256,512,2048} × pad∈{2,4,6,8,32}.
Key findings:
- Pipeline health solid: shellsRejected=0, overlapShellPairs=0,
coverage=1.00 across every cell.
- defectScore = stretched+zeroArea+oob is dominated by Carousel
(N-fold rotational, 25-32% defective triangles) and by Playground's
invertedCount (which the validator documents as "winding flip is
expected"); stretched+zeroArea on Playground is tiny (~85).
- pad=32 gives modest defect-score win on res=256/512 but pays 8-10x
repack cost; at res=2048 the effect flattens.
- Doubling res ≈ halves texel density; doubling pad ≈ doubles it.
res=2048 pad=2 has the tightest UV2 (texelMedian 72).
- topologyCapHit fires ~5-10% of cells, mostly Carousel LOD3 where
Laplacian enforcement does 34 fixes; worth raising
kMaxTopologyIterations 5→8 as a follow-up.
Recommended defaults (pending Adaptive comparison):
- atlasResolution = 512
- shellPaddingPx = 4
- borderPaddingPx = 0
Follow-ups listed in the doc: Adaptive sweep, Carousel topology-cap
experiment, inspect Playground invertedCount PNGs visually, fill
res=1024 gap, borderPad sweep.
Places a second toggle under 'SymSplit target LODs' on the Setup tab. When enabled, ExecFullPipelineCore skips the ExecSymmetrySplit call in every auto-tune config iteration, so a sweep can be run without symmetry-split fragmentation to isolate whether xatlas packing issues (e.g. Wooden_Box_Long stuffing all shells into ~25% of the atlas) are caused by the shape/count of shells fed to xatlas after SymSplit or by xatlas packing heuristics on the original set. Only a diagnostic — default is false (SymSplit still runs).
Main's PR #106 renamed namespace LightmapUvTool → SashaRX.UnityMeshLab globally. My four new files (BenchmarkRecorder, FbxMetricsExporter, UvPngWriter, TestSuiteAsset) were added on this branch before the migration and still declared the old namespace; the merge commit didn't touch new-on-branch files. Update them to match.
…ashaRX/UnityMeshLab into claude/optimize-transfer-modes-fJYTm
Added a new toggle under 'SymSplit target LODs' on the Setup tab to allow users to skip the ExecSymmetrySplit call during auto-tune config iterations. This feature is intended for diagnostic purposes to help isolate issues related to xatlas packing without the influence of symmetry-split fragmentation. The default setting remains false, ensuring that SymSplit continues to run unless explicitly disabled.
Single-file Python script that scans a BenchmarkReports folder
(*_sweep_*.csv + *_png/) and emits a per-model HTML gallery with:
- res × pad table of UV2 thumbnails
- per-cell metrics overlay (inv/str/0A/tex/topFx)
- 1-5 rating buttons + tag panel (narrow_strips, empty_atlas,
rotation_wrong, stretched, good_pack, broken_shells, small_shells,
overlap_visible) + free-form note
- hotkeys: 1-5 / g/b/u/n rate, Space=next unrated, Tab/arrows nav,
t=tags, e=export
- localStorage persistence (survives reload), JSON export/import
- progress counter "X / Y rated" in sticky bottom bar
Run:
python Tools/build_gallery.py <BenchmarkReports-dir> \
--gallery-id "noSymSplit_2026-04-24"
Output goes alongside the data; open _gallery_index.html. The
gallery-id doubles as the localStorage key so votes for different
runs don't collide.
Some chat clients auto-rewrite '.py' filenames as markdown links when copying instructions. The wrapper avoids ever needing to type the extension at the command prompt — invoke 'Tools\gen.bat <args>' and it forwards everything to build_gallery.py via %~dp0.
…metric xatlas with texelsPerUnit=0 (auto) underfills the atlas on tiled-UV0 models such as Wooden_Box_Long: charts get packed into ~25% of the requested atlas extent, leaving 75% of UV space unused. Per-triangle metrics (inv/stretched/zeroArea) don't catch it because each triangle is locally valid — the whole layout is just shrunk into a corner. Three additions: 1. RepackOptions.normalizeAtlasFill (default true). New post-pack pass NormalizeAtlasFill rescales UV2 bbox uniformly to [padding, 1-padding] in both axes, preserving aspect ratio. No-op when already > ~95% filled (scale ≤ 1.05). Applied in both RepackSingle and RepackMulti, after border inset and overlap fixes. 2. xatlas pack-result logging. Added an Info-level [Repack] message right after PackCharts that prints requested vs actual atlas size and chart count, so underfill or auto-resize behaviour is visible in Console. 3. BenchmarkRecorder.RunRecord.atlasUtilization. New float column = bbox-area of the result UV2 in [0,1] space (1.0 = full, 0.25 = quarter-filled). Written to CSV and JSON. Tools/build_gallery.py renders it per cell and outlines low-util cells (< 50%) in red so they pop visually next to numeric metrics. Together these turn a bug invisible to per-triangle counters into a first-class diagnostic. Existing well-packed runs keep their layout because the rescale guard (scale ≤ 1.05) leaves them alone.
…ollow-up) Unity's FBX Exporter stores Tangent + Bitangent separately rather than the Vector4-tangent (xyz + handedness W) Unity uses internally. With ModelImporterTangents.Import (or whatever the FBX preset happened to have), W can flip on the round-trip — visible on mirrored UVs as inverted normal-map shading. PrepareImportSettings already locks keepQuads, useFileScale, and globalScale=1 when lockForFbxOverwrite is set. Add a parallel lock on importTangents = CalculateMikk so MikkTSpace recomputes tangents+W from positions/normals/UV0 deterministically every time. The recompute matches what Unity uses by default for new imports, so this is a no-op on most assets — but it explicitly prevents a roundtrip-imported .meta from carrying over importTangents=Import that would have lost the handedness bit. Scope: only applied when lockForFbxOverwrite=true (i.e. the dedicated ApplyUv2ToFbx / Export path), matching the existing scale-and-quad locks. Other import flows are untouched.
xatlas's bin-packer is poor on long thin charts. Tiled-mesh-edge shells
(common in Wooden_Box_Long-style box models with repeating UV0) come in
as 5:1 / 10:1 strips, get packed as parallel lines in one corner, and
leave 60-75% of the atlas unused. NormalizeAtlasFill rescales the bbox
back to ~[0,1] but the strips just spread out — the visual ugliness is
unchanged because aspect-correct rescale can't fix the underlying
non-square chart shapes.
PreRotateThinShells operates on the flat UV0 array fed to xatlas
(NOT on mesh.uv) before xatlasAddUvMesh:
- Recompute each shell's bbox in the current flat array.
- If max(w,h) / min(w,h) > 3, rotate every vertex of the shell by 90°
around the shell centroid: (u,v) → (cx + (v-cy), cy - (u-cx)).
- Texel content is unchanged (rigid rotation), only the shape that
xatlas sees becomes more square-ish.
The transfer pipeline is unaffected: mesh.uv (UV0) on the source LOD
is never modified, so source.uv0 ↔ target.uv0 matching uses the same
data as before. The resulting source.UV2 layout, which has the rotated
shell orientations, is copied to target LODs via the existing per-shell
similarity transform — rotation/scale/translation propagate correctly.
Controlled by RepackOptions.preRotateThinShells (default true).
Applied in both RepackSingle and RepackMulti.
Expected impact on Wooden_Box_Long sweep: atlasUtilization should jump
from ~0.66 (NormalizeAtlasFill alone) to >0.85, and visually the
"thin parallel strips with gaps" layout should compress into actual
square-ish chart blocks.
Tiled-UV0 models (Wooden_Box_Long, modular pieces) carry N>>K shells where K representative patches are duplicated N times by tile instancing — all overlapping in UV0. The old flow fed xatlas all N independent charts; xatlas packed each tile as its own atlas region and left 60-75% of the atlas empty regardless of resolution/padding. NormalizeAtlasFill + PreRotateThinShells from earlier commits only rearranged the same wasted layout — they did not address the root cause (xatlas had been told to pack N copies that should have been 1). Real fix is architectural. Per overlap-group: 1. Pick representative shell = group[0] (deterministic by shellId). 2. Mark all other shells in the group as duplicates; build a filtered indices array fed to xatlasAddUvMesh that contains ONLY the representative-shell faces. xatlas now sees ~K charts instead of N. 3. After AssignUv2 dispatches xatlas's output, walk the duplicate shells and for each vertex find the nearest UV0 vertex in the representative shell — copy its uv2 to the duplicate. Tiles share the representative's UV2 region in the atlas (intentional overlap that matches their UV0 overlap). 4. Skip FixOverlapping / FixNearDuplicate / RelocateToFreeSpace when merge fired — those phases would un-do the deliberate sharing. mesh.uv (UV0) is never modified — only the indices/faceShellIds arrays passed to xatlas are filtered. The transfer pipeline matches source.uv0 ↔ target.uv0 against the unmodified full set of shells. Toggled by RepackOptions.mergeOverlappingTiles (default true). When disabled, falls back to the legacy PerturbOverlapShellsUv0 path that SymSplit-style overlap groups still need. Implemented symmetrically in RepackSingle and RepackMulti. Per-mesh group-merge state is retained in parallel arrays so the post-xatlas dispatch loop has access to shellToRep / skipShell / uvFlat. Earlier band-aid attempts removed: PreRotateThinShells (commit 79021fd) and NormalizeAtlasFill (commit ab36b06). The atlasUtilization metric in BenchmarkRecorder is kept — it is the diagnostic that proves this fix works. Next: stable groupId on UvShell + cross-LOD group propagation in GroupedShellTransfer so target LOD UV2 reuses the representative's region directly instead of relying on per-shell similarity matching.
User report: pack hung 19+ minutes with Cancel button greyed out. Root cause was xatlas being a synchronous native call that blocks the main managed thread, so the IMGUI Cancel button never gets an event. Two changes: 1. Run ComputeCharts + PackCharts on a background Task. Main thread loops on DisplayCancelableProgressBar with 50ms polling. Cancel button now works — but xatlas itself has no native cancel API, so "cancelled" means: stop showing progress, wait for the in-flight pack to finish (xatlas state is a process-wide singleton; can't safely abandon it mid-mutation), then return error="cancelled" to the caller. The auto-tune Pipeline picks up the cancellation and stops trying further configs. 2. Cost-budget preflight. Pack cost ≈ shellCount × W × H. Brute force over 500M ops gets auto-downgraded to heuristic; total cost over 20B ops gets refused outright with an Info message. This stops the "internal atlas 8192 × 149 shells × 3 auto-tune configs = hours" class of hang before it starts, instead of relying on the user to notice it's stuck. The previous oversample-based brute force guard is replaced with the cost-based one (more accurate — small-shell-count scenes can brute force a bigger atlas than dense ones).
User test showed oversample 4× had zero measurable effect on density spread (still 14.56× at postUV2), and 8× hung pack for 19+ minutes. Root cause: thin shells (aspect 100:1+) still have sub-pixel short- dimension extents even at 8× oversample. Atlas needs ~24× oversample to fully neutralise — too expensive in practice. Post-pack density correction (already in tree as opt-in) provably worked: 14.5× → 2.94× on the same Carousel test, no pack-time cost. Defaults flipped: - PostPackDensityCorrection: false → true - InternalOversample: 4 → 1 (off) Oversample knob stays in the UI for experimentation but documented as not generally useful. Cost preflight + cancellable pack thread stay since they protect against the user manually cranking oversample up.
Vendors xatlas master @ today into Native~/third_party/xatlas/ and patches the per-chart ceil(extents) rescale in PackCharts (xatlas.cpp ~line 8345 — "Scale charts to use the entire texel area"). Upstream Issue #18 ("Add packer option to retain texture sizes") is closed wontfix; the patch lives in our fork instead. When PreserveChartScale is true: - chart extents still round up to integer pixel bbox (for packer slot) - but per-chart UV vertices keep their input scale (no rescale loop) - uniform per-shell texel density end-to-end (au2/a3 ratio = 1.0×) - trade-off: sub-pixel-thin shells take a full 1×1 slot with empty margin → some atlas utilization is lost When false (= stock xatlas behaviour): - charts non-uniformly stretch to fill their integer bbox - tightest pack but per-chart density variance up to ~14× on real artist UVs (the bug we've been chasing) Plumbing: - Native~/CMakeLists.txt: use local third_party/xatlas/ instead of FetchContent (so the patch survives CI builds) - xatlas-unity-bridge.cpp: xatlasPackCharts grows a new preserveChartScale int parameter, wired to a thread-safe setter in the patched xatlas - Editor/XatlasNative.cs: matching P/Invoke signature - RepackOptions.preserveChartScale (default true) - UvToolContext.PreserveChartScale (default true) - UI toggle "Preserve chart scale (forked xatlas)" in Pre-pack panel The build-native.yml workflow already triggers on Native~/** changes, will produce fresh DLLs for Windows/Linux/macOS and auto-commit them.
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2b95a11b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Density spread persisted at 14.77× after the PreserveChartScale patch was supposedly built and committed (auto-build a2b95a1). Windows DLL showed 0-byte diff vs prior, which suggests either the patched source didn't make it into the build or Unity is loading a cached older copy. Bridge now prints "[xatlas-bridge] PATCHED build, preserveChartScale=N" to stdout on every PackCharts call. If we see this line in Unity's console after the next CI rebuild + Unity restart, we know the patched DLL is loaded. If we don't see it, DLL is stale. This is a temporary diagnostic — will revert once root cause is found.
…nsity diagnostics
fprintf(stdout, ...) from a native DLL on Windows doesn't reach Unity's Console — Unity Editor isn't attached to the process stdout. That's why the previous marker line never showed up despite the patched DLL being loaded. Replaced with: - xatlasIsPatchedBuild() C export returning the magic 0x5A5A5A5A - C# probe call at the start of RunPackCancelable that logs via UvtLog (managed → Debug.Log → Unity Console) - Falls back to a Warn log if the symbol is missing, which lets us tell apart "patched DLL with passthrough bug" from "stock DLL still cached" Look for either of these lines per pack: [xatlas-bridge] probe=0x5A5A5A5A preserveChartScale=1 (expected …) [xatlas-bridge] STOCK DLL loaded (patched probe failed: …)
Probe confirms patched DLL is loaded and preserveChartScale=1 is being passed in, but density spread still measures ~14×. Either the if(!s_preserveChartScale) gate isn't taken at runtime (some scope or linkage issue I'm missing) or there's a second per-chart scale step downstream that the upstream Issue #18 discussion didn't cover. Adds two counters bumped from inside Stage B itself: - s_stageBSeenCount — entered the "if (extents>0)" block - s_stageBRescaledCount — actually ran the rescale loop After PackCharts returns, C# reads them via xatlasGetStageBSeenCount / xatlasGetStageBRescaledCount and logs: [xatlas-bridge] post-pack: flag=N stageB-seen=K stageB-rescaled=M Expected on patched + flag=1: seen=149 (number of charts), rescaled=0. If rescaled>0 with flag=1 → the gate isn't taking the patched branch. If rescaled=0 but density 14× → the rescale loop isn't the source; look for a second scale step.
Approach swap: instead of patching xatlas's per-chart ceil(extents) rescale (xatlas.cpp:8345-8362, upstream Issue #18 wontfix), prepare the input so that ceil() becomes a no-op. SnapShellsToIntegerPixels scales each shell per-axis around its UV centroid so the bbox extent is already an integer number of atlas texels. Uniform per-shell texel density survives the pack without forking xatlas. Native: - Drop vendored Native~/third_party/xatlas/ (~10K LOC). - CMakeLists.txt back to FetchContent_Declare(xatlas) against upstream master. - xatlas-unity-bridge.cpp: drop probe / Stage B counters / preserveChartScale. Managed: - XatlasRepack.SnapShellsToIntegerPixels — per-axis pre-pack snap, called from RepackSingle and RepackMulti after TexelDensityNormalizer.Normalize. - RepackOptions.snapShellsToIntegerPixels (default true) replaces preserveChartScale. - UvToolContext.SnapShellsToIntegerPixels + UI toggle in Pre-pack panel. Codex review fixes: - ExecSweep: force RepackResolutionMode = Manual for the sweep, restore on exit. Previously AutoFromTexelDensity overrode every cell's atlasRes and collapsed the resolution dimension of the sweep. - BenchmarkRecorder.SetResolvedAtlasResolution — ExecRepackCore stamps the recorder with the resolution xatlas actually packed at, so the atlasRes column reflects post auto-compute, not the raw ctx setting. - ExecFullPipeline / ExecTransferAll RecordMesh loops skip entries with include == false (user-deselected meshes were surfacing as failed rows in sweep aggregates). - BenchmarkSweep totalMs uses pipelineMs (already wraps inner stages); falls back to repack+transfer+validate only for standalone runs.
Previously SnapShellsToIntegerPixels aligned shell extents to integer texels at tpu = opts.resolution, but xatlasPackCharts was called with texelsPerUnit = 0 — so xatlas auto-computed its own tpu and grew the atlas (e.g. 256 requested → 401×407 packed). The snap grid and the pack grid never matched, so xatlas's Stage B ceil(extents * tpu) fired anyway and density variance stayed ~24× (only post-pack correction brought it down to 3.76×). Fix: compute tpu = internalRes × sqrt(targetUvCoverage) on the C# side (ComputeEffectiveTpu) and pass that same value to both SnapShellsToIntegerPixels and xatlasPackCharts. After TexelDensityNormalizer the total chart area equals targetUvCoverage, so total texel-space area equals internalRes² × targetUvCoverage — xatlas does not need to grow the atlas, the snap grid is preserved end-to-end, and ceil() becomes a no-op for every chart. SnapShellsToIntegerPixels signature changed from `uint atlasRes` to `float tpu` to consume the same value the pack call uses.
The previous fix (passing effectiveTpu to xatlas) made the atlas land at the requested 256×256 instead of 401×407, so the snap grid and pack grid finally agreed on resolution. But density variance only dropped a little: maxRatio stayed at 20× postAssign and 3738× in xatlasRaw chart areas. snap maxScale of 1.51 in the log shows the snap itself ran cleanly. Root cause: xatlas rotates each chart to minimise its axis-aligned bbox BEFORE computing extents. So even when the input UV bbox is integer-pixel aligned, the post-rotation bbox is fractional and Stage B ceil(extents * tpu) amplifies it again. The snap is done on the pre-rotation UVs, so it has no effect. Fix: when snap is on, force rotateCharts=0 and rotateChartsToAxis=0 so xatlas operates on the snapped input UVs directly. Pack efficiency drops slightly without rotation, but for lightmap UV2 uniform density is the priority — that's the whole point of the snap.
The snap was aligning shell extents to integer texels at tpu=221.7, but xatlas multiplies chart UVs by sqrt(surfaceArea3D/parametricAreaUV)*tpu before its Stage B ceil() rescale (xatlas.cpp:8318). For our post-Normalize density (au/a3 = 0.0728), that sqrt factor is 3.706, so the real per-vertex multiplier inside xatlas is 821.5, not 221.7. Snapping to 221.7-grid lands at fractional positions in xatlas's pack space — Stage B ceil() then amplifies sub-pixel ribbons up to ~20x in area, which matches what we saw in the log (postAssign maxRatio=23x). Fix: compute the actual snap target with ComputeXatlasSnapTpu, which multiplies effectiveTpu by sqrt(sum3D/sumUV). For uniform-density input that's the same per-chart factor xatlas will apply, so snapped extents stay integer through Stage B.
Two bugs found by reading xatlas.cpp closely: 1) For UvMesh input, xatlas hardcodes surfaceArea = parametricArea (xatlas.cpp:8255). The per-chart scale collapses from sqrt(s/p)*tpu to plain tpu. My previous commit added that sqrt factor — it was wrong and made snap target the wrong grid. Reverted ComputeXatlasSnapTpu, snap now uses effectiveTpu directly. 2) When PackOptions has texelsPerUnit>0 AND resolution>0, xatlas.cpp:8367-8385 force-clamps every chart whose post-scale extent exceeds (resolution - 2*padding) by per-chart rescale. That rescale is the actual source of the 23x postUV2 density variance — large shells get squeezed independently, breaking the uniform density set up by TexelDensityNormalizer. Fix: when snap is on, pass resolution=0 to xatlasPackCharts so xatlas only packs charts and lets the atlas grow to fit. The user-facing resolution becomes purely a downstream normalisation target. Combined with rotateCharts=0 (from previous commit), the input snap grid should now survive end-to-end: integer pixel extents in, no per-chart rescale, no rotation, integer pixel extents out.
postUV2 maxRatio stayed byte-identical at 23.11x across three very different xatlas configurations (sqrt(s/p) on/off, resolution=256 vs 0, rotateCharts on/off). That meant the variance wasn't coming from xatlas at all — it was already baked into the UVs we hand xatlas. Cause: PerturbOverlapShellsUv0 runs *after* SnapShellsToIntegerPixels and rescales each overlap-group member by 1 + g×strength around the group rep's centroid. For a 92-shell overlap group that compounds into a 1.0..(1+91×s) range of scales and translations per shell. Shells that were snapped to integer-pixel extents leave perturb with fractional extents, and xatlas's Stage B ceil() then amplifies them back into the 23x density variance. Fix: swap the order — perturb first, snap second. Snap now operates on the final pre-xatlas UVs, so the integer-pixel grid actually survives into xatlas.
Five commits of pre-pack integer-pixel snap (716f8bd..9c03555) all failed to reduce density variance. Net result on Carousel: postUV2 density spread grew from ~14x (no snap) to 20-23x with snap, and PostPackDensityCorrection went from 2.95x to 4x — i.e. the snap made things measurably worse. Math shows why: xatlas's per-chart scale = sqrt(s/p) × tpu, computed from current parametricArea. Our per-shell snap (sx, sy) around the centroid changes p by sx×sy, so xatlas's scale rebalances by 1/sqrt(sx×sy) and partially undoes the snap. For anisotropic snap (sx ≠ sy), post-xatlas pixel extent = original × sqrt(sx/sy) — still not integer, so Stage B ceil() still amplifies. For isotropic snap (sx = sy = s), xatlas's scale = 1/s fully undoes the snap. No combination of pre-pack manipulation can survive xatlas's per-chart parametricArea recompute. Only options that would work are (a) forking xatlas, (b) skipping its pack stage via a custom packer, or (c) computing density post-pack and shrinking (which is what PostPackDensityCorrection already does). Reverts: - SnapShellsToIntegerPixels + ComputeEffectiveTpu helpers - RepackOptions.snapShellsToIntegerPixels field - UvToolContext.SnapShellsToIntegerPixels field - UI toggle "Snap shells to integer atlas pixels (pre-pack)" - forced rotateCharts=0 / resolution=0 branches in pack calls - swapped perturb/snap order Now pipeline is: Normalize (uniform au/a3 in UV0) → Perturb (break xatlas dedup on overlap groups) → xatlas pack → PostPackCorrection (shrink-only fix to ~3x density spread). Same as before snap experiment. Documented findings in Documentation~/EXPERIMENTS.md so the next iteration won't repeat the same dead ends.
Defaults - internalOversample: 1 → 4. xatlas's Stage B does ceil(extent)/extent per-axis per-chart; sub-pixel shells get massive amplification at the user-facing 256 resolution. Running the pack internally at 4× resolution turns a 0.25 px shell into a 1.0 px shell, dropping amplification from 4× to 1× on typical shells. Output is normalised via /atlasW so the effective user atlas stays at opts.resolution. - rotateChartsToAxis: true → false. For UvMesh input (repack of existing UVs) PCA rotation is an extra mutation that shrinks pixel extents and worsens Stage B amplification. rotateCharts (90° pack placement) stays on. Diagnostic - LogStageBRisk predicts xatlas Stage B amplification BEFORE xatlas runs: estimates tpu the way xatlas does (sqrt(res²/(area/0.75))) and computes per-shell ceil(extent)/extent on each axis. Logs worst shell, count of sub-pixel shells, count with areaBoost>1.5× and >3×. Top-5 worst go to Verbose log. - Called twice in RepackSingle and RepackMulti: postNormalize (baseline) and postPerturb (to see if PerturbOverlapShellsUv0 introduces extra amplification risk). This lets us see whether the remaining ~3× density spread is from Stage B on real sub-pixel ribbons or from something else in the pipeline. Combined with the oversample bump, expected outcome is that DensityRisk subPixel count drops, postUV2 maxRatio drops toward ~2× without post-pack correction.
…le=4 Two issues found by the DensityRisk diagnostic: 1) PerturbOverlapShellsUv0 was scaling each overlap-group member by 1 + g×strength around the group rep's centroid. For a 92-shell group with strength=0.03 that compounds to scale=3.73 → area ×14. On the Carousel sample sumUV jumped from 0.75 to ~73 (97× growth) which collapses xatlas's auto-computed tpu from ~256 to ~104, pushing every shell back into the sub-pixel regime — exactly what we were trying to avoid. The whole purpose of Perturb was to break xatlas's UV-similarity dedup, but xatlas does NOT dedup UvMesh charts that way: at addUvMeshCharts → ComputeUvMeshChartsTask (xatlas.cpp:6228-6275) it segments faces into charts by faceMaterial (our shellID, unique per shell) plus colocal-UV walk gated by vertexToChartMap. Distinct shellIDs always land in distinct charts regardless of UV overlap. Perturb was a costly no-op. Removed the calls from RepackSingle and RepackMulti. The PerturbOverlapShellsUv0 helper itself is kept (internal) in case a non-UvMesh path ever needs UV-dedup mitigation — but if so it should be an area-preserving shear, not cumulative scale. 2) UvToolContext.InternalOversample default was 1 — it overrode the RepackOptions.Default = 4 set in the previous commit via ExecRepackCore's opts.internalOversample = ctx.InternalOversample. Bumped UvToolContext.InternalOversample to 4 so the default actually reaches xatlas. Diagnostic also simplified: one [DensityRisk:prePack] log right before xatlas, instead of pre/post-Perturb pair.
Captures the working configuration from a218a2b in EXPERIMENTS.md: - InternalOversample=4 default in both RepackOptions and UvToolContext - rotateChartsToAxis=false in RepackOptions.Default - Removal of PerturbOverlapShellsUv0 from both pack paths (xatlas does not dedup UvMesh charts by UV similarity) - [DensityRisk:prePack] diagnostic that predicts Stage B amplification with the same tpu formula xatlas uses internally Plus measured before/after on the Carousel sample (149 shells, 5 overlap groups). End-to-end density spread 14x to 1.17x, atlas utilization 28-34% to 55%, all 149 shells within +/-10% of median. Known regressions left for next session: - Pack at internalRes=1024 (4x oversample of 256) is noticeably slower under bruteForce — needs a soft fallback to heuristic pack above some cost threshold. - Transfer quality dropped on some test meshes — likely from the enlarged atlas (1389x1360) shifting epsilon thresholds in GroupedShellTransfer overlap detection. Needs a sweep over the test suite and either an epsilon fix or early normalisation.
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4b5d3cbe0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
BenchmarkSweep.cs: - Skip winner.json / index.html when every cell scored -Infinity (all hadFailure=true). Was emitting summaries[0] as winner from entirely invalid data. Now writes only summary.csv and logs a warning. (P2) - Mark a run as hadFailure=true when targetRowCount == 0. Failure detection previously only ran inside the !isSource branch, so a CSV with no target-LOD rows (single-LOD model or all targets dropped) passed scoring with zeroes. (P1) - Include atlasUtilization == 0 in the mean. Was filtering util > 0f, which dropped zero rows (failed/degenerate outputs) and inflated the mean by keeping only good rows. Now counts every successfully- parsed numeric value; only missing/unparseable values skip. (P1) LightmapTransferTool.cs: - Use yyyyMMdd_HHmmss_fff for sweep_<stamp> directory. Two sweeps in the same second used to land in the same folder and overwrite each other's summary/winner artefacts. (P2) FbxMetricsExporter.cs: - Stop concatenating "modelName:lodGroupName" into the modelName column. Pass them as separate args to AnalyzeMesh — the lodGroup is already its own column, so the concatenation duplicated dimensions and broke grouping by raw model identifier. (P2)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ExecFullPipeline,ExecRepack,ExecTransferAll) and writes per-mesh CSV + JSON reports toBenchmarkReports/on disposal. Captures transfer results, validation metrics, topology/symmetry counters, and UV2 snapshots as PNG per mesh.SweepMatrixfor automated parameter sweeps (atlasRes × shellPad × borderPad).UvtLogwith per-category mute mask (SymSplit, Repack, Match, Dedup, Overlap, Topology, Validation, Export, Benchmark) configurable in Pipeline Settings.LastFallbackCount,LastTotalSplitCounttoSymmetrySplitShellsandLastTopologyIterations,LastTopologyFixed,LastTopologyCapHittoGroupedShellTransferfor metrics capture.Changed Zones
Editor/— Editor tools / UITRANSFER_BENCHMARK.md)Checklist
.metafiles present for all new files/directoriesTest Plan
TestSuiteAssetviaAssets → Create → Lightmap UV Tool → Test Suite; add a test case pointing to an FBX.Mesh Lab → Export FBX Metrics (Selected Assets)to generate baseline CSV + UV0/UV2 PNGs inBenchmarkReports/.BenchmarkReports/with correct filenames and per-mesh records.{fileBase}_png/subdirectory with per-shell coloring.Review Notes
BenchmarkRecorder.NewRun()returns a no-op scope ifCurrent != null, preventing double-wrapping.SymmetrySplitShells.LastFallbackCountandGroupedShellTransfer.LastTopologyIterationsare reset by the caller (BenchmarkRecorder) at session start so each run's delta is captured.RunRecordto enable per-mesh PNG dumps; only written if both UV2 and triangles are present.UvShellExtractor.Extract()so visual diffs between sweep cells are consistent.ResetWorkingCopies()https://claude.ai/code/session_01WTRFtxYxhhb62t4hhzqspY