Add engine-debug-tools and fix critter animation rendering bugs
build / build (push) Successful in 2m52s
build / build (push) Successful in 2m52s
- engine-debug-tools/: a debug dump hotkey + dump2png to inspect decoded critter frames independent of the renderer (see root README.md's "Debugging" section). - engine-patches/04,05: fix critter sprite scale and an OpenGL texture cache collision, found using that tooling. - mdlp: fix encodeFrames RLE-diffing upscaled frames against a previous-frame reference the real RSD8 decoder never implements (verified against Night Dive's original Mac source down to the 68K assembly) - was rendering as scattered static in-game. - Upscale pipeline: better color quantization (dithering, pure-ramp exclusion) and per-resource model selection.
This commit is contained in:
@@ -358,17 +358,25 @@ which the desktop build does not apply.
|
||||
mod that replaces a critter's art with a higher-resolution bitmap for
|
||||
more detail made the critter itself render bigger, proportionally to
|
||||
the resolution increase. Fixed by caching every critter view's base-game
|
||||
bitmap size (`objsim.c`'s `cache_critter_native_size()`/
|
||||
`get_critter_native_size()`, keyed by resource ID) while
|
||||
`objart2.res`/`objart3.res`'s base-game data is still the active
|
||||
resolver in `obj_set_secondary_properties()` (before patch 03's
|
||||
`LoadModFiles()` call hands priority to a mod), then using that cached
|
||||
size instead of the bitmap's own (possibly modded) dimensions when
|
||||
computing the sprite's on-screen extent - threaded through
|
||||
bitmap size per animation frame (`objsim.c`'s
|
||||
`cache_critter_native_size()`/`get_critter_native_size()`, keyed by
|
||||
resource ID and frame index - an animation's bounding box commonly
|
||||
changes size between poses, so a single per-view size doesn't fit every
|
||||
frame of it) while `objart2.res`/`objart3.res`'s base-game data is still
|
||||
the active resolver in `obj_set_secondary_properties()` (before patch
|
||||
03's `LoadModFiles()` call hands priority to a mod), then using that
|
||||
cached size instead of the bitmap's own (possibly modded) dimensions
|
||||
when computing the sprite's on-screen extent - threaded through
|
||||
`_fr_draw_bitmap()` and `g3_light_anchor_bitmap()`'s new `native_w`/
|
||||
`native_h` parameters, consumed and reset by `g3_bitmap_common()` so an
|
||||
unrelated bitmap draw never inherits a stale override. A mod's higher
|
||||
resolution now only changes visible detail, not the object's size.
|
||||
resolution now only changes visible detail, not the object's size. Since
|
||||
this same size also sets `g3_bitmap_common()`'s texture coordinate
|
||||
range, not just its world-space extent, caching only the first frame's
|
||||
size and reusing it for every frame of an animation also visibly
|
||||
corrupted the UV mapping on any frame whose real size differed from the
|
||||
first - caught once a mod's per-frame sizes started genuinely varying
|
||||
(see `modding/README.md` section 5).
|
||||
This depends on the mod's bitmap keeping a real anchor point in its own
|
||||
`Header.Area`, in the original, un-upscaled unit space (see
|
||||
`modding/src/mdlp/upscale.go`'s `imageToBitmap()`) - `_fr_draw_bitmap()`
|
||||
@@ -379,6 +387,95 @@ which the desktop build does not apply.
|
||||
the sprite from where it should sit, since it shares its coordinate
|
||||
system with this patch's own size override rather than with the
|
||||
bitmap's real pixel dimensions.
|
||||
- **`05-desktop-fix-texture-cache-collision.patch`** - `OpenGL.cc`'s
|
||||
per-bitmap GL texture cache (`texturesByBitsPtr`) was keyed by
|
||||
`bm->bits | (w << 32) | (h << 48)`, a plain bitwise OR rather than a
|
||||
genuinely disjoint encoding - `bm->bits` is a full pointer, and a 64-bit
|
||||
Linux heap/mmap address routinely has non-zero bits in the 32-47 range
|
||||
the width field occupies, so a different `(bits, w, h)` triple can
|
||||
collide with an already-cached entry's key. `bm->bits` itself is a
|
||||
recycled slot in the resource system's own bounded LRU cache, not a
|
||||
stable per-resource address, making a collision a real risk rather than
|
||||
a theoretical one. On a collision, `set_texture()` (same file) treats it
|
||||
as a genuine cache hit and reuses that entry's SDL surfaces - allocated
|
||||
for whatever `(w, h)` was cached before - for an `SDL_memmove` and
|
||||
`glTexImage2D` sized to the *current* bitmap's byte count, corrupting
|
||||
the result with a buffer overflow or leftover stale pixel data from a
|
||||
previous, differently-sized bitmap. Fixed by keying the cache on a
|
||||
`std::tuple<void *, int, int>` instead, which `std::map` compares
|
||||
field-by-field with no collision risk - a hit now always means an exact
|
||||
`(bits, w, h)` match, so the cached surfaces are guaranteed to already
|
||||
be sized correctly for the current bitmap.
|
||||
|
||||
A second, independent bug in the same cache: on a hit, `set_texture()`
|
||||
"refreshed" the cached `SDL_Surface` with
|
||||
`SDL_memmove(t->bitmap->pixels, bm->bits, ...)` - but `t->bitmap` was
|
||||
created by `convert_texture()` via `SDL_CreateRGBSurfaceFrom(bm->bits,
|
||||
...)`, which wraps the given pointer rather than copying it, so
|
||||
`t->bitmap->pixels` *is* `bm->bits` from whenever this entry was first
|
||||
cached. Since a hit means the current call's `bm->bits` is that same
|
||||
pointer by definition (it's part of the key), that memmove was always a
|
||||
no-op self-copy - it never refreshed anything. `bm->bits` is a recycled
|
||||
resource-cache slot reused for different content across draws while its
|
||||
address stays the same, so a hit here was never a guarantee the pixels
|
||||
underneath were still current, only that this entry's GL texture and
|
||||
converted-surface buffers are the right size to reuse. Fixed by
|
||||
re-pointing `t->bitmap->pixels`/`pitch` at the current frame's real
|
||||
source (`bm->bits`, or the RSD8-decoded buffer) before blitting, instead
|
||||
of copying into what was already the same address. Most noticeable with
|
||||
content whose per-frame bitmap dimensions vary a lot, such as critter
|
||||
animations after an AI upscale mod re-exports each frame at its own
|
||||
independently upscaled size (see `modding/README.md` section 5) -
|
||||
vanilla assets mostly dodge the first bug because many critter
|
||||
animations reuse the same bounding-box size across most of their
|
||||
frames, but the second bug affects any critter whose rendering revisits
|
||||
the same resource-cache slot across different draws, regardless of mod
|
||||
content.
|
||||
|
||||
### 4.5. Debugging
|
||||
|
||||
`engine-debug-tools/` holds engine instrumentation that's useful for
|
||||
diagnosing rendering bugs but has no place in a normal build - kept
|
||||
separate from `engine-patches/` (section 4.4) so it's never silently
|
||||
included in `make dist`/`make package`.
|
||||
|
||||
- `engine-debug-tools/engine-patches/` - patches in the same format and
|
||||
numbered the same way as `engine-patches/` (section 4.4's own process
|
||||
applies: scratch copy, `patch -p1`, regenerate via `diff -u` rather than
|
||||
hand-editing hunks), applied on top of the regular `engine-patches/`
|
||||
series, never in place of it. Currently one patch, adding a debug dump
|
||||
hotkey: pressing **K** in-game dumps every frame of the
|
||||
currently/most-recently-drawn critter's animation to `dumps/` (created
|
||||
next to the running binary) as raw palette-index pixel data - 2-byte LE
|
||||
width, 2-byte LE height, then width\*height index bytes, one file per
|
||||
frame, named `id<resource-id>_frame<N>.raw`. Deliberately independent of
|
||||
the renderer's own texture/GL state, so a frame already corrupted in the
|
||||
dump is a decode-time bug, while a frame that's clean in the dump but
|
||||
wrong on screen is a rendering-time one.
|
||||
- `engine-debug-tools/dump2png/` - converts those `.raw` dumps into
|
||||
viewable PNGs using the real game palette (the dump format has no
|
||||
palette of its own, to keep the in-game dump path from depending on the
|
||||
renderer's own palette/SDL state). Builds the same way
|
||||
`modding/gen_animgallery.sh` builds `animgallery` - against
|
||||
`modding/src/hacked` inside `modding/mdlp-image`'s Go toolchain, since
|
||||
neither has a cgo dependency of its own - via
|
||||
`engine-debug-tools/build-dump2png.sh <output-path>`.
|
||||
|
||||
`make dist_debug` builds everything above into `dist/` alongside the
|
||||
normal `make dist` output: the engine with `engine-debug-tools/
|
||||
engine-patches/` applied in addition to the regular `engine-patches/`
|
||||
series, plus `dist/dump2png` and `dist/convert-dumps.sh` (a `dump2png`
|
||||
wrapper, same convention as `run-mods.sh` wrapping `run.sh`, pointed at
|
||||
`dist/`'s own `res/data` and `dumps/` so no arguments are needed). There's
|
||||
one `dist/` - whichever of `dist`/`dist_debug` ran last determines whether
|
||||
the debug hotkey, `dump2png` and `convert-dumps.sh` are present; running
|
||||
the other target again overwrites it back.
|
||||
|
||||
```sh
|
||||
make dist_debug
|
||||
./dist/run.sh # or run-mods.sh - play until the bug shows, press K, quit
|
||||
./dist/convert-dumps.sh # writes dist/dumps-png/*.png from dist/dumps/*.raw
|
||||
```
|
||||
|
||||
## 5. Android / Quest build
|
||||
|
||||
|
||||
Reference in New Issue
Block a user