The inwards move after an external loop was still randomly generated outwards in some cases. Perimeters are now generated with a distinct iterator for each slice. Nested islands are also correctly supported too. Various regression tests included. #2253

lib/Slic3r/GCode.pm

@@ -207,8 +207,11 @@ sub extrude_loop {
         my $last_path_polyline = $paths[-1]->polyline;
         # detect angle between last and first segment
         # the side depends on the original winding order of the polygon (left for contours, right for holes)
-        my @points = $was_clockwise ? (-2, 1) : (1, -2);
+        my @points = ($paths[0][1], $paths[-1][-2]);
-        my $angle = Slic3r::Geometry::angle3points(@$last_path_polyline[0, @points]) / 3;
+        @points = reverse @points if $was_clockwise;
+        my $angle = $paths[0]->first_point->ccw_angle(@points) / 3;
+        
+        # turn left if contour, turn right if hole
         $angle *= -1 if $was_clockwise;
         
         # create the destination point along the first segment and rotate it

lib/Slic3r/Layer/PerimeterGenerator.pm

@@ -8,7 +8,7 @@ use Slic3r::Geometry::Clipper qw(union_ex diff diff_ex intersection_ex offset of
     offset_ex offset2_ex union_pt intersection_ppl diff_ppl);
 use Slic3r::Surface ':types';
 
-has 'slices'                => (is => 'ro', required => 1);
+has 'slices'                => (is => 'ro', required => 1);  # SurfaceCollection
 has 'lower_slices'          => (is => 'ro', required => 0);
 has 'layer_height'          => (is => 'ro', required => 1);
 has 'layer_id'              => (is => 'ro', required => 0, default => sub { -1 });
@@ -16,9 +16,9 @@ has 'perimeter_flow'        => (is => 'ro', required => 1);
 has 'ext_perimeter_flow'    => (is => 'ro', required => 1);
 has 'overhang_flow'         => (is => 'ro', required => 1);
 has 'solid_infill_flow'     => (is => 'ro', required => 1);
-has 'config'                => (is => 'ro', default => sub { Slic3r::Config::Region->new });
+has 'config'                => (is => 'ro', default => sub { Slic3r::Config::PrintRegion->new });
 has 'print_config'          => (is => 'ro', default => sub { Slic3r::Config::Print->new });
-has '_lower_slices_p'       => (is => 'rw');
+has '_lower_slices_p'       => (is => 'rw', default => sub { [] });
 has '_holes_pt'             => (is => 'rw');
 has '_ext_mm3_per_mm'       => (is => 'rw');
 has '_mm3_per_mm'           => (is => 'rw');
@@ -34,6 +34,19 @@ has 'gap_fill'      => (is => 'ro', default => sub { Slic3r::ExtrusionPath::Coll
 # generated fill surfaces will be put here
 has 'fill_surfaces' => (is => 'ro', default => sub { Slic3r::Surface::Collection->new });
 
+sub BUILDARGS {
+    my ($class, %args) = @_;
+    
+    if (my $flow = delete $args{flow}) {
+        $args{perimeter_flow}       //= $flow;
+        $args{ext_perimeter_flow}   //= $flow;
+        $args{overhang_flow}        //= $flow;
+        $args{solid_infill_flow}    //= $flow;
+    }
+    
+    return { %args };
+}
+
 sub process {
     my ($self) = @_;
     
@@ -62,13 +75,25 @@ sub process {
     my $min_spacing         = $pspacing * (1 - &Slic3r::INSET_OVERLAP_TOLERANCE);
     my $ext_min_spacing     = $ext_pspacing * (1 - &Slic3r::INSET_OVERLAP_TOLERANCE);
     
-    my @contours    = ();    # array of Polygons with ccw orientation
+    # prepare grown lower layer slices for overhang detection
-    my @holes       = ();    # array of Polygons with cw orientation
+    if ($self->lower_slices && $self->config->overhangs) {
-    my @thin_walls  = ();    # array of ExPolygons
+        # We consider overhang any part where the entire nozzle diameter is not supported by the
+        # lower layer, so we take lower slices and offset them by half the nozzle diameter used 
+        # in the current layer
+        my $nozzle_diameter = $self->print_config->get_at('nozzle_diameter', $self->config->perimeter_extruder-1);
+        
+        $self->_lower_slices_p(
+            offset([ map @$_, @{$self->lower_slices} ], scale +$nozzle_diameter/2)
+        );
+    }
     
     # we need to process each island separately because we might have different
     # extra perimeters for each one
     foreach my $surface (@{$self->slices}) {
+        my @contours    = ();    # array of Polygons with ccw orientation
+        my @holes       = ();    # array of Polygons with cw orientation
+        my @thin_walls  = ();    # array of ExPolygons
+        
         # detect how many perimeters must be generated for this island
         my $loop_number = $self->config->perimeters + ($surface->extra_perimeters || 0);
         
@@ -194,60 +219,48 @@ sub process {
                     -($pspacing/2 + $min_perimeter_infill_spacing/2),
                     +$min_perimeter_infill_spacing/2,
                 )};
-    }
     
     
-    # process thin walls by collapsing slices to single passes
+        # process thin walls by collapsing slices to single passes
-    if (@thin_walls) {
+        if (@thin_walls) {
-        # the following offset2 ensures almost nothing in @thin_walls is narrower than $min_width
+            # the following offset2 ensures almost nothing in @thin_walls is narrower than $min_width
-        # (actually, something larger than that still may exist due to mitering or other causes)
+            # (actually, something larger than that still may exist due to mitering or other causes)
-        my $min_width = $pwidth / 4;
+            my $min_width = $pwidth / 4;
-        @thin_walls = @{offset2_ex([ map @$_, @thin_walls ], -$min_width/2, +$min_width/2)};
+            @thin_walls = @{offset2_ex([ map @$_, @thin_walls ], -$min_width/2, +$min_width/2)};
         
-        # the maximum thickness of our thin wall area is equal to the minimum thickness of a single loop
+            # the maximum thickness of our thin wall area is equal to the minimum thickness of a single loop
-        $self->_thin_wall_polylines([ map @{$_->medial_axis($pwidth + $pspacing, $min_width)}, @thin_walls ]);
+            $self->_thin_wall_polylines([ map @{$_->medial_axis($pwidth + $pspacing, $min_width)}, @thin_walls ]);
-        Slic3r::debugf "  %d thin walls detected\n", scalar(@{$self->_thin_wall_polylines}) if $Slic3r::debug;
+            Slic3r::debugf "  %d thin walls detected\n", scalar(@{$self->_thin_wall_polylines}) if $Slic3r::debug;
         
-        if (0) {
+            if (0) {
-            require "Slic3r/SVG.pm";
+                require "Slic3r/SVG.pm";
-            Slic3r::SVG::output(
+                Slic3r::SVG::output(
-                "medial_axis.svg",
+                    "medial_axis.svg",
-                no_arrows => 1,
+                    no_arrows => 1,
-                expolygons      => \@thin_walls,
+                    expolygons      => \@thin_walls,
-                green_polylines => [ map $_->polygon->split_at_first_point, @{$self->perimeters} ],
+                    green_polylines => [ map $_->polygon->split_at_first_point, @{$self->perimeters} ],
-                red_polylines   => $self->_thin_wall_polylines,
+                    red_polylines   => $self->_thin_wall_polylines,
-            );
+                );
+            }
         }
+        
+        # find nesting hierarchies separately for contours and holes
+        my $contours_pt = union_pt(\@contours);
+        $self->_holes_pt(union_pt(\@holes));
+    
+        # order loops from inner to outer (in terms of object slices)
+        my @loops = $self->_traverse_pt($contours_pt, 0, 1);
+    
+        # if brim will be printed, reverse the order of perimeters so that
+        # we continue inwards after having finished the brim
+        # TODO: add test for perimeter order
+        @loops = reverse @loops
+            if $self->config->external_perimeters_first
+                || ($self->layer_id == 0 && $self->print_config->brim_width > 0);
+        
+        # append perimeters for this slice as a collection
+        $self->loops->append(Slic3r::ExtrusionPath::Collection->new(@loops));
     }
-    
-    # find nesting hierarchies separately for contours and holes
-    my $contours_pt = union_pt(\@contours);
-    $self->_holes_pt(union_pt(\@holes));
-    
-    # prepare grown lower layer slices for overhang detection
-    my $lower_slices = Slic3r::ExPolygon::Collection->new;
-    if ($self->lower_slices && $self->config->overhangs) {
-        # We consider overhang any part where the entire nozzle diameter is not supported by the
-        # lower layer, so we take lower slices and offset them by half the nozzle diameter used 
-        # in the current layer
-        my $nozzle_diameter = $self->print_config->get_at('nozzle_diameter', $self->config->perimeter_extruder-1);
-        $lower_slices->append($_)
-            for @{offset_ex([ map @$_, @{$self->lower_slices} ], scale +$nozzle_diameter/2)};
-    }
-    $self->_lower_slices_p($lower_slices->polygons);
-    
-    # order loops from inner to outer (in terms of object slices)
-    my @loops = $self->_traverse_pt($contours_pt, 0, 1);
-    
-    # if brim will be printed, reverse the order of perimeters so that
-    # we continue inwards after having finished the brim
-    # TODO: add test for perimeter order
-    @loops = reverse @loops
-        if $self->config->external_perimeters_first
-            || ($self->layer_id == 0 && $self->print_config->brim_width > 0);
-    
-    # append perimeters
-    $self->loops->append($_) for @loops;
 }
 
 sub _traverse_pt {
@@ -328,9 +341,8 @@ sub _traverse_pt {
         # return ccw contours and cw holes
         # GCode.pm will convert all of them to ccw, but it needs to know
         # what the holes are in order to compute the correct inwards move
-        # We do this on the final Loop object instead of the polygon because
+        # We do this on the final Loop object because overhang clipping
-        # overhang clipping might have reversed its order since Clipper does
+        # does not keep orientation.
-        # not preserve polyline orientation.
         if ($is_contour) {
             $loop->make_counter_clockwise;
         } else {
@@ -358,10 +370,13 @@ sub _traverse_pt {
     
     # use a nearest neighbor search to order these children
     # TODO: supply second argument to chained_path() too?
-    # (We used to skip this chiained_path() when $is_contour &&
+    # (We used to skip this chained_path() when $is_contour &&
     # $depth == 0 because slices are ordered at G_code export 
     # time, but multiple top-level perimeters might belong to
     # the same slice actually, so that was a broken optimization.)
+    # We supply no_reverse = false because we want to permit reversal
+    # of thin walls, but we rely on the fact that loops will never
+    # be reversed anyway.
     my $sorted_collection = $collection->chained_path_indices(0);
     my @orig_indices = @{$sorted_collection->orig_indices};
     
@@ -407,7 +422,6 @@ sub _fill_gaps {
         1,
     );
     my @polylines = map @{$_->medial_axis($max, $min/2)}, @$this;
-    
     return if !@polylines;
     
     Slic3r::debugf "  %d gaps filled with extrusion width = %s\n", scalar @$this, $w

lib/Slic3r/Print/GCode.pm

@@ -424,17 +424,17 @@ sub process_layer {
             # process perimeters
             {
                 my $extruder_id = $region->config->perimeter_extruder-1;
-                foreach my $perimeter (@{$layerm->perimeters}) {
+                foreach my $perimeter_coll (@{$layerm->perimeters}) {
                     # init by_extruder item only if we actually use the extruder
                     $by_extruder{$extruder_id} //= [];
                     
-                    # $perimeter is an ExtrusionLoop or ExtrusionPath object
+                    # $perimeter_coll is an ExtrusionPath::Collection object representing a single slice
                     for my $i (0 .. $#{$layer->slices}) {
                         if ($i == $#{$layer->slices}
-                            || $layer->slices->[$i]->contour->contains_point($perimeter->first_point)) {
+                            || $layer->slices->[$i]->contour->contains_point($perimeter_coll->first_point)) {
                             $by_extruder{$extruder_id}[$i] //= { perimeters => {} };
                             $by_extruder{$extruder_id}[$i]{perimeters}{$region_id} //= [];
-                            push @{ $by_extruder{$extruder_id}[$i]{perimeters}{$region_id} }, $perimeter;
+                            push @{ $by_extruder{$extruder_id}[$i]{perimeters}{$region_id} }, @$perimeter_coll;
                             last;
                         }
                     }

lib/Slic3r/Print/SupportMaterial.pm

@@ -178,7 +178,7 @@ sub contact_area {
                         # TODO: split_at_first_point() could split a bridge mid-way
                         my @overhang_perimeters =
                             map { $_->isa('Slic3r::ExtrusionLoop') ? $_->polygon->split_at_first_point : $_->polyline->clone }
-                            @{$layerm->perimeters};
+                            map @$_, @{$layerm->perimeters};
                         
                         # workaround for Clipper bug, see Slic3r::Polygon::clip_as_polyline()
                         $_->[0]->translate(1,0) for @overhang_perimeters;

t/perimeters.t

@@ -1,4 +1,4 @@
-use Test::More tests => 11;
+use Test::More tests => 29;
 use strict;
 use warnings;
 
@@ -7,6 +7,8 @@ BEGIN {
     use lib "$FindBin::Bin/../lib";
 }
 
+use Slic3r::ExtrusionLoop ':roles';
+use Slic3r::ExtrusionPath ':roles';
 use List::Util qw(first);
 use Slic3r;
 use Slic3r::Flow ':roles';
@@ -188,7 +190,7 @@ use Slic3r::Test;
     my $pflow = $layerm->flow(FLOW_ROLE_PERIMETER);
     my $iflow = $layerm->flow(FLOW_ROLE_INFILL);
     my $covered_by_perimeters = union_ex([
-        (map @{$_->polygon->split_at_first_point->grow($pflow->scaled_width/2)}, @{$layerm->perimeters}),
+        (map @{$_->polygon->split_at_first_point->grow($pflow->scaled_width/2)}, map @$_, @{$layerm->perimeters}),
     ]);
     my $covered_by_infill = union_ex([
         (map $_->p, @{$layerm->fill_surfaces}),
@@ -285,4 +287,89 @@ use Slic3r::Test;
     $test->('small_dorito');
 }
 
+{
+    my $flow = Slic3r::Flow->new(
+        width           => 1,
+        height          => 1,
+        nozzle_diameter => 1,
+    );
+    
+    my $config = Slic3r::Config->new;
+    my $test = sub {
+        my ($expolygons, %expected) = @_;
+        
+        my $slices = Slic3r::Surface::Collection->new;
+        $slices->append(Slic3r::Surface->new(
+            surface_type => S_TYPE_INTERNAL,
+            expolygon => $_,
+        )) for @$expolygons;
+    
+        my $g = Slic3r::Layer::PerimeterGenerator->new(
+            # input:
+            layer_height    => 1,
+            slices          => $slices,
+            flow            => $flow,
+        );
+        $g->config->apply_dynamic($config);
+        $g->process;
+        
+        is scalar(@{$g->loops}),
+            scalar(@$expolygons), 'expected number of collections';
+        ok !defined(first { !$_->isa('Slic3r::ExtrusionPath::Collection') } @{$g->loops}),
+            'everything is returned as collections';
+        is scalar(map @$_, @{$g->loops}),
+            $expected{total}, 'expected number of loops';
+        is scalar(grep $_->role == EXTR_ROLE_EXTERNAL_PERIMETER, map @$_, map @$_, @{$g->loops}),
+            $expected{external}, 'expected number of external loops';
+        is scalar(grep $_->role == EXTRL_ROLE_CONTOUR_INTERNAL_PERIMETER, map @$_, @{$g->loops}),
+            $expected{cinternal}, 'expected number of internal contour loops';
+        is scalar(grep $_->polygon->is_counter_clockwise, map @$_, @{$g->loops}),
+            $expected{ccw}, 'expected number of ccw loops';
+        
+        return $g;
+    };
+    
+    $config->set('perimeters', 3);
+    $test->(
+        [
+            Slic3r::ExPolygon->new(
+                Slic3r::Polygon->new_scale([0,0], [100,0], [100,100], [0,100]),
+            ),
+        ],
+        total       => 3,
+        external    => 1,
+        cinternal   => 1,
+        ccw         => 3,
+    );
+    $test->(
+        [
+            Slic3r::ExPolygon->new(
+                Slic3r::Polygon->new_scale([0,0], [100,0], [100,100], [0,100]),
+                Slic3r::Polygon->new_scale([40,40], [40,60], [60,60], [60,40]),
+            ),
+        ],
+        total       => 6,
+        external    => 2,
+        cinternal   => 1,
+        ccw         => 3,
+    );
+    $test->(
+        [
+            Slic3r::ExPolygon->new(
+                Slic3r::Polygon->new_scale([0,0], [200,0], [200,200], [0,200]),
+                Slic3r::Polygon->new_scale([20,20], [20,180], [180,180], [180,20]),
+            ),
+            # nested:
+            Slic3r::ExPolygon->new(
+                Slic3r::Polygon->new_scale([50,50], [150,50], [150,150], [50,150]),
+                Slic3r::Polygon->new_scale([80,80], [80,120], [120,120], [120,80]),
+            ),
+        ],
+        total       => 4*3,
+        external    => 4,
+        cinternal   => 2,
+        ccw         => 2*3,
+    );
+}
+
 __END__

xs/src/libslic3r/ExtrusionEntity.hpp

@@ -35,6 +35,9 @@ enum ExtrusionLoopRole {
 class ExtrusionEntity
 {
     public:
+    virtual bool is_loop() const {
+        return false;
+    };
     virtual ExtrusionEntity* clone() const = 0;
     virtual ~ExtrusionEntity() {};
     virtual void reverse() = 0;
@@ -84,6 +87,9 @@ class ExtrusionLoop : public ExtrusionEntity
     ExtrusionLoopRole role;
     
     ExtrusionLoop(ExtrusionLoopRole role = elrDefault) : role(role) {};
+    bool is_loop() const {
+        return true;
+    };
     operator Polygon() const;
     ExtrusionLoop* clone() const;
     bool make_clockwise();

xs/src/libslic3r/ExtrusionEntityCollection.cpp

@@ -37,7 +37,9 @@ void
 ExtrusionEntityCollection::reverse()
 {
     for (ExtrusionEntitiesPtr::iterator it = this->entities.begin(); it != this->entities.end(); ++it) {
-        (*it)->reverse();
+        // Don't reverse it if it's a loop, as it doesn't change anything in terms of elements ordering
+        // and caller might rely on winding order
+        if (!(*it)->is_loop()) (*it)->reverse();
     }
     std::reverse(this->entities.begin(), this->entities.end());
 }
@@ -96,7 +98,8 @@ ExtrusionEntityCollection::chained_path_from(Point start_near, ExtrusionEntityCo
         int start_index = start_near.nearest_point_index(endpoints);
         int path_index = start_index/2;
         ExtrusionEntity* entity = my_paths.at(path_index);
-        if (start_index % 2 && !no_reverse) {
+        // never reverse loops, since it's pointless for chained path and callers might depend on orientation
+        if (start_index % 2 && !no_reverse && !entity->is_loop()) {
             entity->reverse();
         }
         retval->entities.push_back(my_paths.at(path_index));