From d30fee4a99e644cf63b7f09cca11b800184d1d69 Mon Sep 17 00:00:00 2001 From: frosch Date: Wed, 30 Apr 2025 10:14:16 +0200 Subject: [PATCH] Codechange: Improve debug/error messages when reading baseset metadata. --- src/base_media_base.h | 6 +++- src/base_media_func.h | 77 +++++++++++++++++++++++++++++-------------- src/gfxinit.cpp | 25 +++++++------- 3 files changed, 70 insertions(+), 38 deletions(-) diff --git a/src/base_media_base.h b/src/base_media_base.h index 423057acea..229ea4bca1 100644 --- a/src/base_media_base.h +++ b/src/base_media_base.h @@ -16,8 +16,9 @@ #include "3rdparty/md5/md5.h" #include -/* Forward declare these; can't do 'struct X' in functions as older GCCs barf on that */ struct IniFile; +struct IniGroup; +struct IniItem; struct ContentInfo; /** Structure holding filename and MD5 information about a single file */ @@ -96,6 +97,9 @@ struct BaseSet { return BaseSet::NUM_FILES - this->valid_files; } + void LogError(std::string_view full_filename, std::string_view detail, int level = 0) const; + const IniItem *GetMandatoryItem(std::string_view full_filename, const IniGroup &group, std::string_view name) const; + bool FillSetDetails(const IniFile &ini, const std::string &path, const std::string &full_filename, bool allow_empty_filename = true); void CopyCompatibleConfig([[maybe_unused]] const T &src) {} diff --git a/src/base_media_func.h b/src/base_media_func.h index cdcf3c2200..5090138c29 100644 --- a/src/base_media_func.h +++ b/src/base_media_func.h @@ -19,16 +19,33 @@ extern void CheckExternalFiles(); /** - * Try to read a single piece of metadata and return false if it doesn't exist. + * Log error from reading basesets. + * @param full_filename the full filename of the loaded file + * @param detail detail log message + * @param level debug level + */ +template +void BaseSet::LogError(std::string_view full_filename, std::string_view detail, int level) const +{ + Debug(misc, level, "Loading base {}set details failed: {}", BaseSet::SET_TYPE, full_filename); + Debug(misc, level, " {}", detail); +} + +/** + * Try to read a single piece of metadata and return nullptr if it doesn't exist. + * Log error, if the data is missing. + * @param full_filename the full filename of the loaded file (for error reporting purposes) + * @param group ini group to read from * @param name the name of the item to fetch. */ -#define fetch_metadata(name) \ - item = metadata->GetItem(name); \ - if (item == nullptr || !item->value.has_value() || item->value->empty()) { \ - Debug(grf, 0, "Base {}set detail loading: {} field missing.", BaseSet::SET_TYPE, name); \ - Debug(grf, 0, " Is {} readable for the user running OpenTTD?", full_filename); \ - return false; \ - } +template +const IniItem *BaseSet::GetMandatoryItem(std::string_view full_filename, const IniGroup &group, std::string_view name) const +{ + auto *item = group.GetItem(name); + if (item != nullptr && item->value.has_value() && !item->value->empty()) return item; + this->LogError(full_filename, fmt::format("{}.{} field missing.", group.name, name)); + return nullptr; +} /** * Read the set information from a loaded ini. @@ -43,16 +60,17 @@ bool BaseSet::FillSetDetails(const IniFile &ini, const std::string &path, con { const IniGroup *metadata = ini.GetGroup("metadata"); if (metadata == nullptr) { - Debug(grf, 0, "Base {}set detail loading: metadata missing.", BaseSet::SET_TYPE); - Debug(grf, 0, " Is {} readable for the user running OpenTTD?", full_filename); + this->LogError(full_filename, "Is the file readable for the user running OpenTTD?"); return false; } const IniItem *item; - fetch_metadata("name"); + item = this->GetMandatoryItem(full_filename, *metadata, "name"); + if (item == nullptr) return false; this->name = *item->value; - fetch_metadata("description"); + item = this->GetMandatoryItem(full_filename, *metadata, "description"); + if (item == nullptr) return false; this->description[std::string{}] = *item->value; item = metadata->GetItem("url"); @@ -65,15 +83,17 @@ bool BaseSet::FillSetDetails(const IniFile &ini, const std::string &path, con this->description[titem.name.substr(12)] = titem.value.value_or(""); } - fetch_metadata("shortname"); + item = this->GetMandatoryItem(full_filename, *metadata, "shortname"); + if (item == nullptr) return false; for (uint i = 0; (*item->value)[i] != '\0' && i < 4; i++) { this->shortname |= ((uint8_t)(*item->value)[i]) << (i * 8); } - fetch_metadata("version"); + item = this->GetMandatoryItem(full_filename, *metadata, "version"); + if (item == nullptr) return false; auto value = ParseInteger(*item->value); if (!value.has_value()) { - Debug(grf, 0, "Base {}set detail loading: {} field is invalid: {}", BaseSet::SET_TYPE, item->name, *item->value); + this->LogError(full_filename, fmt::format("metadata.version field is invalid: {}", *item->value)); return false; } this->version = *value; @@ -86,13 +106,18 @@ bool BaseSet::FillSetDetails(const IniFile &ini, const std::string &path, con const IniGroup *md5s = ini.GetGroup("md5s"); const IniGroup *origin = ini.GetGroup("origin"); auto file_names = BaseSet::GetFilenames(); + bool original_set = + std::byteswap(this->shortname) == 'TTDD' || // TTD DOS graphics, TTD DOS music + std::byteswap(this->shortname) == 'TTDW' || // TTD WIN graphics, TTD WIN music + std::byteswap(this->shortname) == 'TTDO' || // TTD sound + std::byteswap(this->shortname) == 'TTOD'; // TTO music for (uint i = 0; i < BaseSet::NUM_FILES; i++) { MD5File *file = &this->files[i]; /* Find the filename first. */ item = files != nullptr ? files->GetItem(file_names[i]) : nullptr; if (item == nullptr || (!item->value.has_value() && !allow_empty_filename)) { - Debug(grf, 0, "No {} file for: {} (in {})", BaseSet::SET_TYPE, file_names[i], full_filename); + this->LogError(full_filename, fmt::format("files.{} field missing", file_names[i])); return false; } @@ -110,7 +135,7 @@ bool BaseSet::FillSetDetails(const IniFile &ini, const std::string &path, con /* Then find the MD5 checksum */ item = md5s != nullptr ? md5s->GetItem(filename) : nullptr; if (item == nullptr || !item->value.has_value()) { - Debug(grf, 0, "No MD5 checksum specified for: {} (in {})", filename, full_filename); + this->LogError(full_filename, fmt::format("md5s.{} field missing", filename)); return false; } const char *c = item->value->c_str(); @@ -123,7 +148,7 @@ bool BaseSet::FillSetDetails(const IniFile &ini, const std::string &path, con } else if ('A' <= *c && *c <= 'F') { j = *c - 'A' + 10; } else { - Debug(grf, 0, "Malformed MD5 checksum specified for: {} (in {})", filename, full_filename); + this->LogError(full_filename, fmt::format("md5s.{} is malformed: {}", filename, *item->value)); return false; } if (i % 2 == 0) { @@ -137,7 +162,7 @@ bool BaseSet::FillSetDetails(const IniFile &ini, const std::string &path, con item = origin != nullptr ? origin->GetItem(filename) : nullptr; if (item == nullptr) item = origin != nullptr ? origin->GetItem("default") : nullptr; if (item == nullptr || !item->value.has_value()) { - Debug(grf, 1, "No origin warning message specified for: {}", filename); + this->LogError(full_filename, fmt::format("origin.{} field missing", filename), 1); file->missing_warning.clear(); } else { file->missing_warning = item->value.value(); @@ -154,12 +179,14 @@ bool BaseSet::FillSetDetails(const IniFile &ini, const std::string &path, con break; case MD5File::CR_MISMATCH: - Debug(grf, 1, "MD5 checksum mismatch for: {} (in {})", filename, full_filename); + /* This is normal for original sample.cat, which either matches with orig_dos or orig_win. */ + this->LogError(full_filename, fmt::format("MD5 checksum mismatch for: {}", filename), original_set ? 1 : 0); this->found_files++; break; case MD5File::CR_NO_FILE: - Debug(grf, 1, "The file {} specified in {} is missing", filename, full_filename); + /* Missing files is normal for the original basesets. Use lower debug level */ + this->LogError(full_filename, fmt::format("File is missing: {}", filename), original_set ? 1 : 0); break; } } @@ -171,7 +198,7 @@ template bool BaseMedia::AddFile(const std::string &filename, size_t basepath_length, const std::string &) { bool ret = false; - Debug(grf, 1, "Checking {} for base {} set", filename, BaseSet::SET_TYPE); + Debug(misc, 1, "Checking {} for base {} set", filename, BaseSet::SET_TYPE); Tbase_set *set = new Tbase_set(); IniFile ini{}; @@ -197,7 +224,7 @@ bool BaseMedia::AddFile(const std::string &filename, size_t basepath_ /* The more complete set takes precedence over the version number. */ if ((duplicate->valid_files == set->valid_files && duplicate->version >= set->version) || duplicate->valid_files > set->valid_files) { - Debug(grf, 1, "Not adding {} ({}) as base {} set (duplicate, {})", set->name, set->version, + Debug(misc, 1, "Not adding {} ({}) as base {} set (duplicate, {})", set->name, set->version, BaseSet::SET_TYPE, duplicate->valid_files > set->valid_files ? "less valid files" : "lower version"); set->next = BaseMedia::duplicate_sets; @@ -217,7 +244,7 @@ bool BaseMedia::AddFile(const std::string &filename, size_t basepath_ * version number until a new game is started which isn't a big problem */ if (BaseMedia::used_set == duplicate) BaseMedia::used_set = set; - Debug(grf, 1, "Removing {} ({}) as base {} set (duplicate, {})", duplicate->name, duplicate->version, + Debug(misc, 1, "Removing {} ({}) as base {} set (duplicate, {})", duplicate->name, duplicate->version, BaseSet::SET_TYPE, duplicate->valid_files < set->valid_files ? "less valid files" : "lower version"); duplicate->next = BaseMedia::duplicate_sets; @@ -232,7 +259,7 @@ bool BaseMedia::AddFile(const std::string &filename, size_t basepath_ ret = true; } if (ret) { - Debug(grf, 1, "Adding {} ({}) as base {} set", set->name, set->version, BaseSet::SET_TYPE); + Debug(misc, 1, "Adding {} ({}) as base {} set", set->name, set->version, BaseSet::SET_TYPE); } } else { delete set; diff --git a/src/gfxinit.cpp b/src/gfxinit.cpp index 81987ae1b0..b6aa3888bf 100644 --- a/src/gfxinit.cpp +++ b/src/gfxinit.cpp @@ -354,20 +354,21 @@ GraphicsSet::~GraphicsSet() = default; bool GraphicsSet::FillSetDetails(const IniFile &ini, const std::string &path, const std::string &full_filename) { - bool ret = this->BaseSet::FillSetDetails(ini, path, full_filename, false); - if (ret) { - const IniGroup *metadata = ini.GetGroup("metadata"); - assert(metadata != nullptr); /* ret can't be true if metadata isn't present. */ - const IniItem *item; + if (!this->BaseSet::FillSetDetails(ini, path, full_filename, false)) return false; - fetch_metadata("palette"); - this->palette = ((*item->value)[0] == 'D' || (*item->value)[0] == 'd') ? PAL_DOS : PAL_WINDOWS; + const IniGroup *metadata = ini.GetGroup("metadata"); + assert(metadata != nullptr); /* already checked by the inherited FillSetDetails. */ + const IniItem *item; - /* Get optional blitter information. */ - item = metadata->GetItem("blitter"); - this->blitter = (item != nullptr && (*item->value)[0] == '3') ? BLT_32BPP : BLT_8BPP; - } - return ret; + item = this->GetMandatoryItem(full_filename, *metadata, "palette"); + if (item == nullptr) return false; + this->palette = ((*item->value)[0] == 'D' || (*item->value)[0] == 'd') ? PAL_DOS : PAL_WINDOWS; + + /* Get optional blitter information. */ + item = metadata->GetItem("blitter"); + this->blitter = (item != nullptr && (*item->value)[0] == '3') ? BLT_32BPP : BLT_8BPP; + + return true; } /**