Merge branch 'Terrain-Selection-Issues' into 'master'

Various fixes for the editor's "Terrain land editing" mode (fixes #5473, #6022, #6023, #6024, #6035, #6036)

Closes #6036, #6035, #6024, #6023, #6022, and #5473

See merge request OpenMW/openmw!840

CHANGELOG.md

@@ -64,6 +64,7 @@
     Bug #5452: Autowalk is being included in savegames
     Bug #5469: Local map is reset when re-entering certain cells
     Bug #5472: Mistify mod causes CTD in 0.46 on Mac
+    Bug #5473: OpenMW-CS: Cell border lines don't update properly on terrain change
     Bug #5479: NPCs who should be walking around town are standing around without walking
     Bug #5484: Zero value items shouldn't be able to be bought or sold for 1 gold
     Bug #5485: Intimidate doesn't increase disposition on marginal wins
@@ -124,7 +125,11 @@
     Bug #5995: NiUVController doesn't calculate the UV offset properly
     Bug #6007: Crash when ending cutscene is playing
     Bug #6016: Greeting interrupts Fargoth's sneak-walk
+    Bug #6022: OpenMW-CS: Terrain selection is not updated when undoing/redoing terrain changes
+    Bug #6023: OpenMW-CS: Clicking on a reference in "Terrain land editing" mode discards corresponding select/edit action
     Bug #6028: Particle system controller values are incorrectly used
+    Bug #6035: OpenMW-CS: Circle brush in "Terrain land editing" mode sometimes includes vertices outside its radius
+    Bug #6036: OpenMW-CS: Terrain selection at the border of cells omits certain corner vertices
     Bug #6043: Actor can have torch missing when torch animation is played
     Bug #6047: Mouse bindings can be triggered during save loading
     Feature #390: 3rd person look "over the shoulder"
@@ -166,6 +171,7 @@
     Feature #5814: Bsatool should be able to create BSA archives, not only to extract it
     Feature #5828: Support more than 8 lights
     Feature #5910: Fall back to delta time when physics can't keep up
+    Feature #6024: OpenMW-CS: Selecting terrain in "Terrain land editing" should support "Add to selection" and "Remove from selection" modes
     Feature #6033: Include pathgrid to navigation mesh
     Feature #6034: Find path based on area cost depending on NPC stats
     Task #5480: Drop Qt4 support

CHANGELOG_PR.md

