Return Surface objects by reference from SurfaceCollection objects and fix a bug in XS code causing some shell options to be ignored

lib/Slic3r/Fill.pm

@@ -54,20 +54,15 @@ sub make_fill {
     
     my @surfaces = ();
     
-    # if hollow object is requested, remove internal surfaces
-    # (this needs to be done after internal-solid shells are created)
-    if ($fill_density == 0) {
-        @surfaces = grep $_->surface_type != S_TYPE_INTERNAL, @surfaces;
-    }
-    
     # merge adjacent surfaces
     # in case of bridge surfaces, the ones with defined angle will be attached to the ones
     # without any angle (shouldn't this logic be moved to process_external_surfaces()?)
     {
-        my @surfaces_with_bridge_angle = grep defined $_->bridge_angle, @{$layerm->fill_surfaces};
+        my @fill_surfaces = @{$layerm->fill_surfaces};
+        my @surfaces_with_bridge_angle = grep defined $_->bridge_angle, @fill_surfaces;
         
         # give priority to bridges
-        my @groups = Slic3r::Surface->group({merge_solid => 1}, @{$layerm->fill_surfaces});
+        my @groups = Slic3r::Surface->group({merge_solid => 1}, @fill_surfaces);
         @groups = sort { defined $a->[0]->bridge_angle ? -1 : 0 } @groups;
         
         foreach my $group (@groups) {

lib/Slic3r/Layer/Region.pm

@@ -412,39 +412,30 @@ sub _fill_gaps {
 sub prepare_fill_surfaces {
     my $self = shift;
     
-    my @surfaces = @{$self->fill_surfaces};
-    
     # if no solid layers are requested, turn top/bottom surfaces to internal
     if ($self->config->top_solid_layers == 0) {
-        for my $i (0..$#surfaces) {
+        $_->surface_type(S_TYPE_INTERNAL) for @{$self->fill_surfaces->filter_by_type(S_TYPE_TOP)};
-            next unless $surfaces[$i]->surface_type == S_TYPE_TOP;
-            $self->fill_surfaces->set_surface_type($i, S_TYPE_INTERNAL);
-        }
     }
     if ($self->config->bottom_solid_layers == 0) {
-        for my $i (0..$#surfaces) {
+        $_->surface_type(S_TYPE_INTERNAL) for @{$self->fill_surfaces->filter_by_type(S_TYPE_BOTTOM)};
-            next unless $surfaces[$i]->surface_type == S_TYPE_BOTTOM;
-            $self->fill_surfaces->set_surface_type($i, S_TYPE_INTERNAL);
-        }
     }
         
     # turn too small internal regions into solid regions according to the user setting
     if ($self->config->fill_density > 0) {
         my $min_area = scale scale $self->config->solid_infill_below_area; # scaling an area requires two calls!
-        for my $i (0..$#surfaces) {
+        $_->surface_type(S_TYPE_INTERNALSOLID)
-            next unless $surfaces[$i]->surface_type == S_TYPE_INTERNAL && $surfaces[$i]->expolygon->contour->area <= $min_area;
+            for grep { $_->area <= $min_area } @{$self->fill_surfaces->filter_by_type(S_TYPE_INTERNAL)};
-            $self->fill_surfaces->set_surface_type($i, S_TYPE_INTERNALSOLID);
-        }
     }
 }
 
 sub process_external_surfaces {
     my $self = shift;
     
+    my @surfaces = @{$self->fill_surfaces};
     my $margin = scale &Slic3r::EXTERNAL_INFILL_MARGIN;
     
     my @bottom = ();
-    foreach my $surface (grep $_->surface_type == S_TYPE_BOTTOM, @{$self->fill_surfaces}) {
+    foreach my $surface (grep $_->surface_type == S_TYPE_BOTTOM, @surfaces) {
         my $grown = $surface->expolygon->offset_ex(+$margin);
         
         # detect bridge direction before merging grown surfaces otherwise adjacent bridges
@@ -459,7 +450,7 @@ sub process_external_surfaces {
     }
     
     my @top = ();
-    foreach my $surface (grep $_->surface_type == S_TYPE_TOP, @{$self->fill_surfaces}) {
+    foreach my $surface (grep $_->surface_type == S_TYPE_TOP, @surfaces) {
         # give priority to bottom surfaces
         my $grown = diff_ex(
             $surface->expolygon->offset(+$margin),
@@ -471,8 +462,8 @@ sub process_external_surfaces {
     # if we're slicing with no infill, we can't extend external surfaces
     # over non-existent infill
     my @fill_boundaries = $self->object->config->fill_density > 0
-        ? @{$self->fill_surfaces}
+        ? @surfaces
-        : grep $_->surface_type != S_TYPE_INTERNAL, @{$self->fill_surfaces};
+        : grep $_->surface_type != S_TYPE_INTERNAL, @surfaces;
     
     # intersect the grown surfaces with the actual fill boundaries
     my @new_surfaces = ();
@@ -487,14 +478,15 @@ sub process_external_surfaces {
     }
     
     # subtract the new top surfaces from the other non-top surfaces and re-add them
-    my @other = grep $_->surface_type != S_TYPE_TOP && $_->surface_type != S_TYPE_BOTTOM, @{$self->fill_surfaces};
+    my @other = grep $_->surface_type != S_TYPE_TOP && $_->surface_type != S_TYPE_BOTTOM, @surfaces;
     foreach my $group (Slic3r::Surface->group(@other)) {
         push @new_surfaces, map $group->[0]->clone(expolygon => $_), @{diff_ex(
             [ map $_->p, @$group ],
             [ map $_->p, @new_surfaces ],
         )};
     }
-    @{$self->fill_surfaces} = @new_surfaces;
+    $self->fill_surfaces->clear;
+    $self->fill_surfaces->append(@new_surfaces);
 }
 
 sub _detect_bridge_direction {

lib/Slic3r/Print/Object.pm

@@ -464,11 +464,11 @@ sub clip_fill_surfaces {
                 ),
                 @{intersection_ex(
                     [ map @$_, @overhangs ],
-                    [ map @{$_->expolygon}, grep $_->surface_type == S_TYPE_INTERNAL, @{$layerm->fill_surfaces} ],
+                    [ map $_->p, @{$layerm->fill_surfaces->filter_by_type(S_TYPE_INTERNAL)} ],
                 )};
             my @new_surfaces = (
                 @new_internal,
-                (map $_->clone, grep $_->surface_type != S_TYPE_INTERNAL, @{$layerm->fill_surfaces}),
+                (map $_->clone, @{$layerm->fill_surfaces->filter_by_type(S_TYPE_INTERNAL)}),
             );
             $layerm->fill_surfaces->clear;
             $layerm->fill_surfaces->append(@new_surfaces);
@@ -502,8 +502,8 @@ sub bridge_over_infill {
         
         foreach my $layerm (@{$layer->regions}) {
             # compute the areas needing bridge math 
-            my @internal_solid = grep $_->surface_type == S_TYPE_INTERNALSOLID, @{$layerm->fill_surfaces};
+            my @internal_solid = @{$layerm->fill_surfaces->filter_by_type(S_TYPE_INTERNALSOLID)};
-            my @lower_internal = grep $_->surface_type == S_TYPE_INTERNAL, map @{$_->fill_surfaces}, @{$lower_layer->regions};
+            my @lower_internal = map @{$_->fill_surfaces->filter_by_type(S_TYPE_INTERNAL)}, @{$lower_layer->regions};
             my $to_bridge = intersection_ex(
                 [ map $_->p, @internal_solid ],
                 [ map $_->p, @lower_internal ],
@@ -578,11 +578,7 @@ sub discover_horizontal_shells {
             
             if ($self->config->solid_infill_every_layers && $self->config->fill_density > 0
                 && ($i % $self->config->solid_infill_every_layers) == 0) {
-                my @surfaces = @{$layerm->fill_surfaces};
+                $_->surface_type(S_TYPE_INTERNALSOLID) for @{$layerm->fill_surfaces->filter_by_type(S_TYPE_INTERNAL)};
-                for my $i (0..$#surfaces) {
-                    next unless $surfaces[$i]->surface_type == S_TYPE_INTERNAL;
-                    $layerm->fill_surfaces->set_surface_type($i, S_TYPE_INTERNALSOLID);
-                }
             }
             
             EXTERNAL: foreach my $type (S_TYPE_TOP, S_TYPE_BOTTOM) {
@@ -593,7 +589,7 @@ sub discover_horizontal_shells {
                 # not work in some situations, as there won't be any grown region in the perimeter 
                 # area (this was seen in a model where the top layer had one extra perimeter, thus
                 # its fill_surfaces was thinner than the lower layer's infill)
-                my $solid = offset_ex([ map $_->p, grep $_->surface_type == $type, @{$layerm->slices} ], $margin);
+                my $solid = offset_ex([ map $_->p, @{$layerm->slices->filter_by_type($type)} ], $margin);
                 next if !@$solid;
                 Slic3r::debugf "Layer %d has %s surfaces\n", $i, ($type == S_TYPE_TOP) ? 'top' : 'bottom';
                 
@@ -745,13 +741,13 @@ sub combine_infill {
                 # we need to perform a multi-layer intersection, so let's split it in pairs
                 
                 # initialize the intersection with the candidates of the lowest layer
-                my $intersection = [ map $_->expolygon, grep $_->surface_type == $type, @{$layerms[0]->fill_surfaces} ];
+                my $intersection = [ map $_->expolygon, @{$layerms[0]->fill_surfaces->filter_by_type($type)} ];
                 
                 # start looping from the second layer and intersect the current intersection with it
                 for my $layerm (@layerms[1 .. $#layerms]) {
                     $intersection = intersection_ex(
                         [ map @$_, @$intersection ],
-                        [ map @{$_->expolygon}, grep $_->surface_type == $type, @{$layerm->fill_surfaces} ],
+                        [ map @{$_->expolygon}, @{$layerm->fill_surfaces->filter_by_type($type)} ],
                     );
                 }
                 
@@ -778,12 +774,12 @@ sub combine_infill {
 
                 
                 foreach my $layerm (@layerms) {
-                    my @this_type   = grep $_->surface_type == $type, @{$layerm->fill_surfaces};
+                    my @this_type   = @{$layerm->fill_surfaces->filter_by_type($type)};
                     my @other_types = map $_->clone, grep $_->surface_type != $type, @{$layerm->fill_surfaces};
                     
                     my @new_this_type = map Slic3r::Surface->new(expolygon => $_, surface_type => $type),
                         @{diff_ex(
-                            [ map @{$_->expolygon}, @this_type ],
+                            [ map $_->p, @this_type ],
                             [ @intersection_with_clearance ],
                         )};
                     

xs/lib/Slic3r/XS.pm

@@ -181,6 +181,11 @@ sub clone {
     );
 }
 
+package Slic3r::Surface::Ref;
+our @ISA = 'Slic3r::Surface';
+
+sub DESTROY {}
+
 package Slic3r::Surface::Collection;
 use overload
     '@{}' => sub { $_[0]->arrayref },

xs/src/Surface.cpp

@@ -0,0 +1,18 @@
+#include "Surface.hpp"
+
+namespace Slic3r {
+
+SV*
+Surface::to_SV_ref() {
+    SV* sv = newSV(0);
+    sv_setref_pv( sv, "Slic3r::Surface::Ref", (void*)this );
+    return sv;
+}
+
+double
+Surface::area() const
+{
+    return this->expolygon.area();
+}
+
+}

xs/src/Surface.hpp

@@ -16,6 +16,8 @@ class Surface
     unsigned short  thickness_layers;   // in layers
     double          bridge_angle;
     unsigned short  extra_perimeters;
+    SV* to_SV_ref();
+    double area() const;
 };
 
 typedef std::vector<Surface> Surfaces;

xs/t/05_surface.t

@@ -4,7 +4,7 @@ use strict;
 use warnings;
 
 use Slic3r::XS;
-use Test::More tests => 13;
+use Test::More tests => 14;
 
 my $square = [  # ccw
     [100, 100],
@@ -58,8 +58,9 @@ is $surface->extra_perimeters, 2, 'extra_perimeters';
     is scalar(@$collection), 1, 'append to collection';
     
     my $item = $collection->[0];
+    isa_ok $item, 'Slic3r::Surface::Ref';
     $item->surface_type(Slic3r::Surface::S_TYPE_INTERNAL);
-    isnt $item->surface_type, $collection->[0]->surface_type, 'collection returns copies of items';
+    is $item->surface_type, $collection->[0]->surface_type, 'collection returns items by reference';
 }
 
 __END__

xs/xsp/Surface.xsp

@@ -13,6 +13,7 @@
         %code{% RETVAL = THIS->thickness; %};
     unsigned short thickness_layers()
         %code{% RETVAL = THIS->thickness_layers; %};
+    double area();
 %{
 
 Surface*

xs/xsp/SurfaceCollection.xsp

@@ -31,9 +31,19 @@ SurfaceCollection::arrayref()
         av_fill(av, THIS->surfaces.size()-1);
         int i = 0;
         for (Surfaces::iterator it = THIS->surfaces.begin(); it != THIS->surfaces.end(); ++it) {
-            SV* sv = newSV(0);
+            av_store(av, i++, (*it).to_SV_ref());
-            sv_setref_pv( sv, "Slic3r::Surface", new Surface(*it) );
+        }
-            av_store(av, i++, sv);
+        RETVAL = newRV_noinc((SV*)av);
+    OUTPUT:
+        RETVAL
+
+SV*
+SurfaceCollection::filter_by_type(surface_type)
+    SurfaceType     surface_type;
+    CODE:
+        AV* av = newAV();
+        for (Surfaces::iterator it = THIS->surfaces.begin(); it != THIS->surfaces.end(); ++it) {
+            if ((*it).surface_type == surface_type) av_push(av, (*it).to_SV_ref());
         }
         RETVAL = newRV_noinc((SV*)av);
     OUTPUT:
@@ -43,6 +53,7 @@ void
 SurfaceCollection::append(...)
     CODE:
         for (unsigned int i = 1; i < items; i++) {
+            // Note: a COPY of the input is stored
             THIS->surfaces.push_back(*(Surface *)SvIV((SV*)SvRV( ST(i) )));
         }