From e53e941a2f4b16bd5caec83cfb532f5b009ef1eb Mon Sep 17 00:00:00 2001
From: Erwan MATHIEU <erwan.mathieu@ultimaker.com>
Date: Tue, 10 Dec 2024 21:28:38 +0100
Subject: [PATCH] Properly set parent when changing operations list at once

CURA-12250
---
 .../DirectTravelMoveGenerator.h               |  5 +-
 .../LayerPlanTravelMovesInserter.h            |  2 +-
 .../ContinuousExtruderMoveSequence.h          |  4 --
 .../print_operation/PrintOperationSequence.h  | 57 ++++++++++++-----
 include/print_operation/TravelRoute.h         |  3 +
 .../DirectTravelMoveGenerator.cpp             |  1 +
 .../InsertOperationsProcessor.cpp             |  4 +-
 .../LayerPlanTravelMovesInserter.cpp          | 46 ++++++++------
 .../ContinuousExtruderMoveSequence.cpp        |  7 ++-
 src/print_operation/PrintOperation.cpp        |  3 +-
 .../PrintOperationSequence.cpp                | 62 ++++++++++++-------
 11 files changed, 123 insertions(+), 71 deletions(-)

diff --git a/include/operation_transformation/DirectTravelMoveGenerator.h b/include/operation_transformation/DirectTravelMoveGenerator.h
index 23198cd621..92af8c4fb4 100644
--- a/include/operation_transformation/DirectTravelMoveGenerator.h
+++ b/include/operation_transformation/DirectTravelMoveGenerator.h
@@ -1,8 +1,7 @@
 // Copyright (c) 2024 UltiMaker
 // CuraEngine is released under the terms of the AGPLv3 or higher
 
-#ifndef PATHPROCESSING_DIRECTTRAVELMOVEGENERATOR_H
-#define PATHPROCESSING_DIRECTTRAVELMOVEGENERATOR_H
+#pragma once
 
 #include "operation_transformation/TravelMoveGenerator.h"
 
@@ -16,5 +15,3 @@ class DirectTravelMoveGenerator : public TravelMoveGenerator
 };
 
 } // namespace cura
-
-#endif // PATHPROCESSING_DIRECTTRAVELMOVEGENERATOR_H
diff --git a/include/operation_transformation/LayerPlanTravelMovesInserter.h b/include/operation_transformation/LayerPlanTravelMovesInserter.h
index 1156f49886..07fbf1beee 100644
--- a/include/operation_transformation/LayerPlanTravelMovesInserter.h
+++ b/include/operation_transformation/LayerPlanTravelMovesInserter.h
@@ -27,7 +27,7 @@ class LayerPlanTravelMovesInserter : public PrintOperationTransformer<LayerPlan>
 
     void appendTravelMovesRecursively(const std::shared_ptr<PrintOperation>& operation, const SpeedDerivatives& speed);
 
-    const std::shared_ptr<PrintOperation> makeTravelMove(const Point3LL& start_position, const Point3LL& end_position, const SpeedDerivatives& speed);
+    const std::shared_ptr<PrintOperation> makeTravelRoute(const Point3LL& start_position, const Point3LL& end_position, const SpeedDerivatives& speed);
 
 private:
     std::vector<std::shared_ptr<TravelMoveGenerator>> generators_;
diff --git a/include/print_operation/ContinuousExtruderMoveSequence.h b/include/print_operation/ContinuousExtruderMoveSequence.h
index db8900d4fb..332b720951 100644
--- a/include/print_operation/ContinuousExtruderMoveSequence.h
+++ b/include/print_operation/ContinuousExtruderMoveSequence.h
@@ -3,12 +3,8 @@
 
 #pragma once
 
-#include "GCodePathConfig.h"
-#include "SpaceFillType.h"
 #include "geometry/Point3LL.h"
 #include "print_operation/PrintOperationSequence.h"
