Return objects by reference instead of always cloning

lib/Slic3r/Fill.pm

@@ -180,9 +180,10 @@ sub make_fill {
         # save into layer
         push @fills, my $collection = Slic3r::ExtrusionPath::Collection->new;
         $collection->no_sort($params->{no_sort});
+        
         $collection->append(
             map Slic3r::ExtrusionPath->new(
-                polyline => Slic3r::Polyline->new(@$_),
+                polyline => $_,
                 role => ($surface->surface_type == S_TYPE_INTERNALBRIDGE
                     ? EXTR_ROLE_INTERNALBRIDGE
                     : $is_bridge

lib/Slic3r/Fill/Honeycomb.pm

@@ -108,7 +108,9 @@ sub fill_surface {
                         next;
                     }
                 }
-                push @paths, $path;
+                
+                # make a clone before $collection goes out of scope
+                push @paths, $path->clone;
             }
         }
         

lib/Slic3r/Fill/Rectilinear.pm

@@ -91,7 +91,9 @@ sub fill_surface {
                     next;
                 }
             }
-            push @polylines, $polyline;
+            
+            # make a clone before $collection goes out of scope
+            push @polylines, $polyline->clone;
         }
     }
     

lib/Slic3r/Layer/Region.pm

@@ -252,7 +252,7 @@ sub make_perimeters {
         # use a nearest neighbor search to order these children
         # TODO: supply second argument to chained_path_items() too?
         my @nodes = @{Slic3r::Geometry::chained_path_items(
-            [ map [ Slic3r::Point->new(@{$_->{outer} ? $_->{outer}[0] : $_->{hole}[0]}), $_ ], @$polynodes ],
+            [ map [ ($_->{outer} // $_->{hole})->first_point->clone, $_ ], @$polynodes ],
         )};
         
         my @loops = ();
@@ -262,7 +262,9 @@ sub make_perimeters {
             # 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
-            my $polygon = Slic3r::Polygon->new(defined $polynode->{outer} ? @{$polynode->{outer}} : reverse @{$polynode->{hole}});
+            
+            my $polygon = ($polynode->{outer} // $polynode->{hole})->clone;
+            $polygon->reverse if defined $polynode->{hole};
             $polygon->reverse if !$is_contour;
             
             my $role = EXTR_ROLE_PERIMETER;
@@ -302,7 +304,7 @@ sub make_perimeters {
     # add thin walls as perimeters
     push @{ $self->perimeters }, @{Slic3r::ExtrusionPath::Collection->new(
         map Slic3r::ExtrusionPath->new(
-                polyline        => ($_->isa('Slic3r::Polygon') ? $_->split_at_first_point : $_),
+                polyline        => ($_->isa('Slic3r::Polygon') ? $_->split_at_first_point : $_->clone),
                 role            => EXTR_ROLE_EXTERNAL_PERIMETER,
                 flow_spacing    => $self->perimeter_flow->spacing,
         ), @{ $self->thin_walls }

xs/lib/Slic3r/XS.pm

@@ -12,11 +12,21 @@ use overload
     '@{}' => sub { $_[0]->arrayref },
     'fallback' => 1;
 
+package Slic3r::Line::Ref;
+our @ISA = 'Slic3r::Line';
+
+sub DESTROY {}
+
 package Slic3r::Point;
 use overload
     '@{}' => sub { $_[0]->arrayref },
     'fallback' => 1;
 
+package Slic3r::Point::Ref;
+our @ISA = 'Slic3r::Point';
+
+sub DESTROY {}
+
 package Slic3r::ExPolygon;
 use overload
     '@{}' => sub { $_[0]->arrayref },

xs/src/ClipperUtils.cpp

@@ -339,9 +339,9 @@ polynode2perl(const ClipperLib::PolyNode& node)
     Slic3r::Polygon p;
     ClipperPolygon_to_Slic3rPolygon(node.Contour, p);
     if (node.IsHole()) {
-        (void)hv_stores( hv, "hole", p.to_SV() );
+        (void)hv_stores( hv, "hole", p.to_SV_clone_ref() );
     } else {
-        (void)hv_stores( hv, "outer", p.to_SV() );
+        (void)hv_stores( hv, "outer", p.to_SV_clone_ref() );
     }
     (void)hv_stores( hv, "children", polynode_children_2_perl(node) );
     return (SV*)newRV_noinc((SV*)hv);

xs/src/ExPolygon.cpp

@@ -57,19 +57,23 @@ ExPolygon::to_SV() {
     av_extend(av, num_holes);  // -1 +1
     
     SV* sv = newSV(0);
-    sv_setref_pv( sv, "Slic3r::Polygon", new Polygon(this->contour) );
-    av_store(av, 0, sv);
+    av_store(av, 0, this->contour.to_SV_ref());
     
     for (unsigned int i = 0; i < num_holes; i++) {
-        sv = newSV(0);
-        sv_setref_pv( sv, "Slic3r::Polygon", new Polygon(this->holes[i]) );
-        av_store(av, i+1, sv);
+        av_store(av, i+1, this->holes[i].to_SV_ref());
     }
     return newRV_noinc((SV*)av);
 }
 
 SV*
 ExPolygon::to_SV_ref() {
+    SV* sv = newSV(0);
+    sv_setref_pv( sv, "Slic3r::ExPolygon::Ref", this );
+    return sv;
+}
+
+SV*
+ExPolygon::to_SV_clone_ref() {
     SV* sv = newSV(0);
     sv_setref_pv( sv, "Slic3r::ExPolygon", new ExPolygon(*this) );
     return sv;

xs/src/ExPolygon.hpp

@@ -15,6 +15,7 @@ class ExPolygon
     void from_SV_check(SV* poly_sv);
     SV* to_SV();
     SV* to_SV_ref();
+    SV* to_SV_clone_ref();
     SV* to_SV_pureperl();
     void scale(double factor);
     void translate(double x, double y);

xs/src/Line.cpp

@@ -66,11 +66,11 @@ Line::to_SV() {
     av_extend(av, 1);
     
     SV* sv = newSV(0);
-    sv_setref_pv( sv, "Slic3r::Point", new Point(this->a) );
+    sv_setref_pv( sv, "Slic3r::Point::Ref", &(this->a) );
     av_store(av, 0, sv);
     
     sv = newSV(0);
-    sv_setref_pv( sv, "Slic3r::Point", new Point(this->b) );
+    sv_setref_pv( sv, "Slic3r::Point::Ref", &(this->b) );
     av_store(av, 1, sv);
     
     return newRV_noinc((SV*)av);
@@ -78,6 +78,13 @@ Line::to_SV() {
 
 SV*
 Line::to_SV_ref() {
+    SV* sv = newSV(0);
+    sv_setref_pv( sv, "Slic3r::Line::Ref", this );
+    return sv;
+}
+
+SV*
+Line::to_SV_clone_ref() {
     SV* sv = newSV(0);
     sv_setref_pv( sv, "Slic3r::Line", new Line(*this) );
     return sv;

xs/src/Line.hpp

@@ -17,6 +17,7 @@ class Line
     void from_SV_check(SV* line_sv);
     SV* to_SV();
     SV* to_SV_ref();
+    SV* to_SV_clone_ref();
     SV* to_SV_pureperl();
     void scale(double factor);
     void translate(double x, double y);

xs/src/MultiPoint.cpp

@@ -32,16 +32,16 @@ MultiPoint::reverse()
     std::reverse(this->points.begin(), this->points.end());
 }
 
-const Point*
-MultiPoint::first_point() const
+Point*
+MultiPoint::first_point()
 {
     return &(this->points.front());
 }
 
 const Point*
-MultiPoint::last_point() const
+MultiPoint::first_point() const
 {
-    return &(this->points.back());
+    return &(this->points.front());
 }
 
 void
@@ -73,9 +73,7 @@ MultiPoint::to_SV() {
     AV* av = newAV();
     av_extend(av, num_points-1);
     for (unsigned int i = 0; i < num_points; i++) {
-        SV* sv = newSV(0);
-        sv_setref_pv( sv, "Slic3r::Point", new Point(this->points[i]) );
-        av_store(av, i, sv);
+        av_store(av, i, this->points[i].to_SV_ref());
     }
     return newRV_noinc((SV*)av);
 }

xs/src/MultiPoint.hpp

@@ -19,8 +19,9 @@ class MultiPoint
     void translate(double x, double y);
     void rotate(double angle, Point* center);
     void reverse();
+    Point* first_point();
     const Point* first_point() const;
-    const Point* last_point() const;
+    virtual Point* last_point() = 0;
 };
 
 }

xs/src/Point.cpp

@@ -73,7 +73,21 @@ Point::distance_to(const Point* point) const
 }
 
 SV*
-Point::to_SV_pureperl() {
+Point::to_SV_ref() const {
+    SV* sv = newSV(0);
+    sv_setref_pv( sv, "Slic3r::Point::Ref", (void*)this );
+    return sv;
+}
+
+SV*
+Point::to_SV_clone_ref() const {
+    SV* sv = newSV(0);
+    sv_setref_pv( sv, "Slic3r::Point", new Point(*this) );
+    return sv;
+}
+
+SV*
+Point::to_SV_pureperl() const {
     AV* av = newAV();
     av_fill(av, 1);
     av_store(av, 0, newSViv(this->x));

xs/src/Point.hpp

@@ -18,7 +18,9 @@ class Point
     explicit Point(long _x = 0, long _y = 0): x(_x), y(_y) {};
     void from_SV(SV* point_sv);
     void from_SV_check(SV* point_sv);
-    SV* to_SV_pureperl();
+    SV* to_SV_ref() const;
+    SV* to_SV_clone_ref() const;
+    SV* to_SV_pureperl() const;
     void scale(double factor);
     void translate(double x, double y);
     void rotate(double angle, Point* center);

xs/src/Polygon.cpp

@@ -5,8 +5,21 @@
 
 namespace Slic3r {
 
+Point*
+Polygon::last_point()
+{
+    return &(this->points.front());  // last point == first point for polygons
+}
+
 SV*
-Polygon::to_SV_ref() {
+Polygon::to_SV_ref() const {
+    SV* sv = newSV(0);
+    sv_setref_pv( sv, "Slic3r::Polygon::Ref", (void*)this );
+    return sv;
+}
+
+SV*
+Polygon::to_SV_clone_ref() const {
     SV* sv = newSV(0);
     sv_setref_pv( sv, "Slic3r::Polygon", new Polygon(*this) );
     return sv;

xs/src/Polygon.hpp

@@ -11,7 +11,9 @@ namespace Slic3r {
 
 class Polygon : public MultiPoint {
     public:
-    SV* to_SV_ref();
+    Point* last_point();
+    SV* to_SV_ref() const;
+    SV* to_SV_clone_ref() const;
     Lines lines();
     Polyline* split_at(const Point* point);
     Polyline* split_at_index(int index);

xs/src/Polyline.cpp

@@ -2,6 +2,18 @@
 
 namespace Slic3r {
 
+Point*
+Polyline::last_point()
+{
+    return &(this->points.back());
+}
+
+const Point*
+Polyline::last_point() const
+{
+    return &(this->points.back());
+}
+
 Lines
 Polyline::lines()
 {
@@ -14,6 +26,14 @@ Polyline::lines()
 
 SV*
 Polyline::to_SV_ref() const
+{
+    SV* sv = newSV(0);
+    sv_setref_pv( sv, "Slic3r::Polyline::Ref", (void*)this );
+    return sv;
+}
+
+SV*
+Polyline::to_SV_clone_ref() const
 {
     SV* sv = newSV(0);
     sv_setref_pv( sv, "Slic3r::Polyline", new Polyline(*this) );

xs/src/Polyline.hpp

@@ -8,8 +8,11 @@ namespace Slic3r {
 
 class Polyline : public MultiPoint {
     public:
+    Point* last_point();
+    const Point* last_point() const;
     Lines lines();
     SV* to_SV_ref() const;
+    SV* to_SV_clone_ref() const;
 };
 
 typedef std::vector<Polyline> Polylines;

xs/src/PolylineCollection.cpp

@@ -5,9 +5,7 @@ namespace Slic3r {
 PolylineCollection*
 PolylineCollection::chained_path(bool no_reverse) const
 {
-    if (this->polylines.empty()) {
-        return new PolylineCollection ();
-    }
+    if (this->polylines.empty()) return new PolylineCollection ();
     return this->chained_path_from(this->polylines.front().first_point(), no_reverse);
 }
 

xs/t/04_expolygon.t

@@ -28,20 +28,19 @@ is ref($expolygon->pp), 'ARRAY', 'expolygon pp is unblessed';
 is_deeply $expolygon->pp, [$square, $hole_in_square], 'expolygon roundtrip';
 
 is ref($expolygon->arrayref), 'ARRAY', 'expolygon arrayref is unblessed';
-isa_ok $expolygon->[0], 'Slic3r::Polygon', 'expolygon polygon is blessed';
-isa_ok $expolygon->contour, 'Slic3r::Polygon', 'expolygon contour is blessed';
-isa_ok $expolygon->holes->[0], 'Slic3r::Polygon', 'expolygon hole is blessed';
-isa_ok $expolygon->[0][0], 'Slic3r::Point', 'expolygon point is blessed';
+isa_ok $expolygon->[0], 'Slic3r::Polygon::Ref', 'expolygon polygon is blessed';
+isa_ok $expolygon->contour, 'Slic3r::Polygon::Ref', 'expolygon contour is blessed';
+isa_ok $expolygon->holes->[0], 'Slic3r::Polygon::Ref', 'expolygon hole is blessed';
+isa_ok $expolygon->[0][0], 'Slic3r::Point::Ref', 'expolygon point is blessed';
 
 {
-    my $polygon = $expolygon->[0];
+    my $expolygon2 = $expolygon->clone;
+    my $polygon = $expolygon2->[0];
     $polygon->scale(2);
-    isnt $expolygon->[0][0][0], $polygon->[0][0], 'a copy of polygons is returned';
+    is $expolygon2->[0][0][0], $polygon->[0][0], 'polygons are returned by reference';
 }
 
 is_deeply $expolygon->clone->pp, [$square, $hole_in_square], 'clone';
-# The following tests implicitely check that modifying clones
-# does not modify the original one.
 
 is $expolygon->area, 100*100-20*20, 'area';
 
@@ -100,7 +99,7 @@ is $expolygon->area, 100*100-20*20, 'area';
     
     my $exp = $collection->[0];
     $exp->scale(3);
-    isnt $collection->[0][0][0][0], $exp->[0][0][0], 'collection items are not returned by reference';
+    is $collection->[0][0][0][0], $exp->[0][0][0], 'collection items are returned by reference';
     
     is_deeply $collection->[0]->clone->pp, $collection->[0]->pp, 'clone collection item';
 }

xs/t/06_polygon.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],
@@ -18,7 +18,7 @@ ok $polygon->is_valid, 'is_valid';
 is_deeply $polygon->pp, $square, 'polygon roundtrip';
 
 is ref($polygon->arrayref), 'ARRAY', 'polygon arrayref is unblessed';
-isa_ok $polygon->[0], 'Slic3r::Point', 'polygon point is blessed';
+isa_ok $polygon->[0], 'Slic3r::Point::Ref', 'polygon point is blessed';
 
 my $lines = $polygon->lines;
 is_deeply [ map $_->pp, @$lines ], [
@@ -44,4 +44,16 @@ ok $polygon->is_counter_clockwise, 'is_counter_clockwise';
     ok $clone->is_counter_clockwise, 'make_counter_clockwise';
 }
 
+isa_ok $polygon->first_point, 'Slic3r::Point::Ref', 'first_point';
+
+# this is not a test: this just demonstrates bad usage, where $polygon->clone gets
+# DESTROY'ed before the derived object ($point), causing bad memory access
+if (0) {
+    my $point;
+    {
+        $point = $polygon->clone->[0];
+    }
+    $point->scale(2);
+}
+
 __END__

xs/t/09_polyline.t

@@ -16,7 +16,7 @@ my $polyline = Slic3r::Polyline->new(@$points);
 is_deeply $polyline->pp, $points, 'polyline roundtrip';
 
 is ref($polyline->arrayref), 'ARRAY', 'polyline arrayref is unblessed';
-isa_ok $polyline->[0], 'Slic3r::Point', 'polyline point is blessed';
+isa_ok $polyline->[0], 'Slic3r::Point::Ref', 'polyline point is blessed';
 
 my $lines = $polyline->lines;
 is_deeply [ map $_->pp, @$lines ], [

xs/t/10_line.t

@@ -15,7 +15,7 @@ my $line = Slic3r::Line->new(@$points);
 is_deeply $line->pp, $points, 'line roundtrip';
 
 is ref($line->arrayref), 'ARRAY', 'line arrayref is unblessed';
-isa_ok $line->[0], 'Slic3r::Point', 'line point is blessed';
+isa_ok $line->[0], 'Slic3r::Point::Ref', 'line point is blessed';
 
 {
     my $clone = $line->clone;

xs/xsp/ExPolygon.xsp

@@ -14,9 +14,9 @@
     SV* pp()
         %code{% RETVAL = THIS->to_SV_pureperl(); %};
     Polygon* contour()
-        %code{% const char* CLASS = "Slic3r::Polygon"; RETVAL = new Polygon(THIS->contour); %};
-    Polygons holes()
-        %code{% RETVAL = THIS->holes; %};
+        %code{% const char* CLASS = "Slic3r::Polygon::Ref"; RETVAL = &(THIS->contour); %};
+    Polygons* holes()
+        %code{% RETVAL = &(THIS->holes); %};
     void scale(double factor);
     void translate(double x, double y);
     double area();

xs/xsp/ExtrusionPath.xsp

@@ -17,9 +17,9 @@
     Lines lines()
         %code{% RETVAL = THIS->polyline.lines(); %};
     Point* first_point()
-        %code{% const char* CLASS = "Slic3r::Point"; RETVAL = new Point(*(THIS->first_point())); %};
+        %code{% const char* CLASS = "Slic3r::Point::Ref"; RETVAL = THIS->first_point(); %};
     Point* last_point()
-        %code{% const char* CLASS = "Slic3r::Point"; RETVAL = new Point(*(THIS->last_point())); %};
+        %code{% const char* CLASS = "Slic3r::Point::Ref"; RETVAL = THIS->last_point(); %};
 %{
 
 ExtrusionPath*

xs/xsp/Line.xsp

@@ -14,9 +14,9 @@
     SV* pp()
         %code{% RETVAL = THIS->to_SV_pureperl(); %};
     Point* a()
-        %code{% const char* CLASS = "Slic3r::Point"; RETVAL = new Point(THIS->a); %};
+        %code{% const char* CLASS = "Slic3r::Point::Ref"; RETVAL = &(THIS->a); %};
     Point* b()
-        %code{% const char* CLASS = "Slic3r::Point"; RETVAL = new Point(THIS->b); %};
+        %code{% const char* CLASS = "Slic3r::Point::Ref"; RETVAL = &(THIS->b); %};
     void reverse();
     void scale(double factor);
     void translate(double x, double y);

xs/xsp/Polygon.xsp

@@ -30,7 +30,7 @@
     bool make_clockwise();
     bool is_valid();
     Point* first_point()
-        %code{% const char* CLASS = "Slic3r::Point"; RETVAL = new Point(*(THIS->first_point())); %};
+        %code{% const char* CLASS = "Slic3r::Point::Ref"; RETVAL = THIS->first_point(); %};
 %{
 
 Polygon*

xs/xsp/Polyline.xsp

@@ -20,9 +20,9 @@
     void reverse();
     Lines lines();
     Point* first_point()
-        %code{% const char* CLASS = "Slic3r::Point"; RETVAL = new Point(*(THIS->first_point())); %};
+        %code{% const char* CLASS = "Slic3r::Point::Ref"; RETVAL = THIS->first_point(); %};
     Point* last_point()
-        %code{% const char* CLASS = "Slic3r::Point"; RETVAL = new Point(*(THIS->last_point())); %};
+        %code{% const char* CLASS = "Slic3r::Point::Ref"; RETVAL = THIS->last_point(); %};
 %{
 
 Polyline*

xs/xsp/my.map

@@ -23,6 +23,8 @@ Lines           T_ARRAYREF
 Polygons        T_ARRAYREF
 ExPolygons      T_ARRAYREF
 
+Polygons*       T_ARRAYREF_POLYGONS_PTR
+
 INPUT
 
 T_ARRAYREF
@@ -49,6 +51,15 @@ T_ARRAYREF
 	av_extend(av, $var.size()-1);
 	int i = 0;
     for (${type}::iterator it = $var.begin(); it != $var.end(); ++it) {
-        av_store(av, i++, (*it).to_SV_ref());
+        av_store(av, i++, (*it).to_SV_clone_ref());
 	}
 	$var.clear();
+
+T_ARRAYREF_POLYGONS_PTR
+    AV* av = newAV();
+	$arg = newRV_noinc((SV*)av);
+	av_extend(av, $var->size()-1);
+	int i = 0;
+    for (Polygons::iterator it = $var->begin(); it != $var->end(); ++it) {
+        av_store(av, i++, (*it).to_SV_ref());
+	}

xs/xsp/typemap.xspt

@@ -14,6 +14,7 @@
 %typemap{Lines};
 %typemap{Polygons};
 %typemap{ExPolygons};
+%typemap{Polygons*};
 
 %typemap{SurfaceType}{parsed}{
   %cpp_type{SurfaceType};