Refactoring to Model API for making it stricter and safer

lib/Slic3r/Format/AMF/Parser.pm

@@ -126,7 +126,7 @@ sub end_document {
         foreach my $instance (@{ $self->{_instances}{$object_id} }) {
             $self->{_model}->objects->[$new_object_id]->add_instance(
                 rotation => $instance->{rz} || 0,
-                offset   => [ $instance->{deltax} || 0, $instance->{deltay} || 0 ],
+                offset   => Slic3r::Pointf->new($instance->{deltax} || 0, $instance->{deltay} || 0),
             );
         }
     }

lib/Slic3r/GUI/Plater.pm

@@ -409,7 +409,7 @@ sub load_model_objects {
         
             # add a default instance and center object around origin
             $o->center_around_origin;
-            $o->add_instance(offset => [ @{$self->{config}->print_center} ]);
+            $o->add_instance(offset => Slic3r::Pointf->new(@{$self->{config}->print_center}));
         }
     
         $self->{print}->auto_assign_extruders($o);
@@ -487,7 +487,7 @@ sub increase {
     my $model_object = $self->{model}->objects->[$obj_idx];
     my $last_instance = $model_object->instances->[-1];
     my $i = $model_object->add_instance(
-        offset          => [ map 10+$_, @{$last_instance->offset} ],
+        offset          => Slic3r::Pointf->new(map 10+$_, @{$last_instance->offset}),
         scaling_factor  => $last_instance->scaling_factor,
         rotation        => $last_instance->rotation,
     );
@@ -654,10 +654,10 @@ sub split_object {
         for my $instance_idx (0..$#{ $current_model_object->instances }) {
             my $current_instance = $current_model_object->instances->[$instance_idx];
             $model_object->add_instance(
-                offset          => [
+                offset          => Slic3r::Pointf->new(
                     $current_instance->offset->[X] + ($instance_idx * 10),
                     $current_instance->offset->[Y] + ($instance_idx * 10),
-                ],
+                ),
                 rotation        => $current_instance->rotation,
                 scaling_factor  => $current_instance->scaling_factor,
             );

lib/Slic3r/Model.pm

@@ -31,48 +31,34 @@ sub merge {
 sub add_object {
     my $self = shift;
     
-    my $new_object;
     if (@_ == 1) {
         # we have a Model::Object
         my ($object) = @_;
-
-        $new_object = $self->add_object(
-            input_file          => $object->input_file,
-            config              => $object->config,
-            layer_height_ranges => $object->layer_height_ranges,    # TODO: clone!
-            origin_translation  => $object->origin_translation,
-        );
-        
-        foreach my $volume (@{$object->volumes}) {
-            $new_object->add_volume($volume);
-        }
-        
-        $new_object->add_instance(
-            offset              => [ @{$_->offset} ],
-            rotation            => $_->rotation,
-            scaling_factor      => $_->scaling_factor,
-        ) for @{ $object->instances // [] };
+        return $self->_add_object_clone($object);
     } else {
         my (%args) = @_;
-        $new_object = $self->_add_object(
-            $args{input_file},
-            $args{config} // Slic3r::Config->new,
-            $args{layer_height_ranges} // [],
-            $args{origin_translation} // Slic3r::Pointf->new,
-            );
+        
+        my $new_object = $self->_add_object;
+        
+        $new_object->set_input_file($args{input_file})
+            if defined $args{input_file};
+        $new_object->config->apply($args{config})
+            if defined $args{config};
+        $new_object->set_layer_height_ranges($args{layer_height_ranges})
+            if defined $args{layer_height_ranges};
+        $new_object->set_origin_translation($args{origin_translation})
+            if defined $args{origin_translation};
+        
+        return $new_object;
     }
-    
-    return $new_object;
 }
 
 sub set_material {
     my $self = shift;
     my ($material_id, $attributes) = @_;
     
-    $attributes //= {};
-    
-    my $material = $self->_set_material($material_id);
-    $material->set_attribute($_, $attributes->{$_}) for keys %$attributes;
+    my $material = $self->add_material($material_id);
+    $material->apply($attributes // {});
     return $material;
 }
 
@@ -89,10 +75,10 @@ sub duplicate_objects_grid {
     for my $x_copy (1..$grid->[X]) {
         for my $y_copy (1..$grid->[Y]) {
             $object->add_instance(
-                offset => [
+                offset => Slic3r::Pointf->new(
                     ($size->[X] + $distance) * ($x_copy-1),
                     ($size->[Y] + $distance) * ($y_copy-1),
-                ],
+                ),
             );
         }
     }
@@ -106,12 +92,7 @@ sub duplicate_objects {
     foreach my $object (@{$self->objects}) {
         my @instances = @{$object->instances};
         foreach my $instance (@instances) {
-            ### $object->add_instance($instance->clone);  if we had clone()
-            $object->add_instance(
-                offset          => [ @{$instance->offset} ],
-                rotation        => $instance->rotation,
-                scaling_factor  => $instance->scaling_factor,
-            ) for 2..$copies_num;
+            $object->add_instance($instance) for 2..$copies_num;
         }
     }
     
@@ -151,9 +132,8 @@ sub duplicate {
         my @instances = @{$object->instances};  # store separately to avoid recursion from add_instance() below
         foreach my $instance (@instances) {
             foreach my $pos (@positions) {
-                ### $object->add_instance($instance->clone);  if we had clone()
                 $object->add_instance(
-                    offset          => [ $instance->offset->[X] + $pos->[X], $instance->offset->[Y] + $pos->[Y] ],
+                    offset          => Slic3r::Pointf->new($instance->offset->[X] + $pos->[X], $instance->offset->[Y] + $pos->[Y]),
                     rotation        => $instance->rotation,
                     scaling_factor  => $instance->scaling_factor,
                 );
@@ -187,8 +167,8 @@ sub add_default_instances {
     # apply a default position to all objects not having one
     my $added = 0;
     foreach my $object (@{$self->objects}) {
-        if (!defined $object->instances) {
-            $object->add_instance(offset => [0,0]);
+        if ($object->instances_count == 0) {
+            $object->add_instance(offset => Slic3r::Pointf->new(0,0));
             $added = 1;
         }
     }
@@ -286,7 +266,7 @@ sub split_meshes {
             
             # add one instance per original instance
             $new_object->add_instance(
-                offset          => [ @{$_->offset} ],
+                offset          => Slic3r::Pointf->new(@{$_->offset}),
                 rotation        => $_->rotation,
                 scaling_factor  => $_->scaling_factor,
             ) for @{ $object->instances // [] };
@@ -314,6 +294,11 @@ sub get_material_name {
 
 package Slic3r::Model::Material;
 
+sub apply {
+    my ($self, $attributes) = @_;
+    $self->set_attribute($_, $attributes{$_}) for keys %$attributes;
+}
+
 package Slic3r::Model::Object;
 
 use File::Basename qw(basename);
@@ -328,8 +313,7 @@ sub add_volume {
         # we have a Model::Volume
         my ($volume) = @_;
         
-        $new_volume = $self->_add_volume(
-            $volume->material_id, $volume->mesh->clone, $volume->modifier);
+        $new_volume = $self->_add_volume_clone($volume);
         
         # TODO: material_id can't be undef.
         if (defined $volume->material_id) {
@@ -345,10 +329,13 @@ sub add_volume {
         }
     } else {
         my %args = @_;
-        $new_volume = $self->_add_volume(
-            $args{material_id},
-            $args{mesh},
-            $args{modifier} // 0);
+        
+        $new_volume = $self->_add_volume($args{mesh});
+        
+        $new_volume->set_material_id($args{material_id})
+            if defined $args{material_id};
+        $new_volume->set_modifier($args{modifier})
+            if defined $args{modifier};
     }
     
     if (defined $new_volume->material_id && !defined $self->model->get_material($new_volume->material_id)) {
@@ -356,7 +343,7 @@ sub add_volume {
         $self->model->set_material($new_volume->material_id);
     }
     
-    $self->invalidate_bounding_box();
+    $self->invalidate_bounding_box;
     
     return $new_volume;
 }
@@ -364,16 +351,25 @@ sub add_volume {
 sub add_instance {
     my $self = shift;
     my %params = @_;
-
-    return $self->_add_instance(
-        $params{rotation} // 0,
-        $params{scaling_factor} // 1,
-        $params{offset} // []);
-}
-
-sub instances_count {
-    my $self = shift;
-    return scalar(@{ $self->instances // [] });
+    
+    if (@_ == 1) {
+        # we have a Model::Instance
+        my ($instance) = @_;
+        return $self->_add_instance_clone($instance);
+    } else {
+        my (%args) = @_;
+        
+        my $new_instance = $self->_add_instance;
+        
+        $new_instance->set_rotation($args{rotation})
+            if defined $args{rotation};
+        $new_instance->set_scaling_factor($args{scaling_factor})
+            if defined $args{scaling_factor};
+        $new_instance->set_offset($args{offset})
+            if defined $args{offset};
+        
+        return $new_instance;
+    }
 }
 
 sub raw_mesh {
@@ -446,11 +442,12 @@ sub center_around_origin {
     $self->translate(@shift);
     $self->origin_translation->translate(@shift[X,Y]);
     
-    if (defined $self->instances) {
+    if ($self->instances_count > 0) {
         foreach my $instance (@{ $self->instances }) {
             $instance->set_offset(Slic3r::Pointf->new(
                 $instance->offset->x - $shift[X],
-                $instance->offset->y - $shift[Y]));
+                $instance->offset->y - $shift[Y],   #--
+            ));
         }
         $self->update_bounding_box;
     }
@@ -538,23 +535,12 @@ sub cut {
     my ($self, $z) = @_;
     
     # clone this one
-    my $upper = Slic3r::Model::Object->new(
-        $self->model,
-        $self->input_file,
-        $self->config,  # config is cloned by new()
-        $self->layer_height_ranges,
-        $self->origin_translation,
-    );
-    my $lower = Slic3r::Model::Object->new(
-        $self->model,
-        $self->input_file,
-        $self->config,  # config is cloned by new()
-        $self->layer_height_ranges,
-        $self->origin_translation,
-    );
+    my $upper = $self->model->add_object($self);
+    my $lower = $self->model->add_object($self);
+    
     foreach my $instance (@{$self->instances}) {
-        $upper->add_instance(offset => [ @{$instance->offset} ]);
-        $lower->add_instance(offset => [ @{$instance->offset} ]);
+        $upper->add_instance(offset => Slic3r::Pointf->new(@{$instance->offset}));
+        $lower->add_instance(offset => Slic3r::Pointf->new(@{$instance->offset}));
     }
     
     foreach my $volume (@{$self->volumes}) {

lib/Slic3r/Test.pm

@@ -124,9 +124,9 @@ sub model {
     $model->set_material($model_name);
     $object->add_volume(mesh => mesh($model_name, %params), material_id => $model_name);
     $object->add_instance(
-        offset      => [0,0],
-        rotation    => $params{rotation} // 0,
-        scaling_factor => $params{scale} // 1,
+        offset          => Slic3r::Pointf->new(0,0),
+        rotation        => $params{rotation} // 0,
+        scaling_factor  => $params{scale} // 1,
     );
     return $model;
 }
@@ -142,7 +142,8 @@ sub init_print {
     $print->apply_config($config);
     
     $models = [$models] if ref($models) ne 'ARRAY';
-    for my $model (map { ref($_) ? $_ : model($_, %params) } @$models) {
+    $models = [ map { ref($_) ? $_ : model($_, %params) } @$models ];
+    for my $model (@$models) {
         die "Unknown model in test" if !defined $model;
         if (defined $params{duplicate} && $params{duplicate} > 1) {
             $model->duplicate($params{duplicate} // 1, $print->config->min_object_distance);
@@ -156,12 +157,18 @@ sub init_print {
     }
     $print->validate;
     
-    return $print;
+    # We return a proxy object in order to keep $models alive as required by the Print API.
+    return Slic3r::Test::Print->new(
+        print   => $print,
+        models  => $models,
+    );
 }
 
 sub gcode {
     my ($print) = @_;
     
+    $print = $print->print if $print->isa('Slic3r::Test::Print');
+    
     my $fh = IO::Scalar->new(\my $gcode);
     $print->process;
     $print->export_gcode(output_fh => $fh, quiet => 1);
@@ -189,4 +196,10 @@ sub add_facet {
     }
 }
 
+package Slic3r::Test::Print;
+use Moo;
+
+has 'print'     => (is => 'ro', required => 1);
+has 'models'    => (is => 'ro', required => 1);
+
 1;

t/custom_gcode.t

@@ -57,7 +57,7 @@ use Slic3r::Test;
     $config->set('start_gcode', "TRAVEL:[travel_speed] HEIGHT:[layer_height]\n");
     my $print = Slic3r::Test::init_print('20mm_cube', config => $config);
     
-    my $output_file = $print->expanded_output_filepath;
+    my $output_file = $print->print->expanded_output_filepath;
     ok $output_file !~ /\[travel_speed\]/, 'print config options are replaced in output filename';
     ok $output_file !~ /\[layer_height\]/, 'region config options are replaced in output filename';
     

t/gcode.t

@@ -78,7 +78,7 @@ use Slic3r::Test;
         
         
     });
-    ok $print->total_used_filament > 0, 'final retraction is not considered in total used filament';
+    ok $print->print->total_used_filament > 0, 'final retraction is not considered in total used filament';
 }
 
 {

t/multi.t

@@ -173,7 +173,7 @@ sub stacked_cubes {
     my $object = $model->add_object;
     $object->add_volume(mesh => Slic3r::Test::mesh('20mm_cube'), material_id => 'lower');
     $object->add_volume(mesh => Slic3r::Test::mesh('20mm_cube', translate => [0,0,20]), material_id => 'upper');
-    $object->add_instance(offset => [0,0]);
+    $object->add_instance(offset => Slic3r::Pointf->new(0,0));
     
     return $model;
 }

t/print.t

@@ -38,21 +38,21 @@ use Slic3r::Test;
     my $print = Slic3r::Test::init_print(my $model = Slic3r::Test::model('20mm_cube'), config => $config);
     
     # user sets a per-region option
-    $print->objects->[0]->model_object->config->set('fill_density', 100);
-    $print->reload_object(0);
+    $print->print->objects->[0]->model_object->config->set('fill_density', 100);
+    $print->print->reload_object(0);
     
     # user exports G-code, thus the default config is reapplied
-    $print->apply_config($config);
+    $print->print->apply_config($config);
     
-    is $print->regions->[0]->config->fill_density, 100, 'apply_config() does not override per-object settings';
+    is $print->print->regions->[0]->config->fill_density, 100, 'apply_config() does not override per-object settings';
     
     # user assigns object extruders
-    $print->objects->[0]->model_object->config->set('extruder', 3);
-    $print->objects->[0]->model_object->config->set('perimeter_extruder', 2);
-    $print->reload_object(0);
+    $print->print->objects->[0]->model_object->config->set('extruder', 3);
+    $print->print->objects->[0]->model_object->config->set('perimeter_extruder', 2);
+    $print->print->reload_object(0);
     
-    is $print->regions->[0]->config->infill_extruder, 3, 'extruder setting is correctly expanded';
-    is $print->regions->[0]->config->perimeter_extruder, 2, 'extruder setting does not override explicitely specified extruders';
+    is $print->print->regions->[0]->config->infill_extruder, 3, 'extruder setting is correctly expanded';
+    is $print->print->regions->[0]->config->perimeter_extruder, 2, 'extruder setting does not override explicitely specified extruders';
 }
 
 __END__

t/retraction.t

@@ -42,8 +42,8 @@ use Slic3r::Test qw(_eq);
         
             if ($info->{dist_Z}) {
                 # lift move or lift + change layer
-                if (_eq($info->{dist_Z}, $print->config->get_at('retract_lift', $tool))
-                    || (_eq($info->{dist_Z}, $conf->layer_height + $print->config->get_at('retract_lift', $tool)) && $print->config->get_at('retract_lift', $tool) > 0)) {
+                if (_eq($info->{dist_Z}, $print->print->config->get_at('retract_lift', $tool))
+                    || (_eq($info->{dist_Z}, $conf->layer_height + $print->print->config->get_at('retract_lift', $tool)) && $print->print->config->get_at('retract_lift', $tool) > 0)) {
                     fail 'only lifting while retracted' if !$retracted[$tool] && !($conf->g0 && $info->{retracting});
                     fail 'double lift' if $lifted;
                     $lifted = 1;
@@ -51,8 +51,8 @@ use Slic3r::Test qw(_eq);
                 if ($info->{dist_Z} < 0) {
                     fail 'going down only after lifting' if !$lifted;
                     fail 'going down by the same amount of the lift or by the amount needed to get to next layer'
-                        if !_eq($info->{dist_Z}, -$print->config->get_at('retract_lift', $tool))
-                            && !_eq($info->{dist_Z}, -$print->config->get_at('retract_lift', $tool) + $conf->layer_height);
+                        if !_eq($info->{dist_Z}, -$print->print->config->get_at('retract_lift', $tool))
+                            && !_eq($info->{dist_Z}, -$print->print->config->get_at('retract_lift', $tool) + $conf->layer_height);
                     $lifted = 0;
                 }
                 fail 'move Z at travel speed' if ($args->{F} // $self->F) != $conf->travel_speed * 60;
@@ -60,9 +60,9 @@ use Slic3r::Test qw(_eq);
             if ($info->{retracting}) {
                 $retracted[$tool] = 1;
                 $retracted_length[$tool] += -$info->{dist_E};
-                if (_eq($retracted_length[$tool], $print->config->get_at('retract_length', $tool))) {
+                if (_eq($retracted_length[$tool], $print->print->config->get_at('retract_length', $tool))) {
                     # okay
-                } elsif (_eq($retracted_length[$tool], $print->config->get_at('retract_length_toolchange', $tool))) {
+                } elsif (_eq($retracted_length[$tool], $print->print->config->get_at('retract_length_toolchange', $tool))) {
                     $wait_for_toolchange = 1;
                 } else {
                     fail 'retracted by the correct amount';
@@ -73,9 +73,9 @@ use Slic3r::Test qw(_eq);
             if ($info->{extruding}) {
                 fail 'only extruding while not lifted' if $lifted;
                 if ($retracted[$tool]) {
-                    my $expected_amount = $retracted_length[$tool] + $print->config->get_at('retract_restart_extra', $tool);
+                    my $expected_amount = $retracted_length[$tool] + $print->print->config->get_at('retract_restart_extra', $tool);
                     if ($changed_tool && $toolchange_count[$tool] > 1) {
-                        $expected_amount = $print->config->get_at('retract_length_toolchange', $tool) + $print->config->get_at('retract_restart_extra_toolchange', $tool);
+                        $expected_amount = $print->print->config->get_at('retract_length_toolchange', $tool) + $print->print->config->get_at('retract_restart_extra_toolchange', $tool);
                         $changed_tool = 0;
                     }
                     fail 'unretracted by the correct amount'
@@ -84,7 +84,7 @@ use Slic3r::Test qw(_eq);
                     $retracted_length[$tool] = 0;
                 }
             }
-            if ($info->{travel} && $info->{dist_XY} >= $print->config->get_at('retract_before_travel', $tool)) {
+            if ($info->{travel} && $info->{dist_XY} >= $print->print->config->get_at('retract_before_travel', $tool)) {
                 fail 'retracted before long travel move' if !$retracted[$tool];
             }
         });