Add xatlas native library and improve tool robustness - #191
Merged
Conversation
Use jq + bash regex for package.json parsing and semver validation, stop persisting the checkout credential, and pass step outputs via env instead of interpolating them into the shell. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
Drop the contents:write publish job from build-native.yml, default the workflow to read-only permissions, and pin the actions to verified SHAs. Native artifacts are now uploaded only; a maintainer commits them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
Forwarding %* let cmd.exe re-parse metacharacters from the original command line. Whitelist the documented flag/positional forms and quote each argument explicitly instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
Phase 0 wrote raw package.json values into /tmp/skills-overhaul.env, which Phase 3 later sources — a command-substitution injection chain. Emit the file with `declare -p` so values cannot be reinterpreted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
npm ignores .gitignore entirely once .npmignore exists, so local Unity, IDE and build-intermediate artifacts could be published. Add the missing patterns; no tracked file is affected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
FetchContent pulled jpcy/xatlas at GIT_TAG master, so every native build compiled whatever HEAD happened to be. Vendor a reviewed snapshot under Native~/third_party/xatlas and point CMake at it; meshoptimizer keeps its pinned tag. The vendored xatlas.cpp/xatlas.h are byte-identical to upstream jpcy/xatlas f700c77. The PR's copy carried ~34 extra lines (an unreferenced s_preserveChartScale flag plus "SashaRX.UnityMeshLab fork" comments) that are called from nowhere in this repo; those were stripped so the snapshot stays a verbatim upstream copy. The prebuilt binaries under Plugins/ are intentionally left at their current versions — they must be rebuilt from these vendored sources via the build-native workflow and committed by a maintainer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
resolution * internalOversample was computed in uint and the pack cost in long, so both could wrap and silently bypass the pack-cost safety budget before reaching native xatlas. Resolve dimensions through ulong and reject out-of-range values; saturate ComputePackCost at long.MaxValue. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
BenchmarkSweep.WriteSummaryCsv wrote user-controlled paths unescaped, so a cell could start with =, +, - or @ and be evaluated as a formula on open. Prefix such values with an apostrophe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
FbxMetricsExporter.WriteCsv wrote modelName/lodGroupName/rendererName unescaped, so a cell could start with =, +, - or @ and be evaluated as a formula on open. Prefix such values with an apostrophe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
BenchmarkRecorder.Csv wrote user-controlled asset names unescaped. Prefix values leading with =, +, -, @, tab, CR or LF with an apostrophe so the exported cell stays plain text. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
render_model interpolated the model name into the nav link href without escaping, unlike render_index which already escaped the same pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…, #159, #161) Follow-up to the three CSV formula-injection fixes, which each grew a near-identical private Csv() helper with slightly different prefix sets. Move the logic into internal static CsvUtil.Escape (superset behaviour: =, +, -, @, tab, CR, LF) and make BenchmarkSweep, FbxMetricsExporter and BenchmarkRecorder delegate to it. Each type keeps its private Csv entry point, so the tests added by those PRs are unchanged and still pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
#151) Bounds-check hull/vertex/triangle ranges and index encodings from project-controlled sidecars before allocating or building meshes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…189) Clamp atlas resolution and padding before they reach xatlas, and reject sidecar save paths that escape the Assets folder. The resolution ceiling is 16384 so manually typed values are not silently reduced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…182) Store the optimized mesh colors in the sidecar and use them directly on replay, so merged and orphan vertices no longer end up black. The remap path stays as the fallback for legacy sidecars without color data. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
Reject sidecar UV channel indices outside Mesh.SetUVs' 0..7 range before replay: the primary channel falls back to UV2, the auxiliary channel is skipped with a warning. Channel 0 stays valid because AO bakes can legitimately target UV0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
Reject negative or non-multiple-of-three submesh index counts and accumulate the running total as long, so crafted counts can no longer wrap past the equality guard into a bad allocation. Bumps the postprocessor version so affected models are reimported. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
A partially rebuilt remap left zeroed optimized vertices still referenced by restored triangles. Abort the replay instead, and stop counting legitimate -1 entries as matches. The quadratic nearest-neighbour pass 2 is removed with it: sidecars are imported automatically, so that scan can stall the Editor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
Cap candidate comparisons in PickBestCandidate and RemapUvSetIfNeeded so degenerate sidecars cannot make an import quadratic, replace the duplicate-bucket rescan with a per-bucket cursor, and make the unused-sidecar fallback use swap-remove instead of List.RemoveAt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
Finite doubles above float.MaxValue cast to Infinity when packed into the flat UV buffer handed to native xatlas; reject them like other non-finite results and keep the original UVs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
Cap diagnostic UV2 snapshots per run, skip meshes above the UvPngWriter vertex/index limits, and reject oversized or negatively indexed input in UvPngWriter.Render. The snapshot budget is only consumed by meshes that actually yielded UV2 data. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
TransferResult.uv2 is allocated before the cancel checkpoints, so a cancelled transfer returned a zero-filled UV2 array that callers wrote to the mesh. Route every checkpoint through CancelTransfer, which restores the null-UV2 failure contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
Async repack yields back to the editor while the native bridge still owns a single process-global atlas, so a second entry could destroy an in-flight atlas. Guard every managed session with a fail-fast reentrancy flag released after xatlasDestroy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
Reject negative padding (which underflows to a huge uint in native xatlas), NaN/out-of-range stretch thresholds, out-of-range ARAP iterations, and cartesian products that explode the cell count. Surface the reason in the sweep UI and refuse to start. Also states the 0..200 ARAP range in the suite tooltip (folded in from #133). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…131) ComputeTotal3DAreaMeters copies mesh.vertices and every submesh index array; it ran at two OnGUI sites on every repaint. Cache the result keyed by the exact source-mesh references and recompute only on a mesh-set change; ExecRepackCoreImpl refreshes the same cache with the value it already computes. Labels stay live and the control count is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
Mathf.Abs(int.MinValue) throws OverflowException, and the centroid/bbox hash can produce it. Route all five palette-index sites in UvCanvasView and ShellColorModelPreview through a shared NonNegativeColorKey helper and mark the hash arithmetic unchecked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
FindOverlapGroups is a quadratic shell-pair scan. Skip it above 512 source shells, log a warning, and fall back to the original fixed retry count so a highly fragmented mesh cannot stall the Editor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
SpatialPartitioner.DetectOverlap compared every face pair inside each grid cell, worst case O(gridCells * faces^2). Replace it with an inclusion-exclusion incidence count that is linear in face-cell memberships while preserving the shared-vertex exclusion contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
The hover pick read mesh.vertices and ran shell extraction over every triangle on each ~33 ms mousemove. Check index metadata first and enforce a per-hover triangle budget, sized (100k) to still cover typical LOD0 game meshes, with a rate-limited warning so a skipped mesh is visible rather than silently unhoverable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
Cleanup, LOD generation and Model Builder all parsed the trailing _LOD<n> suffix with int.Parse and trusted the result: overflowing digits threw OverflowException and huge indices sized LOD arrays/loops unboundedly. Parse with int.TryParse and reject indices beyond the eight levels LODGroup supports. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
Owner
Author
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/meta-check.yml (1)
89-97: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winИспользуйте строгий шаблон SemVer в обоих workflow.
Текущий шаблон принимает ведущие нули, например
01.02.003. Замените его в.github/workflows/meta-check.yml#L92и.github/workflows/version-bump.yml#L31на^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/meta-check.yml around lines 89 - 97, Replace the version-validation regex in the Check version format step of .github/workflows/meta-check.yml (89-97) with the strict SemVer pattern ^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$, rejecting leading zeros while allowing zero; apply the same regex change in .github/workflows/version-bump.yml (30-34).
🧹 Nitpick comments (4)
Native~/third_party/xatlas/xatlas.cpp (1)
1-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winЗафиксируйте ревизию снапшота xatlas.
README.mdсодержит URL upstream, но не содержит хэш коммита и дату. ДобавьтеNative~/third_party/xatlas/VERSIONс этими данными. Код xatlas не изменяйте.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Native`~/third_party/xatlas/xatlas.cpp around lines 1 - 42, Add Native~/third_party/xatlas/VERSION containing the xatlas upstream commit hash and snapshot date, using the upstream URL referenced by README.md to identify the revision. Do not modify xatlas.cpp or any other xatlas source code.Native~/src/collision.cpp (1)
66-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winПроверьте остальные параметры на границе native ABI.
ConvexDecomp_Computeвсё ещё преобразуетmaxHulls,resolution,maxVertsPerHullиminEdgeLengthвuint32_t. Отрицательные значения превращаются в большие значения и нарушают ограничения V-HACD. Отклоняйте такие значения в native-коде. ПроверяйтеfillModeпо диапазону0..2; преобразование вVHACD::FillModeне является неопределённым поведением, но неизвестное значение не выбирает ни одну ветвьVoxelize.Управляемый UI передаёт
maxHulls: 1..64,resolution: 10000..1000000,maxVertsPerHull: 8..255,minEdgeLength: 1..8иfillMode: 0..2.ConvexDecompSettingsи P/Invoke-метод остаются публичными, поэтому вызывающий код может обойти эти ограничения.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Native`~/src/collision.cpp around lines 66 - 74, Validate all remaining ConvexDecomp_Compute ABI inputs natively: reject non-positive or otherwise invalid values for maxHulls, resolution, maxVertsPerHull, and minEdgeLength before converting them to uint32_t, and reject fillMode values outside 0..2 before constructing VHACD::FillMode. Preserve the managed UI ranges as the accepted native boundary and return the existing failure result for invalid input.Editor/Tools/CleanupTool.cs (1)
1531-1542: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winДублирование
TryParseLodIndexвModelBuilderTool.cs.Согласно графу связей,
Editor/Tools/ModelBuilderTool.csсодержит идентичную реализациюTryParseLodIndex(та же регулярка, тот же порогMaxLodLevels). Логика разбора и ограничения индекса LOD дублирована в двух файлах. Обновление порога или паттерна в одном месте без синхронизации с другим создаёт риск расхождения поведения.Перенесите
TryParseLodIndex(и константуMaxLodLevels) в общий утилитарный класс (например,MeshHygieneUtility), уже используемый обоими инструментами.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Editor/Tools/CleanupTool.cs` around lines 1531 - 1542, Move the shared TryParseLodIndex implementation and MaxLodLevels constant from the tool-specific classes into the existing MeshHygieneUtility class, then update CleanupTool and ModelBuilderTool to call the utility and remove their duplicate definitions while preserving the current regex, case-insensitive matching, parsing, and bounds behavior.Editor/Framework/UvCanvasView.cs (1)
913-919: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueДублирование
NonNegativeColorKeyв двух файлах.Editor/Framework/UvCanvasView.csиEditor/ShellColorModelPreview.csнезависимо определяют идентичный приватный статический методNonNegativeColorKey, решающий одну и ту же задачу — безопасную нормализацию хеш-ключа без переполненияMathf.Abs(int.MinValue). Общий корень причины — отсутствие разделяемого helper-класса для этой операции.
Editor/Framework/UvCanvasView.cs#L913-L919: удалите локальный методNonNegativeColorKey, перенесите его в общий статический helper-класс (например, рядом сUvtLogили в новыйUvColorUtil).Editor/ShellColorModelPreview.cs#L107-L111: замените локальный методNonNegativeColorKeyвызовом того же общего helper-класса.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Editor/Framework/UvCanvasView.cs` around lines 913 - 919, Вынесите дублирующийся статический метод NonNegativeColorKey в общий helper-класс, сохранив безопасную обработку int.MinValue. В Editor/Framework/UvCanvasView.cs, строки 913-919, удалите локальную реализацию и используйте общий helper; в Editor/ShellColorModelPreview.cs, строки 107-111, также удалите локальную реализацию и замените её вызовом того же helper-класса.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-native.yml:
- Line 41: Update the actions/checkout step in the native build workflow to set
persist-credentials to false, ensuring the checkout does not store GITHUB_TOKEN
in .git/config.
In @.github/workflows/meta-check.yml:
- Line 87: Require exactly one root JSON object in both workflows: update
.github/workflows/meta-check.yml lines 87-87 to use jq -e -s 'length == 1 and
(.[0] | type == "object")' package.json, and add the same validation in
.github/workflows/version-bump.yml lines 30-40 before package.json is read or
written.
In @.github/workflows/version-bump.yml:
- Around line 35-37: In the version-bump step around CURRENT parsing and
NEW_PATCH calculation, validate that PATCH is below Bash’s maximum supported
integer before performing PATCH + 1. Reject the overflow boundary and stop the
workflow with a clear error instead of constructing or committing an invalid
NEW_VERSION.
In @.npmignore:
- Around line 21-56: Добавьте шаблон `.codex/` в `.npmignore`, чтобы каталог с
внутренними инструкциями, включая `.codex/agents/build.toml`, исключался из
npm-пакета при публикации.
In `@Editor/CsvUtil.cs`:
- Around line 21-31: Update the CSV handling used by BenchmarkSweep.AggregateRun
to preserve logical RFC 4180 records when fields contain CR/LF, rather than
splitting physical lines via File.ReadAllLines; alternatively normalize CR/LF in
Escape before writing. Ensure BenchmarkRecorder.BuildCsv and AggregateRun
round-trip a renderer name containing a newline without corrupting metrics or
sweep selection, and add the requested regression test.
In `@Editor/SpatialPartitioner.cs`:
- Around line 302-333: In DetectOverlap, deduplicate the vertex-pair keys
generated for each triangle before incrementing pairCounts, so a degenerate
triangle contributes at most once per unique pair. Update the pair-counting
logic around VertexPairKey and preserve the existing vertex and triple counting
behavior.
In `@Editor/Tools/ModelBuilderTool.cs`:
- Around line 13-14: Apply MaxLodLevels consistently in NormalizeHierarchy,
RebuildLodGroupFromNames, and AddLodLevel so no LOD name or group entry exceeds
eight levels; clamp or reject higher indices while preserving valid levels, and
emit an explicit warning when higher levels are encountered.
In `@Editor/Uv2AssetPostprocessor.cs`:
- Around line 806-818: Update the color restoration logic around
entry.optimizedColors so optColors is allocated whenever entry.optimizedColors
is non-null and its length equals optCount, regardless of rawColors
availability. Preserve copying optimized colors as the authoritative source,
while only using rawColors/remap for fallback reconstruction when optimized
colors are unavailable.
In `@Editor/UvPngWriter.cs`:
- Around line 50-53: Update the validation condition in UvPngWriter to reject
tris arrays whose length is not divisible by three by adding a tris.Length % 3
!= 0 check. Preserve the existing validation and return false behavior for all
invalid inputs.
In `@Editor/VertexAOBaker.Blur.cs`:
- Around line 80-95: В циклах поиска вокруг вершины добавьте отдельный счётчик
проверенных кандидатов, увеличивайте его непосредственно перед вызовом
TryConnectSeamVerts и ограничивайте им все три цикла и внутренний цикл.
Сохраните matched только для подсчёта успешных соединений, чтобы поиск
прекращался после MaxSeamCandidatesPerVertex проверок даже при отсутствии
совпадений.
In `@Editor/VertexAOBaker.cs`:
- Around line 216-219: Update the parallel baking flow in the method containing
Parallel.For so cancellation sets a shared cancellation flag and all iterations
stop contributing to correction and totalWeight once cancellation is observed.
After Parallel.For, check that flag and return the original ao copy instead of
applying partial results; preserve normal result application when the bake
completes without cancellation.
In `@Editor/XatlasRepack.cs`:
- Around line 187-203: Update the public repack flow around AcquireNativeSession
so session contention is reported through RepackResult.error instead of escaping
as an InvalidOperationException, preserving the existing result-based error
contract for RepackSingle and RepackMultiCore. Also ensure RepackUv always
destroys its temporary Instantiate mesh in a finally block, including when
RepackSingle fails before returning.
In `@Tests/Editor/VertexAOBakerBlurTests.cs`:
- Line 4: Удалите проверку времени выполнения через Stopwatch и условие
ElapsedMilliseconds < 20000 в тесте VertexAOBakerBlurTests, оставив ограничение
Timeout(30000) для зависших тестов; удалите ставший ненужным using
System.Diagnostics.
In `@Tools`~/gen.bat:
- Line 25: Validate the gallery-id and output arguments before the command
invocations in the batch script, especially the `%~5` value used by the
generation command and the corresponding arguments at the other referenced
commands. Permit only a strict safe character set, reject invalid values before
invoking `python`, and ensure the validated values cannot inject quotes or
cmd.exe metacharacters.
---
Outside diff comments:
In @.github/workflows/meta-check.yml:
- Around line 89-97: Replace the version-validation regex in the Check version
format step of .github/workflows/meta-check.yml (89-97) with the strict SemVer
pattern ^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$, rejecting leading
zeros while allowing zero; apply the same regex change in
.github/workflows/version-bump.yml (30-34).
---
Nitpick comments:
In `@Editor/Framework/UvCanvasView.cs`:
- Around line 913-919: Вынесите дублирующийся статический метод
NonNegativeColorKey в общий helper-класс, сохранив безопасную обработку
int.MinValue. В Editor/Framework/UvCanvasView.cs, строки 913-919, удалите
локальную реализацию и используйте общий helper; в
Editor/ShellColorModelPreview.cs, строки 107-111, также удалите локальную
реализацию и замените её вызовом того же helper-класса.
In `@Editor/Tools/CleanupTool.cs`:
- Around line 1531-1542: Move the shared TryParseLodIndex implementation and
MaxLodLevels constant from the tool-specific classes into the existing
MeshHygieneUtility class, then update CleanupTool and ModelBuilderTool to call
the utility and remove their duplicate definitions while preserving the current
regex, case-insensitive matching, parsing, and bounds behavior.
In `@Native`~/src/collision.cpp:
- Around line 66-74: Validate all remaining ConvexDecomp_Compute ABI inputs
natively: reject non-positive or otherwise invalid values for maxHulls,
resolution, maxVertsPerHull, and minEdgeLength before converting them to
uint32_t, and reject fillMode values outside 0..2 before constructing
VHACD::FillMode. Preserve the managed UI ranges as the accepted native boundary
and return the existing failure result for invalid input.
In `@Native`~/third_party/xatlas/xatlas.cpp:
- Around line 1-42: Add Native~/third_party/xatlas/VERSION containing the xatlas
upstream commit hash and snapshot date, using the upstream URL referenced by
README.md to identify the revision. Do not modify xatlas.cpp or any other xatlas
source code.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db1a2353-bdde-489b-89e6-4120bcc5df6f
📒 Files selected for processing (59)
.claude/skills/skills-overhaul-plan.md.github/workflows/build-native.yml.github/workflows/meta-check.yml.github/workflows/version-bump.yml.npmignoreCHANGELOG.mdDocumentation~/EXPERIMENTS.mdDocumentation~/TRANSFER_BENCHMARK.mdEditor/ArapParameterization.csEditor/BenchmarkRecorder.csEditor/BenchmarkSweep.csEditor/CollisionMeshBuilder.csEditor/CsvUtil.csEditor/CsvUtil.cs.metaEditor/FbxMetricsExporter.csEditor/Framework/UvCanvasView.csEditor/Framework/UvToolHub.csEditor/GroupedShellTransfer.csEditor/MeshOptimizer.csEditor/Settings/TestSuiteAsset.csEditor/ShellColorModelPreview.csEditor/SpatialPartitioner.csEditor/Tools/CleanupTool.csEditor/Tools/CollisionMeshTool.csEditor/Tools/LightmapTransferTool.csEditor/Tools/LodGenerationTool.csEditor/Tools/ModelBuilderTool.csEditor/Tools/VertexAOTool.csEditor/Uv2AssetPostprocessor.csEditor/Uv2DataAsset.csEditor/UvPngWriter.csEditor/VertexAOBaker.Blur.csEditor/VertexAOBaker.Gpu.csEditor/VertexAOBaker.csEditor/XatlasRepack.csNative~/CMakeLists.txtNative~/src/collision.cppNative~/third_party/xatlas/xatlas.cppNative~/third_party/xatlas/xatlas.hREADME.mdTests/Editor/BenchmarkRecorderTests.csTests/Editor/BenchmarkRecorderTests.cs.metaTests/Editor/BenchmarkSweepTests.csTests/Editor/BenchmarkSweepTests.cs.metaTests/Editor/CleanupToolTests.csTests/Editor/CleanupToolTests.cs.metaTests/Editor/FbxMetricsExporterTests.csTests/Editor/FbxMetricsExporterTests.cs.metaTests/Editor/LodGenerationToolTests.csTests/Editor/LodGenerationToolTests.cs.metaTests/Editor/SpatialPartitionerTests.csTests/Editor/SpatialPartitionerTests.cs.metaTests/Editor/ToolSettingsValidationTests.csTests/Editor/ToolSettingsValidationTests.cs.metaTests/Editor/VertexAOBakerBlurTests.csTests/Editor/VertexAOBakerBlurTests.cs.metaTests/Editor/XatlasRepackGroupMergeTests.csTools~/build_gallery.pyTools~/gen.bat
![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: 78a6e7a0f5
ℹ️ 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".
Comment on lines
+55
to
+63
| if (!vertices || !indices || vertexCount <= 0 || indexCount < 3 || indexCount % 3 != 0) | ||
| return nullptr; | ||
|
|
||
| // V-HACD treats indices as unsigned and dereferences them without bounds checks. | ||
| // Validate at the native API boundary so malformed meshes cannot cause OOB reads. | ||
| for (int i = 0; i < indexCount; i++) | ||
| { | ||
| if (indices[i] < 0 || indices[i] >= vertexCount) | ||
| return nullptr; |
There was a problem hiding this comment.
Package consumers execute the prebuilt files under Plugins/, but all three plugin blobs are unchanged even though this native boundary now adds index validation and recursion limits. The checked-in Linux plugin still returns a non-null V-HACD context for an out-of-range triangle index that this source rejects, so the advertised native hardening is absent from the shipped package; rebuild from this source and commit the resulting platform artifacts.
AGENTS.md reference: AGENTS.md:L19-L20
Useful? React with 👍 / 👎.
Comment on lines
+1491
to
+1493
| if (candidateChecksRemaining <= 0) | ||
| UvtLog.Warn($"[UV2 Postprocess] '{mesh.name}': {channelLabel} remap comparison limit reached; " + | ||
| "remaining vertices will keep zero."); |
There was a problem hiding this comment.
When a legitimate remap requires more than one million comparisons—for example, a few thousand vertices that miss their quantized buckets after a small position/order change—the fallback stops here and returns an array whose remaining entries are zero. ApplyUv2Entry then unconditionally passes that array to mesh.SetUVs, so an automatic reimport silently replaces part of the stored UV channel with (0,0); reaching the budget should make this replay fail without modifying the mesh.
Useful? React with 👍 / 👎.
Comment on lines
+92
to
+95
| if (TryConnectSeamVerts(neighbors, positions, normals, uv0, | ||
| vi, vj, posEpsSq, normThresh, uvEps, | ||
| crossHardEdges, crossUvSeams); | ||
| crossHardEdges, crossUvSeams)) | ||
| matched++; |
There was a problem hiding this comment.
On dense meshes containing several nearby but non-coincident vertex clusters in one spatial cell, TryConnectSeamVerts returns false for most candidates, so matched is never incremented and the new 256-candidate limit does not bound those comparisons. Since the cell is ten times wider than the position tolerance, a crafted or degenerate cell can still make every vertex scan nearly the whole group and retain the original quadratic Editor stall; track comparison attempts separately from successful position matches.
Useful? React with 👍 / 👎.
Comment on lines
+174
to
+175
| if (snapshotMesh != null && pngSnapshotsCaptured < MaxPngSnapshots && | ||
| IsPngSnapshotWithinLimits(snapshotMesh)) |
There was a problem hiding this comment.
Once a run records 32 PNG snapshots, or whenever a mesh exceeds the PNG safety limits, this condition leaves uv2Snap and trisSnap null; the later atlas-utilization calculation therefore records the default 0 for that mesh. BenchmarkSweep.Score averages this field with a weight of 100, so larger suites can select a different winner solely because later rows were denied diagnostic PNGs; compute the metric independently and apply the cap only to retained image data.
Useful? React with 👍 / 👎.
Comment on lines
+806
to
+807
| if (optColors != null && entry.optimizedColors != null && entry.optimizedColors.Length == optCount) | ||
| System.Array.Copy(entry.optimizedColors, optColors, optCount); |
There was a problem hiding this comment.
When AO creates vertex colors on an FBX that originally had no color stream, optimizedColors is saved but replay initializes optColors to null because rawColors is empty. This new copy is consequently skipped and the final mesh.SetColors is also skipped, so reimport silently drops the baked AO data; allocate optColors when a valid stored optimized-color array exists, even if the raw mesh lacks that attribute.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
|
|
||
| if (CanApplyUv2(ctx.HasRepack, ctx.HasTransfer)) |
There was a problem hiding this comment.
A cancelled or failed ExecRepackCoreImpl still sets ctx.HasRepack = true unconditionally after processing its failed results, and it can also leave an older repackedMesh attached to an entry. Because this new condition exposes Apply based solely on that flag, a user can apply original or stale UV2 data immediately after the UI reported a repack failure; derive the state from successful current results and clear prior repack outputs before starting a new run.
Useful? React with 👍 / 👎.
…191 review) - build-native.yml: checkout with persist-credentials: false so the job's read-only intent isn't undermined by a token left in .git/config. - meta-check.yml / version-bump.yml: `jq empty` accepts a stream of several root values; slurp and assert a single object root instead. Verified: a two-document file passes `jq empty` and makes `jq -er .version` emit two lines. - Both workflows: strict SemVer regex — the previous one accepted leading zeros (01.2.3). - version-bump.yml: bound the patch component before $((PATCH + 1)), which wraps silently at the 64-bit boundary. Compares digit count because a numeric test on a 20-digit value errors out instead of comparing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…ge (PR #191 review) The reported path (.codex/agents/build.toml) does not exist on this branch, but the same leak is real for .claude/: `npm pack --dry-run` listed 42 .claude/skills/** files plus .vscode/settings.json and .prettierignore in the tarball. Ignore all of them (.codex/ included for the day it appears). Verified: packed file count 237 -> 195, no dotfiles remain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…ne (PR #191 review) CsvUtil.Escape kept CR/LF inside a quoted field (valid RFC 4180), but BenchmarkSweep.AggregateRun reads the report back with File.ReadAllLines and parses each physical line as one record — an asset name containing a newline therefore shifted every column after it. Flatten CR/LF/TAB to spaces before quoting; the formula-neutralising check still runs on the original string so a leading control character keeps its apostrophe. Tests: the three CR/LF/TAB escape cases now expect flattened output, plus a line-break flattening case and a BuildCsv -> ParseCsvRow round-trip that asserts the record survives a File.ReadAllLines-style split. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
review) A face with a repeated index collapses two of its three edges onto the same unordered pair, so DetectOverlap counted that pair twice per face. The inclusion-exclusion read side subtracts the pair count once, so the face's shared-face total came out too low and the face was reported as UV overlap. Emit each distinct pair once per triangle; vertex and triple counting are unchanged (they already skip repeated indices). Verified by replaying both code paths over the test inputs: face (0,7,7) sharing vertex 0 with three other faces in the same grid cell scored 3 of 4 before and 4 of 4 after, and both existing test cases keep their results. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…tes (PR #191 review) TryParseLodIndex + MaxLodLevels were copy-pasted in CleanupTool and ModelBuilderTool. Both now call one internal helper in MeshHygieneUtility (name hygiene already lives there), with the regex compiled once instead of re-parsed on every call. Behaviour is identical apart from a null-name guard. The cap was only honoured on one of the three ModelBuilderTool paths: - RebuildLodGroupFromNames already went through TryParseLodIndex — unchanged. - NormalizeHierarchy numbered mesh children sequentially with no ceiling, so a root with nine mesh children got a _LOD8 name that RebuildLodGroupFromNames then silently rejected. Stop numbering at the ceiling and warn instead of inventing unusable suffixes. - AddLodLevel appended levels without a ceiling — refuse past the eighth. GetLodIndexFromName is left alone: it only builds an inspector label and never indexes a LOD array. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
… remaps (PR #191 review) Confirmed both reports by reading the allocation site: 1. optColors was allocated only when the RAW FBX mesh carried a color channel (mesh.colors32 with rawCount entries), so entry.optimizedColors — the authoritative copy, since merged and orphan vertices cannot be rebuilt from the remap — was silently dropped for any mesh whose colors were produced after import (baked vertex AO on a color-less FBX). Allocate when either source exists; the remap fallback now runs only when the sidecar has no optimized colors AND the raw mesh has some. The legacy path is unaffected: optimized colors require ground truth, which that path does not have. 2. When the remap comparison budget ran out mid-fallback, the half-filled array (zeros for everything after the cut) was handed back and written to the mesh unconditionally, flattening part of the UV channel to (0,0) on every auto-reimport. Abort the entry instead and leave the mesh untouched, matching the stale-remap abort. The primary channel also gets the length check the auxiliary channel already had. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…view) Render already rejected too-few and too-many indices but accepted a trailing partial triangle, which every consumer then truncated with tris.Length / 3 — the diagnostic PNG would quietly not match its input. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…d correction (PR #191 review) - BlurAO's seam pass only counted candidates that actually connected, so a vertex whose grid neighbourhood holds thousands of near-but-not-matching candidates still scanned all of them — the quadratic worst case the cap was supposed to remove. Add a second cap on candidates examined (4096) that bounds all three cell loops and the inner scan; the 256 matched cap is unchanged, and it still trips first on the coincident-vertex case the existing test covers. - FaceAreaCorrection applied whatever correction/totalWeight the cancelled Parallel.For had already accumulated: loopState.Stop() only stops new iterations. Record the cancellation and return the untouched AO copy; normal completion is unchanged. - VertexAOBakerBlurTests: drop the ElapsedMilliseconds < 20000 assertion (it measures the CI runner, not the budget) and the now-unused System.Diagnostics import. [Timeout(30000)] still guards the test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…mp mesh (PR #191 review) AcquireNativeSession threw InvalidOperationException straight through the public RepackSingle / RepackMulti / RepackMultiAsync surface, while every caller (LightmapTransferTool's per-mesh loop, the repack tests) consumes the RepackResult.error contract — so a concurrent repack surfaced as an unhandled exception instead of a per-mesh failure. Return the busy state as RepackResult.error; RepackMultiCore stamps it on every mesh since nothing was packed. The release side is untouched: acquisition still happens outside the try, so a failed claim can never release someone else's session. RepackUv now destroys its temporary Instantiate copy in a finally. The failed -result path already destroyed it, but an exception escaping RepackSingle — including the one above — leaked the mesh into the editor session. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…et (PR #191 review) RecordMesh read UV2 + triangles only while the 32-PNG cap had room, but that same data feeds atlasUtilization — a scored metric that BenchmarkSweep weights x100. Past the 33rd recorded mesh (or for any mesh over the PNG size limits) the metric silently recorded 0, so a large suite could crown a different sweep winner purely from recording order. Read the data for every row (still bounded by the existing mesh-size sanity limits) and apply the 32-snapshot cap only to what is retained for the PNG dump. The pngSnapshotsSkipped counter keeps its original meaning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…es (PR #191 review) ExecRepackCoreImpl set ctx.HasRepack = true after the result loop regardless of outcome, and left each entry's previous repackedMesh in place when the new run failed. Since #170 the Apply UV2 section is drawn purely off that flag, so after a failed or cancelled repack the user could apply a stale result — or, with no prior repack, the original UV2 (GetResultMesh falls back to originalMesh). Now each run clears its entries' repack output up front (destroying the old mesh, which the success path used to overwrite and leak), and HasRepack is derived from whether any entry currently holds a repacked mesh. Deriving it rather than assigning false keeps per-mesh grouping correct: that path calls this method once per group, and a later failing group must not erase an earlier group's success. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…review) ConvexDecomp_Compute checked the geometry arguments and clamped maxRecursionDepth, but cast maxHulls, resolution, maxVertsPerHull and minEdgeLength straight to uint32_t and fillMode straight to the FillMode enum. A negative int wraps to a huge unsigned value in V-HACD (a negative resolution becomes a multi-billion-voxel grid) and an out-of-range fillMode matches no enum case (confirmed 0..2 in third_party/VHACD.h). Reject all of them with the existing nullptr failure result, before CreateVHACD allocates anything. The editor-side ranges all remain valid. Checked with g++ -fsyntax-only -std=c++17 -I third_party (clean). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…view) The xatlas sources under Native~/third_party/xatlas carried no provenance, so nothing recorded which upstream revision the shipped binaries were built from. Add a VERSION file naming the upstream repo, commit f700c77, the license and the snapshot/verification date, plus the rule that local changes go in the bridge rather than the snapshot. xatlas.h / xatlas.cpp are untouched, and Native~ is tilde-hidden so the file needs no .meta. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…eview) Confirmed the #135 whitelist only constrains the option NAMES (%~2 / %~4). The values — %~1, %~3 and %~5 — went through untouched, and cmd.exe substitutes an argument into the python line before parsing that line, so a value carrying a double quote could close the quoting and run whatever followed it. Each forwarded value is now matched against a strict charset (letters, digits, _ - . ~ : \ / and space) and rejected with exit /b 3 otherwise. The test runs through delayed expansion, which substitutes the value after the line is parsed, so the value under test cannot itself be read as syntax. No cmd.exe in this environment, so the batch flow was desk-checked and the charset decisions were verified with the equivalent POSIX class: the example invocation, Windows drive paths and paths with spaces are accepted; quotes, & | < > ^ % ! ( ) ; and apostrophes are rejected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
NonNegativeColorKey was duplicated verbatim in UvCanvasView (2D canvas) and ShellColorModelPreview (3D model preview) — the two systems must agree on how a shell hash maps to a palette slot, and a copy each is how they drift. One internal helper in Editor/UvHashUtil.cs (with .meta, fresh GUID) now serves both; the int.MinValue fold is documented in one place. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…eader The vendored xatlas directory is on the native include path; on case-insensitive filesystems (macOS/Windows) #include <version> resolved to third_party/xatlas/VERSION and broke the build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
Owner
Author
|
Раунд ревью обработан: все 22 замечания (15 CodeRabbit + SemVer/nitpicks + 3 уникальных от Codex) проверены по коду и закрыты 15 коммитами ( Ключевое из раунда:
По замечанию Codex о пересборке Также напоминание: C#-код не компилировался в CI этого репозитория (нет Unity-джоба) — перед merge стоит открыть проект в Unity и прогнать EditMode-тесты. Generated by Claude Code |
The LOD/group FBX export path created two more kinds of temporary meshes that were never released: the per-entry `exportMesh` clones (Instantiate of the result mesh, for both the replace-in-clone and the add-missing-LOD branches) and the `stripped` collision meshes rebuilt for every `_COL` node. DestroyImmediate on tempRoot only frees GameObjects, so both leaked on every export. Route them through the same sink + DestroyTempMeshes pattern introduced in 78a6e7a: the per-group list (renamed `bakedMeshes` -> `tempMeshes`, since it now also carries export clones and stripped collision meshes) collects each mesh right after it is created, and the existing finally block releases them after ModelExporter.ExportObjects has read their vertex data. Only meshes created here are destroyed. The export clones live solely on tempRoot and in a local dictionary; `stripped` copies srcCol's vertex and index data instead of aliasing it, so srcCol (which may be an FBX sub-asset) is untouched. Nothing downstream holds them: renameMap is string->string, RelinkSceneMeshReferences reloads meshes from the reimported FBX, and the sidecar path builds its own clone from resultMesh. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…(leak cleanup) CollisionMeshTool.GetCollisionMeshesFromSidecar allocates a fresh Mesh per hull from the sidecar's serialized position/index arrays. ExportFbx attached them to _COL nodes on tempRoot and then replaced them with the stripped copies, so they were unreachable but never released — a leak on every export of an FBX that has sidecar collision data. Append them to the same per-group tempMeshes sink at the point where they are attached, so the existing DestroyTempMeshes call in the finally block frees them after ModelExporter.ExportObjects. Collecting at the sidecar loop (not the strip loop) is what makes this safe: every mesh on both of that method's return paths comes from the single `new Mesh()` construction site, so nothing shared or asset-backed can enter the sink. The strip loop's srcCol is deliberately still not collected — for collision nodes that came from the source FBX rather than the sidecar it is a real FBX sub-asset. The same caller-owns-the-meshes contract is already assumed by VertexAOTool, which puts them in batch.temporaryMeshesToDestroy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
…abled BenchmarkRecorder.NewRun opened a recording session on every Run Full Pipeline / Repack / Transfer All click, so a production transfer run wrote <projectRoot>/BenchmarkReports/ plus a per-mesh <run>_png/ subfolder of CSV/JSON/PNG analysis artefacts that nobody asked for. Gate the session on MeshLabProjectSettings.showDebugUI — the flag that already hides Parameter Sweep, Log filters, UV0 Analysis & Fix, the Repack "Advanced (debug)" block, the Mesh Lab ▸ Export FBX Metrics menu items and the Sweep Test Suite create action. With the flag off NewRun returns the existing NoOpScope, Current stays null and no folder or file is produced; every consumer already guards on `Current != null` / `_bench is BenchmarkRecorder`, so nothing logs or throws. Sweep and the FBX metrics exporter are reachable only from debug-gated UI, so they keep working unchanged. Existing report folders on disk are untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
The security sweep dropped the commit job from build-native.yml, which meant every native source change required a maintainer to download the CI artifacts and copy them into Plugins/ by hand. That traded away the workflow's whole point for a risk that does not apply here: the job never runs for pull_request, so a fork can never reach the token, and anyone who can push the branch that triggers it can already commit a binary directly. Restore the publish job, keeping the parts of the hardening that cost nothing: the workflow still defaults to a read-only token, contents: write is scoped to the publish job alone (the compile jobs never see it), every action stays pinned to a verified SHA, and the auto-commit keeps [skip ci] so it cannot retrigger the workflow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Editor/BenchmarkRecorder.cs (1)
352-353: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winИсправьте причину пропуска PNG в сообщении.
pngSnapshotsSkippedтакже увеличивается при исчерпанииMaxPngSnapshots. Текст сообщает только о safety limits. Укажите оба условия, чтобы отчёт не вводил в заблуждение.- $"{(pngSnapshotsSkipped > 0 ? $" ({pngSnapshotsSkipped} PNG skipped by safety limits)" : "")} → {csvPath}"); + $"{(pngSnapshotsSkipped > 0 ? $" ({pngSnapshotsSkipped} PNG skipped by safety limits or snapshot cap)" : "")} → {csvPath}");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Editor/BenchmarkRecorder.cs` around lines 352 - 353, Обновите формирование сообщения в BenchmarkRecorder, использующее pngSnapshotsSkipped, чтобы оно явно отражало оба условия пропуска PNG: safety limits и исчерпание MaxPngSnapshots. Сохраните текущий формат отчёта и вывод количества пропущенных снимков.Editor/Tools/LightmapTransferTool.cs (1)
2916-2917: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winОсвобождайте копии Mesh из
CopyVertexDataToClone.
CopyVertexDataToCloneсоздаётcloneMeshчерезInstantiateи назначает егоMeshFilter. Этот Mesh не добавляется вbakedMeshes.
DestroyTempMeshes(bakedMeshes)поэтому не освобождает эти копии после экспорта vertex colors. Повторный экспорт оставляет временные Mesh в памяти.Передайте список временных Mesh в
CopyVertexDataToCloneи добавляйте в него каждый созданныйcloneMesh.Also applies to: 2961-2964
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Editor/Tools/LightmapTransferTool.cs` around lines 2916 - 2917, Update CopyVertexDataToClone to accept the temporary-mesh list, add each instantiated cloneMesh to it immediately after creation, and pass bakedMeshes from its callers so DestroyTempMeshes(bakedMeshes) releases all copies after export.
🧹 Nitpick comments (1)
Editor/UvHashUtil.cs (1)
17-20: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winДобавьте регрессионный тест для
int.MinValue.В
Line 19находится специальная ветка для единственного переполняемого значения. Покройтеint.MinValue,-1,0иint.MaxValue. Проверьте, что результат неотрицателен, аint.MinValueпреобразуется вint.MaxValue. Это защищает контракт индексации палитры вUvCanvasView.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Editor/UvHashUtil.cs` around lines 17 - 20, Добавьте регрессионный тест для метода NonNegativeColorKey, проверяющий входы int.MinValue, -1, 0 и int.MaxValue. Убедитесь, что каждый результат неотрицателен, а для int.MinValue результатом является int.MaxValue; сохраните проверку контракта, используемого UvCanvasView.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-native.yml:
- Around line 3-5: Ограничьте `contents: write` отдельным job публикации вместо
всего `commit` job: оставьте compile jobs и checkout с read-only доступом,
настройте `actions/checkout` с отключённым сохранением credentials, а в шаге
`git push` передайте write-токен только через временный `http.extraheader`.
In @.github/workflows/meta-check.yml:
- Around line 92-95: Update the “Check version format (semver)” regex to enforce
the same maximum nine-digit limit for the PATCH component as the version-bump
workflow, while preserving the existing validation of MAJOR and MINOR and the
overall semantic-version format.
In `@Editor/Tools/LightmapTransferTool.cs`:
- Around line 2158-2169: Перед первым repack очистите состояние transfer у всех
target entries: уничтожьте и обнулите transferredMesh, сбросьте
shellTransferResult и validationReport, затем установите ctx.HasTransfer =
false. Внесите это в общий reset-путь рядом с очисткой repackedMesh и сохраните
сброс repackedAtlasWidth/repackedAtlasHeight, чтобы GetResultMesh и
ApplyUv2ToFbx не использовали результаты предыдущего запуска.
In `@Native`~/src/collision.cpp:
- Around line 66-76: Extend the ABI validation near the existing
maxHulls/resolution checks to reject minVolumePerHull when it is not finite or
is negative, using std::isfinite before assigning
params.m_minimumVolumePercentErrorAllowed. Preserve valid finite non-negative
values and the existing nullptr rejection behavior.
In `@Tools`~/gen.bat:
- Around line 30-35: Переработайте разбор аргументов в блоке вокруг ARG1, ARG3 и
ARG5 так, чтобы все позиционные параметры безопасно проверялись до любой
интерполяции в set, if или вызове python. Уберите прямое использование %~N при
включённом EnableDelayedExpansion, добавьте проверку также для %~2 и %~4 и
передавайте только безопасно разобранные значения, исключив возможность закрытия
кавычек и command injection.
---
Outside diff comments:
In `@Editor/BenchmarkRecorder.cs`:
- Around line 352-353: Обновите формирование сообщения в BenchmarkRecorder,
использующее pngSnapshotsSkipped, чтобы оно явно отражало оба условия пропуска
PNG: safety limits и исчерпание MaxPngSnapshots. Сохраните текущий формат отчёта
и вывод количества пропущенных снимков.
In `@Editor/Tools/LightmapTransferTool.cs`:
- Around line 2916-2917: Update CopyVertexDataToClone to accept the
temporary-mesh list, add each instantiated cloneMesh to it immediately after
creation, and pass bakedMeshes from its callers so
DestroyTempMeshes(bakedMeshes) releases all copies after export.
---
Nitpick comments:
In `@Editor/UvHashUtil.cs`:
- Around line 17-20: Добавьте регрессионный тест для метода NonNegativeColorKey,
проверяющий входы int.MinValue, -1, 0 и int.MaxValue. Убедитесь, что каждый
результат неотрицателен, а для int.MinValue результатом является int.MaxValue;
сохраните проверку контракта, используемого UvCanvasView.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf53de40-bb28-4a5a-a4dd-349b8edda4d6
📒 Files selected for processing (30)
.github/workflows/build-native.yml.github/workflows/meta-check.yml.github/workflows/version-bump.yml.npmignoreCHANGELOG.mdDocumentation~/TRANSFER_BENCHMARK.mdEditor/BenchmarkRecorder.csEditor/CsvUtil.csEditor/Framework/UvCanvasView.csEditor/MeshHygieneUtility.csEditor/ShellColorModelPreview.csEditor/SpatialPartitioner.csEditor/Tools/CleanupTool.csEditor/Tools/LightmapTransferTool.csEditor/Tools/ModelBuilderTool.csEditor/Uv2AssetPostprocessor.csEditor/UvHashUtil.csEditor/UvHashUtil.cs.metaEditor/UvPngWriter.csEditor/VertexAOBaker.Blur.csEditor/VertexAOBaker.csEditor/XatlasRepack.csNative~/src/collision.cppNative~/third_party/xatlas/UPSTREAM_VERSION.txtTests/Editor/BenchmarkRecorderTests.csTests/Editor/BenchmarkSweepTests.csTests/Editor/CleanupToolTests.csTests/Editor/SpatialPartitionerTests.csTests/Editor/VertexAOBakerBlurTests.csTools~/gen.bat
🚧 Files skipped from review as they are similar to previous changes (13)
- .npmignore
- Tests/Editor/BenchmarkSweepTests.cs
- .github/workflows/version-bump.yml
- Editor/ShellColorModelPreview.cs
- Documentation~/TRANSFER_BENCHMARK.md
- Editor/Framework/UvCanvasView.cs
- Editor/UvPngWriter.cs
- Editor/VertexAOBaker.cs
- Editor/SpatialPartitioner.cs
- CHANGELOG.md
- Tests/Editor/VertexAOBakerBlurTests.cs
- Editor/XatlasRepack.cs
- Editor/Uv2AssetPostprocessor.cs
Comment on lines
+3
to
+5
| # Default for every job: read-only. The `commit` job below opts into | ||
| # `contents: write` for itself, so only the step that publishes the built | ||
| # binaries carries a writable token — the compile jobs never see one. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file='.github/workflows/build-native.yml'
sed -n '3,5p;59,105p' "$file"
printf '\nКлючевые настройки:\n'
rg -n -C 4 'contents: write|actions/checkout|persist-credentials|git push' "$file"Repository: SashaRX/UnityMeshLab
Length of output: 3285
🌐 Web query:
site:github.com/actions/checkout v4.2.2 persist-credentials default git push http.extraheader README
💡 Result:
The persist-credentials input is a configuration option in the GitHub actions/checkout action (used to check out repositories in GitHub Actions) that determines whether the authentication token used for the checkout is persisted in the local Git configuration [1][2]. When set to its default value of true, the action automatically configures the local Git environment—historically by adding an http.<url>.extraheader to the .git/config file—to allow subsequent Git commands (like git fetch or git push) within the same job to use the same authentication credentials [1][2][3]. Setting persist-credentials: false allows you to opt out of this behavior, preventing the action from automatically adding these headers [1][3]. Regarding the technical implementation: - Historical Context: Older versions of actions/checkout relied heavily on setting http.extraheader to inject the AUTHORIZATION header [3][4]. This approach can sometimes cause conflicts with other Git tools, such as Git LFS, or lead to issues when multiple headers are present [4]. - Version v4.2.2 and Updates: GitHub actions/checkout v4.2.2 is a release version (published October 2024) [5]. Recent improvements in the action have transitioned away from direct extraheader usage toward using Git credential helpers (e.g., credential.helper), which is considered a more robust and secure method for handling credentials in Git [6][2]. - Troubleshooting: If you encounter issues such as HTTP 400 errors or authentication conflicts, it is often due to malformed or persistent http.extraheader configurations in your runner's global or local Git config [7][8]. Ensuring that persist-credentials is managed correctly or manually clearing problematic headers from your Git config can resolve these issues [7][4]. In summary, persist-credentials manages whether the GitHub Action makes your job's authentication token available to Git commands via your local Git config, and the underlying mechanism for this has evolved from extraheader to more modern credential-handling approaches in recent versions [6][2][3].
Citations:
- 1: https://github.com/actions/checkout/blob/main/README.md
- 2: https://www.github.com/actions/checkout
- 3: Header Token overrides user token actions/checkout#181
- 4: 'actions/checkout@v4' with LFS fails because of double auth header actions/checkout#1830
- 5: https://github.com/actions/checkout/releases/tag/v4.2.2
- 6: Use credentials store instead of extraheader actions/checkout#1754
- 7: Malformed auth header in global git config causing HTTP 400 actions/checkout#838
- 8: Fetching submodules fails irregularly actions/checkout#752
🌐 Web query:
site:docs.github.com GitHub Actions permissions job step GITHUB_TOKEN persist credentials checkout
💡 Result:
In GitHub Actions, the actions/checkout action uses the GITHUB_TOKEN to authenticate git operations by default [1][2]. The persist-credentials input for this action determines whether these credentials remain configured in the local git environment after the checkout step completes [1]. Key details regarding persist-credentials and GITHUB_TOKEN: 1. Behavior: When persist-credentials is set to true (which is the default), the actions/checkout action configures the local git environment to use the provided token, allowing subsequent git commands in the same job to authenticate automatically using those same credentials [1][2]. If you set persist-credentials to false, the action will not persist the token in the git configuration, preventing subsequent git commands from using it for authentication [1]. 2. Security: As a security best practice, you should grant the GITHUB_TOKEN only the minimum permissions required for your workflow [3][4][5]. You can configure these permissions at the workflow or job level using the permissions key [3][6][7]. For example, if you only need to clone a repository, you might set the contents permission to read [8][9]. 3. Usage Example: permissions: contents: read jobs: example-job: runs-on: ubuntu-latest steps: - uses: actions/checkout@v6 with: persist-credentials: false # Disables token persistence for git - name: Git commands run: | # Git commands here will not use the GITHUB_TOKEN automatically
Citations:
- 1: https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows
- 2: https://docs.github.com/en/actions/reference/security/securely-using-pull_request_target
- 3: https://docs.github.com/en/actions/tutorials/authenticate-with-github_token
- 4: https://docs.github.com/en/actions/concepts/security/github_token
- 5: https://docs.github.com/en/actions/reference/security/secure-use
- 6: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax
- 7: https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions
- 8: https://docs.github.com/en/enterprise-cloud@latest/actions/how-tos/secure-your-work/security-harden-deployments/oidc-in-aws
- 9: https://docs.github.com/en/actions/how-tos/secure-your-work/security-harden-deployments/oidc-in-aws
Ограничьте доступ к contents: write и отключите автоматическое сохранение credentials.
permissions: contents: write задан на уровне job и действует для всех шагов commit. actions/checkout по умолчанию сохраняет credentials для последующих Git-команд, поэтому текущий git push использует write-токен автоматически. Добавьте persist-credentials: false и передайте токен только команде git push через временный http.extraheader. GitHub Actions не поддерживает permissions на уровне шага; для изоляции вынесите публикацию в отдельный job.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-native.yml around lines 3 - 5, Ограничьте `contents:
write` отдельным job публикации вместо всего `commit` job: оставьте compile jobs
и checkout с read-only доступом, настройте `actions/checkout` с отключённым
сохранением credentials, а в шаге `git push` передайте write-токен только через
временный `http.extraheader`.
Source: Linters/SAST tools
Comment on lines
92
to
+95
| - name: Check version format (semver) | ||
| run: | | ||
| VERSION=$(python3 -c "import json; print(json.load(open('package.json'))['version'])") | ||
| if echo "$VERSION" | grep -qP '^\d+\.\d+\.\d+$'; then | ||
| VERSION=$(jq -er '.version | select(type == "string")' package.json) | ||
| if [[ "$VERSION" =~ ^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$ ]]; then |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Синхронизируйте ограничение PATCH с workflow обновления версии.
Эта проверка принимает третью компоненту версии любой длины. В .github/workflows/version-bump.yml (Lines 30–40) значение PATCH длиной более 9 цифр отклоняется до арифметической операции. Поэтому версия, например 1.2.1000000000, проходит этот job, но затем ломает обновление версии.
Добавьте здесь такое же ограничение или вынесите правило в общий валидатор.
Предлагаемое исправление
if [[ "$VERSION" =~ ^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$ ]]; then
+ PATCH="${VERSION##*.}"
+ if [ "${`#PATCH`}" -gt 9 ]; then
+ echo "::error::Patch component out of range: $PATCH (expected < 1000000000)"
+ exit 1
+ fi
echo "Version: $VERSION"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Check version format (semver) | |
| run: | | |
| VERSION=$(python3 -c "import json; print(json.load(open('package.json'))['version'])") | |
| if echo "$VERSION" | grep -qP '^\d+\.\d+\.\d+$'; then | |
| VERSION=$(jq -er '.version | select(type == "string")' package.json) | |
| if [[ "$VERSION" =~ ^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$ ]]; then | |
| - name: Check version format (semver) | |
| run: | | |
| VERSION=$(jq -er '.version | select(type == "string")' package.json) | |
| if [[ "$VERSION" =~ ^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$ ]]; then | |
| PATCH="${VERSION##*.}" | |
| if [ "${`#PATCH`}" -gt 9 ]; then | |
| echo "::error::Patch component out of range: $PATCH (expected < 1000000000)" | |
| exit 1 | |
| fi | |
| echo "Version: $VERSION" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/meta-check.yml around lines 92 - 95, Update the “Check
version format (semver)” regex to enforce the same maximum nine-digit limit for
the PATCH component as the version-bump workflow, while preserving the existing
validation of MAJOR and MINOR and the overall semantic-version format.
Comment on lines
+2158
to
+2169
| // Drop the previous run's output before producing a new one: | ||
| // otherwise a failed re-run leaves a stale repackedMesh that | ||
| // Apply UV2 would happily write to the FBX (and the successful | ||
| // path used to overwrite the reference, leaking the old mesh). | ||
| if (e.repackedMesh != null) | ||
| { | ||
| UnityEngine.Object.DestroyImmediate(e.repackedMesh); | ||
| e.repackedMesh = null; | ||
| } | ||
| e.repackedAtlasWidth = 0; | ||
| e.repackedAtlasHeight = 0; | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Сбрасывайте результаты transfer перед новым repack.
Новый repack удаляет только repackedMesh. Он оставляет transferredMesh, shellTransferResult, validationReport и ctx.HasTransfer от прежнего source UV2.
GetResultMesh затем выбирает старый transferredMesh для target LOD. ApplyUv2ToFbx может экспортировать source и target UV2 из разных запусков repack.
Перед первым repack очистите transfer-состояние всех target entries и установите ctx.HasTransfer = false.
Предлагаемое исправление
+void InvalidateTransferResults()
+{
+ foreach (var entry in ctx.MeshEntries)
+ {
+ if (entry.transferredMesh != null)
+ UnityEngine.Object.DestroyImmediate(entry.transferredMesh);
+ entry.transferredMesh = null;
+ entry.shellTransferResult = null;
+ entry.validationReport = null;
+ }
+ crossLodHints.Clear();
+ ctx.HasTransfer = false;
+}
+
async Task ExecRepackImpl(List<MeshEntry> entries, bool useAsync)
{
+ InvalidateTransferResults();
if (entries.Count == 0) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Editor/Tools/LightmapTransferTool.cs` around lines 2158 - 2169, Перед первым
repack очистите состояние transfer у всех target entries: уничтожьте и обнулите
transferredMesh, сбросьте shellTransferResult и validationReport, затем
установите ctx.HasTransfer = false. Внесите это в общий reset-путь рядом с
очисткой repackedMesh и сохраните сброс repackedAtlasWidth/repackedAtlasHeight,
чтобы GetResultMesh и ApplyUv2ToFbx не использовали результаты предыдущего
запуска.
Comment on lines
+66
to
+76
| // The remaining tunables are handed to V-HACD as uint32_t or as an enum. | ||
| // A negative int wraps to a huge unsigned value there — a negative | ||
| // resolution becomes a multi-billion-voxel grid, a negative maxHulls an | ||
| // effectively unbounded hull budget — and a fillMode outside the enum | ||
| // selects no case at all. Reject them at the ABI boundary; the editor-side | ||
| // ranges (maxHulls 1..64, resolution 10000..1000000, maxVertsPerHull | ||
| // 8..255, minEdgeLength 1..8, fillMode 0..2) all stay valid. | ||
| if (maxHulls <= 0 || resolution <= 0 || maxVertsPerHull <= 0 || minEdgeLength <= 0) | ||
| return nullptr; | ||
| if (fillMode < 0 || fillMode > 2) | ||
| return nullptr; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 \
'minVolumePerHull|m_minimumVolumePercentErrorAllowed|ConvexDecomp_Compute|ConvexDecompSettings' \
Native~ EditorRepository: SashaRX/UnityMeshLab
Length of output: 10859
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Native~/src/collision.cpp ---'
sed -n '40,110p' Native~/src/collision.cpp
printf '%s\n' '--- V-HACD parameter declaration and uses ---'
rg -n -C 8 \
'm_minimumVolumePercentErrorAllowed|minimumVolumePercentErrorAllowed|m_volumeError' \
Native~/third_party/VHACD.h
printf '%s\n' '--- Editor settings declaration, validation, and inspector ranges ---'
rg -n -C 8 \
'convexMinVolume|minVolumePerHull|minVolume' \
Editor Native~
printf '%s\n' '--- All V-HACD parameter validation and compute call sites ---'
rg -n -C 5 \
'ConvexDecomp_Compute|Parameters params|m_maxConvexHulls|m_resolution|m_maxNumVerticesPerCH' \
Native~ EditorRepository: SashaRX/UnityMeshLab
Length of output: 29216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import math
# V-HACD completes a voxel hull when:
# volume_error < m_minimumVolumePercentErrorAllowed
# or when the recursion-depth limit is reached.
volume_errors = [0.0, 0.5, 1.0, 10.0]
thresholds = [-1.0, 0.0, 1.0, float("inf"), float("nan")]
for threshold in thresholds:
outcomes = [
volume_error < threshold
for volume_error in volume_errors
]
print(f"threshold={threshold!r}: complete_by_error={outcomes}")
for value in [-1.0, 0.0, 1.0, float("inf"), float("nan")]:
print(
f"value={value!r}: finite={math.isfinite(value)}, "
f"non_negative={value >= 0.0}"
)
PY
printf '%s\n' '--- All references and comparisons for the threshold ---'
rg -n -C 12 \
'm_minimumVolumePercentErrorAllowed|m_volumeError|minimumVolumePercentErrorAllowed' \
Native~/third_party/VHACD.hRepository: SashaRX/UnityMeshLab
Length of output: 6125
Проверяйте minVolumePerHull на границе ABI.
Native~/third_party/VHACD.h не задаёт явный диапазон. API использует значение как процентный порог в сравнении m_volumeError < m_minimumVolumePercentErrorAllowed. До присваивания params.m_minimumVolumePercentErrorAllowed отклоняйте значения, для которых !std::isfinite(minVolumePerHull) или minVolumePerHull < 0.0f. Отрицательное значение отключает критерий ошибки, +Infinity завершает каждый конечный hull сразу, а NaN также отключает этот критерий.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Native`~/src/collision.cpp around lines 66 - 76, Extend the ABI validation
near the existing maxHulls/resolution checks to reject minVolumePerHull when it
is not finite or is negative, using std::isfinite before assigning
params.m_minimumVolumePercentErrorAllowed. Preserve valid finite non-negative
values and the existing nullptr rejection behavior.
Comment on lines
+30
to
+35
| set "ARG1=%~1" | ||
| set "ARG3=%~3" | ||
| set "ARG5=%~5" | ||
| for %%V in (ARG1 ARG3 ARG5) do ( | ||
| if defined %%V ( | ||
| echo(!%%V!| findstr /r /c:"^[A-Za-z0-9_.~:/\\ -][A-Za-z0-9_.~:/\\ -]*$" >nul || goto badarg |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Проверка аргументов выполняется после небезопасной интерполяции.
%~1, %~3 и %~5 сначала разворачиваются в set при включённом EnableDelayedExpansion. Внутренняя " может закрыть кавычки команды до выполнения findstr. Кроме того, %~2 и %~4 не проверяются и напрямую разворачиваются в условиях if.
Не используйте %~N в set, if или вызове python до безопасного разбора всех аргументов. Эта реализация не устраняет command injection из предыдущего комментария.
#!/bin/bash
set -euo pipefail
nl -ba 'Tools~/gen.bat' | sed -n '11,80p'
rg -n -C 2 '%~[1-9]|EnableDelayedExpansion|ARG[135]|python "%SCRIPT%"' 'Tools~/gen.bat'🧰 Tools
🪛 Blinter (1.0.113)
[error] 35-35: UNC path without UAC elevation check. Explanation: UNC path operations may fail under UAC without proper elevation checks. Recommendation: Check for administrator privileges before UNC operations using NET SESSION. Context: UNC path operation may fail under UAC without elevation check
(SEC020)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Tools`~/gen.bat around lines 30 - 35, Переработайте разбор аргументов в блоке
вокруг ARG1, ARG3 и ARG5 так, чтобы все позиционные параметры безопасно
проверялись до любой интерполяции в set, if или вызове python. Уберите прямое
использование %~N при включённом EnableDelayedExpansion, добавьте проверку также
для %~2 и %~4 и передавайте только безопасно разобранные значения, исключив
возможность закрытия кавычек и command injection.
SashaRX
pushed a commit
that referenced
this pull request
Aug 9, 2026
…erarchy mode, intent-gated FBX export, headless LOD ops (PR #103) Vertex AO tab becomes Vertex Color Baking (hierarchy-mode bake across a MeshRenderer subtree with no LODGroup, per-FBX include map, partial rebake). FBX export gains FbxExportIntent bit-gating plus an atomic write core (.tmp -> size verify -> File.Replace) with .meta backup/restore. LOD generation extracted headless into LodPipelineOps / LodGroupUtility. Adds MeshLabArtifactValidator, BuildValidator, UvPackHierarchyTool, VariantExportPipeline. Conflicts resolved by composing PR #191's safety guards around PR #103's capabilities: - VertexColorBakingTool.cs: kept #191 TryValidateBakeWorkload alongside #103 FindSelectedHierarchyEntry/ExecuteRebakeSelected (needed an added closing brace, not a bare marker delete); kept #191's previewedMeshFilters dedup set with #103's hierarchy-aware ActiveEntries(). - LodGenerationTool.cs: took #103's LodPipelineOps.Generate delegation and relocated #191's NormalizeSingleLodTransitionForGeneration call into LodPipelineOps.Generate, whose extraction had silently dropped it. - LightmapTransferTool.cs: kept #103's intent gating, RunPreflight and atomic write, restored #191's NormalizeExportHierarchy(tempRoot, bakedMeshes) leak sink and the overflow-safe unchecked((uint)...) path hash in place of Math.Abs, and kept both tempMeshes.Add and exportMesh.name at the two wide-path sites. Also closed a fifth temp-mesh leak of the same family: CopyIsolatedSnapshotsToClone now takes a sink and its clones are destroyed with the rest of the export scratch. Plugins/** binaries deliberately kept at main's revision — PR #103 carried rebuilds made from the pre-vendoring xatlas that PR #191 replaced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
SashaRX
pushed a commit
that referenced
this pull request
Aug 9, 2026
…asurement harness and hierarchical cascade research (PR #118) Brings the objective-measurement layer the planned transfer rework needs: BenchmarkSweep gains four visual-defect scoring axes, a provenance manifest and zip archiving; BenchmarkRecorder writes into a dated per-session subfolder (never the BenchmarkReports/ root) with a short uv2_png path that survives Windows MAX_PATH; GroupedShellTransfer reports uv2DuplicatePairs / compositeBrokenCount / severeMismatchCount; ExecBenchmark becomes a multi-case driver. Adds HierarchicalDiag / HierarchicalRepack / HierarchicalApply plus TRANSFER_TEST_PLAN.md and HIERARCHICAL_CASCADE_PLAN.md. Conflicts resolved: - LightmapTransferTool.cs: kept #191's TryValidateSweep extraction and its `out int total` (dropping #118's duplicate `int total`, which would have been CS0128) while keeping #118's caseCount line, without which the Run Benchmark button does not compile. Kept #103's FbxExportIntent.None guard over #118's bare #if. - EXPERIMENTS.md: union, #118's June/Stage-E entries then #191's August entry. - README.md: kept #191's line — xatlas is vendored now, #118's text described the pre-vendoring state. - .claude/skills/skills-overhaul-plan.md: kept #191's hardened version rather than honouring #118's deletion; the CHANGELOG Security section cites that fix and would otherwise dangle. Adapted at merge time rather than taken as-is: - TryValidateSweep extended to count the two axes #118 adds (internal oversample, symmetry-split mode). ExecSweep iterates 7 dimensions; the validator counted 5, so the 256-cell cap was bypassed on the new axes and the cell count undercounted. - The FBX define rename UNITY_MESH_LAB_FBX_EXPORTER was NOT propagated. #118 renames the asmdef and the 5 #if sites that exist on main, but #103 added 6 more — taking it would have left the asmdef defining one spelling while 7 guard sites in LightmapTransferTool.cs used the other, silently compiling the FBX export path out with no error. Reverted to LIGHTMAP_UV_TOOL_FBX_EXPORTER everywhere, matching the AGENTS.md rule. - CleanupTool: #118 replaced the legacy Hidden_LightmapUvTool / Hidden/LightmapUvTool material prefixes instead of adding to them, which silently stops Fix Materials cleaning every FBX exported before the rename. Widened to match both generations. - CHANGELOG: restored the released [1.0.0] / [0.15.36] / [0.15.9] entries that #118 rewrote; shipped entries are a contract and the old package id and namespace are what downstream consumers grep for. Kept #118's [Unreleased] bullet. - HierarchicalDiag / HierarchicalApply menu items gated behind MeshLabProjectSettings.showDebugUI, matching #191's debug-UI gating, and HierarchicalDiag's standalone report moved out of the BenchmarkReports/ root into its own dated subfolder per #118's own stated rule. - Removed a doubled /// <summary> above SymmetrySplitShells.UvCoverageRatio. Plugins/** binaries kept at main's revision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
SashaRX
pushed a commit
that referenced
this pull request
Aug 9, 2026
…r host and shared mesh-group palette (PR #111, subset) DELIBERATE PARTIAL MERGE. This branch is "PR-1" of a four-PR prefab-builder plan whose PR-2/3/4 were never written, and the owner intends to rebuild the prefab builder from scratch. Its 3216-line PrefabBuilderTool.cs rewrite was therefore NOT taken; ours (PR #103's) is kept. Taken instead are the pieces that survive a rewrite: - Editor/Framework/MeshGroupColors.cs (+ .meta, guid a6ce99c0…) — deterministic per-mesh-group palette, self-contained, and now wired into the hub's mesh-group buttons. - IUvToolRightSidebar in Editor/Framework/IUvTool.cs — opt-in marker, inert until a tool implements it. - The UvToolHub right-sidebar host: width pre-compute, resize handle, and the left-sidebar clamp widened to 900. - LodGenerationTool's decomposition of OnDrawSidebar into DrawDetectAndCreate / DrawWorkflowHint / DrawExistingLodTable / DrawSettingsPanel / DrawResultsAndClear, which auto-merged cleanly. Not taken, and why: - PrefabBuilderTool.cs — #111 deletes #103's Build Pipeline section, which would have removed the only caller of BuildValidator (9 call sites -> 0) and the Prefab Builder's FBX save (ExportFbxPublic), with the PR body deferring its return to a PR-3 that does not exist. - Editor/LodPipelineOps.cs — #111 reverts main's non-modal progress work back to EditorUtility.DisplayProgressBar and drops the cooperative cancel. Verified post-merge that ours (UvProgress + CancelRequested) is what landed. - Plugins/x86_64/xatlas-unity.dll — rebuilt on #111 with no Native~ source change, and built from the pre-vendoring xatlas. Conflicts: CHANGELOG.md and Documentation~/EXPERIMENTS.md both had an empty "theirs" side (their content arrived earlier via #103), so ours was kept with nothing lost. LightmapTransferTool.cs and PrefabBuilderTool.cs resolved to ours. Verified after merge: #103's LodPipelineOps.Generate delegation, #191's MaxSupportedLodIndex / TryGetSupportedLodIndex guards, and the relocated NormalizeSingleLodTransitionForGeneration call all survive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
6 tasks
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
CsvUtilutility used by BenchmarkRecorder, BenchmarkSweep, and FbxMetricsExporterChanged Zones
Editor/— Editor tools / UIPlugins//Native/— Native plugins.github/— CI / workflowsREADME.md,CHANGELOG.md)Checklist
.metafiles present for all new files/directoriesTest Plan
Review Notes
Native~/third_party/xatlas/(MIT licensed, properly attributed)s_nativeSessionInFlightcounter to prevent concurrent atlas access during async operationshttps://claude.ai/code/session_0195YC4HWo1rQqEdFBb8p8jE
Summary by CodeRabbit