2751: Clean up and refactor Polyhedron (TrenchBroom#2834)

* 2751: clean up and document polyhedron wip * 2751: minor changes * 2751: use forward declarations for Polyhedron where possible, only include it in cpp files * 2751: remove vec3 type alias in Polyhedron * 2751: more polyhedron cleanup and documentation * 2751: minor change, fix cppcheck problem * 2751: fix double semicolons * 2751: ignore build dir * 2751: fix build error * 2751: remove std::set include from Brush.h * 2751: remove some unnecessary includes * 2751: disable PCH on CI builds * 2751: add missing include * 2751: CanMoveVerticesResult should manage its resources properly, says cppcheck * 2751: add missing switchDefault * 2751: self check in assignment * 2751: minor simplification for build instructions * 2751: fix link error in release builds * 2751: replace do / while loops with range base for loops * 2751: replace do / while loops with range based for * 2751: replacing the do / while loop here introduced a change of behaviour, so we revert it * 2751: replace more do / while loops with for loops * 2751: use std::unique_ptr for CanMoveVerticesResult
s3thra · Nov 20, 2019 · 46355e3 · 46355e3
1 parent fefa8ee
commit 46355e3
Show file tree

Hide file tree

Showing 69 changed files with 3,710 additions and 2,075 deletions.
diff --git a/.gitignore b/.gitignore
@@ -5,6 +5,7 @@ Thumbs.db
 
 /cmake-build-debug
 /cmake-build-release
+/cmake-build-relwithdebinfo
 /build-debug
 /build-release
 /build

diff --git a/Build.md b/Build.md
@@ -10,14 +10,7 @@ First, clone the TrenchBroom repository. If you are using the official repositor
 repository by running
 
 ```
-git clone https://github.com/kduske/TrenchBroom.git
-```
-
-TrenchBroom uses a few git submodules for some of its dependencies. To initialize the submodules, issue the following command:
-
-```
-cd TrenchBroom
-git submodule update --init --recursive
+git clone --recursive https://github.com/kduske/TrenchBroom.git
 ```
 
 ## Windows

diff --git a/appveyor.bat b/appveyor.bat
@@ -11,7 +11,7 @@ cppcheck --version
 mkdir cmakebuild
 cd cmakebuild
 
-cmake .. -G"Visual Studio 15 2017" -DCMAKE_PREFIX_PATH="%QT5_INSTALL_DIR%" -T v141 -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS="/WX"
+cmake .. -G"Visual Studio 15 2017" -DCMAKE_PREFIX_PATH="%QT5_INSTALL_DIR%" -T v141 -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS="/WX" -DTB_SUPPRESS_PCH=1
 
 REM  -DCMAKE_CXX_FLAGS=/WX
 

diff --git a/common/CMakeLists.txt b/common/CMakeLists.txt
@@ -720,21 +720,6 @@ set(COMMON_HEADER
         ${COMMON_SOURCE_DIR}/Model/TransformObjectVisitor.h
         ${COMMON_SOURCE_DIR}/Model/World.h
         ${COMMON_SOURCE_DIR}/Model/WorldBoundsIssueGenerator.h
-        ${COMMON_SOURCE_DIR}/Polyhedron.h
-        ${COMMON_SOURCE_DIR}/Polyhedron_BrushGeometryPayload.h
-        ${COMMON_SOURCE_DIR}/Polyhedron_Clip.h
-        ${COMMON_SOURCE_DIR}/Polyhedron_ConvexHull.h
-        ${COMMON_SOURCE_DIR}/Polyhedron_DefaultPayload.h
-        ${COMMON_SOURCE_DIR}/Polyhedron_Edge.h
-        ${COMMON_SOURCE_DIR}/Polyhedron_Face.h
-        ${COMMON_SOURCE_DIR}/Polyhedron_HalfEdge.h
-        ${COMMON_SOURCE_DIR}/Polyhedron_Instantiation.h
-        ${COMMON_SOURCE_DIR}/Polyhedron_Intersect.h
-        ${COMMON_SOURCE_DIR}/Polyhedron_Matcher.h
-        ${COMMON_SOURCE_DIR}/Polyhedron_Misc.h
-        ${COMMON_SOURCE_DIR}/Polyhedron_Queries.h
-        ${COMMON_SOURCE_DIR}/Polyhedron_Subtract.h
-        ${COMMON_SOURCE_DIR}/Polyhedron_Vertex.h
         ${COMMON_SOURCE_DIR}/Renderer/AllocationTracker.h
         ${COMMON_SOURCE_DIR}/Renderer/BoundsGuideRenderer.h
         ${COMMON_SOURCE_DIR}/Renderer/BrushRenderer.h
@@ -1053,6 +1038,23 @@ set(COMMON_HEADER
         ${COMMON_SOURCE_DIR}/Logger.h
         ${COMMON_SOURCE_DIR}/Macros.h
         ${COMMON_SOURCE_DIR}/Notifier.h
+        ${COMMON_SOURCE_DIR}/Polyhedron.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_BrushGeometryPayload.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_Checks.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_Clip.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_ConvexHull.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_CSG.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_DefaultPayload.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_Edge.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_Face.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_Forward.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_HalfEdge.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_Instantiation.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_IO.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_Matcher.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_Misc.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_Queries.h
+        ${COMMON_SOURCE_DIR}/Polyhedron_Vertex.h
         ${COMMON_SOURCE_DIR}/Polyhedron3.h
         ${COMMON_SOURCE_DIR}/Preference.h
         ${COMMON_SOURCE_DIR}/PreferenceManager.h
@@ -1083,7 +1085,7 @@ target_include_directories(common PUBLIC ${COMMON_SOURCE_DIR})
 target_link_libraries(common PUBLIC tinyxml2 vecmath optlite glew miniz freeimage freetype OpenGL::GL Qt5::Widgets)
 
 # use precompiled headers on CMake 3.16 or later
-if (${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.16.0")
+if (NOT TB_SUPPRESS_PCH AND ${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.16.0")
     message(STATUS "Using precompiled headers")
     target_precompile_headers(common PUBLIC
             <algorithm>
@@ -1094,6 +1096,7 @@ if (${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.16.0")
             <cstdlib>
             <ctime>
             <functional>
+            <initializer_list>
             <iterator>
             <iostream>
             <istream>

diff --git a/common/src/AttrString.cpp b/common/src/AttrString.cpp
@@ -82,7 +82,7 @@ namespace TrenchBroom {
 
     void AttrString::lines(LineFunc& func) const {
         for (size_t i = 0; i < m_lines.size(); ++i)
-            func.process(m_lines[i].string, m_lines[i].justify);;
+            func.process(m_lines[i].string, m_lines[i].justify);
     }
 
 

diff --git a/common/src/IO/ObjSerializer.cpp b/common/src/IO/ObjSerializer.cpp
@@ -20,6 +20,7 @@
 #include "ObjSerializer.h"
 
 #include "CollectionUtils.h"
+#include "Polyhedron.h"
 #include "IO/Path.h"
 #include "Model/Brush.h"
 #include "Model/BrushFace.h"

diff --git a/common/src/Model/BoundsIntersectsNodeVisitor.cpp b/common/src/Model/BoundsIntersectsNodeVisitor.cpp
@@ -19,6 +19,7 @@
 
 #include "BoundsIntersectsNodeVisitor.h"
 
+#include "Polyhedron.h"
 #include "Model/Brush.h"
 #include "Model/Entity.h"
 #include "Model/Group.h"

diff --git a/common/src/Model/Brush.cpp b/common/src/Model/Brush.cpp
@@ -21,6 +21,7 @@
 
 #include "CollectionUtils.h"
 #include "Constants.h"
+#include "Polyhedron.h"
 #include "Polyhedron_Matcher.h"
 #include "Model/BrushFace.h"
 #include "Model/BrushGeometry.h"
@@ -49,6 +50,8 @@
 
 #include <algorithm>
 #include <iterator>
+#include <set>
+#include <vector>
 
 namespace TrenchBroom {
     namespace Model {
@@ -166,43 +169,38 @@ namespace TrenchBroom {
         public:
             template <typename I>
             MoveVerticesCallback(const BrushGeometry* geometry, I cur, I end, const vm::vec3& delta) {
-                const auto vertices = Brush::createVertexSet(std::vector<vm::vec3>(cur, end));
+                const auto vertices = std::set<vm::vec3>(cur, end);
                 buildIncidences(geometry, vertices, delta);
             }
 
             MoveVerticesCallback(const BrushGeometry* geometry, const vm::vec3& vertex, const vm::vec3& delta) {
-                VertexSet vertices = Brush::createVertexSet();
-                vertices.insert(vertex);
+                const auto vertices = std::set<vm::vec3>({ vertex });
                 buildIncidences(geometry, vertices, delta);
             }
 
             MoveVerticesCallback(const BrushGeometry* geometry) {
-                buildIncidences(geometry, Brush::createVertexSet(), vm::vec3::zero());
+                buildIncidences(geometry, std::set<vm::vec3>(), vm::vec3::zero());
             }
 
             ~MoveVerticesCallback() override {
                 VectorUtils::clearAndDelete(m_removedFaces);
             }
         private:
-            void buildIncidences(const BrushGeometry* geometry, const VertexSet& verticesToBeMoved, const vm::vec3& delta) {
-                const auto& vertices = geometry->vertices();
-                const auto* firstVertex = vertices.front();
-                const auto* curVertex = firstVertex;
-                do {
+            void buildIncidences(const BrushGeometry* geometry, const std::set<vm::vec3>& verticesToBeMoved, const vm::vec3& delta) {
+                for (const BrushVertex* curVertex : geometry->vertices()) {
                     const auto& position = curVertex->position();
                     if (verticesToBeMoved.count(position)) {
                         m_incidences.insert(std::make_pair(position + delta, collectIncidentFaces(curVertex)));
                     } else {
                         m_incidences.insert(std::make_pair(position, collectIncidentFaces(curVertex)));
                     }
-                    curVertex = curVertex->next();
-                } while (curVertex != firstVertex);
+                }
             }
 
             BrushFaceList collectIncidentFaces(const BrushVertex* vertex) {
                 BrushFaceList result;
-                auto* firstEdge = vertex->leaving();
-                auto* curEdge = firstEdge;
+                const BrushHalfEdge* firstEdge = vertex->leaving();
+                const BrushHalfEdge* curEdge = firstEdge;
                 do {
                     result.push_back(curEdge->face()->payload());
                     curEdge = curEdge->nextIncident();
@@ -617,7 +615,7 @@ namespace TrenchBroom {
 
         bool Brush::hasVertex(const vm::vec3& position, const FloatType epsilon) const {
             ensure(m_geometry != nullptr, "geometry is null");
-            return m_geometry->findVertexByPosition(position, nullptr, epsilon) != nullptr;
+            return m_geometry->findVertexByPosition(position, epsilon) != nullptr;
         }
 
         bool Brush::hasVertices(const std::vector<vm::vec3>& positions, const FloatType epsilon) const {
@@ -761,7 +759,7 @@ namespace TrenchBroom {
             ensure(!vertexPositions.empty(), "no vertex positions");
 
             BrushGeometry testGeometry;
-            const auto vertexSet = Brush::createVertexSet(vertexPositions);
+            const auto vertexSet = std::set<vm::vec3>(std::begin(vertexPositions), std::end(vertexPositions));
 
             for (const auto* vertex : m_geometry->vertices()) {
                 const auto& position = vertex->position();
@@ -779,7 +777,7 @@ namespace TrenchBroom {
             assert(canRemoveVertices(worldBounds, vertexPositions));
 
             BrushGeometry newGeometry;
-            const auto vertexSet = Brush::createVertexSet(vertexPositions);
+            const auto vertexSet = std::set<vm::vec3>(std::begin(vertexPositions), std::end(vertexPositions));
 
             for (const auto* vertex : m_geometry->vertices()) {
                 const auto& position = vertex->position();
@@ -844,7 +842,7 @@ namespace TrenchBroom {
             }
 
             for (const auto& edge : edgePositions) {
-                if (!result.geometry.hasEdge(edge.start() + delta, edge.end() + delta)) {
+                if (!result.geometry->hasEdge(edge.start() + delta, edge.end() + delta)) {
                     return false;
                 }
             }
@@ -887,7 +885,7 @@ namespace TrenchBroom {
             }
 
             for (const auto& face : facePositions) {
-                if (!result.geometry.hasFace(face.vertices() + delta)) {
+                if (!result.geometry->hasFace(face.vertices() + delta)) {
                     return false;
                 }
             }
@@ -915,14 +913,16 @@ namespace TrenchBroom {
             return result;
         }
 
-        Brush::CanMoveVerticesResult::CanMoveVerticesResult(const bool s, const BrushGeometry& g) : success(s), geometry(g) {}
+        Brush::CanMoveVerticesResult::CanMoveVerticesResult(const bool s, BrushGeometry&& g) :
+        success(s),
+        geometry(std::make_unique<BrushGeometry>(std::move(g))) {}
 
         Brush::CanMoveVerticesResult Brush::CanMoveVerticesResult::rejectVertexMove() {
             return CanMoveVerticesResult(false, BrushGeometry());
         }
 
-        Brush::CanMoveVerticesResult Brush::CanMoveVerticesResult::acceptVertexMove(const BrushGeometry& result) {
-            return CanMoveVerticesResult(true, result);
+        Brush::CanMoveVerticesResult Brush::CanMoveVerticesResult::acceptVertexMove(BrushGeometry&& result) {
+            return CanMoveVerticesResult(true, std::move(result));
         }
 
         /*
@@ -959,7 +959,7 @@ namespace TrenchBroom {
                 return CanMoveVerticesResult::rejectVertexMove();
             }
 
-            const auto vertexSet = Brush::createVertexSet(vertexPositions);
+            const auto vertexSet = std::set<vm::vec3>(std::begin(vertexPositions), std::end(vertexPositions));
 
             BrushGeometry remaining;
             BrushGeometry moving;
@@ -984,7 +984,7 @@ namespace TrenchBroom {
 
             // Special case, takes care of the first column.
             if (moving.vertexCount() == vertexCount()) {
-                return CanMoveVerticesResult::acceptVertexMove(result);
+                return CanMoveVerticesResult::acceptVertexMove(std::move(result));
             }
 
             // Will vertices be removed?
@@ -1005,7 +1005,7 @@ namespace TrenchBroom {
             // One of the remaining two ok cases?
             if ((moving.point() && remaining.polygon()) ||
                 (moving.edge() && remaining.edge())) {
-                return CanMoveVerticesResult::acceptVertexMove(result);
+                return CanMoveVerticesResult::acceptVertexMove(std::move(result));
             }
 
             // Invert if necessary.
@@ -1032,7 +1032,7 @@ namespace TrenchBroom {
                 }
             }
 
-            return CanMoveVerticesResult::acceptVertexMove(result);
+            return CanMoveVerticesResult::acceptVertexMove(std::move(result));
         }
 
         void Brush::doMoveVertices(const vm::bbox3& worldBounds, const std::vector<vm::vec3>& vertexPositions, const vm::vec3& delta, const bool uvLock) {
@@ -1041,7 +1041,7 @@ namespace TrenchBroom {
             assert(canMoveVertices(worldBounds, vertexPositions, delta));
 
             BrushGeometry newGeometry;
-            const auto vertexSet = Brush::createVertexSet(vertexPositions);
+            const auto vertexSet = std::set<vm::vec3>(std::begin(vertexPositions), std::end(vertexPositions));
 
             for (auto* vertex : m_geometry->vertices()) {
                 const auto& position = vertex->position();
@@ -1168,22 +1168,22 @@ namespace TrenchBroom {
             rebuildGeometry(worldBounds);
         }
 
-        Brush::VertexSet Brush::createVertexSet(const std::vector<vm::vec3>& vertices) {
-            return VertexSet(std::begin(vertices), std::end(vertices));
-        }
-
         BrushList Brush::subtract(const ModelFactory& factory, const vm::bbox3& worldBounds, const String& defaultTextureName, const BrushList& subtrahends) const {
-            auto result = std::list<BrushGeometry>{*m_geometry};
+            auto result = std::vector<BrushGeometry>{*m_geometry};
 
             for (auto* subtrahend : subtrahends) {
-                auto nextResults = std::list<BrushGeometry>();
+                auto nextResults = std::vector<BrushGeometry>();
 
                 for (const BrushGeometry& fragment : result) {
-                    const auto subFragments = fragment.subtract(*subtrahend->m_geometry);
-                    ListUtils::append(nextResults, subFragments);
+                    auto subFragments = fragment.subtract(*subtrahend->m_geometry);
+
+                    nextResults.reserve(nextResults.size() + subFragments.size());
+                    for (auto& subFragment : subFragments) {
+                        nextResults.push_back(std::move(subFragment));
+                    }
                 }
 
-                result = nextResults;
+                result = std::move(nextResults);
             }
 
             BrushList brushes;

diff --git a/common/src/Model/Brush.h b/common/src/Model/Brush.h
@@ -21,9 +21,9 @@
 #define TrenchBroom_Brush
 
 #include "TrenchBroom.h"
+#include "Macros.h"
 #include "Hit.h"
 #include "ProjectingSequence.h"
-#include "Macros.h"
 #include "Model/BrushGeometry.h"
 #include "Model/Node.h"
 #include "Model/Object.h"
@@ -32,7 +32,7 @@
 #include <vecmath/vec.h>
 #include <vecmath/polygon.h>
 
-#include <set>
+#include <memory>
 #include <vector>
 
 template <typename P>
@@ -68,8 +68,6 @@ namespace TrenchBroom {
             class MoveVerticesCallback;
             using RemoveVertexCallback = MoveVerticesCallback;
             class QueryCallback;
-
-            using VertexSet = std::set<vm::vec3>;
         public:
             using VertexList = ProjectingSequence<BrushVertexList, ProjectToVertex>;
             using EdgeList = ProjectingSequence<BrushEdgeList, ProjectToEdge>;
@@ -196,14 +194,12 @@ namespace TrenchBroom {
             struct CanMoveVerticesResult {
             public:
                 bool success;
-                BrushGeometry geometry;
-
+                std::unique_ptr<BrushGeometry> geometry;
             private:
-                CanMoveVerticesResult(bool s, const BrushGeometry& g);
-
+                CanMoveVerticesResult(bool s, BrushGeometry&& g);
             public:
                 static CanMoveVerticesResult rejectVertexMove();
-                static CanMoveVerticesResult acceptVertexMove(const BrushGeometry& result);
+                static CanMoveVerticesResult acceptVertexMove(BrushGeometry&& result);
             };
 
             CanMoveVerticesResult doCanMoveVertices(const vm::bbox3& worldBounds, const std::vector<vm::vec3>& vertexPositions, vm::vec3 delta, bool allowVertexRemoval) const;
@@ -237,8 +233,6 @@ namespace TrenchBroom {
              */
             void applyUVLock(const PolyhedronMatcher<BrushGeometry>& matcher, BrushFaceGeometry* left, BrushFaceGeometry* right);
             void doSetNewGeometry(const vm::bbox3& worldBounds, const PolyhedronMatcher<BrushGeometry>& matcher, const BrushGeometry& newGeometry, bool uvLock = false);
-
-            static VertexSet createVertexSet(const std::vector<vm::vec3>& vertices = std::vector<vm::vec3>(0));
         public:
             // CSG operations
             /**

diff --git a/common/src/Model/BrushBuilder.cpp b/common/src/Model/BrushBuilder.cpp
@@ -20,7 +20,7 @@
 #include "BrushBuilder.h"
 
 #include "Ensure.h"
-#include "Polyhedron3.h"
+#include "Polyhedron.h"
 #include "Model/Brush.h"
 #include "Model/BrushFace.h"
 #include "Model/ModelFactory.h"