Implemented feature request #1405 minimum shell thickness option #3885

curieos · 2017-04-16T06:05:52Z

This adds a new print config option, minimum shell thickness. It works as described in the issue page by alexrj. The current configuration, when paired with the generate extra perimeters option, will only add perimeters when both the normal and extra perimeters combined don't meet the shell thickness requirements.

Fixes #1405

-Added the option to libslic3r -Created some of the necessary checks for illegal values

-This could be switched to match the extrusion width, but we already know there will only be 1 perimeter so this is probably more logical

-Added a method in PerimeterGenerator to determine the amount of perimeters needed to meet min_shell_thickness

lordofhyphens

Overall, good work. I liked the approach, just had some notes about style and test robustness that should be addressed.

lordofhyphens · 2017-04-16T13:59:39Z

xs/src/libslic3r/PerimeterGenerator.cpp

+    int generated_perimeters = perimeters + extra_perimeters - 1;
+    long int min_shell_thickness, shell_thickness = 0;
+
+    min_shell_thickness = this->config->min_shell_thickness * 1000000;


Style

Prefer use of the scale_ and unscale functions (see libslic3r.h) instead of the magic number multiplication here.

You can also define and assign (with unscale) min_shell_thickness as an auto.

lordofhyphens · 2017-04-16T14:02:23Z

xs/src/libslic3r/PerimeterGenerator.cpp

+int
+PerimeterGenerator::num_loops(int perimeters, int extra_perimeters) {
+    int generated_perimeters = perimeters + extra_perimeters - 1;
+    long int min_shell_thickness, shell_thickness = 0;


Style

We don't use long int, just long (when we aren't using something that typedefs to long, like coord_t).

Suggest auto shell_thickness = this->ext_perimeter_flow.scaled_width() (yay c++11).

lordofhyphens · 2017-04-16T14:08:57Z

xs/src/libslic3r/PrintConfig.cpp

+    def->category = "Layers and Perimeters";
+    def->sidetext = "mm";
+    def->tooltip = "Minimum shell thickness in mm";
+    def->cli = "min-shell-thickness=f";


Documentation

Update slic3r.pl and README.md to add your option to them.

lordofhyphens · 2017-04-16T14:10:00Z

xs/src/libslic3r/PerimeterGenerator.cpp

@@ -54,7 +71,7 @@ PerimeterGenerator::process()
    for (Surfaces::const_iterator surface = this->slices->surfaces.begin();
        surface != this->slices->surfaces.end(); ++surface) {
        // detect how many perimeters must be generated for this island
-        const int loop_number = this->config->perimeters + surface->extra_perimeters -1;  // 0-indexed loops
+        const int loop_number = num_loops(this->config->perimeters, surface->extra_perimeters);//this->config->perimeters + surface->extra_perimeters-1;  // 0-indexed loops


I like the way you extended this to increase the number of loops if necessary.

lordofhyphens · 2017-04-16T14:11:25Z

t/perimeters.t

+            $in_loop = 0;
+        }
+    });
+    ok !(grep { $_ % $config->min_shell_thickness/$config->perimeter_extrusion_width } values %perimeters), 'should be 6 perimeters';


Robustness

Please add another couple test cases to check that it rounds up as expected if the perimeter width isn't a multiple of the min-width. Just in case @alexrj or I blunder in and break that condition somehow ;)

Take a look at the first test in t/support.t, it puts the actual test into a subroutine and then calls that routine with different options. Here you'd just need to round up the division in the test itself to verify the number of perimeters that were generated.

lordofhyphens · 2017-04-16T14:13:42Z

lib/Slic3r/GUI/PresetEditor.pm

