qbsp: use unique_ptr for node_t::facelist

include/qbsp/csg4.hh

@@ -24,6 +24,7 @@
 #include <common/qvec.hh>
 
 #include <list>
+#include <memory>
 #include <tuple>
 #include <vector>
 
@@ -31,10 +32,10 @@ struct bspbrush_t;
 struct face_t;
 struct side_t;
 
-face_t *NewFaceFromFace(const face_t *in);
+std::unique_ptr<face_t> NewFaceFromFace(const face_t *in);
-face_t *CopyFace(const face_t *in);
+std::unique_ptr<face_t> CopyFace(const face_t* in);
-face_t *MirrorFace(const face_t *face);
+std::unique_ptr<face_t> MirrorFace(const face_t *face);
-std::tuple<face_t *, face_t *> SplitFace(face_t *in, const qplane3d &split);
+std::tuple<std::unique_ptr<face_t>, std::unique_ptr<face_t>> SplitFace(std::unique_ptr<face_t> in, const qplane3d &split);
 void UpdateFaceSphere(face_t *in);
 bool BrushGE(const bspbrush_t &a, const bspbrush_t &b);
 std::vector<std::unique_ptr<bspbrush_t>> ChopBrushes(const std::vector<std::unique_ptr<bspbrush_t>> &input);

include/qbsp/merge.hh

@@ -22,10 +22,10 @@
 #pragma once
 
 #include <list>
+#include <memory>
 
 struct face_t;
 struct node_t;
 
 void MergeFaceToList(face_t *face, std::list<face_t *> &list);
-std::list<face_t *> MergeFaceList(std::list<face_t *> input);
+std::list<std::unique_ptr<face_t>> MergeFaceList(std::list<std::unique_ptr<face_t>> input);
-void MergeAll(node_t *headnode);

include/qbsp/qbsp.hh

@@ -367,7 +367,7 @@ struct node_t
     int firstface; // decision node only
     int numfaces; // decision node only
     twosided<std::unique_ptr<node_t>> children; // children[0] = front side, children[1] = back side of plane. only valid for decision nodes
-    std::list<face_t *> facelist; // decision nodes only, list for both sides
+    std::list<std::unique_ptr<face_t>> facelist; // decision nodes only, list for both sides
     side_t *side; // the side that created the node
 
     // information for leafs

qbsp/csg4.cc

@@ -47,9 +47,9 @@ Duplicates the non point information of a face, used by SplitFace and
 MergeFace.
 ==================
 */
-face_t *NewFaceFromFace(const face_t *in)
+std::unique_ptr<face_t> NewFaceFromFace(const face_t *in)
 {
-    face_t *newf = new face_t{};
+    auto newf = std::unique_ptr<face_t>(new face_t{});
 
     newf->planenum = in->planenum;
     newf->texinfo = in->texinfo;
@@ -63,9 +63,9 @@ face_t *NewFaceFromFace(const face_t *in)
     return newf;
 }
 
-face_t* CopyFace(const face_t* in)
+std::unique_ptr<face_t> CopyFace(const face_t* in)
 {
-    face_t *temp = NewFaceFromFace(in);
+    auto temp = NewFaceFromFace(in);
     temp->w = in->w;
     return temp;
 }
@@ -87,7 +87,7 @@ SplitFace
 Frees in. Returns {front, back}
 ==================
 */