-#include "settings/types/Ratio.h"
-#include "utils/Coord_t.h"
 
 namespace cura
 {
diff --git a/include/print_operation/PrintOperationSequence.h b/include/print_operation/PrintOperationSequence.h
index 113efc2940..bcf2ccbe47 100644
--- a/include/print_operation/PrintOperationSequence.h
+++ b/include/print_operation/PrintOperationSequence.h
@@ -1,12 +1,14 @@
 // Copyright (c) 2024 UltiMaker
 // CuraEngine is released under the terms of the AGPLv3 or higher
 
-#ifndef PATHPLANNING_PRINTOPERATIONSEQUENCE_H
-#define PATHPLANNING_PRINTOPERATIONSEQUENCE_H
+#pragma once
+
+#include <range/v3/algorithm/contains.hpp>
 
 #include "geometry/Point3LL.h"
 #include "operation_transformation/PrintOperationTransformer.h"
 #include "print_operation/PrintOperation.h"
+#include "print_operation/PrintOperationPtr.h"
 
 namespace cura
 {
@@ -56,8 +58,8 @@ class PrintOperationSequence : public PrintOperation, public std::enable_shared_
      * @return The first found operation, or a null ptr if none was found
      * @note This function can also be used to iterate over children by providing a search function that always returns false
      */
-    std::shared_ptr<PrintOperation> findOperation(
-        const std::function<bool(const std::shared_ptr<PrintOperation>&)>& search_function,
+    PrintOperationPtr findOperation(
+        const std::function<bool(const PrintOperationPtr&)>& search_function,
         const SearchOrder search_order = SearchOrder::Forward,
         const std::optional<size_t> max_depth = SearchDepth::DirectChildren) const;
 
@@ -65,32 +67,33 @@ class PrintOperationSequence : public PrintOperation, public std::enable_shared_
     std::shared_ptr<OperationType>
         findOperationByType(const SearchOrder search_order = SearchOrder::Forward, const std::optional<size_t> max_depth = SearchDepth::DirectChildren) const;
 
-    const std::vector<std::shared_ptr<PrintOperation>>& getOperations() const noexcept;
-
-    std::vector<std::shared_ptr<PrintOperation>>& getOperations() noexcept;
+    const std::vector<PrintOperationPtr>& getOperations() const noexcept;
 
     template<class OperationType>
     std::vector<std::shared_ptr<OperationType>> getOperationsAs() noexcept;
 
-    void setOperations(std::vector<std::shared_ptr<PrintOperation>>& operations) noexcept;
+    // void setOperations(std::vector<PrintOperationPtr>& operations) noexcept;
+
+    template<class OperationType>
+    void setOperations(std::vector<std::shared_ptr<OperationType>>& operations) noexcept;
 
 protected:
-    void appendOperation(const std::shared_ptr<PrintOperation>& operation);
+    void appendOperation(const PrintOperationPtr& operation);
 
-    void removeOperation(const std::shared_ptr<PrintOperation>& operation);
+    void removeOperation(const PrintOperationPtr& operation);
 
     template<class ChildType>
     void applyProcessorToOperationsRecursively(PrintOperationTransformer<ChildType>& processor);
 
 private:
-    std::vector<std::shared_ptr<PrintOperation>> operations_;
+    std::vector<PrintOperationPtr> operations_;
 };
 
 template<class OperationType>
 std::shared_ptr<OperationType> PrintOperationSequence::findOperationByType(const SearchOrder search_order, const std::optional<size_t> max_depth) const
 {
-    std::shared_ptr<PrintOperation> found_operation = findOperation(
-        [](const std::shared_ptr<PrintOperation>& operation)
+    PrintOperationPtr found_operation = findOperation(
+        [](const PrintOperationPtr& operation)
         {
             return static_cast<bool>(std::dynamic_pointer_cast<OperationType>(operation));
         },
@@ -126,6 +129,32 @@ std::vector<std::shared_ptr<OperationType>> PrintOperationSequence::getOperation
     return result;
 }
 
+template<class OperationType>
+void PrintOperationSequence::setOperations(std::vector<std::shared_ptr<OperationType>>& operations) noexcept
+{
+    for (const PrintOperationPtr& removed_operation : operations_)
+    {
+        if (! ranges::contains(operations, removed_operation))
+        {
+            removed_operation->setParent({});
+        }
+    }
+
+    for (const PrintOperationPtr& added_operation : operations)
+    {
+        if (! ranges::contains(operations_, added_operation))
+        {
+            added_operation->setParent(weak_from_this());
+        }
+    }
+
+    operations_.resize(operations.size());
+    for (size_t index = 0; index < operations.size(); ++index)
+    {
+        operations_[index] = operations[index];
+    }
+}
+
 template<class ChildType>
 void PrintOperationSequence::applyProcessorToOperationsRecursively(PrintOperationTransformer<ChildType>& processor)
 {
@@ -144,5 +173,3 @@ void PrintOperationSequence::applyProcessorToOperationsRecursively(PrintOperatio
 }
 
 } // namespace cura
-
-#endif // PATHPLANNING_PRINTOPERATIONSEQUENCE_H
diff --git a/include/print_operation/TravelRoute.h b/include/print_operation/TravelRoute.h
index f4af70ba7a..7acc6a1c9b 100644
--- a/include/print_operation/TravelRoute.h
+++ b/include/print_operation/TravelRoute.h
@@ -4,12 +4,15 @@
 #ifndef PATHPLANNING_TRAVELROUTE_H
 #define PATHPLANNING_TRAVELROUTE_H
 
+#include "path_planning/SpeedDerivatives.h"
 #include "print_operation/ContinuousExtruderMoveSequence.h"
 
 namespace cura
 {
 
+enum class PrintFeatureType : unsigned char;
 class TravelMove;
+struct Velocity;
 
 class TravelRoute : public ContinuousExtruderMoveSequence
 {
diff --git a/src/operation_transformation/DirectTravelMoveGenerator.cpp b/src/operation_transformation/DirectTravelMoveGenerator.cpp
index 5576ab353c..69a573c97c 100644
--- a/src/operation_transformation/DirectTravelMoveGenerator.cpp
+++ b/src/operation_transformation/DirectTravelMoveGenerator.cpp
@@ -3,6 +3,7 @@
 
 #include "operation_transformation/DirectTravelMoveGenerator.h"
 
+#include "PrintFeatureType.h"
 #include "print_operation/TravelMove.h"
 #include "print_operation/TravelRoute.h"
 
diff --git a/src/operation_transformation/InsertOperationsProcessor.cpp b/src/operation_transformation/InsertOperationsProcessor.cpp
index 100d2d8ac8..11fa11ee37 100644
--- a/src/operation_transformation/InsertOperationsProcessor.cpp
+++ b/src/operation_transformation/InsertOperationsProcessor.cpp
@@ -10,7 +10,7 @@ namespace cura
 
 void InsertOperationsProcessor::process(PrintOperationSequence* operation)
 {
-    std::vector<std::shared_ptr<PrintOperation>>& child_operations = operation->getOperations();
+    std::vector<std::shared_ptr<PrintOperation>> child_operations = operation->getOperations();
     for (size_t index_first = 0; index_first < child_operations.size(); ++index_first)
     {
         const std::shared_ptr<PrintOperation>& operation_first = child_operations[index_first];
@@ -35,6 +35,8 @@ void InsertOperationsProcessor::process(PrintOperationSequence* operation)
             }
         }
     }
+
+    operation->setOperations(child_operations);
 }
 
 } // namespace cura
diff --git a/src/operation_transformation/LayerPlanTravelMovesInserter.cpp b/src/operation_transformation/LayerPlanTravelMovesInserter.cpp
index 2c12232032..ba9df610eb 100644
--- a/src/operation_transformation/LayerPlanTravelMovesInserter.cpp
+++ b/src/operation_transformation/LayerPlanTravelMovesInserter.cpp
@@ -31,33 +31,39 @@ void LayerPlanTravelMovesInserter::process(LayerPlan* layer_plan)
 
 void LayerPlanTravelMovesInserter::appendTravelsMovesBetweenChildren(const std::shared_ptr<PrintOperationSequence>& sequence, const SpeedDerivatives& speed)
 {
-    std::vector<std::shared_ptr<PrintOperation>>& child_operations = sequence->getOperations();
-    for (size_t index_first = 0; index_first < child_operations.size(); ++index_first)
+    std::vector<std::shared_ptr<PrintOperation>> child_operations = sequence->getOperations();
+    for (size_t index_first = 0; index_first < child_operations.size() - 1; ++index_first)
     {
         const std::shared_ptr<PrintOperation>& operation_first = child_operations[index_first];
         std::optional<Point3LL> first_end_position = operation_first->findEndPosition();
-        if (first_end_position.has_value())
+        if (! first_end_position.has_value())
         {
-            for (size_t index_second = index_first + 1; index_second < child_operations.size(); ++index_second)
+            continue;
+        }
+
+        for (size_t index_second = index_first + 1; index_second < child_operations.size(); ++index_second)
+        {
+            const std::shared_ptr<PrintOperation>& operation_second = child_operations[index_second];
+            std::optional<Point3LL> second_start_position = operation_second->findStartPosition();
+            if (! second_start_position.has_value())
             {
-                const std::shared_ptr<PrintOperation>& operation_second = child_operations[index_second];
-                std::optional<Point3LL> second_start_position = operation_second->findStartPosition();
-                if (second_start_position.has_value())
-                {
-                    if (const std::shared_ptr<PrintOperation> travel_move = makeTravelMove(first_end_position.value(), second_start_position.value(), speed))
-                    {
-                        child_operations.insert(std::next(child_operations.begin(), index_second), travel_move);
-                        index_first = index_second;
-                    }
-                    else
-                    {
-                        index_first = index_second - 1;
-                    }
-                    break;
-                }
+                continue;
             }
+
+            if (const std::shared_ptr<PrintOperation> travel_move = makeTravelRoute(first_end_position.value(), second_start_position.value(), speed))
+            {
+                child_operations.insert(std::next(child_operations.begin(), index_second), travel_move);
+                index_first = index_second;
+            }
+            else
+            {
+                index_first = index_second - 1;
+            }
+            break;
         }
     }
+
+    sequence->setOperations(child_operations);
 }
 
 void LayerPlanTravelMovesInserter::appendTravelMovesRecursively(const std::shared_ptr<PrintOperation>& operation, const SpeedDerivatives& speed)
@@ -73,7 +79,7 @@ void LayerPlanTravelMovesInserter::appendTravelMovesRecursively(const std::share
     }
 }
 
-const std::shared_ptr<PrintOperation> LayerPlanTravelMovesInserter::makeTravelMove(const Point3LL& start_position, const Point3LL& end_position, const SpeedDerivatives& speed)
+const std::shared_ptr<PrintOperation> LayerPlanTravelMovesInserter::makeTravelRoute(const Point3LL& start_position, const Point3LL& end_position, const SpeedDerivatives& speed)
 {
     if (end_position != start_position)
     {
diff --git a/src/print_operation/ContinuousExtruderMoveSequence.cpp b/src/print_operation/ContinuousExtruderMoveSequence.cpp
index 2b662a5275..7606af8931 100644
--- a/src/print_operation/ContinuousExtruderMoveSequence.cpp
+++ b/src/print_operation/ContinuousExtruderMoveSequence.cpp
@@ -28,7 +28,7 @@ void ContinuousExtruderMoveSequence::reorderToEndWith(const std::shared_ptr<Extr
 {
     if (closed_)
     {
-        std::vector<std::shared_ptr<PrintOperation>>& operations = getOperations();
+        std::vector<std::shared_ptr<PrintOperation>> operations = getOperations();
         if (operations.size() > 1)
         {
             auto iterator = std::find(operations.begin(), operations.end(), extruder_move);
@@ -44,6 +44,8 @@ void ContinuousExtruderMoveSequence::reorderToEndWith(const std::shared_ptr<Extr
             {
                 spdlog::warn("Unable to change sequence ordering because the given move is not part of the sequence");
             }
+
+            setOperations(operations);
         }
     }
     else
@@ -80,7 +82,8 @@ void ContinuousExtruderMoveSequence::reverse()
             }
         }
 
-        std::reverse(getOperations().begin(), getOperations().end());
+        std::reverse(operations.begin(), operations.end());
+        setOperations(operations);
     }
 }
 
diff --git a/src/print_operation/PrintOperation.cpp b/src/print_operation/PrintOperation.cpp
index 86f44f5a3a..754573e70f 100644
--- a/src/print_operation/PrintOperation.cpp
+++ b/src/print_operation/PrintOperation.cpp
@@ -46,7 +46,8 @@ std::shared_ptr<PrintOperationSequence>
 
         return parent_ptr->findParent(search_function, warn_not_found);
     }
-    else if (warn_not_found)
+
+    if (warn_not_found)
     {
         spdlog::warn("Couldn't find appropriate parent");
     }
diff --git a/src/print_operation/PrintOperationSequence.cpp b/src/print_operation/PrintOperationSequence.cpp
index c03c914112..10aba532c2 100644
--- a/src/print_operation/PrintOperationSequence.cpp
+++ b/src/print_operation/PrintOperationSequence.cpp
@@ -3,7 +3,13 @@
 
 #include "print_operation/PrintOperationSequence.h"
 
+#include <range/v3/algorithm/contains.hpp>
+#include <range/v3/algorithm/copy_if.hpp>
+#include <range/v3/algorithm/remove_if.hpp>
 #include <range/v3/view/remove.hpp>
+#include <range/v3/view/set_algorithm.hpp>
+
+#include "print_operation/PrintOperationPtr.h"
 
 namespace cura
 {
@@ -15,7 +21,7 @@ bool PrintOperationSequence::empty() const noexcept
 
 void PrintOperationSequence::write(PlanExporter& exporter) const
 {
-    for (const std::shared_ptr<PrintOperation>& operation : operations_)
+    for (const PrintOperationPtr& operation : operations_)
     {
         operation->write(exporter);
     }
@@ -28,7 +34,7 @@ void PrintOperationSequence::applyProcessors(const std::vector<const PrintOperat
     std::vector<const PrintOperation*> new_parents = parents;
     new_parents.push_back(this);
 
-    for (const std::shared_ptr<PrintOperation>& operation : operations_)
+    for (const PrintOperationPtr& operation : operations_)
     {
         operation->applyProcessors(new_parents);
     }
@@ -36,7 +42,7 @@ void PrintOperationSequence::applyProcessors(const std::vector<const PrintOperat
 
 std::optional<Point3LL> PrintOperationSequence::findStartPosition() const
 {
-    for (const std::shared_ptr<PrintOperation>& operation : operations_)
+    for (const PrintOperationPtr& operation : operations_)
     {
         std::optional<Point3LL> start_position = operation->findStartPosition();
         if (start_position.has_value())
@@ -50,7 +56,7 @@ std::optional<Point3LL> PrintOperationSequence::findStartPosition() const
 
 std::optional<Point3LL> PrintOperationSequence::findEndPosition() const
 {
-    for (const std::shared_ptr<PrintOperation>& operation : operations_ | ranges::views::reverse)
+    for (const PrintOperationPtr& operation : operations_ | ranges::views::reverse)
     {
         std::optional<Point3LL> end_position = operation->findEndPosition();
         if (end_position.has_value())
@@ -62,7 +68,7 @@ std::optional<Point3LL> PrintOperationSequence::findEndPosition() const
     return std::nullopt;
 }
 
-void PrintOperationSequence::appendOperation(const std::shared_ptr<PrintOperation>& operation)
+void PrintOperationSequence::appendOperation(const PrintOperationPtr& operation)
 {
     if (std::shared_ptr<PrintOperationSequence> actual_parent = operation->getParent())
     {
@@ -70,11 +76,10 @@ void PrintOperationSequence::appendOperation(const std::shared_ptr<PrintOperatio
     }
 
     operations_.push_back(operation);
-    auto shared_this = weak_from_this();
-    operation->setParent(shared_this);
+    operation->setParent(weak_from_this());
 }
 
-void PrintOperationSequence::removeOperation(const std::shared_ptr<PrintOperation>& operation)
+void PrintOperationSequence::removeOperation(const PrintOperationPtr& operation)
 {
     if (operation->getParent() == shared_from_this())
     {
@@ -87,8 +92,8 @@ void PrintOperationSequence::removeOperation(const std::shared_ptr<PrintOperatio
     }
 }
 
-std::shared_ptr<PrintOperation> PrintOperationSequence::findOperation(
-    const std::function<bool(const std::shared_ptr<PrintOperation>&)>& search_function,
+PrintOperationPtr PrintOperationSequence::findOperation(
+    const std::function<bool(const PrintOperationPtr&)>& search_function,
     const SearchOrder search_order,
     const std::optional<size_t> max_depth) const
 {
@@ -96,11 +101,11 @@ std::shared_ptr<PrintOperation> PrintOperationSequence::findOperation(
     {
         const std::optional<size_t> next_depth = max_depth.has_value() ? max_depth.value() - 1 : max_depth;
 
-        const auto find_depth_first = [&search_function, &search_order, &next_depth](auto begin, auto end) -> std::shared_ptr<PrintOperation>
+        const auto find_depth_first = [&search_function, &search_order, &next_depth](auto begin, auto end) -> PrintOperationPtr
         {
             for (auto iterator = begin; iterator != end; ++iterator)
             {
-                const std::shared_ptr<PrintOperation>& operation = *iterator;
+                const PrintOperationPtr& operation = *iterator;
                 if (search_function(operation))
                 {
                     return operation;
@@ -128,7 +133,7 @@ std::shared_ptr<PrintOperation> PrintOperationSequence::findOperation(
     }
     else
     {
-        auto find_in = [&search_function](auto begin, auto end) -> std::shared_ptr<PrintOperation>
+        auto find_in = [&search_function](auto begin, auto end) -> PrintOperationPtr
         {
             auto iterator = std::find_if(begin, end, search_function);
             if (iterator != end)
@@ -151,19 +156,30 @@ std::shared_ptr<PrintOperation> PrintOperationSequence::findOperation(
     return nullptr;
 }
 
-const std::vector<std::shared_ptr<PrintOperation>>& PrintOperationSequence::getOperations() const noexcept
+const std::vector<PrintOperationPtr>& PrintOperationSequence::getOperations() const noexcept
 {
     return operations_;
 }
 
-std::vector<std::shared_ptr<PrintOperation>>& PrintOperationSequence::getOperations() noexcept
-{
-    return operations_;
-}
-
-void PrintOperationSequence::setOperations(std::vector<std::shared_ptr<PrintOperation>>& operations) noexcept
-{
-    operations_ = operations;
-}
+// void PrintOperationSequence::setOperations(std::vector<PrintOperationPtr>& operations) noexcept
+// {
+//     for (const PrintOperationPtr& removed_operation : operations_)
+//     {
+//         if (! ranges::contains(operations, removed_operation))
+//         {
+//             removed_operation->setParent({});
+//         }
+//     }
+//
+//     for (const PrintOperationPtr& added_operation : operations)
+//     {
+//         if (! ranges::contains(operations_, added_operation))
+//         {
+//             added_operation->setParent(weak_from_this());
+//         }
+//     }
+//
+//     operations_ = operations;
+// }
 
 } // namespace cura