Merge pull request #22088 from hyarion/feature/variable-constraint-points

Sketcher: Adds support for constraint with more than 3 points

src/Mod/Sketcher/App/Constraint.cpp

@@ -24,9 +24,16 @@
 #ifndef _PreComp_
 #include <QDateTime>
 #include <boost/random.hpp>
+#include <algorithm>
 #include <cmath>
+#include <ranges>
+#include <stdexcept>
+#include <string>
+#include <vector>
 #endif
 
+#include <fmt/ranges.h>
+
 #include <Base/Reader.h>
 #include <Base/Tools.h>
 #include <Base/Writer.h>
@@ -44,21 +51,6 @@ using namespace Base;
 TYPESYSTEM_SOURCE(Sketcher::Constraint, Base::Persistence)
 
 Constraint::Constraint()
-    : Value(0.0)
-    , Type(None)
-    , AlignmentType(Undef)
-    , First(GeoEnum::GeoUndef)
-    , FirstPos(PointPos::none)
-    , Second(GeoEnum::GeoUndef)
-    , SecondPos(PointPos::none)
-    , Third(GeoEnum::GeoUndef)
-    , ThirdPos(PointPos::none)
-    , LabelDistance(10.f)
-    , LabelPosition(0.f)
-    , isDriving(true)
-    , InternalAlignmentIndex(-1)
-    , isInVirtualSpace(false)
-    , isActive(true)
 {
     // Initialize a random number generator, to avoid Valgrind false positives.
     // The random number generator is not threadsafe so we guard it.  See
@@ -90,19 +82,24 @@ Constraint* Constraint::copy() const
     temp->Type = this->Type;
     temp->AlignmentType = this->AlignmentType;
     temp->Name = this->Name;
-    temp->First = this->First;
-    temp->FirstPos = this->FirstPos;
-    temp->Second = this->Second;
-    temp->SecondPos = this->SecondPos;
-    temp->Third = this->Third;
-    temp->ThirdPos = this->ThirdPos;
     temp->LabelDistance = this->LabelDistance;
     temp->LabelPosition = this->LabelPosition;
     temp->isDriving = this->isDriving;
     temp->InternalAlignmentIndex = this->InternalAlignmentIndex;
     temp->isInVirtualSpace = this->isInVirtualSpace;
     temp->isActive = this->isActive;
+    temp->elements = this->elements;
     // Do not copy tag, otherwise it is considered a clone, and a "rename" by the expression engine.
+
+#if SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS
+    temp->First = this->First;
+    temp->FirstPos = this->FirstPos;
+    temp->Second = this->Second;
+    temp->SecondPos = this->SecondPos;
+    temp->Third = this->Third;
+    temp->ThirdPos = this->ThirdPos;
+#endif
+
     return temp;
 }
 
@@ -160,19 +157,42 @@ void Constraint::Save(Writer& writer) const
                         << "InternalAlignmentIndex=\"" << InternalAlignmentIndex << "\" ";
     }
     writer.Stream() << "Value=\"" << Value << "\" "
-                    << "First=\"" << First << "\" "
-                    << "FirstPos=\"" << (int)FirstPos << "\" "
-                    << "Second=\"" << Second << "\" "
-                    << "SecondPos=\"" << (int)SecondPos << "\" "
-                    << "Third=\"" << Third << "\" "
-                    << "ThirdPos=\"" << (int)ThirdPos << "\" "
                     << "LabelDistance=\"" << LabelDistance << "\" "
                     << "LabelPosition=\"" << LabelPosition << "\" "
                     << "IsDriving=\"" << (int)isDriving << "\" "
                     << "IsInVirtualSpace=\"" << (int)isInVirtualSpace << "\" "
-                    << "IsActive=\"" << (int)isActive << "\" />"
+                    << "IsActive=\"" << (int)isActive << "\" ";
 
-                    << std::endl;
+    // Save elements
+    {
+        // Ensure backwards compatibility with old versions
+        writer.Stream() << "First=\"" << getElement(0).GeoId << "\" "
+                        << "FirstPos=\"" << getElement(0).posIdAsInt() << "\" "
+                        << "Second=\"" << getElement(1).GeoId << "\" "
+                        << "SecondPos=\"" << getElement(1).posIdAsInt() << "\" "
+                        << "Third=\"" << getElement(2).GeoId << "\" "
+                        << "ThirdPos=\"" << getElement(2).posIdAsInt() << "\" ";
+#if SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS
+        auto elements = std::views::iota(size_t {0}, this->elements.size())
+            | std::views::transform([&](size_t i) {
+                            return getElement(i);
+                        });
+#endif
+        auto geoIds = elements | std::views::transform([](const GeoElementId& e) {
+                          return e.GeoId;
+                      });
+        auto posIds = elements | std::views::transform([](const GeoElementId& e) {
+                          return e.posIdAsInt();
+                      });
+
+        const std::string ids = fmt::format("{}", fmt::join(geoIds, " "));
+        const std::string positions = fmt::format("{}", fmt::join(posIds, " "));
+
+        writer.Stream() << "ElementIds=\"" << ids << "\" "
+                        << "ElementPositions=\"" << positions << "\" ";
+    }
+
+    writer.Stream() << "/>\n";
 }
 
 void Constraint::Restore(XMLReader& reader)