-std::tuple<face_t *, face_t *> SplitFace(face_t *in, const qplane3d &split)
+std::tuple<std::unique_ptr<face_t>, std::unique_ptr<face_t>> SplitFace(std::unique_ptr<face_t> in, const qplane3d &split)
 {
     if (in->w.size() < 0)
         Error("Attempting to split freed face");
@@ -96,40 +96,37 @@ std::tuple<face_t *, face_t *> SplitFace(face_t *in, const qplane3d &split)
     double dot = split.distance_to(in->origin);
     if (dot > in->radius) {
         // all in front
-        return {in, nullptr};
+        return {std::move(in), nullptr};
     } else if (dot < -in->radius) {
         // all behind
-        return {nullptr, in};
+        return {nullptr, std::move(in)};
     }
 
     auto [front_winding, back_winding] = in->w.clip(split, options.epsilon.value(), true);
 
     if (front_winding && !back_winding) {
         // all in front
-        return {in, nullptr};
+        return {std::move(in), nullptr};
     } else if (back_winding && !front_winding) {
         // all behind
-        return {nullptr, in};
+        return {nullptr, std::move(in)};
     }
 
-    face_t *new_front = NewFaceFromFace(in);
+    auto new_front = NewFaceFromFace(in.get());
     new_front->w = std::move(front_winding.value());
 
-    face_t *new_back = NewFaceFromFace(in);
+    auto new_back = NewFaceFromFace(in.get());
     new_back->w = std::move(back_winding.value());
 
     if (new_front->w.size() > MAXEDGES || new_back->w.size() > MAXEDGES)
         FError("Internal error: numpoints > MAXEDGES");
 
-    /* free the original face now that it is represented by the fragments */
+    return {std::move(new_front), std::move(new_back)};
-    delete in;
-
-    return {new_front, new_back};
 }
 
