qbsp: fix edge reuse causing software renderer artifacts with liquids

fixes q1_liquid_software.map in tyrquake
thanks to Mankrip for reporting this

docs/qbsp.rst

@@ -36,6 +36,10 @@ Options
 
    Don't perform face merging.
 
+.. option:: -noedgereuse
+
+   Don't reuse edges (may be useful for debugging software rendering).
+
 .. option:: -noclip
 
    Doesn't build clip hulls (only applicable for Q1-like BSP formats).

include/qbsp/map.hh

@@ -173,6 +173,19 @@ struct mapplane_t : qbsp_plane_t
 struct planehash_t;
 struct vertexhash_t;
 
+struct hashedge_t
+{
+    size_t v1;
+    size_t v2;
+
+    int64_t edge_index;
+
+    /**
+     * the face that edge v1 -> v2 belongs to
+     */
+    const face_t *face;
+};
+
 struct mapdata_t
 {
     /* Arrays of actual items */
@@ -223,9 +236,9 @@ struct mapdata_t
     void add_hash_vector(const qvec3d &point, const size_t &num);
 
     // hashed edges; generated by EmitEdges
-    std::map<std::pair<size_t, size_t>, int64_t> hashedges;
+    std::map<std::pair<size_t, size_t>, hashedge_t> hashedges;
 
-    void add_hash_edge(size_t v1, size_t v2, int64_t i);
+    void add_hash_edge(size_t v1, size_t v2, int64_t edge_index, const face_t *face);
 
     /* Misc other global state for the compile process */
     bool leakfile = false; /* Flag once we've written a leak (.por/.pts) file */

include/qbsp/qbsp.hh

@@ -175,6 +175,7 @@ public:
     setting_int32 subdivide;
     setting_bool nofill;
     setting_bool nomerge;
+    setting_bool noedgereuse;
     setting_bool noclip;
     setting_bool noskip;
     setting_bool nodetail;

qbsp/faces.cc

@@ -142,11 +142,16 @@ inline int64_t GetEdge(const size_t &v1, const size_t &v2, const face_t *face, e
     if (!face->contents.front.is_valid(qbsp_options.target_game, false))
         FError("Face with invalid contents");
 
-    // search for existing edges
-    if (auto it = map.hashedges.find(std::make_pair(v1, v2)); it != map.hashedges.end()) {
-        return it->second;
-    } else if (auto it = map.hashedges.find(std::make_pair(v2, v1)); it != map.hashedges.end()) {
-        return -it->second;
+    if (!qbsp_options.noedgereuse.value()) {
+        // search for existing edges
+        if (auto it = map.hashedges.find(std::make_pair(v2, v1)); it != map.hashedges.end()) {
+            const hashedge_t &existing = it->second;
+            // this content check is required for software renderers
+            // (see q1_liquid_software test case)
+            if (existing.face->contents.front.equals(qbsp_options.target_game, face->contents.front)) {
+                return -existing.edge_index;
+            }
+        }
     }
 
     /* emit an edge */
@@ -154,7 +159,7 @@ inline int64_t GetEdge(const size_t &v1, const size_t &v2, const face_t *face, e
 
     map.bsp.dedges.emplace_back(bsp2_dedge_t{static_cast<uint32_t>(v1), static_cast<uint32_t>(v2)});
 
-    map.add_hash_edge(v1, v2, i);
+    map.add_hash_edge(v1, v2, i, face);
 
     stats.unique_edges++;
 

qbsp/map.cc

@@ -165,9 +165,9 @@ void mapdata_t::add_hash_vector(const qvec3d &point, const size_t &num)
     hashverts->hash.emplace(pareto::point<vec_t, 3>({point[0], point[1], point[2]}), num);
 }
 
-void mapdata_t::add_hash_edge(size_t v1, size_t v2, int64_t i)
+void mapdata_t::add_hash_edge(size_t v1, size_t v2, int64_t edge_index, const face_t *face)
 {
-    hashedges.emplace(std::make_pair(v1, v2), i);
+    hashedges.emplace(std::make_pair(v1, v2), hashedge_t{.v1 = v1, .v2 = v2, .edge_index = edge_index, .face = face});
 }
 
 const std::optional<img::texture_meta> &mapdata_t::load_image_meta(const std::string_view &name)
@@ -2545,7 +2545,8 @@ static mapbrush_t ParseBrush(parser_t &parser, mapentity_t &entity, texture_def_
         brush.faces.emplace_back(std::move(face.value()));
     }
 
-    bool is_antiregion = brush.faces[0].texname.ends_with("antiregion"), is_region = !is_antiregion && brush.faces[0].texname.ends_with("region");
+    bool is_antiregion = brush.faces[0].texname.ends_with("antiregion"),
+         is_region = !is_antiregion && brush.faces[0].texname.ends_with("region");
 
     // check regionness
     if (is_antiregion) {
@@ -3313,7 +3314,7 @@ void ProcessMapBrushes()
 
         fs::path name = qbsp_options.bsp_path;
         name.replace_extension("expanded.map");
-        
+
         WriteMapBrushMap(name, map.world_entity().mapbrushes, hull);
     }
 }

qbsp/qbsp.cc

@@ -478,6 +478,7 @@ qbsp_settings::qbsp_settings()
           "change the subdivide threshold, in luxels. 0 will disable subdivision entirely"},
       nofill{this, "nofill", false, &debugging_group, "don't perform outside filling"},
       nomerge{this, "nomerge", false, &debugging_group, "don't perform face merging"},
+      noedgereuse{this, "noedgereuse", false, &debugging_group, "don't reuse edges (for debugging software rendering)"},
       noclip{this, "noclip", false, &common_format_group, "don't write clip nodes (Q1-like BSP formats)"},
       noskip{this, "noskip", false, &debugging_group, "don't remove faces with the 'skip' texture"},
       nodetail{this, "nodetail", false, &debugging_group, "treat all detail brushes to structural"},

tests/test_qbsp.cc

@@ -1831,4 +1831,55 @@ TEST_CASE("q1_liquid_software")
 {
     INFO("map with just 1 liquid brush + a 'skip' platform, has render corruption on tyrquake");
     const auto [bsp, bspx, prt] = LoadTestmap("q1_liquid_software.map");
+
+    const qvec3d top_face_point{-56, -56, 8};
+    const qvec3d side_face_point{-56, -72, -8};
+
+    auto *top = BSP_FindFaceAtPoint(&bsp, &bsp.dmodels[0], top_face_point, {0, 0, 1});
+    auto *top_inwater = BSP_FindFaceAtPoint(&bsp, &bsp.dmodels[0], top_face_point, {0, 0, -1});
+
+    auto *side = BSP_FindFaceAtPoint(&bsp, &bsp.dmodels[0], side_face_point, {0, -1, 0});
+    auto *side_inwater = BSP_FindFaceAtPoint(&bsp, &bsp.dmodels[0], side_face_point, {0, 1, 0});
+
+    REQUIRE(top);
+    REQUIRE(top_inwater);
+    REQUIRE(side);
+    REQUIRE(side_inwater);
+
+    // gather edge set used in and out of water.
+    // recall that if edge 5 is from vert 12 to vert 13,
+    // edge -5 is from vert 13 to vert 12.
+
+    // for this test, we are converting directed to undirected
+    // because we want to make sure there's no reuse across in-water and
+    // out-of-water, which breaks software renderers.
+    std::set<int> outwater_undirected_edges;
+    std::set<int> inwater_undirected_edges;
+
+    auto add_face_edges_to_set = [](const mbsp_t &b, const mface_t &face, std::set<int> &set) {
+        for (int i = face.firstedge; i < (face.firstedge + face.numedges); ++i) {
+            int edge = b.dsurfedges.at(i);
+
+            // convert directed to undirected
+            if (edge < 0) {
+                edge = -edge;
+            }
+
+            set.insert(edge);
+        }
+    };
+
+    add_face_edges_to_set(bsp, *top, outwater_undirected_edges);
+    add_face_edges_to_set(bsp, *side, outwater_undirected_edges);
+
+    add_face_edges_to_set(bsp, *top_inwater, inwater_undirected_edges);
+    add_face_edges_to_set(bsp, *side_inwater, inwater_undirected_edges);
+
+    CHECK(7 == outwater_undirected_edges.size());
+    CHECK(7 == inwater_undirected_edges.size());
+
+    // make sure there's no reuse between out-of-water and in-water
+    for (int e : outwater_undirected_edges) {
+        CHECK(inwater_undirected_edges.find(e) == inwater_undirected_edges.end());
+    }
 }