Fix skirt/Brim (#4669)

* Start implementing skirt/brim tests.

* Move skirt_height_z (representing the highest skirt height in actual Z coordinates) to Print and prime it with make_brim so it can be used in PrintGCode.

Updated test to correctly verify skirt is generated for one and multiple objects.

* Fallout from the config refactor; use correct accessors.

* Fix bug where brim was not generated (inverted logic)

* Finish porting tests, left one intentional failure as support material is not generated yet (segfaults).

src/test/libslic3r/test_skirt_brim.cpp

@@ -1,68 +1,118 @@
 #include <catch.hpp>
 #include "test_data.hpp" // get access to init_print, etc
 #include "GCodeReader.hpp"
+#include "Config.hpp"
+#include "Geometry.hpp"
+#include <regex>
 
 using namespace Slic3r::Test;
 using namespace Slic3r;
 
-SCENARIO("Original Slic3r Skirt/Brim tests", "[!mayfail]") {
-    GIVEN("Configuration with a skirt height of 2") {
-        auto config {Config::new_from_defaults()};
-        config->set("skirts", 1);
-        config->set("skirt_height", 2);
-        config->set("perimeters", 0);
-        config->set("support_material_speed", 99);
-
-        // avoid altering speeds unexpectedly
-        config->set("cooling", 0);
-        config->set("first_layer_speed", "100%");
+/// Helper method to find the tool used for the brim (always the first extrusion)
+int get_brim_tool(std::stringstream& gcode, Slic3r::GCodeReader& parser) {
+    int brim_tool = -1;
+    std::regex tool_regex("^T(\\d+)");
+    std::smatch m;
+    int tool = -1;
 
-        WHEN("multiple objects are printed") {
-            auto gcode {std::stringstream("")};
-            Slic3r::Model model;
-            auto print {Slic3r::Test::init_print({TestMesh::cube_20x20x20, TestMesh::cube_20x20x20}, model, config)};
-            std::map<double, bool> layers_with_skirt;
-            Slic3r::Test::gcode(gcode, print);
-            auto parser {Slic3r::GCodeReader()};
-            parser.parse_stream(gcode, [&layers_with_skirt, &config] (Slic3r::GCodeReader& self, const Slic3r::GCodeReader::GCodeLine& line) 
-            {
-                if (self.Z > 0) {
-                    if (line.extruding() && line.new_F() == config->getFloat("support_material_speed") * 60.0) {
-                        layers_with_skirt[self.Z] = 1;
-                    }
+    parser.parse_stream(gcode, [&tool, &brim_tool, &m, tool_regex] (Slic3r::GCodeReader& self, const Slic3r::GCodeReader::GCodeLine& line)
+        {
+            // if the command is a T command, set the the current tool
+            if (std::regex_match(line.cmd, m, tool_regex)) {
+                tool = std::stoi(m[1].str());
+            } else if (line.cmd == "G1" && line.extruding() && line.dist_XY() > 0 && brim_tool < 0) {
+                brim_tool = tool;
                 }
-            });
+        });
 
-            THEN("skirt_height is honored") {
-                REQUIRE(layers_with_skirt.size() == (size_t)config->getInt("skirt_height"));
-            }
-        }
+    return brim_tool;
+}
+
+
+TEST_CASE("Skirt height is honored") {
+    auto config {Config::new_from_defaults()};
+    config->set("skirts", 1);
+    config->set("skirt_height", 5);
+    config->set("perimeters", 0);
+    config->set("support_material_speed", 99);
+
+    // avoid altering speeds unexpectedly
+    config->set("cooling", false);
+    config->set("first_layer_speed", "100%");
+    auto support_speed = config->get<ConfigOptionFloat>("support_material_speed") * MM_PER_MIN;
+
+    std::map<double, bool> layers_with_skirt;
+    auto gcode {std::stringstream("")};
+    gcode.clear();
+    auto parser {Slic3r::GCodeReader()};
+    Slic3r::Model model;
+
+    SECTION("printing a single object") {
+        auto print {Slic3r::Test::init_print({TestMesh::cube_20x20x20}, model, config)};
+        Slic3r::Test::gcode(gcode, print);
     }
+
+    SECTION("printing multiple objects") {
+        auto print {Slic3r::Test::init_print({TestMesh::cube_20x20x20, TestMesh::cube_20x20x20}, model, config)};
+        Slic3r::Test::gcode(gcode, print);
+    }
+    parser.parse_stream(gcode, [&layers_with_skirt, &support_speed] (Slic3r::GCodeReader& self, const Slic3r::GCodeReader::GCodeLine& line)
+    {
+        if (line.extruding() && self.F == Approx(support_speed)) {
+            layers_with_skirt[self.Z] = 1;
+        }
+    });
+
+    REQUIRE(layers_with_skirt.size() == (size_t)config->getInt("skirt_height"));
+}
+
+SCENARIO("Original Slic3r Skirt/Brim tests", "[!mayfail]") {
+    auto parser {Slic3r::GCodeReader()};
+    Slic3r::Model model;
+    auto gcode {std::stringstream("")};
+    gcode.clear();
     GIVEN("A default configuration") {
         auto config {Config::new_from_defaults()};
         config->set("support_material_speed", 99);
+        config->set("first_layer_height", 0.3);
+        config->set("gcode_comments", true);
 
         // avoid altering speeds unexpectedly
-        config->set("cooling", 0);
+        config->set("cooling", false);
         config->set("first_layer_speed", "100%");
         // remove noise from top/solid layers
         config->set("top_solid_layers", 0);
-        config->set("bottom_solid_layers", 0);
+        config->set("bottom_solid_layers", 1);
 
         WHEN("Brim width is set to 5") {
             config->set("perimeters", 0);
             config->set("skirts", 0);
             config->set("brim_width", 5);
             THEN("Brim is generated") {
-                REQUIRE(false);
+                auto print {Slic3r::Test::init_print({TestMesh::cube_20x20x20}, model, config)};
+                Slic3r::Test::gcode(gcode, print);
+                bool brim_generated = false;
+                auto support_speed = config->get<ConfigOptionFloat>("support_material_speed") * MM_PER_MIN;
+                parser.parse_stream(gcode, [&brim_generated, support_speed] (Slic3r::GCodeReader& self, const Slic3r::GCodeReader::GCodeLine& line)
+                    {
+                        if (self.Z == Approx(0.3) || line.new_Z() == Approx(0.3)) {
+                            if (line.extruding() && self.F == Approx(support_speed)) {
+                                brim_generated = true;
+                            }
+                        }
+                    });
+                REQUIRE(brim_generated);
             }
         }
 
         WHEN("Skirt area is smaller than the brim") {
             config->set("skirts", 1);
             config->set("brim_width", 10);
-            THEN("GCode generates successfully.") {
-                REQUIRE(false);
+            auto print {Slic3r::Test::init_print({TestMesh::cube_20x20x20}, model, config)};
+            THEN("Gcode generates") {
+                Slic3r::Test::gcode(gcode, print);
+                auto exported {gcode.str()};
+                REQUIRE(exported.size() > 0);
             }
         }
 
@@ -70,19 +120,37 @@ SCENARIO("Original Slic3r Skirt/Brim tests", "[!mayfail]") {
             config->set("skirts", 2);
             config->set("skirt_height", 0);
 
-            THEN("GCode generates successfully.") {
-                REQUIRE(false);
+            auto print {Slic3r::Test::init_print({TestMesh::cube_20x20x20}, model, config)};
+            THEN("Gcode generates") {
+                Slic3r::Test::gcode(gcode, print);
+                auto exported {gcode.str()};
+                REQUIRE(exported.size() > 0);
             }
         }
 
         WHEN("Perimeter extruder = 2 and support extruders = 3") {
+            config->set("skirts", 0);
+            config->set("brim_width", 5);
+            config->set("perimeter_extruder", 2);
+            config->set("support_material_extruder", 3);
             THEN("Brim is printed with the extruder used for the perimeters of first object") {
-                REQUIRE(false);
+                auto print {Slic3r::Test::init_print({TestMesh::cube_20x20x20}, model, config)};
+                Slic3r::Test::gcode(gcode, print);
+                int tool = get_brim_tool(gcode, parser);
+                REQUIRE(tool == config->getInt("perimeter_extruder") - 1);
             }
         }
         WHEN("Perimeter extruder = 2, support extruders = 3, raft is enabled") {
+            config->set("skirts", 0);
+            config->set("brim_width", 5);
+            config->set("perimeter_extruder", 2);
+            config->set("support_material_extruder", 3);
+            config->set("raft_layers", 1);
             THEN("brim is printed with same extruder as skirt") {
-                REQUIRE(false);
+                auto print {Slic3r::Test::init_print({TestMesh::cube_20x20x20}, model, config)};
+                Slic3r::Test::gcode(gcode, print);
+                int tool = get_brim_tool(gcode, parser);
+                REQUIRE(tool == config->getInt("support_material_extruder") - 1);
             }
         }
 
@@ -94,17 +162,55 @@ SCENARIO("Original Slic3r Skirt/Brim tests", "[!mayfail]") {
             config->set("support_material_speed", 99);
             config->set("perimeter_extruder", 1);
             config->set("support_material_extruder", 2);
-            config->set("cooling", 0);                     // to prevent speeds to be altered
+            config->set("infill_extruder", 3); // ensure that a tool command gets emitted.
+            config->set("cooling", false);                     // to prevent speeds to be altered
             config->set("first_layer_speed", "100%");      // to prevent speeds to be altered
 
             Slic3r::Model model;
             auto print {Slic3r::Test::init_print({TestMesh::overhang}, model, config)};
             print->process();
-            
-            config->set("support_material", true);      // to prevent speeds to be altered
+
+            // config->set("support_material", true);      // to prevent speeds to be altered
 
             THEN("skirt length is large enough to contain object with support") {
-                REQUIRE(false);
+                CHECK(config->getBool("support_material")); // test is not valid if support material is off
+                double skirt_length = 0.0;
+                Points extrusion_points;
+                int tool = -1;
+                std::regex tool_regex("^T(\\d+)");
+                std::smatch m;
+
+                auto print {Slic3r::Test::init_print({TestMesh::cube_20x20x20}, model, config)};
+                Slic3r::Test::gcode(gcode, print);
+
+                auto support_speed = config->get<ConfigOptionFloat>("support_material_speed") * MM_PER_MIN;
+                parser.parse_stream(gcode, [tool_regex, &m, config, &extrusion_points, &tool, &skirt_length, support_speed] (Slic3r::GCodeReader& self, const Slic3r::GCodeReader::GCodeLine& line)
+                    {
+                        std::cerr << line.cmd << "\n";
+                        if (std::regex_match(line.cmd, m, tool_regex)) {
+                            tool = std::stoi(m[1].str());
+                        } else if (self.Z == Approx(config->get<ConfigOptionFloat>("first_layer_height").value)) {
+                            // on first layer
+                            if (line.extruding() && line.dist_XY() > 0) {
+                                auto speed = ( self.F > 0 ?  self.F : line.new_F());
+                                std::cerr << "Tool " << tool << "\n";
+                                if (speed == Approx(support_speed) && tool == config->getInt("perimeter_extruder") - 1) {
+                                    // Skirt uses first material extruder, support material speed.
+                                    skirt_length += line.dist_XY();
+                                } else {
+                                    extrusion_points.push_back(Slic3r::Point::new_scale(line.new_X(), line.new_Y()));
+                                }
+                            }
+                        }
+
+                        if (self.Z == Approx(0.3) || line.new_Z() == Approx(0.3)) {
+                            if (line.extruding() && self.F == Approx(support_speed)) {
+                            }
+                        }
+                    });
+                auto convex_hull = Slic3r::Geometry::convex_hull(extrusion_points);
+                auto hull_perimeter = unscale(convex_hull.split_at_first_point().length());
+                REQUIRE(skirt_length > hull_perimeter);
             }
         }
         WHEN("Large minimum skirt length is used.") {

src/test/test_data.hpp

@@ -11,6 +11,8 @@
 
 namespace Slic3r { namespace Test {
 
+constexpr double MM_PER_MIN = 60.0;
+
 /// Enumeration of test meshes
 enum class TestMesh {
     A,

xs/src/libslic3r/Print.cpp

@@ -172,7 +172,7 @@ Print::make_skirt()
     // include the thickest object first. It is just guaranteed that a skirt is
     // prepended to the first 'n' layers (with 'n' = skirt_height).
     // $skirt_height_z in this case is the highest possible skirt height for safety.
-    double skirt_height_z {-1.0};
+    this->skirt_height_z = -1.0;
     for (const auto* object : this->objects) {
         const size_t skirt_height {
             this->has_infinite_skirt()
@@ -180,7 +180,7 @@ Print::make_skirt()
                 : std::min(size_t(this->config.skirt_height()), object->layer_count())
         };
         const Layer* highest_layer { object->get_layer(skirt_height - 1) };
-        skirt_height_z = std::max(skirt_height_z, highest_layer->print_z);
+        this->skirt_height_z = std::max(skirt_height_z, highest_layer->print_z);
     }
 
     // collect points from all layers contained in skirt height
@@ -188,16 +188,16 @@ Print::make_skirt()
     for (auto* object : this->objects) {
         Points object_points;
         
-        // get object layers up to skirt_height_z
+        // get object layers up to this->skirt_height_z
         for (const auto* layer : object->layers) {
-            if (layer->print_z > skirt_height_z) break;
+            if (layer->print_z > this->skirt_height_z) break;
             for (const ExPolygon ex : layer->slices)
                 append_to(object_points, static_cast<Points>(ex));
         }
         
-        // get support layers up to skirt_height_z
+        // get support layers up to this->skirt_height_z
         for (const auto* layer : object->support_layers) {
-            if (layer->print_z > skirt_height_z) break;
+            if (layer->print_z > this->skirt_height_z) break;
             for (auto* ee : layer->support_fills)
                 append_to(object_points, ee->as_polyline().points);
             for (auto* ee : layer->support_interface_fills)

xs/src/libslic3r/Print.hpp

@@ -243,6 +243,7 @@ class Print
 
     // ordered collections of extrusion paths to build skirt loops and brim
     ExtrusionEntityCollection skirt, brim;
+    double skirt_height_z {-1.0};
 
     Print();
     ~Print();

xs/src/libslic3r/PrintGCode.cpp

@@ -491,8 +491,11 @@ PrintGCode::process_layer(size_t idx, const Layer* layer, const Points& copies)
 
     // extrude skirt along raft layers and normal obj layers
     // (not along interlaced support material layers)
+
     if (layer->id() < static_cast<size_t>(obj.config.raft_layers)
-        || ((_print.has_infinite_skirt() || _skirt_done.size() == 0 || (_skirt_done.rbegin())->first < _print.config.skirt_height)
+        || ((_print.has_infinite_skirt()
+        || _skirt_done.size() == 0
+        || (_skirt_done.rbegin())->first < scale_(_print.skirt_height_z))
         && _skirt_done.count(scale_(layer->print_z)) == 0
         && typeid(layer) != typeid(SupportLayer*)) ) {
 
@@ -547,7 +550,7 @@ PrintGCode::process_layer(size_t idx, const Layer* layer, const Points& copies)
     }
 
     // extrude brim
-    if (this->_brim_done) {
+    if (!this->_brim_done) {
         gcode += _gcodegen.set_extruder(_print.brim_extruder() - 1);
         _gcodegen.set_origin(Pointf(0,0));
         _gcodegen.avoid_crossing_perimeters.use_external_mp = true;