@@ -38,9 +38,15 @@ Editor Bug Fixes:
 - Disabled record sorting in Topic and Journal Info tables, implemented drag-move for records (#4357)
 - Topic and Journal Info records can now be cloned with a different parent Topic/Journal Id (#4363)
 - Verifier no longer checks for alleged 'race' entries in clothing body parts (#5400)
+- Cell borders are now properly redrawn when undoing/redoing terrain changes (#5473)
 - Loading mods now keeps the master index (#5675)
 - Flicker and crashing on XFCE4 fixed (#5703)
 - Collada models render properly in the Editor (#5713)
+- Terrain-selection grid is now properly updated when undoing/redoing terrain changes (#6022)
+- Tool outline and select/edit actions in "Terrain land editing" mode now ignore references (#6023)
+- Primary-select and secondary-select actions in "Terrain land editing" mode now behave like in "Instance editing" mode (#6024)
+- Using the circle brush to select terrain in the "Terrain land editing" mode no longer selects vertices outside the circle (#6035)
+- Vertices at the NW and SE corners of a cell can now also be selected in "Terrain land editing" mode if the adjacent cells aren't loaded yet (#6036)
 
 Miscellaneous:
 - Prevent save-game bloating by using an appropriate fog texture format (#5108)

apps/opencs/CMakeLists.txt

@@ -89,7 +89,7 @@ opencs_units (view/render
     scenewidget worldspacewidget pagedworldspacewidget unpagedworldspacewidget
     previewwidget editmode instancemode instanceselectionmode instancemovemode
     orbitcameramode pathgridmode selectionmode pathgridselectionmode cameracontroller
-    cellwater terraintexturemode actor terrainselection terrainshapemode brushdraw
+    cellwater terraintexturemode actor terrainselection terrainshapemode brushdraw commands
     )
 
 opencs_units_noqt (view/render

apps/opencs/view/render/cellborder.cpp

@@ -2,7 +2,6 @@
 
 #include <osg/Group>
 #include <osg/PositionAttitudeTransform>
-#include <osg/Geode>
 #include <osg/Geometry>
 #include <osg/PrimitiveSet>
 
@@ -13,15 +12,23 @@
 #include "../../model/world/cellcoordinates.hpp"
 
 const int CSVRender::CellBorder::CellSize = ESM::Land::REAL_SIZE;
-const int CSVRender::CellBorder::VertexCount = (ESM::Land::LAND_SIZE * 4) - 3;
+
+/*
+    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;
 
 
 CSVRender::CellBorder::CellBorder(osg::Group* cellNode, const CSMWorld::CellCoordinates& coords)
     : mParentNode(cellNode)
 {
+    mBorderGeometry = new osg::Geometry();
+    
     mBaseNode = new osg::PositionAttitudeTransform();
     mBaseNode->setNodeMask(Mask_CellBorder);
     mBaseNode->setPosition(osg::Vec3f(coords.getX() * CellSize, coords.getY() * CellSize, 10));
+    mBaseNode->addChild(mBorderGeometry);
 
     mParentNode->addChild(mBaseNode);
 }
@@ -38,56 +45,59 @@ void CSVRender::CellBorder::buildShape(const ESM::Land& esmLand)
     if (!landData)
         return;
 
-    osg::ref_ptr<osg::Geometry> geometry = new osg::Geometry();
+    mBaseNode->removeChild(mBorderGeometry);
+    mBorderGeometry = new osg::Geometry();
 
-    // Vertices
     osg::ref_ptr<osg::Vec3Array> vertices = new osg::Vec3Array();
 
-    int x = 0, y = 0;
-    for (; x < ESM::Land::LAND_SIZE; ++x)
+    int x = 0;
+    int y = 0;
+
+    /*
+        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; ++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)]));
 
-    geometry->setVertexArray(vertices);
+    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));
 
-    geometry->setColorArray(colors, osg::Array::BIND_PER_PRIMITIVE_SET);
+    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);
+        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);
 
-    geometry->addPrimitiveSet(primitives);
-    geometry->getOrCreateStateSet()->setMode(GL_LIGHTING, osg::StateAttribute::OFF);
-
+    mBorderGeometry->addPrimitiveSet(primitives);
+    mBorderGeometry->getOrCreateStateSet()->setMode(GL_LIGHTING, osg::StateAttribute::OFF);
 
-    osg::ref_ptr<osg::Geode> geode = new osg::Geode();
-    geode->addDrawable(geometry);
-    mBaseNode->addChild(geode);
+    mBaseNode->addChild(mBorderGeometry);
 }
 
 size_t CSVRender::CellBorder::landIndex(int x, int y)
 {
-    return y * ESM::Land::LAND_SIZE + x;
+    return static_cast<size_t>(y) * ESM::Land::LAND_SIZE + x;
 }
 
 float CSVRender::CellBorder::scaleToWorld(int value)

apps/opencs/view/render/cellborder.hpp

@@ -7,6 +7,7 @@
 
 namespace osg
 {
+    class Geometry;
     class Group;
     class PositionAttitudeTransform;
 }
@@ -47,7 +48,7 @@ namespace CSVRender
 
         osg::Group* mParentNode;
         osg::ref_ptr<osg::PositionAttitudeTransform> mBaseNode;
-
+        osg::ref_ptr<osg::Geometry> mBorderGeometry;
     };
 }
 

apps/opencs/view/render/commands.cpp

@@ -0,0 +1,19 @@
+#include "commands.hpp"
+
+#include <components/esm/loadland.hpp>
+
+#include "terrainselection.hpp"
+
+CSVRender::DrawTerrainSelectionCommand::DrawTerrainSelectionCommand(TerrainSelection& terrainSelection, QUndoCommand* parent)
+    : mTerrainSelection(terrainSelection)
+{ }
+
+void CSVRender::DrawTerrainSelectionCommand::redo()
+{
+    mTerrainSelection.update();
+}
+
+void CSVRender::DrawTerrainSelectionCommand::undo()
+{
+    mTerrainSelection.update();
+}

apps/opencs/view/render/commands.hpp

@@ -0,0 +1,35 @@
+#ifndef CSV_RENDER_COMMANDS_HPP
+#define CSV_RENDER_COMMANDS_HPP
+
+#include <QUndoCommand>
+
+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:
+        TerrainSelection& mTerrainSelection;
+
+    public:
+        DrawTerrainSelectionCommand(TerrainSelection& terrainSelection, QUndoCommand* parent = nullptr);
+
+        void redo() override;
+        void undo() override;
+    };
+}
+
+#endif

apps/opencs/view/render/terrainselection.cpp

@@ -39,64 +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::pair<int, int> &localPos)
+void CSVRender::TerrainSelection::addSelect(const std::vector<std::pair<int, int>>& localPositions, bool toggleInProgress)
 {
-    if (std::find(mSelection.begin(), mSelection.end(), localPos) == mSelection.end())
-    {
-        mSelection.emplace_back(localPos);
-        update();
-    }
+    handleSelection(localPositions, toggleInProgress, SelectionMethod::AddSelect);
 }
 
-void CSVRender::TerrainSelection::toggleSelect(const std::vector<std::pair<int, int>> &localPositions, bool toggleInProgress)
+void CSVRender::TerrainSelection::removeSelect(const std::vector<std::pair<int, int>>& localPositions, bool toggleInProgress)
 {
-    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);
-                if (iter != mSelection.end())
-                {
-                    mSelection.erase(iter);
-                }
-                else
-                {
-                    mSelection.emplace_back(localPos);
-                }
-            }
+    handleSelection(localPositions, toggleInProgress, SelectionMethod::RemoveSelect);
+}
 
-            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::toggleSelect(const std::vector<std::pair<int, int>>& localPositions, bool toggleInProgress)
+{
+    handleSelection(localPositions, toggleInProgress, SelectionMethod::ToggleSelect);
 }
 
 void CSVRender::TerrainSelection::activate()
@@ -239,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
     {
@@ -36,7 +44,8 @@ namespace CSVRender
             ~TerrainSelection();
 
             void onlySelect(const std::vector<std::pair<int, int>> &localPositions);
-            void addSelect(const std::pair<int, int> &localPos);
+            void addSelect(const std::vector<std::pair<int, int>>& localPositions, bool toggleInProgress);
+            void removeSelect(const std::vector<std::pair<int, int>>& localPositions, bool toggleInProgress);
             void toggleSelect(const std::vector<std::pair<int, int>> &localPositions, bool toggleInProgress);
 
             void activate();
@@ -55,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

@@ -34,6 +34,7 @@
 #include "../../model/world/universalid.hpp"
 
 #include "brushdraw.hpp"
+#include "commands.hpp"
 #include "editmode.hpp"
 #include "pagedworldspacewidget.hpp"
 #include "mask.hpp"
@@ -42,7 +43,7 @@
 #include "worldspacewidget.hpp"
 
 CSVRender::TerrainShapeMode::TerrainShapeMode (WorldspaceWidget *worldspaceWidget, osg::Group* parentNode, QWidget *parent)
-: EditMode (worldspaceWidget, QIcon {":scenetoolbar/editing-terrain-shape"}, Mask_Terrain | Mask_Reference, "Terrain land editing", parent),
+: EditMode (worldspaceWidget, QIcon {":scenetoolbar/editing-terrain-shape"}, Mask_Terrain, "Terrain land editing", parent),
     mParentNode(parentNode)
 {
 }
@@ -285,6 +286,9 @@ void CSVRender::TerrainShapeMode::applyTerrainEditChanges()
 
     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)
     {
         std::string cellId = CSMWorld::CellCoordinates::generateId(cellCoordinates.getX(), cellCoordinates.getY());
@@ -353,6 +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();
 }
@@ -430,7 +437,9 @@ void CSVRender::TerrainShapeMode::editTerrainShapeGrid(const std::pair<int, int>
                 float smoothedByDistance = 0.0f;
                 if (mShapeEditTool == ShapeEditTool_Drag) smoothedByDistance = calculateBumpShape(distance, r, mTotalDiffY);
                 if (mShapeEditTool == ShapeEditTool_PaintToRaise || mShapeEditTool == ShapeEditTool_PaintToLower) smoothedByDistance = calculateBumpShape(distance, r, r + mShapeEditToolStrength);
-                if (distance <= r)
+
+                // Using floating-point radius here to prevent selecting too few vertices.
+                if (distance <= mBrushSize / 2.0f)
                 {
                     if (mShapeEditTool == ShapeEditTool_Drag) alterHeight(cellCoords, x, y, smoothedByDistance);
                     if (mShapeEditTool == ShapeEditTool_PaintToRaise || mShapeEditTool == ShapeEditTool_PaintToLower)
@@ -1033,10 +1042,35 @@ void CSVRender::TerrainShapeMode::handleSelection(int globalSelectionX, int glob
             return;
         int selectionX = globalSelectionX;
         int selectionY = globalSelectionY;
-        if (xIsAtCellBorder)
+
+        /*
+            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))
+            {
+                selections->emplace_back(globalSelectionX, globalSelectionY);
+            }
+        }
+        else if (xIsAtCellBorder)
+        {
             selectionX--;
-        if (yIsAtCellBorder)
+        }
+        else if (yIsAtCellBorder)
+        {
             selectionY--;
+        }
+
         if (isInCellSelection(selectionX, selectionY))
             selections->emplace_back(globalSelectionX, globalSelectionY);
     }
@@ -1071,8 +1105,11 @@ void CSVRender::TerrainShapeMode::selectTerrainShapes(const std::pair<int, int>&
             {
                 int distanceX = abs(i - vertexCoords.first);
                 int distanceY = abs(j - vertexCoords.second);
-                int distance = std::round(sqrt(pow(distanceX, 2)+pow(distanceY, 2)));
-                if (distance <= r) handleSelection(i, j, &selections);
+                float distance = sqrt(pow(distanceX, 2)+pow(distanceY, 2));
+
+                // Using floating-point radius here to prevent selecting too few vertices.
+                if (distance <= mBrushSize / 2.0f)
+                    handleSelection(i, j, &selections);
             }
         }
     }
@@ -1089,9 +1126,21 @@ void CSVRender::TerrainShapeMode::selectTerrainShapes(const std::pair<int, int>&
         }
     }
 
-    if(selectMode == 0) mTerrainShapeSelection->onlySelect(selections);
-    if(selectMode == 1) mTerrainShapeSelection->toggleSelect(selections, dragOperation);
+    std::string selectAction;
 
+    if (selectMode == 0)
+        selectAction = CSMPrefs::get()["3D Scene Editing"]["primary-select-action"].toString();
+    else
+        selectAction = CSMPrefs::get()["3D Scene Editing"]["secondary-select-action"].toString();
+        
+    if (selectAction == "Select only")
+        mTerrainShapeSelection->onlySelect(selections);
+    else if (selectAction == "Add to selection")
+        mTerrainShapeSelection->addSelect(selections, dragOperation);
+    else if (selectAction == "Remove from selection")
+        mTerrainShapeSelection->removeSelect(selections, dragOperation);
+    else if (selectAction == "Invert selection")
+        mTerrainShapeSelection->toggleSelect(selections, dragOperation);
 }
 
 void CSVRender::TerrainShapeMode::pushEditToCommand(const CSMWorld::LandHeightsColumn::DataType& newLandGrid, CSMDoc::Document& document,