-face_t *MirrorFace(const face_t *face)
+std::unique_ptr<face_t> MirrorFace(const face_t *face)
 {
-    face_t *newface = NewFaceFromFace(face);
+    auto newface = NewFaceFromFace(face);
     newface->w = face->w.flip();
     newface->planeside = static_cast<planeside_t>(face->planeside ^ 1);
 

qbsp/exportobj.cc

@@ -131,8 +131,8 @@ static void ExportObj_Nodes_r(const node_t *node, std::vector<const face_t *> *d
         return;
     }
 
-    for (face_t *face : node->facelist) {
+    for (auto &face : node->facelist) {
-        dest->push_back(face);
+        dest->push_back(face.get());
     }
 
     ExportObj_Nodes_r(node->children[0].get(), dest);

qbsp/faces.cc

@@ -48,7 +48,7 @@ static bool ShouldOmitFace(face_t *f)
 
 static void MergeNodeFaces (node_t *node)
 {
-    node->facelist = MergeFaceList(node->facelist);
+    node->facelist = MergeFaceList(std::move(node->facelist));
 }
 
 /*
@@ -59,13 +59,12 @@ If the face is >256 in either texture direction, carve a valid sized
 piece off and insert the remainder in the next link
 ===============
 */
-std::list<face_t *> SubdivideFace(face_t *f)
+std::list<std::unique_ptr<face_t>> SubdivideFace(std::unique_ptr<face_t> f)
 {
     vec_t mins, maxs;
     vec_t v;
     int axis;
     qbsp_plane_t plane;
-    face_t *front, *back;
     const maptexinfo_t *tex;
     vec_t subdiv;
     vec_t extent;
@@ -75,7 +74,9 @@ std::list<face_t *> SubdivideFace(face_t *f)
     tex = &map.mtexinfos.at(f->texinfo);
 
     if (tex->flags.is_skip || tex->flags.is_hint || !options.target_game->surf_is_subdivided(tex->flags)) {
-        return {f};
+        std::list<std::unique_ptr<face_t>> result;
+        result.push_back(std::move(f));
+        return result;
     }
     // subdivision is pretty much pointless other than because of lightmap block limits
     // one lightmap block will always be added at the end, for smooth interpolation
@@ -95,14 +96,15 @@ std::list<face_t *> SubdivideFace(face_t *f)
     // the bsp is possibly going to be used in both engines that support scaling and those that do not. this means we
     // always over-estimate by 16 rather than 1<<lmscale
 
-    std::list<face_t *> surfaces{f};
+    std::list<std::unique_ptr<face_t>> surfaces;
+    surfaces.push_back(std::move(f));
 
     for (axis = 0; axis < 2; axis++) {
         // we'll transfer faces that are chopped down to size to this list
-        std::list<face_t *> chopped;
+        std::list<std::unique_ptr<face_t>> chopped;
 
         while (!surfaces.empty()) {
-            f = surfaces.front();
+            f = std::move(surfaces.front());
             surfaces.pop_front();
 
             mins = VECT_MAX;
@@ -122,7 +124,7 @@ std::list<face_t *> SubdivideFace(face_t *f)
             //          extent = maxs - mins;
             if (extent <= subdiv) {
                 // this face is already good
-                chopped.push_back(f);
+                chopped.push_back(std::move(f));
                 continue;
             }
 
@@ -138,17 +140,19 @@ std::list<face_t *> SubdivideFace(face_t *f)
             //                plane.dist = (mins + subdiv) / v;
             plane.dist = (mins + subdiv - 16) / v;
 
-            std::tie(front, back) = SplitFace(f, plane);
+            std::unique_ptr<face_t> front;
+            std::unique_ptr<face_t> back;
+            std::tie(front, back) = SplitFace(std::move(f), plane);
             if (!front || !back) {
                 //logging::print("didn't split\n");
                 // FError("Didn't split the polygon");
             }
 
             if (front) {
-                surfaces.push_back(front);
+                surfaces.push_back(std::move(front));
             }
             if (back) {
-                chopped.push_front(back);
+                chopped.push_front(std::move(back));
             }
         }
 
@@ -163,14 +167,14 @@ std::list<face_t *> SubdivideFace(face_t *f)
 
 static void SubdivideNodeFaces(node_t *node)
 {
-    std::list<face_t *> result;
+    std::list<std::unique_ptr<face_t>> result;
 
     // subdivide each face and push the results onto subdivided
-    for (face_t *face : node->facelist) {
+    for (auto &face : node->facelist) {
-        result.splice(result.end(), SubdivideFace(face));
+        result.splice(result.end(), SubdivideFace(std::move(face)));
     }
 
-    node->facelist = result;
+    node->facelist = std::move(result);
 }
 
 //===========================================================================
@@ -347,8 +351,8 @@ static int MakeFaceEdges_r(mapentity_t *entity, node_t *node, int progress)
     if (node->planenum == PLANENUM_LEAF)
         return progress;
 
-    for (face_t *f : node->facelist) {
+    for (auto &f : node->facelist) {
-        FindFaceEdges(entity, f);
+        FindFaceEdges(entity, f.get());
     }
 
     progress = MakeFaceEdges_r(entity, node->children[0].get(), progress);
@@ -421,11 +425,11 @@ static void GrowNodeRegion(mapentity_t *entity, node_t *node)
 
     node->firstface = static_cast<int>(map.bsp.dfaces.size());
 
-    for (face_t *face : node->facelist) {
+    for (auto &face : node->facelist) {
         //Q_assert(face->planenum == node->planenum);
 
         // emit a region
-        EmitFace(entity, face);
+        EmitFace(entity, face.get());
     }
 
     node->numfaces = static_cast<int>(map.bsp.dfaces.size()) - node->firstface;
@@ -456,8 +460,8 @@ static void CountData_r(mapentity_t *entity, node_t *node, size_t &facesCount, s
     if (node->planenum == PLANENUM_LEAF)
         return;
 
-    for (face_t *f : node->facelist) {
+    for (auto &f : node->facelist) {
-        CountFace(entity, f, facesCount, vertexesCount);
+        CountFace(entity, f.get(), facesCount, vertexesCount);
     }
 
     CountData_r(entity, node->children[0].get(), facesCount, vertexesCount);
@@ -509,7 +513,7 @@ Adds the given face to the markfaces lists of all descendant leafs of `node`.
 fixme-brushbsp: all leafs in a cluster can share the same marksurfaces, right?
 ================
 */
-static void AddMarksurfaces_r(face_t *face, face_t *face_copy, node_t *node)
+static void AddMarksurfaces_r(face_t *face, std::unique_ptr<face_t> face_copy, node_t *node)
 {
     if (node->planenum == PLANENUM_LEAF) {
         node->markfaces.push_back(face);
@@ -519,12 +523,12 @@ static void AddMarksurfaces_r(face_t *face, face_t *face_copy, node_t *node)
     const auto lock = std::lock_guard(map_planes_lock);
     const qbsp_plane_t &splitplane = map.planes.at(node->planenum);
 
-    auto [frontFragment, backFragment] = SplitFace(face_copy, splitplane);
+    auto [frontFragment, backFragment] = SplitFace(std::move(face_copy), splitplane);
     if (frontFragment) {
-        AddMarksurfaces_r(face, frontFragment, node->children[0].get());
+        AddMarksurfaces_r(face, std::move(frontFragment), node->children[0].get());
     }
     if (backFragment) {
-        AddMarksurfaces_r(face, backFragment, node->children[1].get());
+        AddMarksurfaces_r(face, std::move(backFragment), node->children[1].get());
     }
 }
 
@@ -542,16 +546,16 @@ void MakeMarkFaces(mapentity_t* entity, node_t* node)
     }
 
     // for the faces on this splitting node..
-    for (face_t *face : node->facelist) {
+    for (auto &face : node->facelist) {
         // add this face to all descendant leafs it touches
         
         // make a copy we can clip
-        face_t *face_copy = CopyFace(face);
+        auto face_copy = CopyFace(face.get());
 
         if (face->planeside == 0) {
-            AddMarksurfaces_r(face, face_copy, node->children[0].get());
+            AddMarksurfaces_r(face.get(), std::move(face_copy), node->children[0].get());
         } else {
-            AddMarksurfaces_r(face, face_copy, node->children[1].get());
+            AddMarksurfaces_r(face.get(), std::move(face_copy), node->children[1].get());
         }
     }
 
@@ -576,13 +580,13 @@ Typically, we're in an empty leaf and the other side of the portal is a solid wa
 see also FindPortalSide which populates p->side
 ============
 */
-static face_t *FaceFromPortal(portal_t *p, int pside)
+static std::unique_ptr<face_t> FaceFromPortal(portal_t *p, int pside)
 {
     side_t *side = p->side;
     if (!side)
         return nullptr;	// portal does not bridge different visible contents
 
-    face_t *f = new face_t{};
+    auto f = std::unique_ptr<face_t>(new face_t{});
 
     f->texinfo = side->texinfo;
     f->planenum = side->planenum;
@@ -621,7 +625,7 @@ static face_t *FaceFromPortal(portal_t *p, int pside)
         f->contents = p->nodes[0]->contents;
     }
 
-    UpdateFaceSphere(f);
+    UpdateFaceSphere(f.get());
 
     return f;
 }
@@ -670,12 +674,12 @@ static void MakeFaces_r(node_t *node, makefaces_stats_t& stats)
         // 1 means node is on the back side of planenum
         s = (p->nodes[1] == node);
 
-        face_t *f = FaceFromPortal(p, s);
+        std::unique_ptr<face_t> f = FaceFromPortal(p, s);
         if (f)
         {
             stats.c_nodefaces++;
-            p->face[s] = f;
+            p->face[s] = f.get();
-            p->onnode->facelist.push_back(f);
+            p->onnode->facelist.push_back(std::move(f));
         }
     }
 }

qbsp/merge.cc

@@ -59,10 +59,9 @@ Returns NULL if the faces couldn't be merged, or the new face.
 The originals will NOT be freed.
 =============
 */
-static face_t *TryMerge(face_t *f1, face_t *f2)
+static std::unique_ptr<face_t> TryMerge(const face_t *f1, const face_t *f2)
 {
     qvec3d p1, p2, p3, p4, back;
-    face_t *newf;
     int i, j, k, l;
     qvec3d normal, delta, planenormal;
     vec_t dot;
@@ -133,7 +132,7 @@ static face_t *TryMerge(face_t *f1, face_t *f2)
         return NULL;
     }
 
-    newf = NewFaceFromFace(f1);
+    std::unique_ptr<face_t> newf = NewFaceFromFace(f1);
 
     // copy first polygon
     if (keep2)
@@ -153,7 +152,7 @@ static face_t *TryMerge(face_t *f1, face_t *f2)
         newf->w.push_back(f2->w[l]);
     }
 
-    UpdateFaceSphere(newf);
+    UpdateFaceSphere(newf.get());
 
     return newf;
 }
@@ -163,27 +162,25 @@ static face_t *TryMerge(face_t *f1, face_t *f2)
 MergeFaceToList
 ===============
 */
-void MergeFaceToList(face_t *face, std::list<face_t *> &list)
+void MergeFaceToList(std::unique_ptr<face_t> face, std::list<std::unique_ptr<face_t>> &list)
 {
     for (auto it = list.begin(); it != list.end();) {
 #ifdef PARANOID
         CheckColinear(f);
 #endif
-        face_t *newf = TryMerge(face, *it);
+        std::unique_ptr<face_t> newf = TryMerge(face.get(), it->get());
 
         if (newf) {
-            delete face;
-            delete *it;
             list.erase(it);
             // restart, now trying to merge `newf` into the list
-            face = newf;
+            face = std::move(newf);
             it = list.begin();
         } else {
             it++;
         }
     }
 
-    list.emplace_back(face);
+    list.emplace_back(std::move(face));
 }
 
 /*
@@ -191,62 +188,13 @@ void MergeFaceToList(face_t *face, std::list<face_t *> &list)
 MergeFaceList
 ===============
 */
-std::list<face_t *> MergeFaceList(std::list<face_t *> input)
+std::list<std::unique_ptr<face_t>> MergeFaceList(std::list<std::unique_ptr<face_t>> input)
 {
-    std::list<face_t *> result;
+    std::list<std::unique_ptr<face_t>> result;
 
-    for (face_t * face : input) {
+    for (auto &face : input) {
-        MergeFaceToList(face, result);
+        MergeFaceToList(std::move(face), result);
     }
 
     return result;
 }
-
-#include <tbb/parallel_for_each.h>
-
-static void CollectNodes_R(node_t *node, std::vector<node_t *> &allnodes)
-{
-    allnodes.push_back(node);
-
-    if (node->planenum == PLANENUM_LEAF) {
-        return;
-    }
-
-    CollectNodes_R(node->children[0].get(), allnodes);
-    CollectNodes_R(node->children[1].get(), allnodes);
-}
-
-/*
-============
-MergeAll
-============
-*/
-void MergeAll(node_t *headnode)
-{
-    std::atomic<int> mergefaces = 0, premergefaces = 0;
-
-    logging::print(logging::flag::PROGRESS, "---- {} ----\n", __func__);
-
-    std::vector<node_t *> allnodes;
-    CollectNodes_R(headnode, allnodes);
-
-    tbb::parallel_for_each(allnodes, [&](node_t *node) {
-        const size_t before = node->facelist.size();
-
-        std::list<face_t *> merged_pre_subdivide = MergeFaceList(node->facelist);
-
-        // re-subdivide after merging
-        node->facelist.clear();
-        for (face_t *face : merged_pre_subdivide) {
-            node->facelist.splice(node->facelist.end(), SubdivideFace(face));
-        }
-
-        const size_t after = node->facelist.size();
-
-        premergefaces += before;
-        mergefaces += after;
-    });
-
-    logging::print(logging::flag::STAT, "     {:8} mergefaces (from {}; {:.0}% merged)\n", mergefaces, premergefaces,
-        (static_cast<double>(mergefaces) / premergefaces) * 100.);
-}

qbsp/tjunc.cc

@@ -366,7 +366,7 @@ static void tjunc_count_r(node_t *node)
     if (node->planenum == PLANENUM_LEAF)
         return;
 
-    for (face_t *f : node->facelist) {
+    for (auto &f : node->facelist) {
         cWVerts += f->w.size();
     }
 
@@ -379,8 +379,8 @@ static void tjunc_find_r(node_t *node)
     if (node->planenum == PLANENUM_LEAF)
         return;
 
-    for (face_t *f : node->facelist) {
+    for (auto &f : node->facelist) {
-        AddFaceEdges(f);
+        AddFaceEdges(f.get());
     }
 
     tjunc_find_r(node->children[0].get());
@@ -392,8 +392,8 @@ static void tjunc_fix_r(node_t *node)
     if (node->planenum == PLANENUM_LEAF)
         return;
 
-    for (face_t *face : node->facelist) {
+    for (auto &face : node->facelist) {
-        FixFaceEdges(face);
+        FixFaceEdges(face.get());
     }
 
     tjunc_fix_r(node->children[0].get());

qbsp/tree.cc

@@ -52,10 +52,7 @@ static void ConvertNodeToLeaf(node_t *node, const contentflags_t &contents)
     for (int i = 0; i < 2; ++i) {
         node->children[i] = nullptr;
     }
-    for (auto *face : node->facelist) {
+    node->facelist.clear();
-        delete face;
-    }
-    node->facelist = {};
 
     node->contents = contents;