fix #242

src/libslic3r/PerimeterGenerator.cpp

@@ -107,7 +107,6 @@ void PerimeterGenerator::process()
     Surfaces all_surfaces = this->slices->surfaces;
 
     //store surface for bridge infill to avoid unsupported perimeters (but the first one, this one is always good)
-    
     if (this->config->no_perimeter_unsupported_algo != npuaNone
         && this->lower_slices != NULL && !this->lower_slices->empty()) {
 
@@ -683,8 +682,10 @@ void PerimeterGenerator::process()
             // TODO: add test for perimeter order
             if (this->config->external_perimeters_first ||
                 (this->layer->id() == 0 && this->object_config->brim_width.value > 0)) {
-                if (this->config->external_perimeters_nothole.value) {
+                if (this->config->external_perimeters_nothole.value ||
-                    if (this->config->external_perimeters_hole.value) {
+                    (this->layer->id() == 0 && this->object_config->brim_width.value > 0)) {
+                    if (this->config->external_perimeters_hole.value ||
+                        (this->layer->id() == 0 && this->object_config->brim_width.value > 0)) {
                         entities.reverse();
                     } else {
                         //reverse only not-hole perimeters
@@ -931,26 +932,39 @@ ExtrusionEntityCollection PerimeterGenerator::_traverse_loops(
     Point zero_point(0, 0);
     //result is  [idx, needReverse] ?
     std::vector<std::pair<size_t, bool>> chain = chain_extrusion_entities(coll.entities, &zero_point);
-    ExtrusionEntityCollection entities;
+    ExtrusionEntityCollection coll_out;
+
+    //little check: if you have external holes with only one extrusion and internal things, please draw the internal first, just in case it can help print the hole better.
+    std::vector<std::pair<size_t, bool>> better_chain;
     for (const std::pair<size_t, bool>& idx : chain) {
+        if (!loops[idx.first].is_external() || (!loops[idx.first].is_contour && !loops[idx.first].children.empty()))
+            better_chain.push_back(idx);
+    }
+    for (const std::pair<size_t, bool>& idx : chain) {
+        if (loops[idx.first].is_external() && !(!loops[idx.first].is_contour && !loops[idx.first].children.empty()))
+            better_chain.push_back(idx);
+    }
+
+    //move from coll to coll_out and gettign children of each in the same time. (deep first)
+    for (const std::pair<size_t, bool> &idx : better_chain) {
         
         if (idx.first >= loops.size()) {
             // this is a thin wall
             // let's get it from the sorted collection as it might have been reversed
-            entities.entities.reserve(entities.entities.size() + 1);
+            coll_out.entities.reserve(coll_out.entities.size() + 1);
-            entities.entities.emplace_back(coll.entities[idx.first]);
+            coll_out.entities.emplace_back(coll.entities[idx.first]);
             coll.entities[idx.first] = nullptr;
             if (idx.second)
-                entities.entities.back()->reverse();
+                coll_out.entities.back()->reverse();
             //if thin extrusion is a loop, make it ccw like a normal contour.
-            if (ExtrusionLoop* loop = dynamic_cast<ExtrusionLoop*>(entities.entities.back())) {
+            if (ExtrusionLoop* loop = dynamic_cast<ExtrusionLoop*>(coll_out.entities.back())) {
                 loop->make_counter_clockwise();
             }
         } else {
             const PerimeterGeneratorLoop &loop = loops[idx.first];
             assert(thin_walls.empty());
             ExtrusionEntityCollection children = this->_traverse_loops(loop.children, thin_walls);
-            entities.entities.reserve(entities.entities.size() + children.entities.size() + 1);
+            coll_out.entities.reserve(coll_out.entities.size() + children.entities.size() + 1);
             ExtrusionLoop *eloop = static_cast<ExtrusionLoop*>(coll.entities[idx.first]);
             coll.entities[idx.first] = nullptr;
             if (loop.is_contour) {
@@ -959,16 +973,16 @@ ExtrusionEntityCollection PerimeterGenerator::_traverse_loops(
                     eloop->make_clockwise();
                 else
                     eloop->make_counter_clockwise();
-                entities.append(std::move(children.entities));
+                coll_out.append(std::move(children.entities));
-                entities.append(*eloop);
+                coll_out.append(*eloop);
             } else {
                 eloop->make_clockwise();
-                entities.append(*eloop);
+                coll_out.append(*eloop);
-                entities.append(std::move(children.entities));
+                coll_out.append(std::move(children.entities));
             }
         }
     }
-    return entities;
+    return coll_out;
 }
 
 PerimeterIntersectionPoint

src/libslic3r/PerimeterGenerator.hpp

@@ -43,7 +43,7 @@ public:
         polygon(polygon), is_contour(is_contour), depth(depth), is_steep_overhang(is_steep_overhang) {}
     // External perimeter. It may be CCW or CW oriented (outer contour or hole contour).
     bool is_external() const { return this->depth == 0; }
-    // An island, which may have holes, but it does not have another internal island.
+    // it's the last loop of the contour (not hol), so the first to be printed (if all goes well)
     bool is_internal_contour() const;
 };
 

tests/superslicerlibslic3r/test_print.cpp

@@ -1,5 +1,5 @@
 
-#define CATCH_CONFIG_DISABLE
+//#define CATCH_CONFIG_DISABLE
 
 //#include <catch_main.hpp>
 #include <catch2/catch.hpp>
@@ -176,3 +176,41 @@ SCENARIO("Print: Brim generation") {
         }
     }
 }
+
+SCENARIO("Print: perimeter generation") {
+    GIVEN("cube with hole, just enough space for two loops at a point") {
+        DynamicPrintConfig& config = Slic3r::DynamicPrintConfig::full_print_config();
+        Slic3r::Model model{};
+        config.set_key_value("first_layer_extrusion_width", new ConfigOptionFloatOrPercent(0.42, false));
+        config.set_deserialize("nozzle_diameter", "0.4");
+        config.set_deserialize("layer_height", "0.2");
+        config.set_deserialize("first_layer_height", "0.2");
+        config.set_key_value("only_one_perimeter_top", new ConfigOptionBool(false));
+        Print print{};
+        auto facets = std::vector<Vec3i32>{
+                Vec3i32(1,4,3),Vec3i32(4,1,2),Vec3i32(16,12,14),Vec3i32(16,10,12),Vec3i32(10,4,6),Vec3i32(4,10,16),Vec3i32(8,14,12),Vec3i32(8,2,14),
+                    Vec3i32(6,2,8),Vec3i32(2,6,4),Vec3i32(14,15,16),Vec3i32(15,14,13),Vec3i32(15,4,16),Vec3i32(4,15,3),Vec3i32(13,11,15),Vec3i32(13,7,11),
+                    Vec3i32(7,1,5),Vec3i32(1,7,13),Vec3i32(9,15,11),Vec3i32(9,3,15),Vec3i32(5,3,9),Vec3i32(3,5,1),Vec3i32(1,14,2),Vec3i32(14,1,13),
+                    Vec3i32(9,12,10),Vec3i32(12,9,11),Vec3i32(6,9,10),Vec3i32(9,6,5),Vec3i32(8,5,6),Vec3i32(5,8,7),Vec3i32(7,12,11),Vec3i32(12,7,8)
+        };
+        for (Vec3i32& vec : facets)
+            vec -= Vec3i32(1, 1, 1);
+        TriangleMesh tm = TriangleMesh{ std::vector<Vec3d>{Vec3d(-5,-5,-0.1),Vec3d(-5,-5,0.1),Vec3d(-5,5,-0.1),Vec3d(-5,5,0.1),
+            Vec3d(-1.328430,0,-0.1),Vec3d(-1.328430,0,0.1),Vec3d(1.5,-2.828430,-0.1),Vec3d(1.5,-2.828430,0.1),
+            Vec3d(1.5,2.828430,-0.1),Vec3d(1.5,2.828430,0.1),Vec3d(4.328430,0,-0.1),Vec3d(4.328430,0,0.1),
+            Vec3d(5,-5,-0.1),Vec3d(5,-5,0.1),Vec3d(5,5,-0.1),Vec3d(5,5,0.1)},
+             facets };
+        Slic3r::Test::init_print(print, {tm}, model, &config);
+        print.process();
+        THEN("hole perimeter should not be printed first") {
+            ExtrusionEntity* loop = print.objects()[0]->layers()[0]->regions()[0]->perimeters.entities[0];
+            REQUIRE(loop->is_collection());
+            loop = ((ExtrusionEntityCollection*)loop)->entities.front();
+            REQUIRE(loop->is_loop());
+            // what we don't want in first is something that is (((ExtrusionLoop*)loop)->loop_role() & ExtrusionLoopRole::elrHole) != 0 && loop->role() == elrExternalPerimeter
+            REQUIRE( (((ExtrusionLoop*)loop)->loop_role() & ExtrusionLoopRole::elrHole) == 0);
+
+        }
+    }
+}
+