From 1aaa5133686d4a427e738ba978c199d3e81f83f5 Mon Sep 17 00:00:00 2001 From: Jonathan Date: Sat, 20 Aug 2022 20:13:27 -0400 Subject: [PATCH] collapse certain map face issues into loggable warning stats to lessen maps spamming warnings on things that legacy compilers allow adjust the wording of certain warning messages don't display face crunching on bmodels/hulls by default --- include/common/log.hh | 12 ++-- include/qbsp/map.hh | 18 +++++- qbsp/map.cc | 124 +++++++++++++++++++++++------------------- qbsp/qbsp.cc | 2 +- tests/test_qbsp.cc | 3 +- 5 files changed, 96 insertions(+), 63 deletions(-) diff --git a/include/common/log.hh b/include/common/log.hh index 329372a0..faa4ca7c 100644 --- a/include/common/log.hh +++ b/include/common/log.hh @@ -182,11 +182,13 @@ struct stat_tracker_t { const char *name; bool show_even_if_zero; + bool is_warning; std::atomic_size_t count = 0; - inline stat(const char *name, bool show_even_if_zero) : + inline stat(const char *name, bool show_even_if_zero, bool is_warning) : name(name), - show_even_if_zero(show_even_if_zero) + show_even_if_zero(show_even_if_zero), + is_warning(is_warning) { } @@ -200,16 +202,16 @@ struct stat_tracker_t std::list stats; - inline stat ®ister_stat(const char *name, bool show_even_if_zero = false) + inline stat ®ister_stat(const char *name, bool show_even_if_zero = false, bool is_warning = false) { - return stats.emplace_back(name, show_even_if_zero); + return stats.emplace_back(name, show_even_if_zero, is_warning); } ~stat_tracker_t() { for (auto &stat : stats) { if (stat.show_even_if_zero || stat.count) { - print(flag::STAT, " {:8} {}\n", stat.count, stat.name); + print(flag::STAT, "{}{:{}} {}\n", stat.is_warning ? "WARNING: " : "", stat.count, stat.is_warning ? 0 : 13, stat.name); } } } diff --git a/include/qbsp/map.hh b/include/qbsp/map.hh index 8ea3b3c3..d4663289 100644 --- a/include/qbsp/map.hh +++ b/include/qbsp/map.hh @@ -329,7 +329,23 @@ extern mapdata_t map; void CalculateWorldExtent(void); -bool ParseEntity(parser_t &parser, mapentity_t &entity); +struct texture_def_issues_t : logging::stat_tracker_t +{ + // number of faces that have SKY | NODRAW mixed. this is a Q2-specific issue + // that is a bit weird, because NODRAW indicates that the face should not be + // emitted at all in Q1 compilers, whereas in qbsp3 it only left out a texinfo + // reference (in theory...); this meant that sky brushes would disappear. It + // doesn't really make sense to have these two mixed, because sky is drawn in-game + // and the texture is still referenced on them. + stat &num_sky_nodraw = register_stat("faces have SKY | NODRAW flags mixed; NODRAW removed as this combo makes no sense. Use -verbose to display affected faces.", false, true); + + // Q2 specific: TRANSLUCENT is an internal compiler flag and should never + // be set directly. In older tools, the only side effect this has is to + // turn it into DETAIL effectively. + stat &num_translucent = register_stat("faces have TRANSLUCENT flag swapped to DETAIL; TRANSLUCENT is an internal flag. Use -verbose to display affected faces.", false, true); +}; + +bool ParseEntity(parser_t &parser, mapentity_t &entity, texture_def_issues_t &issues_stats); void ProcessExternalMapEntity(mapentity_t &entity); void ProcessAreaPortal(mapentity_t &entity); diff --git a/qbsp/map.cc b/qbsp/map.cc index 73321468..d71bce65 100644 --- a/qbsp/map.cc +++ b/qbsp/map.cc @@ -1384,7 +1384,7 @@ parse_error: } static void ParseTextureDef(parser_t &parser, mapface_t &mapface, const mapbrush_t &brush, maptexinfo_t *tx, - std::array &planepts, const qplane3d &plane) + std::array &planepts, const qplane3d &plane, texture_def_issues_t &issue_stats) { vec_t rotate; qmat texMat, axis; @@ -1477,14 +1477,23 @@ static void ParseTextureDef(parser_t &parser, mapface_t &mapface, const mapbrush if (!(extinfo.info->flags.native & (Q2_SURF_TRANS33 | Q2_SURF_TRANS66))) { extinfo.info->contents.native |= Q2_CONTENTS_DETAIL; - logging::print("WARNING: {}: swapped TRANSLUCENT for DETAIL\n", mapface.line); + if (qbsp_options.verbose.value()) { + logging::print("WARNING: {}: swapped TRANSLUCENT for DETAIL\n", mapface.line); + } else { + issue_stats.num_translucent++; + } } } // This fixes a bug in some old maps. if ((extinfo.info->flags.native & (Q2_SURF_SKY | Q2_SURF_NODRAW)) == (Q2_SURF_SKY | Q2_SURF_NODRAW)) { extinfo.info->flags.native &= ~Q2_SURF_NODRAW; - logging::print("WARNING: {}: SKY | NODRAW mixed. Removing NODRAW.\n", mapface.line); + + if (qbsp_options.verbose.value()) { + logging::print("WARNING: {}: SKY | NODRAW mixed. Removing NODRAW.\n", mapface.line); + } else { + issue_stats.num_sky_nodraw++; + } } } @@ -1604,7 +1613,7 @@ static void ValidateTextureProjection(mapface_t &mapface, maptexinfo_t *tx) } } -static std::optional ParseBrushFace(parser_t &parser, const mapbrush_t &brush, const mapentity_t &entity) +static std::optional ParseBrushFace(parser_t &parser, const mapbrush_t &brush, const mapentity_t &entity, texture_def_issues_t &issue_stats) { std::array planepts; bool normal_ok; @@ -1618,7 +1627,7 @@ static std::optional ParseBrushFace(parser_t &parser, const mapbrush_ normal_ok = face.set_planepts(planepts); - ParseTextureDef(parser, face, brush, &tx, face.planepts, face.get_plane()); + ParseTextureDef(parser, face, brush, &tx, face.planepts, face.get_plane(), issue_stats); if (!normal_ok) { logging::print("WARNING: {}: Brush plane with no normal\n", parser.location); @@ -2083,7 +2092,7 @@ static contentflags_t Brush_GetContents(const mapentity_t &entity, const mapbrus } if (!contents.types_equal(base_contents, qbsp_options.target_game)) { - logging::print("WARNING: {}: mixed face contents ({} != {})\n", + logging::print("WARNING: {}: brush has multiple face contents ({} vs {}), the former will be used.\n", mapface.line, base_contents.to_string(qbsp_options.target_game), contents.to_string(qbsp_options.target_game)); break; } @@ -2114,7 +2123,7 @@ static contentflags_t Brush_GetContents(const mapentity_t &entity, const mapbrus return base_contents; } -static mapbrush_t ParseBrush(parser_t &parser, mapentity_t &entity) +static mapbrush_t ParseBrush(parser_t &parser, mapentity_t &entity, texture_def_issues_t &issue_stats) { mapbrush_t brush; @@ -2150,7 +2159,7 @@ static mapbrush_t ParseBrush(parser_t &parser, mapentity_t &entity) if (parser.token == "}") break; - std::optional face = ParseBrushFace(parser, brush, entity); + std::optional face = ParseBrushFace(parser, brush, entity, issue_stats); if (!face) { continue; @@ -2193,7 +2202,7 @@ static mapbrush_t ParseBrush(parser_t &parser, mapentity_t &entity) return brush; } -bool ParseEntity(parser_t &parser, mapentity_t &entity) +bool ParseEntity(parser_t &parser, mapentity_t &entity, texture_def_issues_t &issue_stats) { entity.location = parser.location; @@ -2235,7 +2244,7 @@ bool ParseEntity(parser_t &parser, mapentity_t &entity) } } while (parser.token != "}"); } else { - entity.mapbrushes.emplace_back(ParseBrush(parser, entity)); + entity.mapbrushes.emplace_back(ParseBrush(parser, entity, issue_stats)); } } else { ParseEpair(parser, entity); @@ -2358,9 +2367,10 @@ static mapentity_t LoadExternalMap(const std::string &filename) } parser_t parser(file, { filename }); + texture_def_issues_t issue_stats; // parse the worldspawn - if (!ParseEntity(parser, dest)) { + if (!ParseEntity(parser, dest, issue_stats)) { FError("'{}': Couldn't parse worldspawn entity\n", filename); } const std::string &classname = dest.epairs.get("classname"); @@ -2370,7 +2380,7 @@ static mapentity_t LoadExternalMap(const std::string &filename) // parse any subsequent entities, move any brushes to worldspawn mapentity_t dummy{}; - while (ParseEntity(parser, dummy)) { + while (ParseEntity(parser, dummy, issue_stats)) { // move the brushes to the worldspawn dest.mapbrushes.insert(dest.mapbrushes.end(), std::make_move_iterator(dummy.mapbrushes.begin()), std::make_move_iterator(dummy.mapbrushes.end())); @@ -2802,55 +2812,59 @@ void LoadMapFile(void) logging::funcheader(); { - auto file = fs::load(qbsp_options.map_path); + texture_def_issues_t issue_stats; - if (!file) { - FError("Couldn't load map file \"{}\".\n", qbsp_options.map_path); - return; - } + { + auto file = fs::load(qbsp_options.map_path); - parser_t parser(file, { qbsp_options.map_path.string() }); - - for (int i = 0;; i++) { - mapentity_t &entity = map.entities.emplace_back(); - - if (!ParseEntity(parser, entity)) { - break; - } - } - - // Remove dummy entity inserted above - assert(!map.entities.back().epairs.size()); - map.entities.pop_back(); - } - - // -add function - if (!qbsp_options.add.value().empty()) { - auto file = fs::load(qbsp_options.add.value()); - - if (!file) { - FError("Couldn't load map file \"{}\".\n", qbsp_options.add.value()); - return; - } - - parser_t parser(file, { qbsp_options.add.value() }); - - for (int i = 0;; i++) { - mapentity_t &entity = map.entities.emplace_back(); - - if (!ParseEntity(parser, entity)) { - break; + if (!file) { + FError("Couldn't load map file \"{}\".\n", qbsp_options.map_path); + return; } - if (entity.epairs.get("classname") == "worldspawn") { - // The easiest way to get the additional map's worldspawn brushes - // into the base map's is to rename the additional map's worldspawn classname to func_group - entity.epairs.set("classname", "func_group"); + parser_t parser(file, { qbsp_options.map_path.string() }); + + for (int i = 0;; i++) { + mapentity_t &entity = map.entities.emplace_back(); + + if (!ParseEntity(parser, entity, issue_stats)) { + break; + } } + + // Remove dummy entity inserted above + assert(!map.entities.back().epairs.size()); + map.entities.pop_back(); + } + + // -add function + if (!qbsp_options.add.value().empty()) { + auto file = fs::load(qbsp_options.add.value()); + + if (!file) { + FError("Couldn't load map file \"{}\".\n", qbsp_options.add.value()); + return; + } + + parser_t parser(file, { qbsp_options.add.value() }); + + for (int i = 0;; i++) { + mapentity_t &entity = map.entities.emplace_back(); + + if (!ParseEntity(parser, entity, issue_stats)) { + break; + } + + if (entity.epairs.get("classname") == "worldspawn") { + // The easiest way to get the additional map's worldspawn brushes + // into the base map's is to rename the additional map's worldspawn classname to func_group + entity.epairs.set("classname", "func_group"); + } + } + // Remove dummy entity inserted above + assert(!map.entities.back().epairs.size()); + map.entities.pop_back(); } - // Remove dummy entity inserted above - assert(!map.entities.back().epairs.size()); - map.entities.pop_back(); } logging::print(logging::flag::STAT, " {:8} entities\n", map.entities.size()); diff --git a/qbsp/qbsp.cc b/qbsp/qbsp.cc index 1716dde2..4bf54a1d 100644 --- a/qbsp/qbsp.cc +++ b/qbsp/qbsp.cc @@ -457,7 +457,7 @@ static void ProcessEntity(mapentity_t &entity, hull_index_t hullnum) Brush_LoadEntity(entity, hullnum, brushes, num_clipped); if (num_clipped && !qbsp_options.verbose.value()) { - logging::print("WARNING: {} faces were clipped away. This is normal for expanded hulls; use -verbose if you need more info.\n", num_clipped); + logging::print(logging::flag::STAT, "WARNING: {} faces were crunched away by being too small. {}Use -verbose to see which faces were affected.\n", num_clipped, hullnum.value_or(0) ? "This is normal for the hulls. " : ""); } size_t num_sides = 0; diff --git a/tests/test_qbsp.cc b/tests/test_qbsp.cc index a3840db3..3b09c4dc 100644 --- a/tests/test_qbsp.cc +++ b/tests/test_qbsp.cc @@ -43,9 +43,10 @@ static mapentity_t &LoadMap(const char *map) parser_t parser(map, { Catch::getResultCapture().getCurrentTestName() }); mapentity_t &entity = ::map.entities.emplace_back(); + texture_def_issues_t issue_stats; // FIXME: adds the brush to the global map... - Q_assert(ParseEntity(parser, entity)); + Q_assert(ParseEntity(parser, entity, issue_stats)); CalculateWorldExtent();