Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pre-scan the root of texture packs for hash-named files. #17380

Merged
merged 4 commits into from
May 2, 2023
Merged

Pre-scan the root of texture packs for hash-named files. #17380

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
35aff7e
Fix logic in ZipFileReader file listing
hrydgard May 1, 2023
d10fae7
Scan the root of loaded texture packs to find all the hash-named files.
hrydgard May 1, 2023
ee7e8d7
Add a unit test, fix listing zip directories
hrydgard May 2, 2023
919979e
Disable excessive logging
hrydgard May 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions Common/File/VFS/ZipFileReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,26 +132,29 @@ bool ZipFileReader::GetFileListing(const char *orig_path, std::vector<File::File

void ZipFileReader::GetZipListings(const char *path, std::set<std::string> &files, std::set<std::string> &directories) {
size_t pathlen = strlen(path);
if (path[pathlen - 1] == '/')
pathlen--;
if (pathlen == 1 && path[0] == '/') {
// Root. We simply use a zero length string.
pathlen = 0;
}

std::lock_guard<std::mutex> guard(lock_);
int numFiles = zip_get_num_files(zip_file_);
for (int i = 0; i < numFiles; i++) {
const char* name = zip_get_name(zip_file_, i, 0);
if (!name)
continue;
continue; // shouldn't happen, I think
if (!memcmp(name, path, pathlen)) {
// The prefix is right. Let's see if this is a file or path.
const char *slashPos = strchr(name + pathlen + 1, '/');
if (slashPos != 0) {
// A directory.
std::string dirName = std::string(name + pathlen + 1, slashPos - (name + pathlen + 1));
// A directory. Let's pick off the only part we care about.
int offset = pathlen;
std::string dirName = std::string(name + offset, slashPos - (name + offset));
directories.insert(dirName);
} else if (name[pathlen] == '/') {
const char *fn = name + pathlen + 1;
} else {
// It's a file.
const char *fn = name + pathlen;
files.insert(std::string(fn));
} // else, it was a file with the same prefix as the path. like langregion.ini next to lang/.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we protect against this case now?

So let's say path is "assets/lang". We did check it starts with "assets/lang", so that's good. But "assets/langregion" matches that and does not have a /. So won't we match it and add it to the list as "egion" or something?

I might also be missing something about name + pathlen. In my above scenario, "assets/lang/foo.ini" would not find a slash after name + pathlen + 1 (which is "foo.ini") but wouldn't fn become "/foo.ini"? Seems confusing.

It feels like this new logic might only be something that works when path == ""? Which does look like it was broken before.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I somehow convinced myself that since all directories in a zip file always end wih '/', they'd be caught in the first case anyway - but that doesn't even make sense. Don't know what I was thinking.

I'll make a unit test for this today to really make sure all cases comes out right.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this does work - just gotta follow the convention of directory paths ending with "/" consistently throughout.

The unit test revealed a number of bugs, but I've fixed it and it's working now.

Copy link
Collaborator

@unknownbrackets unknownbrackets May 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, yeah, making GetFileListing() add / makes sense.

But oops, now there's a source_assets/ziptest.zip. Oh, it's intentional for testing.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we don't have a designated directory for testdata, at least not that I found. Though I suppose could have put the zip in /unittest too.

}
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions Common/StringUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ inline bool endsWithNoCase(const std::string &str, const std::string &what) {
return strncasecmp(str.c_str() + offset, what.c_str(), what.size()) == 0;
}

inline bool equalsNoCase(const std::string &str, const char *what) {
return strcasecmp(str.c_str(), what) == 0;
}

void DataToHexString(const uint8_t *data, size_t size, std::string *output);
void DataToHexString(int indent, uint32_t startAddr, const uint8_t* data, size_t size, std::string* output);

Expand Down
77 changes: 52 additions & 25 deletions GPU/Common/TextureReplacer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ bool TextureReplacer::LoadIni() {
bool iniLoaded = ini.LoadFromVFS(*dir, INI_FILENAME);

if (iniLoaded) {
if (!LoadIniValues(ini)) {
if (!LoadIniValues(ini, dir)) {
delete dir;
return false;
}
Expand All @@ -160,7 +160,7 @@ bool TextureReplacer::LoadIni() {
}

INFO_LOG(G3D, "Loading extra texture ini: %s", overrideFilename.c_str());
if (!LoadIniValues(overrideIni, true)) {
if (!LoadIniValues(overrideIni, dir, true)) {
delete dir;
return false;
}
Expand Down Expand Up @@ -195,7 +195,7 @@ bool TextureReplacer::LoadIni() {
return true;
}

bool TextureReplacer::LoadIniValues(IniFile &ini, bool isOverride) {
bool TextureReplacer::LoadIniValues(IniFile &ini, VFSBackend *dir, bool isOverride) {
auto options = ini.GetOrCreateSection("options");
std::string hash;
options->Get("hash", &hash, "");
Expand Down Expand Up @@ -231,13 +231,14 @@ bool TextureReplacer::LoadIniValues(IniFile &ini, bool isOverride) {
}

bool filenameWarning = false;

std::map<ReplacementCacheKey, std::map<int, std::string>> filenameMap;

if (ini.HasSection("hashes")) {
auto hashes = ini.GetOrCreateSection("hashes")->ToMap();
// Format: hashname = filename.png
bool checkFilenames = g_Config.bSaveNewTextures && !g_Config.bIgnoreTextureFilenames && !vfsIsZip_;

std::map<ReplacementCacheKey, std::map<int, std::string>> filenameMap;

for (const auto &item : hashes) {
ReplacementCacheKey key(0, 0);
int level = 0; // sscanf might fail to pluck the level, but that's ok, we default to 0. sscanf doesn't write to non-matched outputs.
Expand All @@ -256,31 +257,57 @@ bool TextureReplacer::LoadIniValues(IniFile &ini, bool isOverride) {
ERROR_LOG(G3D, "Unsupported syntax under [hashes]: %s", item.first.c_str());
}
}
}

// Now, translate the filenameMap to the final aliasMap.
for (auto &pair : filenameMap) {
std::string alias;
int mipIndex = 0;
for (auto &level : pair.second) {
if (level.first == mipIndex) {
alias += level.second + "|";
mipIndex++;
} else {
WARN_LOG(G3D, "Non-sequential mip index %d, breaking. filenames=%s", level.first, level.second.c_str());
break;
}
// Scan the root of the texture folder/zip and preinitialize the hash map.
std::vector<File::FileInfo> filesInRoot;
dir->GetFileListing("/", &filesInRoot, nullptr);
for (auto file : filesInRoot) {
if (file.isDirectory)
continue;
if (file.name.empty() || file.name[0] == '.')
continue;
Path path(file.name);
std::string ext = path.GetFileExtension();

std::string hash = file.name.substr(0, file.name.size() - ext.size());
if (!((hash.size() >= 26 && hash.size() <= 27 && hash[24] == '_') || hash.size() == 24)) {
continue;
}
// OK, it's hash-like enough to try to parse it into the map.
if (equalsNoCase(ext, ".ktx2") || equalsNoCase(ext, ".png") || equalsNoCase(ext, ".dds")) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I guess we might as well add ".zim"?

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

late night coding, oops.

ReplacementCacheKey key(0, 0);
int level = 0; // sscanf might fail to pluck the level, but that's ok, we default to 0. sscanf doesn't write to non-matched outputs.
if (sscanf(hash.c_str(), "%16llx%8x_%d", &key.cachekey, &key.hash, &level) >= 1) {
INFO_LOG(G3D, "hash-like file in root, adding: %s", file.name.c_str());
filenameMap[key][level] = file.name;
}
if (alias == "|") {
alias = ""; // marker for no replacement
}
}

// Now, translate the filenameMap to the final aliasMap.
for (auto &pair : filenameMap) {
std::string alias;
int mipIndex = 0;
for (auto &level : pair.second) {
if (level.first == mipIndex) {
alias += level.second + "|";
mipIndex++;
} else {
WARN_LOG(G3D, "Non-sequential mip index %d, breaking. filenames=%s", level.first, level.second.c_str());
break;
}
// Replace any '\' with '/', to be safe and consistent. Since these are from the ini file, we do this on all platforms.
for (auto &c : alias) {
if (c == '\\') {
c = '/';
}
}
if (alias == "|") {
alias = ""; // marker for no replacement
}
// Replace any '\' with '/', to be safe and consistent. Since these are from the ini file, we do this on all platforms.
for (auto &c : alias) {
if (c == '\\') {
c = '/';
}
aliases_[pair.first] = alias;
}
aliases_[pair.first] = alias;
}

if (filenameWarning) {
Expand Down
2 changes: 1 addition & 1 deletion GPU/Common/TextureReplacer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ class TextureReplacer {
bool FindFiltering(u64 cachekey, u32 hash, TextureFiltering *forceFiltering);

bool LoadIni();
bool LoadIniValues(IniFile &ini, bool isOverride = false);
bool LoadIniValues(IniFile &ini, VFSBackend *dir, bool isOverride = false);
void ParseHashRange(const std::string &key, const std::string &value);
void ParseFiltering(const std::string &key, const std::string &value);
void ParseReduceHashRange(const std::string& key, const std::string& value);
Expand Down