Update for cleanmodels Go v4 rewrite#1
Open
plenarius wants to merge 28 commits into
Open
Conversation
Upgrade to Qt6 and add an interactive 3D preview panel for MDL files. Includes orbit camera, ASCII MDL parser, Blinn-Phong shading with smooth normals, TGA texture loading, and Go CLI integration for decompiling binary MDLs before preview.
- Add Compile radio button to mode selector (Clean / Decompile / Compile) - Compile mode passes --compile flag, disables repair/check options - Add "Undo tilefade splits" checkbox with --tilefade-undo flag - Update status labels: "Compiled: N" / "Compiling" for compile mode - Add CliFlag::Compile and CliFlag::TilefadeUndo to constants.h - Add Setting::TilefadeUndo for persistence - Update buildCliArgs() with compile and tilefade-undo branches - Update onCleanFinished() button text for compile mode
Replace legacy flag-based CLI invocation with proper subcommands: - Clean mode: repair --json-lines [flags] <dir> - Decompile mode: decompile --json-lines <dir> - Compile mode: compile --json-lines <dir> - Viewport preview: decompile <file> Add CliCommand namespace (repair, decompile, compile, check) to constants.h as the single source of truth for subcommand names. The NDJSON contract is unchanged — both paths use the same processOne/runBatch/emitEvent infrastructure in the Go CLI.
Parse severity as string ("info","warning","error","fatal") with
numeric fallback for backwards compatibility. Required by cleanmodels
v4 which now serializes Severity via MarshalJSON as strings instead
of raw integers.
The Go rewrite replaced the Prolog CLI; prolog/last_dirs.pl and prolog_files.qrc are no longer used by the CMake build.
…de quality High severity: - Fix integer overflow in TGA parser that could cause heap buffer overflow on crafted textures (use int64_t arithmetic, add dimension cap) - Fix detail panel auto-refresh using text() instead of data(Qt::UserRole), causing the comparison to always fail due to emoji prefixes - Add wait cursor and process cleanup for synchronous CLI preview to prevent UI appearing frozen - Fix destructor to properly kill running subprocess on window close Medium severity: - Fix computeBounds to use full transform hierarchy (rotation/scale/parent) instead of just position offset - Escape HTML in debug log for file paths and subprocess output - Cap MDL parser reserve() calls to prevent DoS via crafted vertex counts - Extract updateModeUI() member method from trapped lambda, fixing wrong icon restoration after decompile - Extract makeStandardForm() factory for 5 duplicated QFormLayout configs Low severity: - Remove duplicated QSurfaceFormat setup (inherit app default) - Delete dead CMakeModules/Qt.cmake (Qt5-era, never included) - Remove unused CLI constants (DecompileOnly, CliCommand::Check, CliFlag::Compile) - Remove unused Renderer accessors (setShowReference, program, renderNodeCount) - Remove unused modelLoaded signal - Remove unused totalFixes/totalWarnings computation - Unify reference rendering with renderNodes() via MaterialOverride - Unify binary MDL detection (fileIsBinaryMdl shared between viewport and table)
- Fix per-axis scaling: use --scale-x/y/z flags instead of averaging into a single --scale value (Go CLI now supports per-axis) - Rewrite build CI: Qt6 via install-qt-action, CMake on all platforms, drop ubuntu-16.04/MXE/qmake/Qt5-from-source, Node.js 24 actions - Update CodeQL: v3 actions, Qt6, explicit cmake build step - Rewrite README: Qt6 build instructions for all platforms, link to Go CLI releases, note about v4+ requirement
Use macdeployqt, windeployqt, and manual ldd-based gathering on Linux to ship all required Qt shared libraries alongside the binary. Users no longer need Qt installed to run the release builds. Also set MACOSX_BUNDLE and WIN32_EXECUTABLE in CMakeLists.txt so macdeployqt has an .app bundle to work with and Windows builds get a proper GUI executable.
The xcb platform plugin requires libxcb-cursor0 at runtime, which isn't part of the Qt installation. Bundle it from the CI runner so users don't need to install it separately.
Replace the manual library gathering + launcher script approach with linuxdeploy + Qt plugin for a proper AppImage. Single file, chmod +x, runs anywhere — no loose .so files or wrapper scripts. Also pin Linux runner to ubuntu-22.04 for FUSE/AppImage compatibility and add cmake install target for linuxdeploy's AppDir workflow.
Adds Help > Report Issue menu action and right-click context menu support for submitting bug reports via the cleanmodels CLI relay. Captures error state from failed runs for one-click reporting. - collectTableFilePaths() helper replaces 4 duplicated loops - ExtendedSelection enables multi-file reporting - ReportLimits constants in constants.h (10MB/file, 25MB total) - Normalize LogColor constants to bare hex values - Extract makeHLine() for repeated QFrame separators - Remove stale buildSidebar/buildWorkspace/connectSignals declarations
New checkboxes in Advanced > Mesh Operations: - "Standardize bitmap → texture0" (--standardize-texture0) - "Strip unused EE fields" (--strip-ee-extras) Both include settings persistence and CLI argument wiring.
Migration: - Add PRODUCT.md (Qt6 desktop frontend identity) and DESIGN.md (visual + interaction system) per the new /impeccable skill format. Audit fixes (P1 optimize): - Make every subprocess invocation signal-driven. previewFile and loadReferenceFile use a new decompileAsync helper with a QTimer watchdog and QPointer-safe callbacks. doClean no longer calls waitForStarted; onCleanStarted / onCleanProcessError run the state machine. Audit fixes (P2 harden): - accessibleName / accessibleDescription on 17 widgets including ModelViewport, custom collapsibles, file table, debug log. - Ctrl+Return / Ctrl+Enter (run), Esc (abort), F1 (help) added via QShortcut and actionHelp. Audit fixes (P2 normalize): - Lift HTML colour-fragment templates into loghtml.h helpers (span / logLine / detailLine / listItem / preBlock); 14 call sites converted, no inline "<span style=\"color:...\">" left. - New LogColor::Placeholder token; setStyleSheet calls now read colours from LogColor::* instead of hex literals. Audit fixes (P3 adapt): - New Layout::*Em / Layout::*Chars design tokens and metrics.h emH / emW helpers convert 13 hard-coded chrome dimensions (table columns, drawer chrome, status sliver, detail panel, clean button, sidebar toggle) to font-metric-based sizing. Audit fixes (P3 polish): - m_scaleLockBtn now uses setFlat(true) instead of an unsanctioned transparent QSS. - Layout::TightSpacing constant for catLayout / drawer margins. - tr() wrapping on the dynamic sidebar toggle label.
Adds a hand-rolled decoder for NWN's proprietary Bioware DDS format (distinct from Microsoft DirectDraw Surface and not handled by Qt's DDS plugin). 20-byte header + BC1 (channelCount=3) or BC3 (=4) blocks. Incorporates review feedback: - Always use 4-colour BC1 palette interpolation. The punchthrough-alpha branch (c0 <= c1) would emit transparent black on palette index 3, which renders as visible black artifacts on the RGB8 upload path (alpha is dropped). NWN's BC1 content is RGB-only by design. - Converge on bottom-up upload across all three load paths. loadTGA was the pre-existing outlier (top-down) vs the deliberate QImage .mirrored(false, true) and the new DDS flip; flip top-origin TGAs instead of bottom-origin TGAs to match. - Extract decodeBC1ColourPalette shared by BC1 and BC3 decoders. - Extract flipRowsRGBA shared by loadTGA and loadBioDDS. - Add readU16LE / readU32LE helpers; replace local lambda and ad-hoc shift-or chains in loadTGA, decodeBC1Block, decodeBC3Block. - Add readTextureFile helper that caps input file size at 64 MiB before f.readAll() to bound DoS via hostile or accidental giant files. - Lower max texture dimension from 8192 to 4096 (~64 MiB peak per texture, well above any realistic NWN content). - Share kMaxTextureDim / kMaxTexturePixels between TGA and DDS validators. - Log every loadBioDDS rejection path via qWarning to match loadTGA's diagnostics; downgrade pitch != expectedMain from a hard reject to a warning, since the file-size check is the actual safety guarantee. - Drop unreachable per-pixel x/y bounds checks inside the BC1/BC3 decoders (loadBioDDS rejects non-multiple-of-4 dimensions and the decoders have anonymous-namespace linkage).
Adds per-vertex skin animation playback to the 3D preview and fixes three rendering bugs that became visible while debugging the NWN "skinmesh animals" HAK (leopard, fox, lion, squirrel, etc.). Animation system - Parse full keyframe lists (positionkey/orientationkey/scalekey) from newanim blocks plus length, transtime and animroot - Add MdlAnimationPlayer modeled after varenx/borealis_nwn_mdl: lerp positions/scales, slerp orientations, and walk the scene graph each frame to produce per-bone world matrices - ModelViewport runs a 60 Hz animation timer, auto-plays a preferred idle (cpause1, cstand, etc.) on load, exposes an animation combo box in the toolbar, and reskins skin meshes via glBufferSubData each frame against the player's bone matrices - GpuMesh gains updateVertices() / GL_DYNAMIC_DRAW for in-place VBO refresh Rendering fixes - gputexture: load all textures top-origin (NWN/D3D MDL convention) rather than flipping to bottom-origin. The previous code flipped pixel data on load but never flipped the V coord in the vertex builder, so every sample was vertically mirrored. Invisible on symmetric body fur, very visible on textures with distinct top/ bottom regions (head atlases, inside-mouth gums, paw soles) - the "red blob on chest" / "pink feet" artifacts. - renderer: refresh world transforms for non-skin meshes from the animation player's bone-world matrices each frame. Without this, separately-parented parts like the squirrel's danglymesh tail stayed pinned in world space while the rest of the body ran out from under them. - renderer: discard fragments with alpha < 0.1 in the fragment shader. Many NWN textures bake binary masks into the alpha channel for fuzzy geometry (lion mane sprites, fox tail wisps, foliage); without discard those transparent quads still wrote to the depth buffer and occluded everything behind them, producing "gray blob" silhouettes.
Consolidates seven rounds of /parallel-review findings on top of the animation + skinmesh + DDS work. Squashed because the rounds are not individually meaningful — each one only exists because the prior one shook out new problems. Security - Cap MDL parser recursion depth (kMaxNodeDepth = 256) and total node count (kMaxNodes = 4096) to prevent stack overflow / OOM via crafted ASCII MDLs. - Replace recursive DFS in Renderer::buildRenderNodes and MdlScene::accumulateBoundsFromRoot with iterative DFS over an explicit QStack + visited set so runtime parent-child graph depth can no longer blow the C stack, and cyclic parent links no longer loop forever. - Tighten skipToTerminator to strict '==' matching so 'endnode_corrupt' can't masquerade as a terminator and desync the parser. - Add skipNodeBlock — a balanced node/endnode skipper — and use it on every cap-skip path so an attacker can't inject a fake early 'endnode' inside a capped subtree to bleed lines into the parent context. Performance - Cache MdlScene::childrenOf in m_childrenByParent at load time; callers now take const QVector<int>& instead of paying O(N) per lookup. Worst-case load-time cost is bounded by kMaxNodes. - buildUploadMesh uses std::iota for sequential index generation. Architecture / refactor - Introduce GlContextGuard (RAII for QOpenGLWidget makeCurrent / doneCurrent) and TexDirGuard (RAII for Renderer::m_textureDir). Both are non-copyable / non-movable; one scope owns the restore. - Extract prepareNodeList helper shared by prepareScene and prepareReferenceModel. - Split buildExpandedTopology into buildUploadMesh and rebuildExpandedVerts sharing an appendExpandedVerts helper, killing the nullable-out-pointer overload. - Switch m_boneWorld from QHash<QString, QMatrix4x4> to QHash<int, QMatrix4x4> (index-keyed); rewrite recomputeBoneMatrices as iterative chain-then-compose. - Add makeLocalTransform(const MdlNode&) overload. - Move file-scope helpers (computeSmoothNormals, appendExpandedVerts) into anonymous namespaces. - Replace magic numbers with kAnimTickIntervalMs / kAnimDefaultDtSeconds. - Extract stopAnimationTick / startAnimationTickIfVisible helpers. Production correctness - MdlAnimationPlayer::stop() unconditionally recomputes m_boneWorld whenever m_scene is valid, so a setScene() swap on an already stopped player can't render a new model with stale bone transforms. - Surface MDL load warnings (cap hits, depth hits) via MdlScene::loadWarnings() → ModelViewport::previewWarning → MainWindow debug log so partial loads aren't silent. - GpuMesh::updateVertices latches a one-shot size-mismatch warning via m_warnedSizeMismatch instead of spamming on every frame.
Three real bugs the rc3 hardening pass classified as "pre-existing,
out of scope" and shouldn't have. None of these were introduced by
the DDS / animation / skinmesh feature work; all of them survive an
attacker-crafted MDL today.
1. Negative face-index OOB in computeSmoothNormals
(renderer.cpp:370-379) used a bare `>= verts.size()` bounds check
and missed `< 0`. A face line with `-1 -1 -1 ...` passed the
check, then read `verts[-1]` (OOB read) and wrote
`out[-1] += fn` (OOB write into Qt's heap allocation metadata).
The expanded-vert path (appendExpandedVerts) already had the
matching `>= 0` guard — this site was the inconsistency. Three-
line defensive fix to match.
2. No scene-wide aggregate cap on verts / faces (mdlscene.cpp).
Per-array cap (kMaxArraySize = 10M) and per-scene node cap
(kMaxNodes = 4096) bound each axis individually but their product
(~4·10^10 verts ≈ 491 TB) is unbounded. A crafted MDL with 50
nodes each declaring 1M verts allocates 600 MB+ of vert storage
before anything else stops it. Added kMaxTotalVerts /
kMaxTotalFaces (16M each) running counters in MdlScene and a new
ArrayMode::Discard that consumes the declared `count` lines
without storing them so the parser cursor stays aligned with the
file (a flat skip would let raw float lines bleed back into the
keyword dispatch). Hits feed coalesced loadWarnings.
3. No size cap on loadFromString input (mdlscene.cpp:186-203).
QString uses 16-bit QChar internally, so a 2 GB attacker file
becomes a 4 GB QString plus a 4 GB+ QStringList from split('\n')
before any per-node cap fires. Added kMaxInputChars =
32 * 1024 * 1024 (32 MB ASCII source / 64 MB QString memory) —
~50× the largest legitimate BioWare MDL. Oversized input emits a
loadWarning and returns false.
Both ModelViewport consumers (loadModel, loadReferenceFile) now
relay loadWarnings BEFORE checking loadFromString's return value,
so the user sees the actual rejection reason instead of just
"Failed to parse MDL scene".
Why these were out of scope in rc3 and aren't now: the honest answer
is review fatigue after six rounds of hardening. "Pre-existing" is
not a synonym for "not exploitable" — the negative-index one is
memory corruption, not a crash.
Brings the Qt UI's release surface in line with cleanmodels CLI, which already publishes both. Previously the GUI shipped only linux-amd64, macos-arm64, and windows-amd64 — Intel macOS users and ARM Linux users had no GUI download. * `ubuntu-22.04-arm`: GitHub-hosted ARM Linux runner (free for public repos as of Jan 2025). linuxdeploy and linuxdeploy-plugin-qt both ship aarch64 AppImages on the continuous channel, so the existing AppImage step Just Works once the URL is arch-aware. Switched the hardcoded `linuxdeploy-x86_64.AppImage` lookups to derive from `uname -m` (x86_64 / aarch64). * `macos-13`: last Intel macOS runner GitHub provides. macos-latest tracks Apple Silicon (macos-14+), so we now pin both rows to keep Intel coverage explicit. Workflow goes from 3 jobs to 5 per push; release tag now produces 5 artifacts instead of 3.
Previously users needed two downloads to use the v4 toolchain — the Qt GUI and the cleanmodels CLI it shells out to. The vault page mirrored both, with all the link-rot that implies. Each platform's Qt build now pulls the matching cleanmodels asset from its GitHub release page and drops it next to the GUI binary inside the package, where the GUI's existing `PATH`/alongside lookup finds it: Linux: AppDir/usr/bin/cleanmodels (rolled into AppImage) macOS: cleanmodels-qt.app/Contents/MacOS/cleanmodels Windows: dist/cleanmodels-qt/cleanmodels.exe (zipped with the GUI) The download step prefers the stable `releases/latest/download/` URL and falls back to the most recent release (incl. pre-releases) via `gh` so PR/CI builds during the RC window still bundle something coherent. Asset naming is mapped in-step (matrix `macos-*` → cleanmodels `darwin-*`). The CLI release continues to publish independently for headless / CI / WASM use cases; this change just removes the second-download step from the GUI install path.
…ches Drops the macos-13 Intel runner row, which was queue-starved on free public-repo minutes (10+ minute waits while every other runner finished in 1-2) and is on Apple/GitHub's deprecation path anyway. macos-12 is already gone; macos-13 follows. The replacement is a single Apple-Silicon job that produces a universal .app bundle: * CMake gets `CMAKE_OSX_ARCHITECTURES='x86_64;arm64'` via a per-row `cmake_extra` matrix variable, so the Linux/Windows rows stay untouched. * The bundled cleanmodels CLI is rebuilt as a universal2 binary by downloading both `cleanmodels-darwin-amd64.zip` and `cleanmodels-darwin-arm64.zip` and `lipo -create`-ing them inside the runner. Apple's `lipo` is preinstalled on every macOS runner. * Qt 6.2+ already ships universal macOS desktop binaries via install-qt-action, so no Qt-side changes were needed. Net effect: one macOS artifact (`cleanmodels-qt-macos.zip`) that runs natively on both Intel and Apple Silicon, faster CI, and no dependence on Intel runner availability.
The per-platform case arms each `cd cli-bundle`, so the trailing `ls -la cli-bundle` after the esac was looking for a nested cli-bundle/cli-bundle/ and failing the step. Resolve from the workspace root explicitly. The lipo, copy, and deploy logic is unchanged - only the post-download verification was broken.
…e release
The upload-artifact and softprops/action-gh-release globs were both
matching `*.AppImage` and `*.zip`, which on the Linux runners pulls in
linuxdeploy-${ARCH}.AppImage and linuxdeploy-plugin-qt-${ARCH}.AppImage
that the deploy step downloads alongside the app. They'd attach to the
GitHub release as bogus 12-18 MB assets next to the actual
cleanmodels-qt-*.AppImage.
Pin both globs to `cleanmodels-qt-*` so only our own artifacts ship,
and add `if-no-files-found: error` so a future packaging regression
surfaces as a CI failure instead of silently uploading nothing.
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
Updates cleanmodels-qt to align with the cleanmodels Go v4 rewrite. The Go rewrite replaces the legacy Prolog-based CLI with a single static Go binary, and this PR ensures the Qt GUI works correctly with it.
Changes
Report Issue integration (
mainwindow.cpp,mainwindow.h,mainwindow.ui,mainwindow_clean.cpp)Users can now report bugs directly from the GUI — model files and error context are submitted to the cleanmodels issue tracker via the Go CLI's
reportsubcommand. No GitHub account needed.m_lastFailedFiles,m_lastErrorOutput,m_lastCommand) from failed CLI runs for one-click reportingQInputDialogwhen no error context is available (e.g. visual bugs)ExtendedSelectionmode on file tableReportLimits::MaxFileSize(10MB/file) andMaxTotalSize(25MB) constantscollectTableFilePaths(bool allRows)helper replaces 4 duplicated row-iteration loopsPer-axis scaling (
constants.h,mainwindow_clean.cpp)The Go CLI now supports
--scale-x,--scale-y,--scale-zfor independent axis scaling. Previously the GUI averaged all three spin box values into a single--scaleargument.Now:
--scalefor brevity--scale-x,--scale-y,--scale-zflagsCliFlag::ScaleX,CliFlag::ScaleY,CliFlag::ScaleZconstantsCode cleanup
constants.h): all color constants now use bare hex values (#3498dbinstead ofcolor:blue), call sites prependcolor:makeHLine()helper (mainwindow.cpp): extracted from 4 identical QFrame separator blocksmainwindow.h):buildSidebar(),buildWorkspace(),connectSignals()were declared but never definedBuild fix (
gputexture.cpp)Fixed call to non-existent
QImage::flipped()— replaced withQImage::mirrored(false, true)which is the correct Qt API for vertical flipping.Build CI overhaul (
.github/workflows/build.yml)The old workflow was completely non-functional:
ubuntu-16.04(removed from GitHub Actions in 2022)configureline (~30 min CI time)qmakefor Windows targets despite the project migrating to CMakev2(Node.js 12/16, long deprecated)Replaced with:
jurplel/install-qt-action@v4— installs Qt 6.7 from the official archive on all platformscmake -B build && cmake --build buildacross Linux, macOS, Windows.appbundle), Windows (windows-latest, MSVC +windeployqt)actions/checkout@v6,actions/upload-artifact@v6,actions/download-artifact@v7softprops/action-gh-release@v3with auto-generated release notesCodeQL update (
.github/workflows/codeql-analysis.yml)github/codeql-actionv1 → v3actions/checkoutv2 → v6apt-getpackage → Qt6 viainstall-qt-actioncmake -B build && cmake --build buildinstead of relying on autobuildREADME rewrite
Test plan
--scaleflag--scale-x/y/zflags