Workaround for detect_surfaces_type() not being idempotent and causing artifacts after multiple runs. #3764

lib/Slic3r/Print/Object.pm

@@ -125,28 +125,44 @@ sub slice {
 sub make_perimeters {
     my ($self) = @_;
     
+    return if $self->step_done(STEP_PERIMETERS);
+    
+    # Temporary workaround for detect_surfaces_type() not being idempotent (see #3764).
+    # We can remove this when idempotence is restored. This make_perimeters() method
+    # will just call merge_slices() to undo the typed slices and invalidate posDetectSurfaces.
+    if ($self->typed_slices) {
+        $self->invalidate_step(STEP_SLICE);
+    }
+    
     # prerequisites
     $self->slice;
     
     $self->_make_perimeters;
 }
 
+# This will assign a type (top/bottom/internal) to $layerm->slices
+# and transform $layerm->fill_surfaces from expolygon 
+# to typed top/bottom/internal surfaces;
+sub detect_surfaces_type {
+    my ($self) = @_;
+    
+    # prerequisites
+    $self->slice;
+    
+    $self->_detect_surfaces_type;
+}
+
 sub prepare_infill {
     my ($self) = @_;
     
     # prerequisites
-    $self->make_perimeters;
+    $self->make_perimeters;  # do we need them? TODO: check
+    $self->detect_surfaces_type;
     
     return if $self->step_done(STEP_PREPARE_INFILL);
     $self->set_step_started(STEP_PREPARE_INFILL);
     $self->print->status_cb->(30, "Preparing infill");
     
-    # this will assign a type (top/bottom/internal) to $layerm->slices
-    # and transform $layerm->fill_surfaces from expolygon 
-    # to typed top/bottom/internal surfaces;
-    $self->detect_surfaces_type;
-    $self->set_typed_slices(1);
-    
     # decide what surfaces are to be filled
     $_->prepare_fill_surfaces for map @{$_->regions}, @{$self->layers};
 

xs/src/libslic3r/Layer.cpp

@@ -197,11 +197,11 @@ Layer::make_perimeters()
             
             // merge the surfaces assigned to each group
             SurfaceCollection new_slices;
-            for (std::map<unsigned short,Surfaces>::const_iterator it = slices.begin(); it != slices.end(); ++it) {
+            for (const auto &it : slices) {
-                ExPolygons expp = union_ex(it->second, true);
+                ExPolygons expp = union_ex(it.second, true);
-                for (ExPolygons::iterator ex = expp.begin(); ex != expp.end(); ++ex) {
+                for (ExPolygon &ex : expp) {
-                    Surface s = it->second.front();  // clone type and extra_perimeters
+                    Surface s = it.second.front();  // clone type and extra_perimeters
-                    s.expolygon = *ex;
+                    s.expolygon = ex;
                     new_slices.surfaces.push_back(s);
                 }
             }
@@ -274,6 +274,8 @@ Layer::detect_surfaces_type()
         Layer* const &lower_layer = this->lower_layer;
     
         // collapse very narrow parts (using the safety offset in the diff is not enough)
+        // TODO: this offset2 makes this method not idempotent (see #3764), so we should
+        // move it to where we generate fill_surfaces instead and leave slices unaltered
         const float offs = layerm.flow(frExternalPerimeter).scaled_width() / 10.f;
 
         const Polygons layerm_slices_surfaces = layerm.slices;

xs/src/libslic3r/Print.hpp

@@ -26,8 +26,8 @@ enum PrintStep {
     psSkirt, psBrim,
 };
 enum PrintObjectStep {
-    posSlice, posPerimeters, posPrepareInfill,
+    posSlice, posPerimeters, posDetectSurfaces,
-    posInfill, posSupportMaterial,
+    posPrepareInfill, posInfill, posSupportMaterial,
 };
 
 // To be instantiated over PrintStep or PrintObjectStep enums.

xs/src/libslic3r/PrintObject.cpp

@@ -311,6 +311,8 @@ PrintObject::invalidate_step(PrintObjectStep step)
         this->invalidate_step(posPrepareInfill);
         this->_print->invalidate_step(psSkirt);
         this->_print->invalidate_step(psBrim);
+    } else if (step == posDetectSurfaces) {
+        this->invalidate_step(posPrepareInfill);
     } else if (step == posPrepareInfill) {
         this->invalidate_step(posInfill);
     } else if (step == posInfill) {
@@ -318,6 +320,7 @@ PrintObject::invalidate_step(PrintObjectStep step)
         this->_print->invalidate_step(psBrim);
     } else if (step == posSlice) {
         this->invalidate_step(posPerimeters);
+        this->invalidate_step(posDetectSurfaces);
         this->invalidate_step(posSupportMaterial);
     } else if (step == posSupportMaterial) {
         this->_print->invalidate_step(psSkirt);
@@ -351,11 +354,20 @@ PrintObject::has_support_material() const
 void
 PrintObject::detect_surfaces_type()
 {
+    // prerequisites
+    // this->slice();
+    
+    if (this->state.is_done(posDetectSurfaces)) return;
+    this->state.set_started(posDetectSurfaces);
+    
     parallelize<Layer*>(
         std::queue<Layer*>(std::deque<Layer*>(this->layers.begin(), this->layers.end())),  // cast LayerPtrs to std::queue<Layer*>
         boost::bind(&Slic3r::Layer::detect_surfaces_type, _1),
         this->_print->config.threads.value
     );
+    
+    this->typed_slices = true;
+    this->state.set_done(posDetectSurfaces);
 }
 
 void
@@ -835,11 +847,15 @@ PrintObject::_make_perimeters()
     this->state.set_started(posPerimeters);
     
     // merge slices if they were split into types
+    // This is not currently taking place because since merge_slices + detect_surfaces_type
+    // are not truly idempotent we are invalidating posSlice here (see the Perl part of 
+    // this method).
     if (this->typed_slices) {
+        // merge_slices() undoes detect_surfaces_type()
         FOREACH_LAYER(this, layer_it)
             (*layer_it)->merge_slices();
         this->typed_slices = false;
-        this->state.invalidate(posPrepareInfill);
+        this->state.invalidate(posDetectSurfaces);
     }
     
     // compare each layer to the one below, and mark those slices needing

xs/xsp/Print.xsp

@@ -14,6 +14,7 @@ _constant()
   ALIAS:
     STEP_SLICE              = posSlice
     STEP_PERIMETERS         = posPerimeters
+    STEP_DETECT_SURFACES    = posDetectSurfaces
     STEP_PREPARE_INFILL     = posPrepareInfill
     STEP_INFILL             = posInfill
     STEP_SUPPORTMATERIAL    = posSupportMaterial
@@ -123,7 +124,7 @@ _constant()
     void set_step_started(PrintObjectStep step)
         %code%{ THIS->state.set_started(step); %};
     
-    void detect_surfaces_type();
+    %name{_detect_surfaces_type} void detect_surfaces_type();
     void process_external_surfaces();
     void bridge_over_infill();
     void _slice();