Rebased branch, reduced code duplication, added comments, adjusted formatting.

apps/opencs/view/render/cellborder.cpp

@@ -12,6 +12,11 @@
 #include "../../model/world/cellcoordinates.hpp"
 
 const int CSVRender::CellBorder::CellSize = ESM::Land::REAL_SIZE;
+
+/*
+    The number of vertices per cell border is equal to the number of vertices per edge
+    minus the duplicated corner vertices. An additional vertex to close the loop is NOT needed.
+*/
 const int CSVRender::CellBorder::VertexCount = (ESM::Land::LAND_SIZE * 4) - 4;
 
 
@@ -43,42 +48,45 @@ void CSVRender::CellBorder::buildShape(const ESM::Land& esmLand)
     mBaseNode->removeChild(mBorderGeometry);
     mBorderGeometry = new osg::Geometry();
 
-    // Vertices
     osg::ref_ptr<osg::Vec3Array> vertices = new osg::Vec3Array();
 
     int x = 0;
     int y = 0;
 
-    for (/* */; x < ESM::Land::LAND_SIZE - 1; ++x)
+    /*
+        Traverse the cell border counter-clockwise starting at the SW corner vertex (0, 0).
+        Each loop starts at a corner vertex and ends right before the next corner vertex.
+    */
+    for (; x < ESM::Land::LAND_SIZE - 1; ++x)
         vertices->push_back(osg::Vec3f(scaleToWorld(x), scaleToWorld(y), landData->mHeights[landIndex(x, y)]));
 
     x = ESM::Land::LAND_SIZE - 1;
-    for (/* */; y < ESM::Land::LAND_SIZE - 1; ++y)
+    for (; y < ESM::Land::LAND_SIZE - 1; ++y)
         vertices->push_back(osg::Vec3f(scaleToWorld(x), scaleToWorld(y), landData->mHeights[landIndex(x, y)]));
 
     y = ESM::Land::LAND_SIZE - 1;
-    for (/* */; x > 0; --x)
+    for (; x > 0; --x)
         vertices->push_back(osg::Vec3f(scaleToWorld(x), scaleToWorld(y), landData->mHeights[landIndex(x, y)]));
 
     x = 0;
-    for (/* */; y > 0; --y)
+    for (; y > 0; --y)
         vertices->push_back(osg::Vec3f(scaleToWorld(x), scaleToWorld(y), landData->mHeights[landIndex(x, y)]));
 
     mBorderGeometry->setVertexArray(vertices);
 
-    // Color
     osg::ref_ptr<osg::Vec4Array> colors = new osg::Vec4Array();
     colors->push_back(osg::Vec4f(0.f, 0.5f, 0.f, 1.f));
 
     mBorderGeometry->setColorArray(colors, osg::Array::BIND_PER_PRIMITIVE_SET);
 
-    // Primitive
     osg::ref_ptr<osg::DrawElementsUShort> primitives =
         new osg::DrawElementsUShort(osg::PrimitiveSet::LINE_STRIP, VertexCount + 1);
 
+    // Assign one primitive to each vertex.
     for (size_t i = 0; i < VertexCount; ++i)
         primitives->setElement(i, i);
 
+    // Assign the last primitive to the first vertex to close the loop.
     primitives->setElement(VertexCount, 0);
 
     mBorderGeometry->addPrimitiveSet(primitives);

apps/opencs/view/render/commands.hpp

@@ -3,16 +3,22 @@
 
 #include <QUndoCommand>
 
