Fix nondeterministic export output and several correctness bugs#71
Open
xmx-emm wants to merge 1 commit into
Open
Fix nondeterministic export output and several correctness bugs#71xmx-emm wants to merge 1 commit into
xmx-emm wants to merge 1 commit into
Conversation
Every re-export of the same asset could produce a file that differs by a few bytes/pixels. Multiple independent causes, all fixed here: - cpakfile.h getStarPakData(): the destination buffer was allocated uninitialized and the stream read was never validated. mip sizes are page aligned, so reads at the end of a (opt)starpak can come up short, leaving random heap bytes in the exported texture tail. Zero initialize the buffer so short reads produce deterministic zeroes. - rmax.cpp ToFile(): the 16 MiB scratch buffer was uninitialized while the writer aligns every data section with IALIGN16 and rounds the final size up to 16, so alignment gaps full of heap garbage were written into every .rmax, making each export differ. - dx.cpp ExportAsPng(): converted the texture to B8G8R8A8 in place. Shared textures (ui atlas rawTxtr/convertedTxtr) were permanently mutated, so a DDS export after a PNG export silently wrote converted data instead of the original format, and two export threads touching the same texture raced on delete/replace of the scratch image. Convert into a temporary image instead. - dx.cpp ExportAsPng/ExportAsDds: exports run on a thread pool and two tasks can write the same output path (materials sharing textures, selected assets sharing dependencies), interleaving writes within one file. Serialize writers per output path via a small mutex pool. - modeldata_qc.cpp: s_QCMaxVerts was a global updated by concurrent QC exports; a parallel export could write another model's \ value. Now passed by reference per export. - ui_image_atlas.cpp: posX/posY truncated while width/height rounded to nearest, so slices could be cut one pixel off when the float product lands just below a whole number (marked 'todo study this'). Also fixed the always-true 'height == height' comparison in the preview texture lookup. - ui_image.cpp: memcpy_s destination sizes for transparent tiles did not account for the tile offset; bc7 slice-pitch assert direction. - texture.cpp: mip table type-column sort compared type against level. - imgui_utility.cpp: progress bar name was passed to ImGui::Text as a format string. - jsonutils.h/.cpp: replace the invalid encoding-mangled copyright symbol (U+FFFD) that fails to compile on non-western codepages (C4819 + warnings-as-errors on codepage 936). Co-authored-by: Cursor <cursoragent@cursor.com>
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
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
Re-exporting the same asset could produce output files that differ by a few bytes/pixels on every run. I tracked this down to several independent sources of nondeterminism, plus a few adjacent correctness bugs found along the way. All are fixed in this PR; each fix is small and local.
Nondeterministic export output
cpakfile.hgetStarPakData(): the destination buffer was allocated uninitialized and the stream read result was never validated. Requested mip sizes are page aligned, so reads near the end of a (opt)starpak can come up short, leaving random heap bytes in the tail of the exported texture data - a few random pixels that change on every run. The buffer is now zero initialized so short reads produce deterministic zeroes.rmax.cppRMAXExporter::ToFile(): the 16 MiB scratch buffer was uninitialized, while the writer advances withIALIGN16after every data section and rounds the final file size up to 16 bytes. The alignment gaps were written to disk as heap garbage, so every exported.rmaxdiffered. The buffer is now zero initialized.dx.cppExportAsPng()/ExportAsDds(): exports run on a thread pool, and two tasks can write the same output path (e.g. two materials sharing a texture with "Export Material Textures" enabled, or two selected assets sharing a dependency with "Export asset dependencies" enabled). Concurrent writers interleave within a single file and corrupt it differently each run. File writes are now serialized per output path through a small hashed mutex pool.modeldata_qc.cpp:s_QCMaxVertswas a file-scope global mutated by concurrent QC exports, so a parallel export could write another model's$maxvertsvalue into the QC file. It is now a local passed by reference throughQC_ParseStudioBodypart/QC_ParseStudioLOD.Shared texture mutation
dx.cppExportAsPng()converted the texture to B8G8R8A8 in place viaConvertToFormat, which deletes and replaces the underlying scratch image. Atlas textures (rawTxtr/convertedTxtr) are shared between preview and both export paths, so exporting PNG first silently changed what a later DDS export wrote (converted data instead of the original block-compressed format), and two threads exporting the same texture raced on the delete/replace. The conversion now happens into a temporaryScratchImage, leaving the source texture untouched.One-pixel offset in ui image atlas slices
ui_image_atlas.cpp:posX/posYwere truncated whilewidth/heightwere rounded to nearest (+ 0.5f). WhenminX * atlasWidthlands just below a whole number (e.g.99.99997), the slice origin came out one pixel off. The position now uses the same round-to-nearest as the size; this resolves the existing// should this have the same rounding as below ? todo study thisnote.uiImage->height == uiImage->heightcomparison in the preview's main-texture check (should compare againstuiAsset->height).Smaller fixes
ui_image.cpp: thememcpy_sdestination sizes for transparent tile fills did not subtracttileOffset; the BC7 slice-pitch assert compared in the wrong direction (BC1 branch had it right).texture.cpp: the mip table "Type" column sort compareda.typeagainstb.level.imgui_utility.cpp: secondary progress bar names were passed toImGui::Textas a format string; nowTextUnformatted.jsonutils.h/.cpp: the copyright line contained an encoding-mangled character (U+FFFD). On systems with a non-western ANSI codepage (e.g. 936), MSVC raises C4819 which the project's warnings-as-errors turns into a build failure -maincurrently does not compile on such systems. Replaced with(c).Not addressed here (worth a follow-up):
RTech::DecompressStreamedBufferpasses the decompressed size as the available input byte count toOodleLZDecoder_DecodeSome, which can over-read the compressed input buffer. Fixing it requires a signature change across many call sites.Test plan
Release_CIsucceeds (warnings-as-errors, /W4)