@@ -181,10 +201,6 @@ void Constraint::Restore(XMLReader& reader)
     Name = reader.getAttribute<const char*>("Name");
     Type = reader.getAttribute<ConstraintType>("Type");
     Value = reader.getAttribute<double>("Value");
-    First = reader.getAttribute<long>("First");
-    FirstPos = reader.getAttribute<PointPos>("FirstPos");
-    Second = reader.getAttribute<long>("Second");
-    SecondPos = reader.getAttribute<PointPos>("SecondPos");
 
     if (this->Type == InternalAlignment) {
         AlignmentType = reader.getAttribute<InternalAlignmentType>("InternalAlignmentType");
@@ -197,12 +213,6 @@ void Constraint::Restore(XMLReader& reader)
         AlignmentType = Undef;
     }
 
-    // read the third geo group if present
-    if (reader.hasAttribute("Third")) {
-        Third = reader.getAttribute<long>("Third");
-        ThirdPos = reader.getAttribute<PointPos>("ThirdPos");
-    }
-
     // Read the distance a constraint label has been moved
     if (reader.hasAttribute("LabelDistance")) {
         LabelDistance = (float)reader.getAttribute<double>("LabelDistance");
@@ -223,19 +233,90 @@ void Constraint::Restore(XMLReader& reader)
     if (reader.hasAttribute("IsActive")) {
         isActive = reader.getAttribute<bool>("IsActive");
     }
+
+    if (reader.hasAttribute("ElementIds") && reader.hasAttribute("ElementPositions")) {
+        auto splitAndClean = [](std::string_view input) {
+            const char delimiter = ' ';
+
+            auto tokens = input | std::views::split(delimiter)
+                | std::views::transform([](auto&& subrange) {
+                              // workaround due to lack of std::ranges::to in c++20
+                              std::string token;
+                              auto size = std::ranges::distance(subrange);
+                              token.reserve(size);
+                              for (char c : subrange) {
+                                  token.push_back(c);
+                              }
+                              return token;
+                          })
+                | std::views::filter([](const std::string& s) {
+                              return !s.empty();
+                          });
+
+            return std::vector<std::string>(tokens.begin(), tokens.end());
+        };
+
+        const std::string elementIds = reader.getAttribute<const char*>("ElementIds");
+        const std::string elementPositions = reader.getAttribute<const char*>("ElementPositions");
+
+        const auto ids = splitAndClean(elementIds);
+        const auto positions = splitAndClean(elementPositions);
+
+        if (ids.size() != positions.size()) {
+            throw Base::ParserError(fmt::format("ElementIds and ElementPositions do not match in "
+                                                "size. Got {} ids and {} positions.",
+                                                ids.size(),
+                                                positions.size()));
+        }
+
+        elements.clear();
+        for (size_t i = 0; i < std::min(ids.size(), positions.size()); ++i) {
+            const int geoId {std::stoi(ids[i])};
+            const PointPos pos {static_cast<PointPos>(std::stoi(positions[i]))};
+            addElement(GeoElementId(geoId, pos));
+        }
+    }
+
+    // Ensure we have at least 3 elements
+    while (getElementsSize() < 3) {
+        addElement(GeoElementId(GeoEnum::GeoUndef, PointPos::none));
+    }
+
+    // Load deprecated First, Second, Third elements
+    // These take precedence over the new elements
+    // Even though these are deprecated, we still need to read them
+    // for compatibility with old files.
+    {
+        constexpr std::array<const char*, 3> names = {"First", "Second", "Third"};
+        constexpr std::array<const char*, 3> posNames = {"FirstPos", "SecondPos", "ThirdPos"};
+        static_assert(names.size() == posNames.size());
+
+        for (size_t i = 0; i < names.size(); ++i) {
+            if (reader.hasAttribute(names[i])) {
+                const int geoId {reader.getAttribute<int>(names[i])};
+                const PointPos pos {reader.getAttribute<PointPos>(posNames[i])};
+                setElement(i, GeoElementId(geoId, pos));
+            }
+        }
+    }
 }
 
 void Constraint::substituteIndex(int fromGeoId, int toGeoId)
 {
-    if (this->First == fromGeoId) {
-        this->First = toGeoId;
+#if SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS
+    for (size_t i = 0; i < elements.size(); ++i) {
+        const GeoElementId element = getElement(i);
+        if (element.GeoId == fromGeoId) {
+            setElement(i, GeoElementId(toGeoId, element.Pos));
+        }
     }
-    if (this->Second == fromGeoId) {
-        this->Second = toGeoId;
-    }
-    if (this->Third == fromGeoId) {
-        this->Third = toGeoId;
+#else
+    for (auto& element : elements) {
+        if (element.GeoId == fromGeoId) {
+            element = GeoElementId(toGeoId, element.Pos);
+        }
     }
+#endif
 }
 
 void Constraint::substituteIndexAndPos(int fromGeoId,
@@ -243,18 +324,22 @@ void Constraint::substituteIndexAndPos(int fromGeoId,
                                        int toGeoId,
                                        PointPos toPosId)
 {
-    if (this->First == fromGeoId && this->FirstPos == fromPosId) {
-        this->First = toGeoId;
-        this->FirstPos = toPosId;
+    const GeoElementId from {fromGeoId, fromPosId};
+
+#if SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS
+    for (size_t i = 0; i < elements.size(); ++i) {
+        const GeoElementId element = getElement(i);
+        if (element == from) {
+            setElement(i, GeoElementId(toGeoId, toPosId));
+        }
     }
-    if (this->Second == fromGeoId && this->SecondPos == fromPosId) {
-        this->Second = toGeoId;
-        this->SecondPos = toPosId;
-    }
-    if (this->Third == fromGeoId && this->ThirdPos == fromPosId) {
-        this->Third = toGeoId;
-        this->ThirdPos = toPosId;
+#else
+    for (auto& element : elements) {
+        if (element == from) {
+            element = GeoElementId(toGeoId, toPosId);
+        }
     }
+#endif
 }
 
 std::string Constraint::typeToString(ConstraintType type)
@@ -266,3 +351,91 @@ std::string Constraint::internalAlignmentTypeToString(InternalAlignmentType alig
 {
     return internalAlignmentType2str[alignment];
 }
+
+bool Constraint::involvesGeoId(int geoId) const
+{
+#if SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS
+    auto elements =
+        std::views::iota(size_t {0}, this->elements.size()) | std::views::transform([&](size_t i) {
+            return getElement(i);
+        });
+#endif
+    return std::ranges::any_of(elements, [geoId](const auto& element) {
+        return element.GeoId == geoId;
+    });
+}
+/// utility function to check if (`geoId`, `posId`) is one of the points/curves
+bool Constraint::involvesGeoIdAndPosId(int geoId, PointPos posId) const
+{
+#if SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS
+    auto elements =
+        std::views::iota(size_t {0}, this->elements.size()) | std::views::transform([&](size_t i) {
+            return getElement(i);
+        });
+#endif
+    return std::ranges::find(elements, GeoElementId(geoId, posId)) != elements.end();
+}
+
+GeoElementId Constraint::getElement(size_t index) const
+{
+    if (index >= elements.size()) {
+        throw Base::IndexError("Constraint::getElement index out of range");
+    }
+
+#if SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS
+    if (index < 3) {
+        switch (index) {
+            case 0:
+                return GeoElementId(First, FirstPos);
+            case 1:
+                return GeoElementId(Second, SecondPos);
+            case 2:
+                return GeoElementId(Third, ThirdPos);
+        }
+    }
+#endif
+    return elements[index];
+}
+void Constraint::setElement(size_t index, GeoElementId element)
+{
+    if (index >= elements.size()) {
+        throw Base::IndexError("Constraint::getElement index out of range");
+    }
+
+    elements[index] = element;
+
+#if SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS
+    if (index < 3) {
+        switch (index) {
+            case 0:
+                First = element.GeoId;
+                FirstPos = element.Pos;
+                break;
+            case 1:
+                Second = element.GeoId;
+                SecondPos = element.Pos;
+                break;
+            case 2:
+                Third = element.GeoId;
+                ThirdPos = element.Pos;
+                break;
+        }
+    }
+#endif
+}
+
+size_t Constraint::getElementsSize() const
+{
+    return elements.size();
+}
+
+void Constraint::addElement(GeoElementId element)
+{
+#if SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS
+    int i = elements.size();
+    elements.resize(i + 1);
+    setElement(i, element);
+#else
+    elements.push_back(element);
+#endif
+}

src/Mod/Sketcher/App/Constraint.h

@@ -33,6 +33,11 @@
 #include "GeoEnum.h"
 
 
+// Flipping this to 0 removes old legazy members First, FirstPos, Second...
+// Will be used when everything has been migrated to new api.
+#define SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS 1
+
+
 namespace Sketcher
 {
 /*!
@@ -129,24 +134,18 @@ public:
             || Type == Diameter || Type == Angle || Type == SnellsLaw || Type == Weight;
     }
 
-    /// utility function to swap the index in First/Second/Third of the provided constraint from the
+    /// utility function to swap the index in elements of the provided constraint from the
     /// fromGeoId GeoId to toGeoId
     void substituteIndex(int fromGeoId, int toGeoId);
-    /// utility function to swap the index and position in First/Second/Third of the provided
+    /// utility function to swap the index and position in elements of the provided
     /// constraint from {fromGeoId, fromPosId} to {toGeoId, toPosId}.
     void substituteIndexAndPos(int fromGeoId, PointPos fromPosId, int toGeoId, PointPos toPosId);
 
     /// utility function to check if `geoId` is one of the geometries
-    bool involvesGeoId(int geoId) const
-    {
-        return First == geoId || Second == geoId || Third == geoId;
-    }
+    bool involvesGeoId(int geoId) const;
+
     /// utility function to check if (`geoId`, `posId`) is one of the points/curves
-    bool involvesGeoIdAndPosId(int geoId, PointPos posId) const
-    {
-        return (First == geoId && FirstPos == posId) || (Second == geoId && SecondPos == posId)
-            || (Third == geoId && ThirdPos == posId);
-    }
+    bool involvesGeoIdAndPosId(int geoId, PointPos posId) const;
 
     std::string typeToString() const
     {
@@ -166,7 +165,7 @@ private:
     Constraint(const Constraint&) = default;  // only for internal use
 
 private:
-    double Value;
+    double Value {0.0};
 
     // clang-format off
     constexpr static std::array<const char*, ConstraintType::NumConstraintTypes> type2str {
@@ -207,24 +206,38 @@ private:
                                     "ParabolaFocalAxis"}};
 
 public:
-    ConstraintType Type;
-    InternalAlignmentType AlignmentType;
+    ConstraintType Type {None};
+    InternalAlignmentType AlignmentType {Undef};
     std::string Name;
-    int First;
-    PointPos FirstPos;
-    int Second;
-    PointPos SecondPos;
-    int Third;
-    PointPos ThirdPos;
-    float LabelDistance;
-    float LabelPosition;
-    bool isDriving;
+    float LabelDistance {10.F};
+    float LabelPosition {0.F};
+    bool isDriving {true};
     // Note: for InternalAlignment Type this index indexes equal internal geometry elements (e.g.
     // index of pole in a bspline). It is not a GeoId!!
-    int InternalAlignmentIndex;
-    bool isInVirtualSpace;
+    int InternalAlignmentIndex {-1};
+    bool isInVirtualSpace {false};
 
-    bool isActive;
+    bool isActive {true};
+
+    GeoElementId getElement(size_t index) const;
+    void setElement(size_t index, GeoElementId element);
+    size_t getElementsSize() const;
+    void addElement(GeoElementId element);
+
+#ifdef SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS
+    // Deprecated, use getElement/setElement instead
+    int First {GeoEnum::GeoUndef};
+    int Second {GeoEnum::GeoUndef};
+    int Third {GeoEnum::GeoUndef};
+    PointPos FirstPos {PointPos::none};
+    PointPos SecondPos {PointPos::none};
+    PointPos ThirdPos {PointPos::none};
+#endif
+
+private:
+    // New way to access point ids and positions.
+    // While the old way is still supported, it is recommended to the getters and setters instead.
+    std::vector<GeoElementId> elements {GeoElementId(), GeoElementId(), GeoElementId()};
 
 protected:
     boost::uuids::uuid tag;

src/Mod/Sketcher/App/PreCompiled.h

@@ -32,12 +32,15 @@
 #ifdef _PreComp_
 
 // standard
+#include <algorithm>
 #include <cassert>
 #include <cmath>
 #include <iostream>
 #include <limits>
 #include <memory>
+#include <ranges>
 #include <sstream>
+#include <string>
 #include <vector>
 
 // Qt

tests/src/Mod/Sketcher/App/CMakeLists.txt

@@ -1,4 +1,5 @@
 add_executable(Sketcher_tests_run
+        Constraint.cpp
         SketcherTestHelpers.cpp
         SketchObject.cpp
         SketchObjectChanges.cpp

tests/src/Mod/Sketcher/App/Constraint.cpp

@@ -0,0 +1,571 @@
+
+#include <FCConfig.h>
+
+#include <Base/Reader.h>
+#include <Base/Writer.h>
+#include <Mod/Sketcher/App/Constraint.h>
+
+#include <gtest/gtest.h>
+#include <xercesc/util/PlatformUtils.hpp>
+#include <fmt/core.h>
+#include <QTemporaryFile>
+
+
+// Ensure Xerces is initialized before running tests which uses xml
+class XercesEnvironment: public ::testing::Environment
+{
+public:
+    void SetUp() override
+    {
+        try {
+            xercesc::XMLPlatformUtils::Initialize();
+        }
+        catch (const xercesc::XMLException& e) {
+            FAIL() << "Xerces init failed: " << xercesc::XMLString::transcode(e.getMessage());
+        }
+    }
+
+    void TearDown() override
+    {
+        xercesc::XMLPlatformUtils::Terminate();
+    }
+};
+
+::testing::Environment* const xercesEnv =
+    ::testing::AddGlobalTestEnvironment(new XercesEnvironment);
+
+
+TEST(ConstraintPointsAccess, testDefaultGeoElementIdsAreSane)  // NOLINT
+{
+    // Arrange
+    auto constraint = Sketcher::Constraint();
+
+    // Act - no action needed, we are testing the default state
+
+    // Assert
+#if SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS
+    // Old way of accessing elements
+    EXPECT_EQ(constraint.First, Sketcher::GeoEnum::GeoUndef);
+    EXPECT_EQ(constraint.FirstPos, Sketcher::PointPos::none);
+
+    EXPECT_EQ(constraint.Second, Sketcher::GeoEnum::GeoUndef);
+    EXPECT_EQ(constraint.SecondPos, Sketcher::PointPos::none);
+
+    EXPECT_EQ(constraint.Third, Sketcher::GeoEnum::GeoUndef);
+    EXPECT_EQ(constraint.ThirdPos, Sketcher::PointPos::none);
+
+    // New way of accessing elements
+#endif
+    EXPECT_EQ(constraint.getElement(0),
+              Sketcher::GeoElementId(Sketcher::GeoEnum::GeoUndef, Sketcher::PointPos::none));
+    EXPECT_EQ(constraint.getElement(1),
+              Sketcher::GeoElementId(Sketcher::GeoEnum::GeoUndef, Sketcher::PointPos::none));
+    EXPECT_EQ(constraint.getElement(2),
+              Sketcher::GeoElementId(Sketcher::GeoEnum::GeoUndef, Sketcher::PointPos::none));
+}
+
+#if SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS
+TEST(ConstraintPointsAccess, testOldWriteIsReadByNew)  // NOLINT
+{
+    // Arrange
+    auto constraint = Sketcher::Constraint();
+
+    // Act
+    constraint.First = 23;
+    constraint.FirstPos = Sketcher::PointPos::start;
+    constraint.Second = 34;
+    constraint.SecondPos = Sketcher::PointPos::end;
+    constraint.Third = 45;
+    constraint.ThirdPos = Sketcher::PointPos::mid;
+
+    // Assert
+    EXPECT_EQ(constraint.getElement(0),
+              Sketcher::GeoElementId(Sketcher::GeoElementId(23, Sketcher::PointPos::start)));
+    EXPECT_EQ(constraint.getElement(1),
+              Sketcher::GeoElementId(Sketcher::GeoElementId(34, Sketcher::PointPos::end)));
+    EXPECT_EQ(constraint.getElement(2),
+              Sketcher::GeoElementId(Sketcher::GeoElementId(45, Sketcher::PointPos::mid)));
+}
+
+TEST(ConstraintPointsAccess, testNewWriteIsReadByOld)  // NOLINT
+{
+    // Arrange
+    auto constraint = Sketcher::Constraint();
+
+    // Act
+    constraint.setElement(0, Sketcher::GeoElementId(23, Sketcher::PointPos::start));
+    constraint.setElement(1, Sketcher::GeoElementId(34, Sketcher::PointPos::end));
+    constraint.setElement(2, Sketcher::GeoElementId(45, Sketcher::PointPos::mid));
+
+    // Assert
+    EXPECT_EQ(constraint.First, 23);
+    EXPECT_EQ(constraint.FirstPos, Sketcher::PointPos::start);
+    EXPECT_EQ(constraint.Second, 34);
+    EXPECT_EQ(constraint.SecondPos, Sketcher::PointPos::end);
+    EXPECT_EQ(constraint.Third, 45);
+    EXPECT_EQ(constraint.ThirdPos, Sketcher::PointPos::mid);
+}
+#endif
+
+TEST(ConstraintPointsAccess, testThreeElementsByDefault)  // NOLINT
+{
+    // Arrange
+    auto constraint = Sketcher::Constraint();
+
+    // Act - no action needed, we are testing the default state
+
+    // Assert
+    EXPECT_EQ(constraint.getElementsSize(), 3);
+}
+
+TEST(ConstraintPointsAccess, testFourElementsWhenAddingOne)  // NOLINT
+{
+    // Arrange
+    auto constraint = Sketcher::Constraint();
+
+    // Act
+    constraint.addElement(Sketcher::GeoElementId(1, Sketcher::PointPos::start));
+
+    // Assert
+    EXPECT_EQ(constraint.getElementsSize(), 4);
+}
+
+#if SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS
+TEST(ConstraintPointsAccess, testElementSerializationWhenAccessingOldWay)  // NOLINT
+{
+    // Arrange
+    auto constraint = Sketcher::Constraint();
+
+    // Act
+    constraint.First = 23;
+    constraint.FirstPos = Sketcher::PointPos::start;
+    constraint.Second = 34;
+    constraint.SecondPos = Sketcher::PointPos::end;
+    constraint.Third = 45;
+    constraint.ThirdPos = Sketcher::PointPos::mid;
+
+    Base::StringWriter writer = {};
+    constraint.Save(writer);
+
+    // Assert
+    std::string serialized = writer.getString();
+    EXPECT_TRUE(serialized.find("First=\"23\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("FirstPos=\"1\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("Second=\"34\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("SecondPos=\"2\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("Third=\"45\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("ThirdPos=\"3\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("ElementIds=\"23 34 45\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("ElementPositions=\"1 2 3\"") != std::string::npos);
+}
+#endif
+
+TEST(ConstraintPointsAccess, testElementSerializationWhenAccessingNewWay)  // NOLINT
+{
+    // Arrange
+    auto constraint = Sketcher::Constraint();
+
+    // Act
+    constraint.setElement(0, Sketcher::GeoElementId(23, Sketcher::PointPos::start));
+    constraint.setElement(1, Sketcher::GeoElementId(34, Sketcher::PointPos::end));
+    constraint.setElement(2, Sketcher::GeoElementId(45, Sketcher::PointPos::mid));
+
+    Base::StringWriter writer = {};
+    constraint.Save(writer);
+
+    // Assert
+    std::string serialized = writer.getString();
+    EXPECT_TRUE(serialized.find("First=\"23\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("FirstPos=\"1\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("Second=\"34\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("SecondPos=\"2\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("Third=\"45\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("ThirdPos=\"3\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("ElementIds=\"23 34 45\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("ElementPositions=\"1 2 3\"") != std::string::npos);
+}
+
+#if SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS
+TEST(ConstraintPointsAccess, testElementSerializationWhenMixingOldAndNew)  // NOLINT
+{
+    // Arrange
+    auto constraint = Sketcher::Constraint();
+
+    // Act
+    constraint.setElement(0, Sketcher::GeoElementId(23, Sketcher::PointPos::start));
+    constraint.setElement(1, Sketcher::GeoElementId(34, Sketcher::PointPos::end));
+    constraint.Second = 45;  // Old way
+    constraint.SecondPos = Sketcher::PointPos::mid;
+
+    Base::StringWriter writer = {};
+    constraint.Save(writer);
+
+    // Assert
+    std::string serialized = writer.getString();
+    EXPECT_TRUE(serialized.find("First=\"23\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("FirstPos=\"1\"") != std::string::npos);
+
+    // Old way wrote this data
+    // ensure mid is 3 for next test
+    EXPECT_EQ(Sketcher::PointPos::mid, static_cast<Sketcher::PointPos>(3));
+    EXPECT_TRUE(serialized.find("SecondPos=\"3\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("Second=\"45\"") != std::string::npos);
+
+    EXPECT_TRUE(serialized.find("Third=\"-2000\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("ThirdPos=\"0\"") != std::string::npos);
+
+    // Second and SecondPos is reflected in the elements data too
+    EXPECT_TRUE(serialized.find("ElementIds=\"23 45 -2000\"") != std::string::npos);
+    EXPECT_TRUE(serialized.find("ElementPositions=\"1 3 0\"") != std::string::npos);
+}
+#endif
+
+TEST(ConstraintPointsAccess, testElementsRestoredFromSerialization)  // NOLINT
+{
+    // Arrange
+    Sketcher::Constraint constraint;
+    constraint.setElement(0, Sketcher::GeoElementId(23, Sketcher::PointPos::start));
+    constraint.setElement(1, Sketcher::GeoElementId(34, Sketcher::PointPos::end));
+    constraint.setElement(2, Sketcher::GeoElementId(45, Sketcher::PointPos::mid));
+
+    Base::StringWriter writer;
+    writer.Stream() << "<root>\n";  // Wrap in a root element to make constraint.Save happy
+    constraint.Save(writer);
+    writer.Stream() << "</root>";
+
+    // Write to temporary file
+    QTemporaryFile tempFile;
+    tempFile.setAutoRemove(true);
+    ASSERT_TRUE(tempFile.open());
+    tempFile.write(writer.getString().c_str(), writer.getString().size());
+    tempFile.flush();
+
+    // Open with std::ifstream and parse
+    std::string filename = tempFile.fileName().toStdString();
+    std::ifstream inputFile(filename);
+    ASSERT_TRUE(inputFile.is_open());
+
+    Base::XMLReader reader(tempFile.fileName().toStdString().c_str(), inputFile);
+    Sketcher::Constraint restoredConstraint;
+    restoredConstraint.Restore(reader);
+
+    // Assert
+    EXPECT_EQ(restoredConstraint.getElement(0),
+              Sketcher::GeoElementId(23, Sketcher::PointPos::start));
+    EXPECT_EQ(restoredConstraint.getElement(1),
+              Sketcher::GeoElementId(34, Sketcher::PointPos::end));
+    EXPECT_EQ(restoredConstraint.getElement(2),
+              Sketcher::GeoElementId(45, Sketcher::PointPos::mid));
+
+    inputFile.close();
+}
+
+TEST(ConstraintPointsAccess,
+     testElementsRestoredFromSerializationWithoutNewElementStorage)  // NOLINT
+{
+    // Arrange
+
+    // Manually craft a serialized version, only parts in "{}" are important.
+    // New way of storing elements is not present, like if it is an older file.
+    std::string serializedConstraint = fmt::format("<Constrain "
+                                                   R"(Name="" )"
+                                                   R"(Type="0" )"
+                                                   R"(Value="0" )"
+                                                   R"(LabelDistance="10" )"
+                                                   R"(LabelPosition="0" )"
+                                                   R"(IsDriving="1" )"
+                                                   R"(IsInVirtualSpace="0" )"
+                                                   R"(IsActive="1" )"
+
+                                                   R"(First="{}" )"
+                                                   R"(Second="{}" )"
+                                                   R"(Third="{}" )"
+                                                   R"(FirstPos="{}" )"
+                                                   R"(SecondPos="{}" )"
+                                                   R"(ThirdPos="{}" )"
+
+                                                   "/>",
+
+                                                   67,
+                                                   78,
+                                                   89,
+                                                   static_cast<int>(Sketcher::PointPos::mid),
+                                                   static_cast<int>(Sketcher::PointPos::start),
+                                                   static_cast<int>(Sketcher::PointPos::end));
+
+    Base::StringWriter writer;
+    auto& stream {writer.Stream()};
+    stream << "<root>\n";  // Wrap in a root element to make constraint.
+    stream << serializedConstraint;
+    stream << "</root>";
+
+    // Write to temporary file
+    QTemporaryFile tempFile;
+    tempFile.setAutoRemove(true);
+    ASSERT_TRUE(tempFile.open());
+    tempFile.write(writer.getString().c_str(), writer.getString().size());
+    tempFile.flush();
+
+    // Open with std::ifstream and parse
+    std::string filename = tempFile.fileName().toStdString();
+    std::ifstream inputFile(filename);
+    ASSERT_TRUE(inputFile.is_open());
+
+    Base::XMLReader reader(tempFile.fileName().toStdString().c_str(), inputFile);
+    Sketcher::Constraint restoredConstraint;
+    restoredConstraint.Restore(reader);
+
+    // Assert
+    EXPECT_EQ(restoredConstraint.getElement(0),
+              Sketcher::GeoElementId(67, Sketcher::PointPos::mid));
+    EXPECT_EQ(restoredConstraint.getElement(1),
+              Sketcher::GeoElementId(78, Sketcher::PointPos::start));
+    EXPECT_EQ(restoredConstraint.getElement(2),
+              Sketcher::GeoElementId(89, Sketcher::PointPos::end));
+
+    inputFile.close();
+}
+
+TEST(ConstraintPointsAccess,
+     testLegacyIsPreferedDuringSerializationWithoutLegacyElementStorage)  // NOLINT
+{
+    // Arrange
+
+    // Manually craft a serialized version, only parts in "{}" are important.
+    // Only new way of storing elements is present.
+    std::string serializedConstraint = fmt::format("<Constrain "
+                                                   R"(Name="" )"
+                                                   R"(Type="0" )"
+                                                   R"(Value="0" )"
+                                                   R"(LabelDistance="10" )"
+                                                   R"(LabelPosition="0" )"
+                                                   R"(IsDriving="1" )"
+                                                   R"(IsInVirtualSpace="0" )"
+                                                   R"(IsActive="1" )"
+
+                                                   // New way
+                                                   R"(ElementIds="{} {} {}" )"
+                                                   R"(ElementPositions="{} {} {}" )"
+
+                                                   "/>",
+                                                   // New way data
+                                                   23,
+                                                   34,
+                                                   45,
+                                                   static_cast<int>(Sketcher::PointPos::start),
+                                                   static_cast<int>(Sketcher::PointPos::end),
+                                                   static_cast<int>(Sketcher::PointPos::mid));
+
+    Base::StringWriter writer;
+    auto& stream {writer.Stream()};
+    stream << "<root>\n";  // Wrap in a root element to make constraint.
+    stream << serializedConstraint;
+    stream << "</root>";
+
+    // Write to temporary file
+    QTemporaryFile tempFile;
+    tempFile.setAutoRemove(true);
+    ASSERT_TRUE(tempFile.open());
+    tempFile.write(writer.getString().c_str(), writer.getString().size());
+    tempFile.flush();
+
+    // Open with std::ifstream and parse
+    std::string filename = tempFile.fileName().toStdString();
+    std::ifstream inputFile(filename);
+    ASSERT_TRUE(inputFile.is_open());
+
+    Base::XMLReader reader(tempFile.fileName().toStdString().c_str(), inputFile);
+    Sketcher::Constraint restoredConstraint;
+    restoredConstraint.Restore(reader);
+
+    // Assert
+    EXPECT_EQ(restoredConstraint.getElement(0),
+              Sketcher::GeoElementId(23, Sketcher::PointPos::start));
+    EXPECT_EQ(restoredConstraint.getElement(1),
+              Sketcher::GeoElementId(34, Sketcher::PointPos::end));
+    EXPECT_EQ(restoredConstraint.getElement(2),
+              Sketcher::GeoElementId(45, Sketcher::PointPos::mid));
+
+    inputFile.close();
+}
+
+TEST(ConstraintPointsAccess, testLegacyIsPreferedDuringSerializationIfContradicting)  // NOLINT
+{
+    // Arrange
+
+    // Manually craft a serialized version, only parts in "{}" are important.
+    // It is not important if legacy is included before or after, legacy should always be preferred.
+    std::string serializedConstraint =
+        fmt::format("<Constrain "
+                    R"(Name="" )"
+                    R"(Type="0" )"
+                    R"(Value="0" )"
+                    R"(LabelDistance="10" )"
+                    R"(LabelPosition="0" )"
+                    R"(IsDriving="1" )"
+                    R"(IsInVirtualSpace="0" )"
+                    R"(IsActive="1" )"
+
+                    // New way
+                    R"(ElementIds="{} {} {}" )"
+                    R"(ElementPositions="{} {} {}" )"
+
+                    // Legacy
+                    R"(First="{}" )"
+                    R"(Second="{}" )"
+                    R"(Third="{}" )"
+                    R"(FirstPos="{}" )"
+                    R"(SecondPos="{}" )"
+                    R"(ThirdPos="{}" )"
+
+                    "/>",
+                    // New way data
+                    23,
+                    34,
+                    45,
+                    static_cast<int>(Sketcher::PointPos::start),
+                    static_cast<int>(Sketcher::PointPos::end),
+                    static_cast<int>(Sketcher::PointPos::mid),
+
+                    // Contradicting legacy data, this should be preferred if available
+                    67,
+                    78,
+                    89,
+                    static_cast<int>(Sketcher::PointPos::mid),
+                    static_cast<int>(Sketcher::PointPos::start),
+                    static_cast<int>(Sketcher::PointPos::end));
+
+    Base::StringWriter writer;
+    auto& stream {writer.Stream()};
+    stream << "<root>\n";  // Wrap in a root element to make constraint.
+    stream << serializedConstraint;
+    stream << "</root>";
+
+    // Write to temporary file
+    QTemporaryFile tempFile;
+    tempFile.setAutoRemove(true);
+    ASSERT_TRUE(tempFile.open());
+    tempFile.write(writer.getString().c_str(), writer.getString().size());
+    tempFile.flush();
+
+    // Open with std::ifstream and parse
+    std::string filename = tempFile.fileName().toStdString();
+    std::ifstream inputFile(filename);
+    ASSERT_TRUE(inputFile.is_open());
+
+    Base::XMLReader reader(tempFile.fileName().toStdString().c_str(), inputFile);
+    Sketcher::Constraint restoredConstraint;
+    restoredConstraint.Restore(reader);
+
+    // Assert
+    EXPECT_EQ(restoredConstraint.getElement(0),
+              Sketcher::GeoElementId(67, Sketcher::PointPos::mid));
+    EXPECT_EQ(restoredConstraint.getElement(1),
+              Sketcher::GeoElementId(78, Sketcher::PointPos::start));
+    EXPECT_EQ(restoredConstraint.getElement(2),
+              Sketcher::GeoElementId(89, Sketcher::PointPos::end));
+
+    inputFile.close();
+}
+
+TEST(ConstraintPointsAccess, testSubstituteIndex)  // NOLINT
+{
+    // Arrange
+    Sketcher::Constraint constraint;
+    constraint.setElement(0, Sketcher::GeoElementId(10, Sketcher::PointPos::start));
+    constraint.setElement(1, Sketcher::GeoElementId(20, Sketcher::PointPos::end));
+    constraint.setElement(2,
+                          Sketcher::GeoElementId(10, Sketcher::PointPos::mid));  // same GeoId as 0
+
+    // Act
+    constraint.substituteIndex(10, 99);
+
+    // Assert
+    EXPECT_EQ(constraint.getElement(0), Sketcher::GeoElementId(99, Sketcher::PointPos::start));
+    EXPECT_EQ(constraint.getElement(1), Sketcher::GeoElementId(20, Sketcher::PointPos::end));
+    EXPECT_EQ(constraint.getElement(2), Sketcher::GeoElementId(99, Sketcher::PointPos::mid));
+}
+
+TEST(ConstraintPointsAccess, testSubstituteIndexAndPos)  // NOLINT
+{
+    // Arrange
+    Sketcher::Constraint constraint;
+    constraint.setElement(0, Sketcher::GeoElementId(10, Sketcher::PointPos::start));
+    constraint.setElement(1, Sketcher::GeoElementId(20, Sketcher::PointPos::start));
+    constraint.setElement(2, Sketcher::GeoElementId(10, Sketcher::PointPos::mid));
+
+    // Act
+    constraint.substituteIndexAndPos(10, Sketcher::PointPos::start, 42, Sketcher::PointPos::end);
+
+    // Assert
+    EXPECT_EQ(constraint.getElement(0), Sketcher::GeoElementId(42, Sketcher::PointPos::end));
+    EXPECT_EQ(constraint.getElement(1), Sketcher::GeoElementId(20, Sketcher::PointPos::start));
+    EXPECT_EQ(constraint.getElement(2),
+              Sketcher::GeoElementId(10, Sketcher::PointPos::mid));  // unchanged
+}
+
+TEST(ConstraintPointsAccess, testInvolvesGeoId)  // NOLINT
+{
+    // Arrange
+    Sketcher::Constraint constraint;
+    constraint.setElement(0, Sketcher::GeoElementId(10, Sketcher::PointPos::start));
+    constraint.setElement(1, Sketcher::GeoElementId(20, Sketcher::PointPos::end));
+
+    // Act & Assert
+    EXPECT_TRUE(constraint.involvesGeoId(10));
+    EXPECT_TRUE(constraint.involvesGeoId(20));
+    EXPECT_FALSE(constraint.involvesGeoId(99));
+}
+
+TEST(ConstraintPointsAccess, testInvolvesGeoIdAndPosId)  // NOLINT
+{
+    // Arrange
+    Sketcher::Constraint constraint;
+    constraint.setElement(0, Sketcher::GeoElementId(10, Sketcher::PointPos::start));
+    constraint.setElement(1, Sketcher::GeoElementId(20, Sketcher::PointPos::mid));
+    constraint.setElement(2, Sketcher::GeoElementId(30, Sketcher::PointPos::end));
+
+    // Act & Assert
+    EXPECT_TRUE(constraint.involvesGeoIdAndPosId(10, Sketcher::PointPos::start));
+    EXPECT_TRUE(constraint.involvesGeoIdAndPosId(20, Sketcher::PointPos::mid));
+    EXPECT_FALSE(constraint.involvesGeoIdAndPosId(20, Sketcher::PointPos::start));
+    EXPECT_FALSE(constraint.involvesGeoIdAndPosId(99, Sketcher::PointPos::end));
+}
+
+#if SKETCHER_CONSTRAINT_USE_LEGACY_ELEMENTS
+TEST(ConstraintPointsAccess, testLegacyWriteReflectedInInvolvesAndSubstitute)  // NOLINT
+{
+    // Arrange
+    Sketcher::Constraint constraint;
+    constraint.First = 10;
+    constraint.FirstPos = Sketcher::PointPos::start;
+    constraint.Second = 20;
+    constraint.SecondPos = Sketcher::PointPos::end;
+
+    // Act & Assert
+    EXPECT_TRUE(constraint.involvesGeoId(10));
+    EXPECT_TRUE(constraint.involvesGeoIdAndPosId(20, Sketcher::PointPos::end));
+
+    // Substitute the legacy-indexed element
+    constraint.substituteIndex(10, 99);
+
+    // Should now reflect the substituted value
+    EXPECT_TRUE(constraint.involvesGeoId(99));
+    EXPECT_FALSE(constraint.involvesGeoId(10));
+}
+
+TEST(ConstraintPointsAccess, testSubstituteUpdatesLegacyFieldsToo)  // NOLINT
+{
+    // Arrange
+    Sketcher::Constraint constraint;
+    constraint.setElement(0, Sketcher::GeoElementId(10, Sketcher::PointPos::start));
+
+    // Act
+    constraint.substituteIndex(10, 42);
+
+    // Assert
+    EXPECT_EQ(constraint.getElement(0), Sketcher::GeoElementId(42, Sketcher::PointPos::start));
+    EXPECT_EQ(constraint.First, 42);
+    EXPECT_EQ(constraint.FirstPos, Sketcher::PointPos::start);
+}
+#endif