feat: rewrite cleanmodels in Go#3
Open
plenarius wants to merge 31 commits into
Open
Conversation
Complete rewrite of cleanmodels from Prolog to Go as a library (pkg/mdl) and CLI (cmd/cleanmodels). Zero external dependencies. Capabilities: - ASCII MDL parser covering all node types and NWN:EE parameters - Binary MDL decompiler with full controller type mapping - Binary MDL compiler (ASCII to binary) validated against the NWN:EE game compiler across 32,707 stock and 49,111 community models - ASCII MDL writer with clean normalized output - 100+ structural, geometry, parameter, and tile checks - Repair operations: pivot points, walkmesh AABB rebuild, tilefade slicing, mesh fixes, scale transforms - CLI with subcommands (check, repair, compile, decompile) - TTY-aware colored output, batch progress, JSON/NDJSON output - Legacy flag compatibility for cleanmodels-qt GUI integration Spec references throughout: nwn.wiki, xoreos-docs (Torlack binmdl, NWN1MDL.bt), neverwinter.nim, nwnmdlcomp.
- Remove the old SWI-Prolog/CMake/Ninja build workflow - Add Go CI: vet + test (race) on Linux/macOS/Windows - Cross-compile releases for linux/darwin (amd64+arm64) and windows/amd64 - Inject version from git tag via ldflags (-X main.version) - Publish GitHub releases on v* tags using softprops/action-gh-release
- checkout v4 -> v6 - setup-go v5 -> v6 - upload-artifact v4 -> v6 - download-artifact v4 -> v7 - action-gh-release v2 -> v3
Full usage docs for all subcommands and flags, download table for all platforms, build instructions, spec references, and a link to the last Prolog release for anyone who needs it.
8 tasks
Add 27 new CLI flags to match the full feature set that cleanmodels-qt sends. All flags registered in both subcommand and legacy modes. Pivot sub-options: --pivot-allow-split, --pivot-below-z0, --pivot-move-bad, --pivot-smoothing, --pivot-min-faces, --pivot-split-first Extends RepairPivots with PivotOptions struct. below-z0 controls the Z>=0 half-space constraint, move-bad adds top/middle/bottom fallback placement when search fails. Tile operations: --water, --water-key, --dynamic-water, --wave-height, --rotate-water, --retile-water, --foliage, --foliage-key, --splotch, --splotch-key, --rotate-ground, --ground-key, --chamfer, --retile-ground, --raise-lower, --raise-amount, --tilefade-undo New repair_tile_ops.go with RetileUVs, SetRotateTexture, ReparentToModela, RaiseLowerTile. UndoTileFade added to repair_tilefade.go. Mesh extras: --tvert-snap, --placeable-transparency, --transparency-key, --remap-walkmesh-material SnapTVerts in repair_mesh.go, RemapAABBMaterial in repair_walkmesh.go, PlaceableTransparency in new repair_placeable.go.
Adds a `cleanmodels report` CLI subcommand that submits bug reports
with model files to a Cloudflare Worker relay, which creates GitHub
issues on behalf of users who may not have GitHub accounts.
Security hardening from parallel review:
- IsRegular() guard prevents hang/OOM on device files and pipes
- Empty issue_url guard prevents silent success with no reference
- Response body capped at 1MB via LimitReader
- Error output truncated to 512 chars
- Named size constants (maxFileSize, maxTotalSize)
- CI uses env vars instead of inline ${{ }} for script injection safety
ci.yml already contains a complete release pipeline (build matrix + publish job). The old release.yml used deprecated Node.js 20 actions and lacked cache:false, version ldflags, and client key injection.
Adds single-letter aliases for frequently used flags: -r (recursive), -v (verbose), -q (quiet), -j (json), -w (workers) -f (fix/force), -n (dry-run), -a (all) Fixes from review: - Use *cf.workers instead of duplicating runtime.NumCPU() for -w default - Align --dry-run description strings across check and repair
- Quick start section for newcomers - Batch processing examples (directory, recursive, output folder) - Output/logging section explaining stderr vs stdout redirection - Short flag aliases shown in all flag tables - Report subcommand documented - All examples use bash syntax highlighting
The "compiled to binary" message was appended to Result.Repairs, causing compile-only runs to show "1 repair" per file. Added a separate Actions field so compile/decompile output is tagged [ACTION] and not counted in the repair tally.
New validation checks: - resref_length: warn when bitmap/texture/material names exceed 16 chars - plt_bitmap_match: warn when PLT bitmap doesn't match CHARACTER model name - Node-type field validation matrix (danglymesh_fields, skin_fields, emitter_fields, light_fields, aabb_fields, misplaced_tile_fields, misplaced_dangly_data) New repair flags: - --standardize-texture0: emit texture0 instead of bitmap in output - --strip-ee-extras: remove wirecolor, specular, shininess from output Behavioral changes: - emitter_percent_range downgraded from fixable warning to info-only - extreme_shininess check removed entirely Includes compiler_tangents handoff notes for future implementation.
Generate per-GPU-vertex tangent and bitangent vectors from positions, normals, and UV0 during binary MDL compilation. Previously the compiler wrote -1 for both p_mdx_tangent and p_mdx_bitangent, forcing NWN:EE to derive them at load time; pre-baking matches the game's own model compiler and removes that startup cost. - compiler_tangents.go: Mikktspace-style tangent accumulation with Gram-Schmidt orthonormalization, degenerate-UV skipping, NaN/Inf guards, and a fallback orthogonal axis for collapsed tangents. Authored Vec4 tangents are preserved through compile/decompile via resolveTangents/expandExistingTangents. - compiler_mesh.go: reserve real MDX pointers when tangent data is present, patch them after writing the Vec3 streams. Extracted a writeVec3Stream helper (mirroring the existing writeUVs closure) so normals/tangents/bitangents share a single guard-and-write path. - compiler_tangents_test.go: unit tests for the simple-quad, no-UV, and degenerate-UV cases plus roundtrip tests covering generated tangents, missing tangents, authored-tangent preservation, and MDX block size growth. The bitangent stream lets the decompiler recover per-vertex W handedness via sign(dot(cross(normal, tangent), bitangent)) without ambiguity. See docs/compiler_tangents_handoff.md for the full design.
Two game-parity adjustments on top of a6fe3ca: - Include face material index and smoothing group in the GPU-vertex dedup key. The game compiler splits at material/SG boundaries even when position/UV/normal/color match; without this, multi-material meshes over-merge by 30-50% and Mikktspace tangents drift at the boundaries (the dominant cause of the ~1-3% bad-alignment corners seen in the oracle suite). - Only generate tangents when RenderHint == NormalAndSpecMapped, with authored tangents still preserved verbatim. Mirrors the game compiler so non-normal-mapped meshes don't waste MDX bytes.
Restores the two repair modes that the Go rewrite had stubbed out (--chamfer add/delete and --dynamic-water no/wavy) and lands the shared geometry primitives they depend on. Repair coverage is now level with the legacy Prolog tool. Repairs - repair_chamfer.go / _add.go: implements chamfer add and delete. Chamfered faces are tagged with smoothing-group 1048576 and material 21 (matching the Prolog marker convention). Add walks half-edges to find tile-boundary edges and inserts the offset geometry; delete removes flagged faces and post-compacts every affected vertex/UV channel (including TexIndices0..3) so the result round-trips cleanly. - repair_water.go: implements dynamic-water "no" (animmesh -> trimesh reclassification) and "wavy" (trimesh -> animmesh, tessellate, weld, attach a 5-keyframe default animation with seeded Z and UV perturbation). water_detect.go houses the bitmap-key matching. Supporting primitives - transforms.go: canonical LocalToWorld / WorldToLocal / LocalNormalToWorld / WorldNormalToLocal. Walks parentChain with cycle detection, applies Node.Scale, and exposes WorldVerticesCached + nodeIndex/nodeIndexFromSlice for callers that hold vertex slices. - mesh_attrs.go: vertexAttrPresence + helpers (compactVertexAttrs, appendDefaultVertexAttrs, filterByMask, remapForKept, remapFaceVerts/UVs, remapTexIndices) so every repair pass that mutates vertex count keeps companion arrays (Normals, Colors, Tangents, Skin.Weights, Dangly.Constraints) in lockstep. - halfedge.go: directed half-edge map for adjacency queries used by the chamfer add pass. - tessellate.go: longest-edge bisection with linear interpolation of every present vertex attribute. Used by wavy water and available for future repairs. - weld.go: vertex welding now keyed by (position, UV0, normal, color, materialIndex, smoothingGroup) and routed through mesh_attrs.go. Refactors - repair_tilefade.go: delegates all transform math to transforms.go and hoists nodeIndex out of the per-face hot path; the index is rebuilt only when reparenting invalidates it. - cmd/cleanmodels/process.go: wires the new repair functions into the --chamfer and --dynamic-water flags.
Adds 14 ASCII + 12 game-binary fixture pairs spanning trimesh, skin, animation, emitter, dangly, and AABB models, plus 17 normal-mapped ASCII/binary pairs under tangent_pairs/ to exercise Mikktspace tangent parity once a normal-mapped fixture is wired into the runner. oracle_test.go now uses nodeIndexFromSlice from transforms.go instead of its local nodeMap helper, so the test suite shares a single source of truth for case-insensitive node lookup with the rest of the package.
Three top-level markdowns describing what each pipeline does, what it has been tested against, and the known gaps. Tone is humble and technical, written for both end users and future contributors: - CLEAN.md: enumerates registered checks (48), repair flags including chamfer and dynamic-water, clean-fixture coverage, known divergences from the legacy Prolog tool. - DECOMPILE.md: binary -> ASCII pipeline, parser coverage, output formatting decisions. - COMPILE.md: ASCII -> binary pipeline including the Mikktspace tangent stage, vertex dedup keys, oracle parity status. Removes the now-superseded docs/compiler_tangents_handoff.md, which was scaffolding for the tangent-compiler handoff and is captured by COMPILE.md.
Migration: - Replace .impeccable.md with split PRODUCT.md (CLI focus) and DESIGN.md (terminal output design system) per the new /impeccable skill format. Audit fixes (P1 harden): - Add --color=auto|always|never on every subcommand and legacy mode; precedence is explicit > NO_COLOR/FORCE_COLOR > TTY. - Empty-batch / no-MDLs-found returns exit 2 (exitUsage) instead of silently exiting 0, so CI catches misrouted invocations. Audit fixes (P2 polish): - Skip the cursor-control live painter when total < 4 files; small batches stream per-line instead, no flicker. - formatBatchLine becomes TTY-width aware via TIOCGWINSZ on darwin/linux/freebsd; the dot leader compresses on narrow terminals instead of wrapping. Coverage in output_test.go. Audit fixes (P3 clarify): - Route batch and single-file summaries to stderr in --json modes and stdout in plain mode, with errors and the empty- batch hint always on stderr regardless of mode. - Document the rule in DESIGN.md's new Stream Routing section.
Stale Windows version-info resource pinned at v3.7.0.0, last touched in 2020. Its only consumer was the C++-era release.yml workflow, which was removed in e0ed90f. The current Go release pipeline builds Windows binaries via plain go build with no syso/rc embedding, so this file has zero references anywhere in the tree.
DecompileFile slurps the whole binary MDL into memory and parses from a bytes.Reader — the binary parser does many small random-access reads via binary.Read, so reading once avoids ~one syscall per field. atomicWriteFile now wraps the temp file in a 64 KiB bufio.Writer so the many small fmt.Fprintf calls in the ASCII writer collapse into a handful of bulk write syscalls. On a 330-file binary tile corpus (single worker, 2030 repairs): before: 14.87s elapsed, 5.15s sys, 9.5s user after: 5.00s elapsed, 0.11s sys, 4.7s user
…aths
The ASCII writer's hot loops (verts, normals, faces, tex indices, colors,
tangents, position/orientation/float/color keys, AABB entries) were
spending ~44% of CPU in fmt.Sprintf("%g", ...) — reflection plus a fresh
string allocation per call, often four per vec3 line.
Adds a reusable scratch []byte on the writer plus a small set of per-line
builders (writeVec3Line, writeVec4Line, writeFaceLine, writeInt3Line,
writeFloatVec3Line, writeFloatVec4Line, writeFloatFloatLine,
writeAabbEntryLine) that build a complete line into scratch via
strconv.AppendFloat / AppendInt and emit it with a single Write. The
public fmtFloat / fmtVec3 helpers stay for cold paths but route through
the same appendFloat to keep output bit-identical.
bitSize=32 in AppendFloat mirrors fmt's %g behaviour for float32, so all
finite values format identically (verified end-to-end on the tile corpus).
330-file binary tile corpus, single worker, on top of the I/O buffering
commit:
before: 5.00s elapsed, 4.7s user
after: ~4.3s elapsed, ~4.0s user (writer drops from ~44% to ~5% of CPU)
Adds a separate profile.go that enables CPU and memory profiling when CLEANMODELS_CPUPROFILE and/or CLEANMODELS_MEMPROFILE are set to an output path. The hook stays inert unless an env var is set, and main defers runProfileCleanups so partial profiles flush on normal exit. Usage: CLEANMODELS_CPUPROFILE=cm.cpu.prof ./cleanmodels repair --all in/ out/ go tool pprof -top -cum cm.cpu.prof
Adds a `cmd/cleanmodels-wasm` Go package that exposes the MDL pipeline
to JavaScript via syscall/js. The wasm module installs a single
`cleanmodels` global with four entrypoints: `version`, `decompile`,
`compile`, and `parse`. All return `{ ok: true, ... }` on success or
`{ ok: false, error: string }` on failure; Go panics are recovered at
the JS boundary so bad input never tears down the wasm runtime.
Also adds `examples/wasm/`: a self-contained drag-and-drop demo page
showing binary→ASCII decompile and ASCII parse diagnostics, plus a
README documenting the JS surface and embedding pattern. Built
artifacts (`cleanmodels.wasm`, `wasm_exec.js`) and any sample `.mdl`
fixtures are gitignored.
CI gains a new `wasm` job that builds `cleanmodels.wasm` on every
push and PR (catches build regressions, since `//go:build js && wasm`
hides the package from the linux/amd64 vet+test matrix). On `v*`
tags the job packages `cleanmodels.wasm` + `wasm_exec.js` flat into
`cleanmodels-wasm.zip` and uploads it as a release artifact, picked
up by the existing publish job.
…skin truncation The MDX vertex section stores per-corner GPU verts (one per unique pos+UV+normal+colour), so the binary's count_vertexes is the post- expansion count. Two correctness gaps fall out of this that 3.7 + nwnmdlcomp + the engine's own compiler all share: 1. Danglymesh constraint count was written as len(dangly.Constraints) even when the mesh had been expanded to a larger GPU vert count. Trailing GPU verts then had undefined behaviour in the engine's dangle simulation. Visibly, this presented as tail-tearing on c_squirrel and most stock danglymesh creatures: a constraint=0 (locked) vert and a constraint=250 (free) vert sharing the same ASCII position would dangle to different places. Fix: expand constraints along the same origVert mapping, so verts and constraints stay 1:1 in the binary. 2. Skin verts with >4 bone influences (c_fox, c_dogzombie, …) were truncated by keeping the first 4 listed in the ASCII source. Sums below 1.0 left those verts under-bound. The binary format hard-caps at 4 (load_binary.pl:577 reads 4 floats per vert; borealis_nwn_mdl Mesh.hpp:124-132 documents "Each vertex can be influenced by up to 4 bones"; nwn.wiki Models page lists the same). Fix: sort by weight descending, keep the heaviest 4, renormalize the kept set to sum to 1.0. skinmesh-test corpus (36 stock animal creatures): round-trip via compile → decompile → check now reports 0 warnings / 0 errors across the board (was 97 warnings concentrated on the danglymesh and 5+ bone verts).
…lines Three related bugs in batch output, all symptomatic in `cleanmodels check --verbose <dir>`: 1. formatBatchLine had no warning branch: a file with checks reporting SevWarning but no errors / repairs / actions fell through to "clean" while the batch summary correctly tallied the warning. The per-line and summary disagreed. 2. liveProgress capped the visible viewport at ~20 lines and showed "... and N more" regardless of --verbose. Verbose has to mean "show me everything" — disable the in-place renderer in verbose mode and stream output instead. 3. --verbose in batch mode never printed the per-file detail block (repair / warning / parse-error / check messages). Only single-file mode did. Now both modes print detail through the shared printResultDetail helper. Adds three regression tests covering the warnings-only and parse-errors-as-warnings paths.
cleanmodels check / repair on a single binary file used to flood the terminal with the entire decompiled model (~22k lines for a typical creature) before printing the check summary, because the binary-mode output branch fell through to mdl.Write(os.Stdout) for any mode that didn't supply an output path. Now only `cleanmodels decompile` streams ASCII to stdout when no output path is given. `check`, `repair`, etc. on a binary input without an output path are silent on the model itself — only the result summary is printed, which is what the user actually asked for. Explicit output paths are still honored across all modes.
…nloads Maintaining a per-asset table on the vault means link rot every time either repo cuts a new platform or asset; the release pages already have the canonical list. Trade the two tables for two `releases/latest` links and a one-liner about Qt-side bundling. Also bumps the Qt packaging line from "Linux/macOS arm64/Windows" (the build0.8.0 surface) to the new five-platform parity matrix (adds linux-arm64 and macos-amd64), and clarifies the migration note now that the v4 CLI ships inside the Qt download.
The Go port of fix_pivots ran the pivot search on every AABB node unconditionally, then overwrote node.Position with the constraint-satisfying fallback when the candidate it produced didn't satisfy its own check. On hand-authored 2-story tile walkmeshes (e.g. ttr01_a01_01 cm154 with `position 0 0 2.5`) the fallback shifted the node origin to e.g. (0.02, 0.69, 2.5), visibly translating the walkmesh in viewers after a `repair -a` round-trip. Fixes #6. Prolog gates `attempt_to_fix_pivots/5` behind a `bad_pivots` check that fires only when a face normal has negative depth or the pivot is below z=0. Match that here: * Skip nodes whose authored Position already satisfies the half-space constraints — leave them alone. * When no candidate satisfies and `move_bad_pivots` is unset, emit a warning but do not modify Position. Matches the Prolog final clause which only marks wirecolor red. A regression test (`TestRepairPivots_AuthoredPivotPreserved`) locks the gate in by feeding an interior walkmesh whose authored pivot is geometrically valid and asserting both the position and the message list stay clean. A deeper deviation from Prolog is not addressed here: when repair does legitimately rebuild a pivot, Prolog's `f_repivot` subtracts the new pivot from every vertex and child position so the world-space geometry stays put. The Go port writes the new position but doesn't rebase verts/children, so a real bad-pivot repair would still translate the model. The reported issue doesn't reach that branch (the gate handles cm154), so the rebase work is left for a follow-up.
…uilt Issue #7: `cleanmodels repair -a ttr01_o07_01.mdl` shifted the cm088 walkmesh visibly. The authored pivot at (0, 0, -5) failed the z>=0 constraint, the bisection search legitimately found a replacement at (-4.84, -4.84, 4.99) in mesh-local space, and the old code overwrote node.Position with that point. The geometry never followed: verts and direct child nodes were still expressed relative to the old origin, so the model translated by (-4.84, -4.84, 9.99) in world space. The v4.0.0-rc5 gate from #6 only covers the "authored pivot is already valid" case — once the search actually fires, we still have to honour Prolog's f_repivot/5: subtract the new local origin from every vertex, subtract it from every direct child's position (grandchildren are unaffected — their parent's local frame did not itself rotate or translate), and rotate the shift back into parent-frame before adding it to the node's own position. Net effect: the local origin moves, the world-space geometry doesn't. New helper applyPivotShift centralises this rebase so both the normal "found a satisfying candidate" path and the move-bad fallback use the same logic, with a 1e-5 short-circuit matching Prolog's threshold for sub-microscale pivots (apply_pivot_shift/5 line 4810). Reproduction (ttr01_o07_01, cm088): before: position 0 0 -5 vert[0]: ( 2.82, -2.38, 5 ) after : position -4.84 -4.84 -0.004 vert[0]: ( 7.66, 2.46, 0 ) world : (2.82, -2.38, 0) in both — identical. Added regression test TestRepairPivots_GeometryStaysInWorldSpace that seeds a node with a below-floor authored position and a direct child, then verifies world-space vert and child positions are byte-identical before and after RepairPivots within 1e-4. Ref: fix_pivots.pl f_repivot/5 (line 348); make_checks.pl apply_pivot_shift/5 (line 4809). Fixes #7
Two bugs in `decompile`'s single-file stdout handling, both surfaced by a report that `decompile ascii.mdl | ...` produced no model: 1. ASCII input emitted nothing on stdout. The write switch only streamed to stdout inside the binMode branch; ASCII input fell to the default branch, which wrote only when given an explicit output path. So piping an already-ASCII model through `decompile` yielded just the summary line. 2. The binary path "worked" but corrupted its output: the "1 file, 0 warnings, 0 errors" summary was written to stdout, so it got appended into the decompiled model after `donemodel`. Any strict MDL consumer downstream would choke on the trailing text. Restructured the write switch to be mode-based rather than input-format-based: explicit output path -> WriteFile; else `decompile` with no path -> stream ASCII to stdout for both binary AND ascii input; else (check/repair with no output target) -> write nothing (preserves the "don't flood the terminal before check results" behaviour). In runSingle, stdoutIsData = decompileOnly && !jsonOut && no output path. When set, the per-file detail and summary route to stderr — the same rule --json already follows — so the model on stdout stays byte-clean. This closes the gap opened by 2e0c4a0, which stopped check/repair from dumping ASCII but left the ascii-decompile path with no stdout writer. Added TestDecompileASCIIPassThrough: swaps os.Stdout/os.Stderr for pipes, drives dispatch end-to-end on an ASCII fixture, and asserts the model lands on stdout, the summary does not, and the summary lands on stderr.
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
Complete rewrite of cleanmodels from Prolog to Go — library (
pkg/mdl,pkg/checks) and CLI (cmd/cleanmodels). Zero external dependencies, stdlib only.check,repair,compile,decompile,reportNO_COLOR), batch progress, JSON/NDJSON for machine consumptioncleanmodels report): submits model files + diagnostics to a Cloudflare Worker that creates GitHub issues — no GitHub account requiredMotivation
Report subcommand
New
cleanmodels reportsubcommand for users to submit bug reports without a GitHub account:IsRegular()guard, 10MB/file and 25MB total limitsTesting
Breaking Changes
main)cleanmodels check,cleanmodels repair, etc.)