-namespace ESM
-{
-    struct Land;
-}
-
 namespace CSVRender
 {
     class TerrainSelection;
-    
 
+    /*
+        Current solution to force a redrawing of the terrain-selection grid
+        when undoing/redoing changes in the editor.
+        This only triggers a simple redraw of the grid, so only use it in
+        conjunction with actual data changes which deform the grid.
+
+        Please note that this command needs to be put onto the QUndoStack twice:
+        at the start and at the end of the related terrain manipulation.
+        This makes sure that the grid is always updated after all changes have
+        been undone or redone -- but it also means that the selection is redrawn
+        once at the beginning of either action. Future refinement may solve that.
+    */
     class DrawTerrainSelectionCommand : public QUndoCommand
     {
     private:

apps/opencs/view/render/terrainselection.cpp

@@ -39,135 +39,23 @@ std::vector<std::pair<int, int>> CSVRender::TerrainSelection::getTerrainSelectio
 void CSVRender::TerrainSelection::onlySelect(const std::vector<std::pair<int, int>> &localPositions)
 {
     mSelection = localPositions;
+
     update();
 }
 
 void CSVRender::TerrainSelection::addSelect(const std::vector<std::pair<int, int>>& localPositions, bool toggleInProgress)
 {
-    if (toggleInProgress)
+    handleSelection(localPositions, toggleInProgress, SelectionMethod::AddSelect);
-    {
-        for (auto const& localPos : localPositions)
-        {
-            auto iterTemp = std::find(mTemporarySelection.begin(), mTemporarySelection.end(), localPos);
-            mDraggedOperationFlag = true;
-
-            if (iterTemp == mTemporarySelection.end())
-            {
-                auto iter = std::find(mSelection.begin(), mSelection.end(), localPos);
-                if (iter == mSelection.end())
-                {
-                    mSelection.emplace_back(localPos);
-                }
-            }
-
-            mTemporarySelection.push_back(localPos);
-        }
-    }
-    else if (mDraggedOperationFlag == false)
-    {
-        for (auto const& localPos : localPositions)
-        {
-            const auto iter = std::find(mSelection.begin(), mSelection.end(), localPos);
-            if (iter == mSelection.end())
-            {
-                mSelection.emplace_back(localPos);
-            }
-        }
-    }
-    else
-    {
-        mDraggedOperationFlag = false;
-        mTemporarySelection.clear();
-    }
-    update();
 }
 
 void CSVRender::TerrainSelection::removeSelect(const std::vector<std::pair<int, int>>& localPositions, bool toggleInProgress)
 {
-    if (toggleInProgress)
+    handleSelection(localPositions, toggleInProgress, SelectionMethod::RemoveSelect);
-    {
-        for (auto const& localPos : localPositions)
-        {
-            auto iterTemp = std::find(mTemporarySelection.begin(), mTemporarySelection.end(), localPos);
-            mDraggedOperationFlag = true;
-
-            if (iterTemp == mTemporarySelection.end())
-            {
-                auto iter = std::find(mSelection.begin(), mSelection.end(), localPos);
-                if (iter != mSelection.end())
-                {
-                    mSelection.erase(iter);
-                }
-            }
-
-            mTemporarySelection.push_back(localPos);
-        }
-    }
-    else if (mDraggedOperationFlag == false)
-    {
-        for (auto const& localPos : localPositions)
-        {
-            const auto iter = std::find(mSelection.begin(), mSelection.end(), localPos);
-            if (iter != mSelection.end())
-            {
-                mSelection.erase(iter);
-            }
-        }
-    }
-    else
-    {
-        mDraggedOperationFlag = false;
-        mTemporarySelection.clear();
-    }
-    update();
 }
 
-void CSVRender::TerrainSelection::toggleSelect(const std::vector<std::pair<int, int>> &localPositions, bool toggleInProgress)
+void CSVRender::TerrainSelection::toggleSelect(const std::vector<std::pair<int, int>>& localPositions, bool toggleInProgress)
 {
-    if (toggleInProgress)
+    handleSelection(localPositions, toggleInProgress, SelectionMethod::ToggleSelect);
-    {
-        for(auto const& localPos: localPositions)
-        {
-            auto iterTemp = std::find(mTemporarySelection.begin(), mTemporarySelection.end(), localPos);
-            mDraggedOperationFlag = true;
-
-            if (iterTemp == mTemporarySelection.end())
-            {
-                auto iter = std::find(mSelection.begin(), mSelection.end(), localPos);
-                if (iter != mSelection.end())
-                {
-                    mSelection.erase(iter);
-                }
-                else
-                {
-                    mSelection.emplace_back(localPos);
-                }
-            }
-
-            mTemporarySelection.push_back(localPos);
-        }
-    }
-    else if (mDraggedOperationFlag == false)
-    {
-        for(auto const& localPos: localPositions)
-        {
-            const auto iter = std::find(mSelection.begin(), mSelection.end(), localPos);
-            if (iter != mSelection.end())
-            {
-                mSelection.erase(iter);
-            }
-            else
-            {
-                mSelection.emplace_back(localPos);
-            }
-        }
-    }
-    else
-    {
-        mDraggedOperationFlag = false;
-        mTemporarySelection.clear();
-    }
-    update();
 }
 
 void CSVRender::TerrainSelection::activate()
@@ -310,6 +198,100 @@ void CSVRender::TerrainSelection::drawTextureSelection(const osg::ref_ptr<osg::V
     }
 }
 
+void CSVRender::TerrainSelection::handleSelection(const std::vector<std::pair<int, int>>& localPositions, bool toggleInProgress, SelectionMethod selectionMethod)
+{
+    if (toggleInProgress)
+    {
+        for (auto const& localPos : localPositions)
+        {
+            auto iterTemp = std::find(mTemporarySelection.begin(), mTemporarySelection.end(), localPos);
+            mDraggedOperationFlag = true;
+
+            if (iterTemp == mTemporarySelection.end())
+            {
+                auto iter = std::find(mSelection.begin(), mSelection.end(), localPos);
+
+                switch (selectionMethod)
+                {
+                case SelectionMethod::AddSelect:
+                    if (iter == mSelection.end())
+                    {
+                        mSelection.emplace_back(localPos);
+                    }
+
+                    break;
+                case SelectionMethod::RemoveSelect:
+                    if (iter != mSelection.end())
+                    {
+                        mSelection.erase(iter);
+                    }
+
+                    break;
+                case SelectionMethod::ToggleSelect:
+                    if (iter == mSelection.end())
+                    {
+                        mSelection.emplace_back(localPos);
+                    }
+                    else
+                    {
+                        mSelection.erase(iter);
+                    }
+
+                    break;
+                default: break;
+                }
+            }
+
+            mTemporarySelection.push_back(localPos);
+        }
+    }
+    else if (mDraggedOperationFlag == false)
+    {
+        for (auto const& localPos : localPositions)
+        {
+            const auto iter = std::find(mSelection.begin(), mSelection.end(), localPos);
+
+            switch (selectionMethod)
+            {
+            case SelectionMethod::AddSelect:
+                if (iter == mSelection.end())
+                {
+                    mSelection.emplace_back(localPos);
+                }
+
+                break;
+            case SelectionMethod::RemoveSelect:
+                if (iter != mSelection.end())
+                {
+                    mSelection.erase(iter);
+                }
+
+                break;
+            case SelectionMethod::ToggleSelect:
+                if (iter == mSelection.end())
+                {
+                    mSelection.emplace_back(localPos);
+                }
+                else
+                {
+                    mSelection.erase(iter);
+                }
+
+                break;
+            default: break;
+            }
+        }
+    }
+    else
+    {
+        mDraggedOperationFlag = false;
+
+        mTemporarySelection.clear();
+    }
+
+    update();
+}
+
 bool CSVRender::TerrainSelection::noCell(const std::string& cellId)
 {
     CSMDoc::Document& document = mWorldspaceWidget->getDocument();

apps/opencs/view/render/terrainselection.hpp

@@ -27,6 +27,14 @@ namespace CSVRender
         Shape
     };
 
+    enum class SelectionMethod
+    {
+        OnlySelect,
+        AddSelect,
+        RemoveSelect,
+        ToggleSelect
+    };
+
     /// \brief Class handling the terrain selection data and rendering
     class TerrainSelection
     {
@@ -56,6 +64,8 @@ namespace CSVRender
 
         private:
 
+            void handleSelection(const std::vector<std::pair<int, int>>& localPositions, bool toggleInProgress, SelectionMethod selectionMethod);
+
             bool noCell(const std::string& cellId);
 
             bool noLand(const std::string& cellId);

apps/opencs/view/render/terrainshapemode.cpp

@@ -285,6 +285,8 @@ void CSVRender::TerrainShapeMode::applyTerrainEditChanges()
     sortAndLimitAlteredCells();
 
     undoStack.beginMacro ("Edit shape and normal records");
+
+    // One command at the beginning of the macro for redrawing the terrain-selection grid when undoing the changes.
     undoStack.push(new DrawTerrainSelectionCommand(*mTerrainShapeSelection));
 
     for(CSMWorld::CellCoordinates cellCoordinates: mAlteredCells)
@@ -355,7 +357,9 @@ void CSVRender::TerrainShapeMode::applyTerrainEditChanges()
         }
         pushNormalsEditToCommand(landNormalsNew, document, landTable, cellId);
     }
+    // One command at the end of the macro for redrawing the terrain-selection grid when redoing the changes.
     undoStack.push(new DrawTerrainSelectionCommand(*mTerrainShapeSelection));
+
     undoStack.endMacro();
     clearTransientEdits();
 }
@@ -1039,8 +1043,18 @@ void CSVRender::TerrainShapeMode::handleSelection(int globalSelectionX, int glob
         int selectionX = globalSelectionX;
         int selectionY = globalSelectionY;
 
+        /*
+            The northern and eastern edges don't belong to the current cell.
+            If the corresponding adjacent cell is not loaded, some special handling is necessary to select border vertices.
+        */
         if (xIsAtCellBorder && yIsAtCellBorder)
         {
+            /* 
+                Handle the NW, NE, and SE corner vertices.
+                NW corner: (+1, -1) offset to reach current cell.
+                NE corner: (-1, -1) offset to reach current cell.
+                SE corner: (-1, +1) offset to reach current cell.
+            */
             if (isInCellSelection(globalSelectionX - 1, globalSelectionY - 1)
                 || isInCellSelection(globalSelectionX + 1, globalSelectionY - 1)
                 || isInCellSelection(globalSelectionX - 1, globalSelectionY + 1))