@@ -759,10 +760,11 @@ sub _update {

    my $config = $self->{config};

-    if ($config->spiral_vase && !($config->perimeters == 1 && $config->top_solid_layers == 0 && $config->fill_density == 0 && $config->support_material == 0)) {
+    if ($config->spiral_vase && !($config->perimeters == 1 && $config->min_shell_thickness == 0  && $config->top_solid_layers == 0 && $config->fill_density == 0 && $config->support_material == 0)) {


Good catch, I probably would have missed this myself.

curieos · 2017-04-16T14:53:02Z

I just realized I forgot about the top and bottom of the model. This should also increase the amount of solid top/bottom layers yeah?

lordofhyphens · 2017-04-16T15:14:07Z

I'm not so sure myself.

-Removed the magic number in PerimeterGenerator::num_loops() -Added more test cases in perimeters.t -Added documentation of the new feature in slic3r.pl and README.md

alranel · 2017-04-20T21:33:51Z

xs/src/libslic3r/PerimeterGenerator.cpp

+PerimeterGenerator::num_loops(int perimeters, int extra_perimeters)
+{
+    int generated_perimeters = perimeters + extra_perimeters - 1;
+    long min_shell_thickness = scale_(this->config->min_shell_thickness);


Use coord_t instead of long

or auto (which is my preference as you are instantiating the value already). If @alexrj disagrees, go with what he said. ;)

@alexrj what is the context for coord_t ? What is the assumption/guarantee with using it?

alranel · 2017-04-20T21:42:51Z

xs/src/libslic3r/PerimeterGenerator.cpp

@@ -54,7 +71,7 @@ PerimeterGenerator::process()
    for (Surfaces::const_iterator surface = this->slices->surfaces.begin();
        surface != this->slices->surfaces.end(); ++surface) {
        // detect how many perimeters must be generated for this island
-        const int loop_number = this->config->perimeters + surface->extra_perimeters -1;  // 0-indexed loops
+        const int loop_number = num_loops(this->config->perimeters, surface->extra_perimeters);//this->config->perimeters + surface->extra_perimeters-1;  // 0-indexed loops


Just a note about semantics: loop_number holds the number of loops diminuished by 1. This is related to the implementation of the subsequent code, and I admit the variable could be named in a better way. But when you expose a num_loops() method, it should output the actual number of loops. Then you can subtract 1 in the caller method if it's needed by its implementation.

By the way, I don't see the need for a separate method just for this calculation. I'd embed the logic in the main method.

By embedding it in the main method you can also cache min_shell_thickness and this->ext_perimeter_flow.scaled_width() without recalculating them every time.

loop_number is a constant. Any preference on how to set it? I thought a new method would be okay but you do bring up a good point.

alranel · 2017-04-20T21:49:33Z

xs/src/libslic3r/PerimeterGenerator.cpp

+    if (shell_thickness < min_shell_thickness) {
+        shell_thickness = min_shell_thickness - this->ext_perimeter_flow.scaled_width();
+        generated_perimeters = ceil((float)shell_thickness/this->ext_perimeter_flow.scaled_width());
+    }


Why calling this->ext_perimeter_flow.scaled_width() and /this->ext_perimeter_flow.scaled_width() twice?

By the way, I would simplify the logic:

Calculate how many loops are needed for the requested min_shell_thickness (considering one ext_perimeter followed by many normal perimeters, of course).

Increase loop number until min_shell_thickness is covered.

Also, write some comments.

And you should just skip all of this math if min_shell_thickness is 0.

alranel · 2017-04-20T21:53:17Z

xs/src/libslic3r/PrintConfig.cpp

+    def->label = "Minimum shell thickness";
+    def->category = "Layers and Perimeters";
+    def->sidetext = "mm";
+    def->tooltip = "Minimum shell thickness in mm";


This needs to be more clear. Minimum shell thickness is an alternative way for configuring perimeters and solid shells. If it's set to a non-zero value, they will be automatically increased until the configured value is reached.

alranel · 2017-04-20T21:59:06Z

Thank you for this. I added some comments to the code. Here's more:

Solid layers (top/solid) shall be increased as well. Shell = vertical shell + horizontal shell.
Option shall be added to the GUI (PresetEditor) as well.
Option shall be added to the relevant place in PrintRegion::invalidate_state_by_config() as well.

curieos · 2017-04-21T00:19:51Z

Thank you for the comments. I will get to these soon.

curieos · 2017-04-21T00:54:13Z

I made changes that should work. I'm running a build on my end real quick to make sure and I will then push the changes. I'll then get to working on the 3 other things you mentioned.

lordofhyphens · 2017-04-21T00:54:50Z

We could break constness. Inlining the new code would make semantic sense for a non-const number. So would using another variable and assigning to the const loop_num

curieos · 2017-04-21T00:56:32Z

I came up with a solution that I think you will like that I based on some of the semantics Alex talked about. Should be pushed in the next couple minutes.

-Removed the num_loops function from PerimeterGenerator -Added a scalled min_shell_thickness variable to PerimeterGenerator -Changed the loop_number logic to use a previously defined variable loops

curieos · 2017-04-21T01:25:41Z

I forgot to change the tooltip. I will change it and push it with the next set of changes.

curieos · 2017-04-21T04:47:59Z

I've been trying to find where slic3r creates the layer objects and determines whether or not they are solid and it seems like its in Object.pm. Does this sound right?

-Added min shell to invalidate state method -Created additional checks in PresetEditor and LayerRegion for min shell -Fixed the tooltip to be more descriptive

curieos · 2017-04-24T05:07:04Z

The top and bottom solid layers implementation is in perl. I don't know much perl so I'm going to create another branch+pull request for a version of the method written in C++ before I finish this up.

lordofhyphens · 2018-05-22T23:25:32Z

The code as-is looks good; merging so that it may be extended.

foreachthing · 2018-05-23T09:02:48Z

I hope this is no real issue! I get this, when Slic3r is started with Slic3r-debug-console.exe:
Possible precedence problem on bitwise & operator at C:/Slic3r-master.2018.05.23.1703.f3b5909/lib/Slic3r/GUI/PresetEditor.pm line 806.

Samir55 · 2018-05-23T12:27:43Z

@foreachthing I think it's because a missing '&' in line 806

$config->min_shell_thickness == 0 & $config->top_solid_layers == 0
# Instead of
$config->min_shell_thickness == 0 && $config->top_solid_layers == 0

curieos added 4 commits April 14, 2017 01:58

Added Minimum shell thickness option

10a3960

-Added the option to libslic3r -Created some of the necessary checks for illegal values

Enforce min_shell_thickness as 0 when spiral vase is enabled

2123925

-This could be switched to match the extrusion width, but we already know there will only be 1 perimeter so this is probably more logical

Feature is implemented

a451021

-Added a method in PerimeterGenerator to determine the amount of perimeters needed to meet min_shell_thickness

Added a test in perimeters.t

15eb4a1

lordofhyphens requested review from alranel and lordofhyphens April 16, 2017 13:49

lordofhyphens added Feature request This is an idea for a new feature in Slic3r Perimeters labels Apr 16, 2017

lordofhyphens added this to the 1.3.0 milestone Apr 16, 2017

lordofhyphens assigned lordofhyphens and alranel Apr 16, 2017

lordofhyphens requested changes Apr 16, 2017

View reviewed changes

Fixed styling as requested

27402c5

-Removed the magic number in PerimeterGenerator::num_loops() -Added more test cases in perimeters.t -Added documentation of the new feature in slic3r.pl and README.md

lordofhyphens approved these changes Apr 16, 2017

View reviewed changes

alranel requested changes Apr 20, 2017

View reviewed changes

Implemented requested changes

3bf24a3

-Removed the num_loops function from PerimeterGenerator -Added a scalled min_shell_thickness variable to PerimeterGenerator -Changed the loop_number logic to use a previously defined variable loops

Resolved more implementation issues

06ae86c

-Added min shell to invalidate state method -Created additional checks in PresetEditor and LayerRegion for min shell -Fixed the tooltip to be more descriptive

alranel modified the milestones: 1.3.1, 1.3.0 May 1, 2017

alranel mentioned this pull request May 4, 2017

[GSoC] cvrieos / Wireframe printing #3933

Closed

lordofhyphens added 2 commits June 15, 2017 21:24

Merge branch 'master' into issue-1405

e435dad

Merge branch 'master' into issue-1405

ec86ee3

lordofhyphens merged commit f3b5909 into slic3r:master May 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented feature request #1405 minimum shell thickness option #3885

Implemented feature request #1405 minimum shell thickness option #3885

curieos commented Apr 16, 2017 •

edited by lordofhyphens

Loading

lordofhyphens left a comment

lordofhyphens Apr 16, 2017 •

edited

Loading

lordofhyphens Apr 16, 2017

lordofhyphens Apr 16, 2017

lordofhyphens Apr 16, 2017

lordofhyphens Apr 16, 2017

lordofhyphens Apr 16, 2017

curieos commented Apr 16, 2017

lordofhyphens commented Apr 16, 2017

alranel Apr 20, 2017

lordofhyphens Apr 20, 2017

lordofhyphens Apr 21, 2017

alranel Apr 20, 2017

alranel Apr 20, 2017

curieos Apr 21, 2017

alranel Apr 20, 2017

alranel Apr 20, 2017

alranel Apr 20, 2017

alranel commented Apr 20, 2017

curieos commented Apr 21, 2017

curieos commented Apr 21, 2017

lordofhyphens commented Apr 21, 2017 via email

curieos commented Apr 21, 2017

curieos commented Apr 21, 2017

curieos commented Apr 21, 2017

curieos commented Apr 24, 2017

lordofhyphens commented May 22, 2018

foreachthing commented May 23, 2018

Samir55 commented May 23, 2018

Implemented feature request #1405 minimum shell thickness option #3885

Implemented feature request #1405 minimum shell thickness option #3885

Conversation

curieos commented Apr 16, 2017 • edited by lordofhyphens Loading

lordofhyphens left a comment

Choose a reason for hiding this comment

lordofhyphens Apr 16, 2017 • edited Loading

Choose a reason for hiding this comment

Style

Choose a reason for hiding this comment

Style

Choose a reason for hiding this comment

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robustness

Choose a reason for hiding this comment

curieos commented Apr 16, 2017

lordofhyphens commented Apr 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alranel commented Apr 20, 2017

curieos commented Apr 21, 2017

curieos commented Apr 21, 2017

lordofhyphens commented Apr 21, 2017 via email

curieos commented Apr 21, 2017

curieos commented Apr 21, 2017

curieos commented Apr 21, 2017

curieos commented Apr 24, 2017

lordofhyphens commented May 22, 2018

foreachthing commented May 23, 2018

Samir55 commented May 23, 2018

curieos commented Apr 16, 2017 •

edited by lordofhyphens

Loading

lordofhyphens Apr 16, 2017 •

edited

Loading