From b9501df52953c3040f91bc5baae0e57a00c4d5b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Tue, 29 Sep 2026 13:28:21 -0600 Subject: [PATCH] TextureReplacer: Fix stale and shared lookup results - Reloading the ini clears the per-key lookup caches, which could keep saying "no replacement" for textures the new ini replaces. - With ignoreAddress, hash ranges were skipped when sizing the replacement, though ComputeHash applies them. cache_ is now keyed by the full key, so its lookups hit too. - Textures sharing files but differing in size, hash range or filtering no longer share one ReplacedTexture (the first one's settings won). Co-Authored-By: Claude Opus 5.5 (1M context) --- GPU/Common/ReplacedTexture.h | 2 +- GPU/Common/TextureReplacer.cpp | 30 +++++++---- unittest/TestTextureReplacer.cpp | 86 ++++++++++++++++++++++++++++++-- 3 files changed, 101 insertions(+), 17 deletions(-) diff --git a/GPU/Common/ReplacedTexture.h b/GPU/Common/ReplacedTexture.h index 17817f77f8..7be90e24ff 100644 --- a/GPU/Common/ReplacedTexture.h +++ b/GPU/Common/ReplacedTexture.h @@ -119,7 +119,7 @@ class ReplacedTexture; // replacement (texture == nullptr). struct ReplacedTextureRef { ReplacedTexture *texture; // shortcut - std::string hashfiles; // key into the cache + std::string hashfiles; // key into levelCache_ }; // Metadata about a given texture level. diff --git a/GPU/Common/TextureReplacer.cpp b/GPU/Common/TextureReplacer.cpp index 44e3b7009f..aa36770b69 100644 --- a/GPU/Common/TextureReplacer.cpp +++ b/GPU/Common/TextureReplacer.cpp @@ -147,6 +147,10 @@ bool TextureReplacer::LoadIni(std::string *error, bool notify) { hashranges_.clear(); filtering_.clear(); reducehashranges_.clear(); + // These hold what the old ini said about each texture, including "no replacement" markers. + // Only references go, the textures stay in levelCache_. + cache_.clear(); + savedCache_.clear(); ignoreAddress_ = false; reduceHash_ = false; @@ -670,15 +674,18 @@ ReplacedTexture *TextureReplacer::FindReplacement(ReplacementCacheKey replacemen desc.cacheKey = replacementKey; desc.forceFiltering = (TextureFiltering)0; // invalid value + // Hash ranges are per address even with ignoreAddress, since ComputeHash applies them. + LookupHashRange(replacementKey.Address(), w, h, &desc.newW, &desc.newH); + + // cache_ stays keyed by the full key. ignoreAddress only affects finding the files. + ReplacementCacheKey lookupKey = replacementKey; if (ignoreAddress_) { - replacementKey.ZeroAddress(); - } else { - LookupHashRange(replacementKey.Address(), w, h, &desc.newW, &desc.newH); + lookupKey.ZeroAddress(); } bool foundAlias = false; bool ignored = false; - std::string hashfiles = LookupHashFile(replacementKey, &foundAlias, &ignored); + std::string hashfiles = LookupHashFile(lookupKey, &foundAlias, &ignored); // Early-out for ignored textures, let's not bother even starting a thread task. if (ignored) { @@ -689,7 +696,7 @@ ReplacedTexture *TextureReplacer::FindReplacement(ReplacementCacheKey replacemen return nullptr; } - FindFiltering(replacementKey, &desc.forceFiltering); + FindFiltering(lookupKey, &desc.forceFiltering); if (foundAlias) { desc.logId = hashfiles; @@ -707,12 +714,14 @@ ReplacedTexture *TextureReplacer::FindReplacement(ReplacementCacheKey replacemen } _dbg_assert_(!hashfiles.empty()); - // OK, we might already have a matching texture, we use hashfiles as a key. Look it up in the level cache. - auto iter = levelCache_.find(hashfiles); + // OK, we might already have a matching texture. Textures sharing files can still differ in how + // they're scaled and filtered, so those go in the level cache key too. + std::string levelKey = StringFromFormat("%s#%dx%d>%dx%d#%d", hashfiles.c_str(), desc.w, desc.h, desc.newW, desc.newH, (int)desc.forceFiltering); + auto iter = levelCache_.find(levelKey); if (iter != levelCache_.end()) { // Insert an entry into the cache for faster lookup next time. ReplacedTextureRef ref; - ref.hashfiles = hashfiles; + ref.hashfiles = levelKey; ref.texture = iter->second; cache_.emplace(std::make_pair(replacementKey, ref)); return iter->second; @@ -725,12 +734,11 @@ ReplacedTexture *TextureReplacer::FindReplacement(ReplacementCacheKey replacemen ReplacedTexture *texture = new ReplacedTexture(vfs_, desc); ReplacedTextureRef ref; - ref.hashfiles = hashfiles; + ref.hashfiles = levelKey; ref.texture = texture; cache_.emplace(std::make_pair(replacementKey, ref)); - // Also, insert the level in the level cache so we can look up by desc_->hashfiles again. - levelCache_.emplace(std::make_pair(hashfiles, texture)); + levelCache_.emplace(std::make_pair(levelKey, texture)); return texture; } diff --git a/unittest/TestTextureReplacer.cpp b/unittest/TestTextureReplacer.cpp index 3f31ad0f56..9b780ccf32 100644 --- a/unittest/TestTextureReplacer.cpp +++ b/unittest/TestTextureReplacer.cpp @@ -22,6 +22,10 @@ static const u64 KEY_D = 0xA0000004C0000004ULL; // ignored (empty filename) static const u32 HASH_D = 0x10000004; static const u64 KEY_E = 0xA0000005C0000005ULL; // file missing static const u32 HASH_E = 0x10000005; +static const u64 KEY_F = 0xA0000006C0000006ULL; // same file as A, own hashrange +static const u32 HASH_F = 0x10000006; +static const u64 KEY_G = 0xA0000007C0000007ULL; // ignoreAddress pack, hashrange +static const u32 HASH_G = 0x10000007; static bool CreateTestPNG(const Path &filename, int w, int h, u32 color) { std::vector buf((size_t)w * h * 4); @@ -34,6 +38,16 @@ static bool CreateTestPNG(const Path &filename, int w, int h, u32 color) { return pngSave(filename, buf.data(), w, h, 4); } +static bool WriteIni(const Path &packDir, const char *iniContent) { + FILE *f = File::OpenCFile(packDir / "textures.ini", "w"); + if (!f) { + return false; + } + fwrite(iniContent, 1, strlen(iniContent), f); + fclose(f); + return true; +} + static bool CreateTestPack(const Path &packDir) { File::DeleteDirRecursively(packDir); if (!File::CreateDir(packDir)) { @@ -52,19 +66,18 @@ static bool CreateTestPack(const Path &packDir) { "A0000003C000000310000003 = tex_c.png\n" "A0000004C000000410000004 =\n" "A0000005C000000510000005 = missing.png\n" + "A0000006C000000610000006 = tex_a.png\n" "\n" "[hashranges]\n" "A0000003,512,512 = 256,256\n" + "A0000006,64,64 = 32,32\n" "\n" "[filtering]\n" "A0000001C000000110000001 = nearest\n"; - FILE *f = File::OpenCFile(packDir / "textures.ini", "w"); - if (!f) { + if (!WriteIni(packDir, iniContent)) { return false; } - fwrite(iniContent, 1, strlen(iniContent), f); - fclose(f); if (!CreateTestPNG(packDir / "tex_a.png", 64, 64, 0xFF0000FF)) return false; if (!CreateTestPNG(packDir / "tex_b0.png", 64, 64, 0x00FF00FF)) return false; @@ -94,11 +107,16 @@ static bool TestLookups(TextureReplacer *replacer) { ReplacedTexture *texE = replacer->FindReplacement(ReplacementCacheKey(KEY_E, HASH_E), 16, 16); EXPECT_TRUE(texE != nullptr); + // Key F: the same file as A, but its own hashrange and no filtering, so it can't share A's texture. + ReplacedTexture *texF = replacer->FindReplacement(ReplacementCacheKey(KEY_F, HASH_F), 64, 64); + EXPECT_TRUE(texF != nullptr); + EXPECT_TRUE(texF != texA); + // Unknown key: not in the ini at all. ReplacementCacheKey unknownKey(0x2000000020000000ULL, 0x20000000); EXPECT_TRUE(replacer->FindReplacement(unknownKey, 16, 16) == nullptr); - if (!texA || !texB || !texC || !texE) { + if (!texA || !texB || !texC || !texE || !texF) { return false; } @@ -133,6 +151,13 @@ static bool TestLookups(TextureReplacer *replacer) { EXPECT_EQ_INT(w, 512); EXPECT_EQ_INT(h, 512); + // Key F: hashrange maps 64x64 -> 32x32, so the 64x64 image covers 128x128. + EXPECT_TRUE(texF->Poll(1.0)); + texF->GetSize(0, &w, &h); + EXPECT_EQ_INT(w, 128); + EXPECT_EQ_INT(h, 128); + EXPECT_FALSE(texF->ForceFiltering(&filtering)); + // Key E: missing file should end up NOT_FOUND. EXPECT_TRUE(texE->Poll(1.0)); EXPECT_TRUE(texE->State() == ReplacementState::NOT_FOUND); @@ -141,6 +166,47 @@ static bool TestLookups(TextureReplacer *replacer) { return true; } +// ignoreAddress matches files without the address, but hash ranges still apply per address. +static bool TestIgnoreAddress(const Path &packDir) { + static const char *iniContent = + "[options]\n" + "hash = xxh32\n" + "ignoreAddress = true\n" + "version = 1\n" + "\n" + "[hashes]\n" + "00000000C000000710000007 = tex_c.png\n" + "\n" + "[hashranges]\n" + "A0000007,512,512 = 256,256\n"; + if (!WriteIni(packDir, iniContent)) { + return false; + } + + TextureReplacer replacer(nullptr); + std::string error; + if (!replacer.LoadPackForTesting(packDir, &error)) { + return false; + } + + ReplacementCacheKey key(KEY_G, HASH_G); + ReplacedTexture *texG = replacer.FindReplacement(key, 512, 512); + EXPECT_TRUE(texG != nullptr); + if (!texG) { + return false; + } + EXPECT_TRUE(replacer.FindReplacement(key, 512, 512) == texG); + + g_threadManager.Init(1, 1); + EXPECT_TRUE(texG->Poll(1.0)); + int w = 0, h = 0; + texG->GetSize(0, &w, &h); + EXPECT_EQ_INT(w, 512); + EXPECT_EQ_INT(h, 512); + g_threadManager.Teardown(); + return true; +} + bool TestTextureReplacer() { Path packDir = Path("unittest_texture_pack"); if (!CreateTestPack(packDir)) { @@ -160,6 +226,16 @@ bool TestTextureReplacer() { return false; } + // Reloading the ini forgets what the old one said about each texture. + EXPECT_TRUE(replacer.GetNumTrackedTextures() != 0); + EXPECT_TRUE(replacer.LoadPackForTesting(packDir, &error)); + EXPECT_EQ_INT(replacer.GetNumTrackedTextures(), 0); + + if (!TestIgnoreAddress(packDir)) { + File::DeleteDirRecursively(packDir); + return false; + } + File::DeleteDirRecursively(packDir); return true; }