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

@@ -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<stat> stats;
 
-    inline stat &register_stat(const char *name, bool show_even_if_zero = false)
+    inline stat &register_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);
             }
         }
     }

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);

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<qvec3d, 3> &planepts, const qplane3d &plane)
+    std::array<qvec3d, 3> &planepts, const qplane3d &plane, texture_def_issues_t &issue_stats)
 {
     vec_t rotate;
     qmat<vec_t, 2, 3> 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<mapface_t> ParseBrushFace(parser_t &parser, const mapbrush_t &brush, const mapentity_t &entity)
+static std::optional<mapface_t> ParseBrushFace(parser_t &parser, const mapbrush_t &brush, const mapentity_t &entity, texture_def_issues_t &issue_stats)
 {
     std::array<qvec3d, 3> planepts;
     bool normal_ok;
@@ -1618,7 +1627,7 @@ static std::optional<mapface_t> 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<mapface_t> face = ParseBrushFace(parser, brush, entity);
+        std::optional<mapface_t> 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);
+            auto file = fs::load(qbsp_options.map_path);
-            return;
-        }
 
-        parser_t parser(file, { qbsp_options.map_path.string() });
+            if (!file) {
-
+                FError("Couldn't load map file \"{}\".\n", qbsp_options.map_path);
-        for (int i = 0;; i++) {
+                return;
-            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 (entity.epairs.get("classname") == "worldspawn") {
+            parser_t parser(file, { qbsp_options.map_path.string() });
-                // 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
+            for (int i = 0;; i++) {
-                entity.epairs.set("classname", "func_group");
+                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());

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;

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();