Thin wall improvements (v2) #4534

supermerill · 2018-09-06T15:27:49Z

now keep the bit with 0 width to merge them instead of just delete them
merging algorithm re-coded (extensively)
change behavior of thick polyline to only 1 width value per point instead of 2 per pair of adjacent points.
do not post-process the voronoi diagram in polylines, now it's don in expolygon.medial_axis.
failsafes in perimeter_generator (too many splits, too small)
some new tests.

also, a test will fail (the one that check if the half-circle has only angle of the same direction:

From my test, changing my port-processing algorithm to push it more to the inner side has many unwanted side-effects. It's intended that it's going a bit more to the outer rim because there are more space to fill, and i can't know for sure the circle continue and it's not a strait after that.

Tell me if i forgot some formatting.

- now keep the bit with 0 width to merge them instead of just delete them - merging algorithm re-coded (extensively) - change behavior of thick polyline to only 1 width value per point instead of 2 per pair of adjacent points. - do not post-process the voronoi diagram in polylines, now it's don in expolygon.medial_axis. - failsafes in perimeter_generator (too many splits, too small) - some new tests.

ledvinap

just random remarks, I'll have to look at medial_axis a bit more to understant it...

Also int32_ts look ugly and resulting code is probably slower than using size_t / int / usingned

ledvinap · 2018-09-06T15:53:21Z

xs/src/libslic3r/ExPolygon.cpp

+remove_point_too_near(ThickPolyline* to_reduce) {
+        const int32_t smallest = SCALED_EPSILON * 2;
+        uint32_t id = 1;
+        while (id < to_reduce->points.size() - 2) {


This looks like last two points won't be checked for smallest, is that intentional?

i won't check the first and the last. But it seems I forgot to check the last-1 point... Thank you.

ledvinap · 2018-09-06T15:55:55Z

xs/src/libslic3r/ExPolygon.cpp

+        const int32_t smallest = SCALED_EPSILON * 2;
+        uint32_t id = 1;
+        while (id < to_reduce->points.size() - 2) {
+            uint32_t newdist = min(to_reduce->points[id].distance_to(to_reduce->points[id - 1])


(subtle performance - to_reduce->points[id].distance_to(to_reduce->points[id - 1] can be small only if id==1)

you're right

There is another (theoretical) problem: If points are less than smallest apart, it will remove all of them, one by one ...

ledvinap · 2018-09-06T16:10:49Z

xs/src/libslic3r/ExPolygon.cpp

+            coordf_t new_width = to_modify->width[idx_other - 1] * (1 - percent_dist);
+            new_width += to_modify->width[idx_other] * (percent_dist);
+            Point new_point;
+            new_point.x = (coord_t)((double)(to_modify->points[idx_other - 1].x) * (1 - percent_dist));


It may be better to calculate to_modify->points[idx_other - 1].x * (1 - percent_dist) + to_modify->points[idx_other].x * (percent_dist) and then round to integer (coord_t)

note: I forgot to remove the unnecessary (coord_t)((double)( )).
I don't see a cast problem.
Maybe i can pre-compute some things to remove some unnecessary operations here.

the code truncates to both parts to integer before adding the (+=). Adding doubles and then truncating will slightly improve accuracy. But that is minot detail.

Function to blend/interpolate points (in point class) may be good idea - it will improve code readability

coordf_t is a double, or i doesn't understand.

coord_t is integer type

ledvinap · 2018-09-06T16:30:33Z

xs/src/libslic3r/ExPolygon.cpp

+    Point point_before = id_nearest == 0 ? contour.contour.points.back() : contour.contour.points[id_nearest - 1];
+    Point point_after = id_nearest == contour.contour.points.size()-1 ? contour.contour.points.front() : contour.contour.points[id_nearest + 1];
+    //compute angle
+    angle = min(nearest.ccw_angle(point_before, point_after), nearest.ccw_angle(point_after, point_before));


(this is equivalent to

angle = nearest.ccw_angle(point_before, point_after); if (angle >= PI) angle = PI - angle; // smaller angle

Thanks. i think i was tired...

The correct one is

angle = nearest.ccw_angle(point_before, point_after);
if (angle >= PI) angle = 2*PI - angle; // smaller angle

Yes, sorry ..

ledvinap · 2018-09-06T16:39:33Z

xs/src/libslic3r/ExPolygon.cpp

+    angle = min(nearest.ccw_angle(point_before, point_after), nearest.ccw_angle(point_after, point_before));
+    //compute the diff from 90�
+    angle = abs(angle - PI / 2);
+    if (near.x != nearest.x && near.y != nearest.y && max(nearestDist, nearDist) + SCALED_EPSILON < nearest.distance_to(near)) {


nearDist is not updated in loop, it is still distance from contour.contour.points.front()

no need to test near.x != nearest.x && near.y != nearest.y, nearest.distance_to(near) will handle that

near.x != nearest.x && near.y != nearest.y

I used near != nearest in slic3r pe but the comparison operator isn't made here. You think nearest.distance_to(near) != 0 is better? I'm okay with that

i will look for nearDist tomorrow

distance_to use sqrt(distance_to_sq(point));, it may be more efficient to use the comparison.
I may just copy the operator== from slic3rPE

ledvinap · 2018-09-06T18:07:14Z

xs/src/libslic3r/ExPolygon.cpp

+            polyline.width.erase(polyline.width.begin());
+            changes = true;
+        }
+        while (polyline.points.size() > 1 && polyline.width.back() < min_width && polyline.endpoints.second) {


(may be cleaner to reverse polyline and reuse front code)

you're right

xs/src/libslic3r/MultiPoint.cpp

ledvinap · 2018-09-06T18:49:09Z

xs/src/libslic3r/Polyline.cpp

+                if (j == i) continue;
+                ThickPolyline *other = &pp[j];
+                if (polyline->last_point().coincides_with(other->last_point())) {
+                    other->reverse();


(it may be better to delay reverse until other is actually chosen for concatenation)

i will look at it

ledvinap · 2018-09-06T18:53:36Z

xs/src/libslic3r/Polyline.cpp

+            }
+            if (id_candidate_last_point == id_candidate_first_point && nbCandidate_first_point == 1 && nbCandidate_last_point == 1) {
+                // it's a trap! it's a  loop!
+                if (pp[id_candidate_first_point].points.size() > 2) {


this won't close polyline - is it different from case when 3 polylines are joined together. But I'm not sure which one is correct

It test if a polyline loop on itself. There are a test with a square that trigger this, if i remember well.

Yes - but in his case first and last point of resulting polyline won't overlap. It is different when 3 polylines are joined in one step

xs/src/libslic3r/Polyline.hpp

supermerill · 2018-09-06T21:41:45Z

Thank you for your review!

I can replace some int32_t by coord_t when it's semantically useful. I didn't re-check all the types, there are maybe some improvement to do.

Don't ever use int / unsigned if you want more than 2^16 values or less than 2^64, it's not cross-platform safe. They are the c++ correct way to define a type with a defined number of bytes. https://en.cppreference.com/w/cpp/types/integer

For the performance impact: if i need the precision, i can't do something else. If there are no need for precision, maybe int is better.

ledvinap

int_fast32_t should be used then ...
int32_t may be slower that int on 64bit system ... (int16_t is slower that int on ARM)
An int32_t is distracting - it makes me think 'why is 32bit important`

Personally I use int / signed / unsigned as meaning 'reasonably large integer type without special requirements' .. only 8-bit CPU may be a bit problematic, but even then int should be enough to index most object that fit in memory ...

ledvinap · 2018-09-06T21:56:19Z

xs/src/libslic3r/ExPolygon.cpp

+        const int32_t smallest = SCALED_EPSILON * 2;
+        uint32_t id = 1;
+        while (id < to_reduce->points.size() - 2) {
+            uint32_t newdist = min(to_reduce->points[id].distance_to(to_reduce->points[id - 1])


There is another (theoretical) problem: If points are less than smallest apart, it will remove all of them, one by one ...

ledvinap · 2018-09-06T21:58:20Z

xs/src/libslic3r/ExPolygon.cpp

+/// add points  from pattern to to_modify at the same % of the length
+/// so not add if an other point is present at the correct position
+void
+add_point_same_percent(ThickPolyline* pattern, ThickPolyline* to_modify) {


(not exactly sure how merging works) - it may work better if point distance (not percentage) is matched (starting from branching point)

need % because two polylines don't have the same length

You can use length of shorter one to do pairwise merging and them handle rest of somewow. But that is probably not important, lines should be short anyway.

ledvinap · 2018-09-06T22:01:37Z

xs/src/libslic3r/ExPolygon.cpp

+            coordf_t new_width = to_modify->width[idx_other - 1] * (1 - percent_dist);
+            new_width += to_modify->width[idx_other] * (percent_dist);
+            Point new_point;
+            new_point.x = (coord_t)((double)(to_modify->points[idx_other - 1].x) * (1 - percent_dist));


the code truncates to both parts to integer before adding the (+=). Adding doubles and then truncating will slightly improve accuracy. But that is minot detail.

Function to blend/interpolate points (in point class) may be good idea - it will improve code readability

ledvinap · 2018-09-06T22:05:11Z

xs/src/libslic3r/ExPolygon.cpp

+                    if (k == i | k == j) continue;
+                    ThickPolyline& main = pp[k];
+                    if (polyline.first_point().coincides_with(main.last_point())) {
+                        main.reverse();


Yes, I missed the reference ... on first look is looks like only main is affected ...

ledvinap · 2018-09-06T22:07:04Z

xs/src/libslic3r/ExPolygon.cpp

+            if (polyline.width[1] > min_width) {
+                double percent_can_keep = (min_width - polyline.width[0]) / (polyline.width[1] - polyline.width[0]);
+                if (polyline.points.front().distance_to(polyline.points[1]) * percent_can_keep > max_width / 2
+                    && polyline.points.front().distance_to(polyline.points[1])* (1 - percent_can_keep) > max_width / 2) {


Yes, but why test part that is removed ? (1 - percent_can_keep) ?
(I assume this code only cuts end , does not split)

xs/src/libslic3r/MultiPoint.cpp

ledvinap · 2018-09-06T22:16:16Z

xs/src/libslic3r/Polyline.cpp

+            }
+            if (id_candidate_last_point == id_candidate_first_point && nbCandidate_first_point == 1 && nbCandidate_last_point == 1) {
+                // it's a trap! it's a  loop!
+                if (pp[id_candidate_first_point].points.size() > 2) {


Yes - but in his case first and last point of resulting polyline won't overlap. It is different when 3 polylines are joined in one step

xs/src/libslic3r/Polyline.cpp

xs/src/libslic3r/Polyline.hpp

supermerill · 2018-09-07T19:22:12Z

I've implemented most of your advices, changed most of int32 into size_t (you're right, that is the right thing)
But some corrections trigger something and now i have an other post-process to do, so it will takes some time for the v2.1

ledvinap · 2018-09-09T16:16:18Z

@supermerill : Can you post some screenshots of interesting Voronoi cases this PR tries to handle, please?

supermerill · 2018-09-10T15:19:38Z

)

green : polygon used to create the voronoi.
grey: surface with the anchor inside the part.
black : voronoi

ledvinap · 2018-09-10T16:15:43Z

@supermerill :
Thank you!

One quick observation is that it may be useful to create voronoi over whole area including anchors and too thin areas and then prune/mark branches that lead outside of green area. This will remove most Y-branches (ot there will be middle branch - Ψ)...

supermerill · 2018-09-10T16:20:06Z

I tried that first. Problem is the anchor is too wide and this lead to absurdly complex branch.
If i want to improve more, i will put in the trash the voronoi and create a special algo from scratch that try to connect the anchor together (or anchor-farest point).

ledvinap · 2018-09-10T16:38:48Z

xs/src/libslic3r/ExPolygon.cpp

-
+            double dot_poly_branch = 0;
+            double dot_candidate_branch = 0;
+
            // find another polyline starting here
            for (size_t j = i + 1; j < pp.size(); ++j) {


Maybe it would be worth finding all polylines starting from (ends of) polyline, assign scores to them and select good candidates ... It would partially remove k loop and may be easier to understand the algorithm ...

ledvinap · 2018-09-10T16:43:27Z

@supermerill : Just a guess, but voronoi / medial axis is mathematically sound approach to this. Trying to reinvent wheel isn't usually good idea ...

BTW: the branch complexity is inside current voronoi area?

supermerill · 2018-09-10T17:01:48Z

From memory, if i use the "anchor" area:

ledvinap · 2018-09-11T11:09:57Z

@supermerill : the tree in anchor may be better if anchor areas for voronoi are wider (1 or 2 extrusion widths in size)

supermerill · 2018-09-14T12:03:50Z

The corrections made since i post this PR deteriorate the results, and i didn't succeed to apply a good enough rubber band to it.
I may:

not applying corrections that deteriorate the result (easiest)
redo all the post-proccess steps in a different way (I already use too much time to do that).
create a custom "voronoi" algo (more difficult & time-consuming).
found an idea that resolve everything magically (unlikely)

ledvinap · 2018-09-14T12:52:22Z

@supermerill :
@lordofhyphens should probably state his opinion.
I was interested in thin wall code, so I reviewed your PR where reading it ...
Personally I would prefer more math and less heuristics ;-), but that is out of scope of this PR ...
(and I still didn't find time to port pathfinding to new slic3r version)

lordofhyphens · 2018-09-14T13:47:13Z

@ledvinap I haven't butted in yet mostly because I thought you were doing a fine review job ;)

@supermerill I do note that a test is failing on the Perl build specifically around thin wall so far.

I would prefer that tests be applied to the C++ testing apparatus instead of the Perl side, but that can be covered as part of porting the existing tests.

ledvinap · 2018-09-17T13:57:16Z

@supermerill :

Slic3r/xs/src/libslic3r/Geometry.cpp

Line 882 in d5a2e1b

if (w0 > this->max_width && w1 > this->max_width)

may influence anchor points - it will probably discard voronoi edges in anchor areas

* corrections from review * more functions for a clearer code * now simplify the frontier with the anchor to avoid weird edge-cases. * new post-process: remove "curve edge" before merging * new post-process: cube corner: the small bit that go from the voronoi corner to the cube corner is now a "pulling string" that pull the voronoi corner a bit to make a nicer cube. * _variable_width : reduce the threshold for creating a new extrusion by half vs threshold to create segments (if not, it doesn't create enough)

supermerill · 2018-10-04T08:19:29Z

I do note that a test is failing on the Perl build specifically around thin wall so far.

It's explained on the initial post. I talked to you about that and you said "you want to see it by yourself before making an opinion".

btw, here is the v3. I move all the code in a new medial_axis.cpp file, in a dozen of functions for clarity.

lordofhyphens · 2018-10-14T19:14:46Z

xs/src/libslic3r/PerimeterGenerator.cpp

+                            no_thin_zone = diff(last, offset(expp, (float)(min_width / 2)), true);
+                        }
+                        // compute a bit of overlap to anchor thin walls inside the print.
+                        for (Polygon &ex : expp) {


Should this be an ExPolygon? I'm getting a build error on my system when trying to do a clean build of your branch.

lordofhyphens · 2018-10-14T19:18:05Z

Tried to build after your refactor; compile failed because Slic3r::Polygon doesn't define remove_point_too_near() and you use it during PerimeterGenerator (line 133).

lordofhyphens · 2018-10-15T00:02:09Z

I backed up to 6ba61fe so I could build.

Looks like with that version, there's some undesired side effects. Left is with your build, right is current master.

Config + model: thinwall-problems.zip

VanessaE · 2018-12-02T01:42:40Z

Fwiw, I get the same failure as @lordofhyphens..

x86_64-linux-gnu-gcc -I/usr/lib/x86_64-linux-gnu/perl/5.28/CORE -fPIC -D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -xc++ -Isrc/libslic3r -D_GLIBCXX_USE_C99 -DHAS_BOOL -DNOGDI -DSLIC3RXS -DBOOST_ASIO_DISABLE_KQUEUE -Dexprtk_disable_rtl_io_file -Dexprtk_disable_return_statement -Dexprtk_disable_rtl_vecops -Dexprtk_disable_string_capabilities -Dexprtk_disable_enhanced_features -U__STRICT_ANSI__ -std=c++11 -DBOOST_LIBS -DNDEBUG -O -Wno-undefined-var-template -Isrc -Ibuildtmp -c -D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -DDEBUG_SERIAL -O2 -g -o src/libslic3r/ClipperUtils.o src/libslic3r/ClipperUtils.cpp
x86_64-linux-gnu-gcc -I/usr/lib/x86_64-linux-gnu/perl/5.28/CORE -fPIC -D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -xc++ -Isrc/libslic3r -D_GLIBCXX_USE_C99 -DHAS_BOOL -DNOGDI -DSLIC3RXS -DBOOST_ASIO_DISABLE_KQUEUE -Dexprtk_disable_rtl_io_file -Dexprtk_disable_return_statement -Dexprtk_disable_rtl_vecops -Dexprtk_disable_string_capabilities -Dexprtk_disable_enhanced_features -U__STRICT_ANSI__ -std=c++11 -DBOOST_LIBS -DNDEBUG -O -Wno-undefined-var-template -Isrc -Ibuildtmp -c -D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -DDEBUG_SERIAL -O2 -g -o src/libslic3r/PerimeterGenerator.o src/libslic3r/PerimeterGenerator.cpp
src/libslic3r/PerimeterGenerator.cpp: In member function ‘void Slic3r::PerimeterGenerator::process()’:
src/libslic3r/PerimeterGenerator.cpp:133:32: error: ‘class Slic3r::Polygon’ has no member named ‘remove_point_too_near’
                             ex.remove_point_too_near(SCALED_RESOLUTION);
                                ^~~~~~~~~~~~~~~~~~~~~
At global scope:
cc1plus: warning: unrecognized command line option ‘-Wno-undefined-var-template’
error building src/libslic3r/PerimeterGenerator.o from 'src/libslic3r/PerimeterGenerator.cpp' at /usr/share/perl/5.28/ExtUtils/CBuilder/Base.pm line 185.
FAIL
! Installing ./xs failed. See /home/vanessa/.cpanm/work/1543714510.6618/build.log for details. Retry with --force to force install it.
The XS/C++ code failed to compile, aborting

supermerill · 2018-12-02T16:54:53Z

I'll look at why. Maybe a file i forgot to add to a commit.

…wall

VanessaE · 2018-12-03T13:23:39Z

Still no good (applied on top of commit 21eb603)

x86_64-linux-gnu-gcc -I/usr/lib/x86_64-linux-gnu/perl/5.28/CORE -fPIC -D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -xc++ -Isrc/libslic3r -D_GLIBCXX_USE_C99 -DHAS_BOOL -DNOGDI -DSLIC3RXS -DBOOST_ASIO_DISABLE_KQUEUE -Dexprtk_disable_rtl_io_file -Dexprtk_disable_return_statement -Dexprtk_disable_rtl_vecops -Dexprtk_disable_string_capabilities -Dexprtk_disable_enhanced_features -U__STRICT_ANSI__ -std=c++11 -DBOOST_LIBS -DNDEBUG -O -Wno-undefined-var-template -Isrc -Ibuildtmp -c -D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -DDEBUG_SERIAL -O2 -g -o src/libslic3r/PerimeterGenerator.o src/libslic3r/PerimeterGenerator.cpp
src/libslic3r/PerimeterGenerator.cpp: In member function ‘void Slic3r::PerimeterGenerator::process()’:
src/libslic3r/PerimeterGenerator.cpp:137:91: error: no matching function for call to ‘intersection_ex(Slic3r::ExPolygons, Slic3r::Polygons&, bool)’
                        CLIPPER_OFFSET_SCALE, jtSquare, 3), no_thin_zone, true);
                                                                              ^

In file included from src/libslic3r/PerimeterGenerator.cpp:2:
src/libslic3r/ClipperUtils.hpp:146:1: note: candidate: ‘Slic3r::ExPolygons Slic3r::intersection_ex(const Polygons&, const Polygons&, bool)’
 intersection_ex(const Slic3r::Polygons &subject, const Slic3r::Polygons &clip, bool safety_offset_ = false)
 ^~~~~~~~~~~~~~~
src/libslic3r/ClipperUtils.hpp:146:1: note:   no known conversion for argument 1 from ‘Slic3r::ExPolygons’ {aka ‘std::vector<Slic3r::ExPolygon>’} to ‘const Polygons&’ {aka ‘const std::vector<Slic3r::Polygon>&’}
src/libslic3r/ClipperUtils.hpp:152:1: note: candidate: ‘Slic3r::ExPolygons Slic3r::intersection_ex(const ExPolygons&, const ExPolygons&, bool)’
 intersection_ex(const Slic3r::ExPolygons &subject, const Slic3r::ExPolygons &clip, bool safety_offset_ = false)
 ^~~~~~~~~~~~~~~
src/libslic3r/ClipperUtils.hpp:152:1: note:   no known conversion for argument 2 from ‘Slic3r::Polygons’ {aka ‘std::vector<Slic3r::Polygon>’} to ‘const ExPolygons&’ {aka ‘const std::vector<Slic3r::ExPolygon>&’}
At global scope:
cc1plus: warning: unrecognized command line option ‘-Wno-undefined-var-template’
error building src/libslic3r/PerimeterGenerator.o from 'src/libslic3r/PerimeterGenerator.cpp' at /usr/share/perl/5.28/ExtUtils/CBuilder/Base.pm line 185.
FAIL
! Installing ./xs failed. See /home/vanessa/.cpanm/work/1543842712.21555/build.log for details. Retry with --force to force install it.
The XS/C++ code failed to compile, aborting

(needs rebased, also)

AppVeyorBot · 2019-02-18T14:07:23Z

❌ Build Slic3r 1.3.0-master-2285 failed (commit 26aeaec7b5 by @)

AppVeyorBot · 2019-02-18T14:42:48Z

❌ Build Slic3r 1.3.0-master-2286 failed (commit a9ae12dba4 by @)

* concatenate_polylines_with_crossing: now connected the good end with the other good end * discretize_variable_width: now use TP.width->flow.spacing for gap-fill and TP.width->flow.width if possible for thin-walls. * bugfix thin wall (medial axis): use correct temp variables for main_fusion.

AppVeyorBot · 2019-02-18T16:34:09Z

❌ Build Slic3r 1.3.0-master-2289 failed (commit 47c0f543d8 by @)

VanessaE · 2019-03-25T08:20:37Z

@supermerill just want to remind you that thin wall mode is negatively affecting gap fill...

I am at commit 05a2f90 with #4743 and your thin walls PR applied on top.

Consider a wall of variable thickness (due to tapering or curving), ranging from around 1 mm to around 1.5 mm.

With thin walls enabled, and 2 perimeter loops requested, Slic3r sees that the wall is too thin for two perimeter loops, ignores that setting, and produces a perfect result, with one loop plus gap fill as needed:

With thin walls DISabled, and still 2 perimeter loops requested, the result is one perimeter plus gap fill in the thinner areas, and two perimeter loops where the thickest gap fill should have been, with the inner loop overlapping itself, causing over-filling of the gap:

With thin walls disabled, and now just 1 perimeter loop requested, the result is one perimeter loop plus gap fill in the thinner places, and empty in the places where the thickest gap fill should have been:

In all three cases, gap fill is of course enabled.

The model pictured above: Penguin Extruder MK6 - cold end fan shroud.stl.zip
Config: slic3r-configs-20190323-2.ini.zip

As an aside, some CI tests are failing, maybe you'll want to look into it -- it looks like just some failed unit tests, but I had no such failures on my build.

supermerill · 2019-03-25T08:54:07Z

@VanessaE

thin wall mode is negatively affecting gap fill.

I agree with each of three use-cases. What is the problem (the negative part)?

VanessaE · 2019-03-25T09:08:45Z

The negative part is that a "thin wall" is not (or should not be) related to gap fill; turning thin wall mode off should not cause any change in the gap fill behavior of a wall that's already too thick for "thin wall" mode to affect to begin with, so all three cases should produce the same result, as in the first image. Especially since trying to cram a second perimeter loop into a space that's only big enough for a little over half of its volume just over-stuffs the wall, leading to poor print quality.

supermerill · 2019-03-25T14:54:35Z

It's not my fault, I kept the original design (or maybe i made a mistake?).

I can change that but I won't unless a majority are for the change

also you said "thin wall mode is negatively affecting gap fill", but maybe "not using thin wall mode is negatively affecting gap fill" was the right thing to understand?

for the some CI tests failing, i have to devote some time to push my fixes

VanessaE · 2019-03-25T18:52:11Z

Yeah perhaps my terminology is a little off. However,

I can change that but I won't unless a majority are for the change

Considering that both overfilling a wall and leaving it empty results in bad print quality, and gap fill being affected by the thin wall setting is clearly a bug anyway, I would advise fixing this. I doubt anyone will complain. 😃 @lordofhyphens @alranel @dwillmore @platsch ?

platsch · 2019-04-03T13:43:01Z

I didn't run the test provided by @VanessaE myself. Why is there a difference between the 1. and 3. case? Does thin wall change the extrusion width in the 1. case or is it same in 1 and 3 and the gap fill behaves different?

I generally agree with @VanessaE but would expect that in the 3. case either additional perimeters are generated in the non-filled region or, if that doesn't fit, it would be covered by infill or gap fill and currently don't understand why that isn't happening.

The 2. case looks like a bug, but not in the thin wall logic?

supermerill · 2019-04-05T13:06:47Z

Hello
I'm reviewing some tests to see what's going wrong with my code and I have two complicated questions.

adaptative-width.t

the first test is about how many perimeters & gap fill we need to fill a wall.
width = 0.7
height = 0.4
=> spacing = 0.7-0.086 = 0.614

width mult	wall thickness	perimeter count	gap fill	thin wall	my calculus
1	0.7	0	0	1	0+0.7
1.3	0.91	0	0	1	0+1.3
1.5	1.05	0	0	1	0+1.5
2	1.4	1	0	0	1.314+0
2.5	1.75	1	1	0	1.314 + 0.436 = 1.75
3	2.1	1	1	0	1.314 + 0.786= 2.1
3.5	2.45	2	0	0	2.542 + 0
4	2.8	2	1	0	2.542 + 0.258

the 5 first column are what is written in the test. the last column is my calculus of the wall thickness from the width+spacing and the perimeter count + gapfill/thinwall.

the fourth line, in my opinion, may need a small gapfill of 0.086.
Currently, i have a minimum thickness for gap fill at 0.08 * width (hence, why i fail the current test), should I upped the minimum gapfill width to 0.2 * width? I can also check that the minimum width have to create a strictly positive spacing (more than 0.086 of width for a height of 0.4), it should do it.

The 7 line over-extrude because it uses a "INSET_OVERLAP_TOLERANCE" (of 40%) to "avoid using medial axis".
I won't erase that behaviour in this pr, but maybe I should in the next one?

testfill.cpp

It's not related to this pr, but i will ask it here for convenience. I can open a new issue if the answer isn't trivial.
In this test, in "Solid surface fill", you create a point by scaling an already scaled number (like 20327272.01297), so it creates the point with a y value of 20327272012970nm, (=127C CFD3 DCAA) (multiplied by 1 000 000, the scaling factor)
As it's also multiplied by CLIPPER_OFFSET_SCALE (100 000), so clipper lib is given a coordinate of 2032727201290000000 (1C35 B29E A694 E640).
The max value for clipper is 3FFF FFFF FFFF FFFF, so everything is alright (for now).
But this test increases the size of the polygon, from 100% to 260%
and by multiplying it by 2.6, we have 4958 6A02 E33A 3A80 > 3FFF FFFF FFFF FFFF.

So how can this test pass? i tried it against my fork and it fails.

AppVeyorBot · 2019-04-06T21:56:44Z

❌ Build Slic3r 1.3.0-master-2305 failed (commit 6c0396a3c2 by @)

AppVeyorBot · 2019-04-07T12:48:40Z

❌ Build Slic3r 1.3.0-master-2306 failed (commit c5eb936f53 by @)

AppVeyorBot · 2019-04-07T19:55:27Z

✅ Build Slic3r 1.3.0-master-2307 completed (commit 2b145d55ee by @)

supermerill · 2019-04-15T16:27:39Z

strange, github doesn't want to add more commit from my branch inside this pr...

lordofhyphens · 2019-04-15T16:54:21Z

Force push again?

supermerill · 2019-04-17T17:15:40Z

corrected the last issue.
I tried to force-push, something has begun to move but it was shut down. Maybe I should re-open a pr but from this repo?
I'm writing some cpp tests.

Do i have something more to correct?
I had a comment about returning MAX_SIZE(size_t) instead of -1 (because of unsigned type) for "no values", Is it ok or what is the good thing to do?

VanessaE · 2019-04-18T21:45:46Z

Continued at #4790

lordofhyphens · 2019-04-18T22:37:06Z

Closing as it is continued elsewhere.

ledvinap reviewed Sep 6, 2018

View reviewed changes

ledvinap reviewed Sep 10, 2018

View reviewed changes

supermerill added 2 commits October 1, 2018 19:14

forgot medial_axis.hpp/cpp files

e4484c4

lordofhyphens reviewed Oct 14, 2018

View reviewed changes

Merge remote-tracking branch 'remotes/origin/master' into alexrj_thin…

418c770

…wall

supermerill requested a review from alranel as a code owner December 3, 2018 13:09

supermerill force-pushed the alexrj_thinwall branch 2 times, most recently from 0143d53 to 6f61c00 Compare December 3, 2018 14:04

supermerill force-pushed the alexrj_thinwall branch from 0f0147f to bab76a9 Compare February 18, 2019 14:26

supermerill force-pushed the alexrj_thinwall branch 2 times, most recently from 4c3946d to 0cadc74 Compare February 18, 2019 15:28

supermerill force-pushed the alexrj_thinwall branch from 0cadc74 to f06491c Compare February 18, 2019 15:42

supermerill mentioned this pull request Mar 27, 2019

Inconsitent handling of tiny structures prusa3d/PrusaSlicer#1907

Open

VanessaE mentioned this pull request Mar 29, 2019

Gibberish/invalid feedrates in exported g-code #4769

Closed

little bugfix for thinwall : concatenate_polylines_with_crossing

1687c5c

Add sanity check for gapfill's min width.

1c45045

supermerill force-pushed the alexrj_thinwall branch from 0818ce6 to 1c45045 Compare April 7, 2019 19:18

lordofhyphens closed this Apr 18, 2019

VanessaE mentioned this pull request Jun 22, 2020

[FR] Import and finish-up "Thin wall improvements (v3)" from upstream Slic3r prusa3d/PrusaSlicer#4422

Open

Thin wall improvements (v2) #4534

Thin wall improvements (v2) #4534

Conversation

supermerill commented Sep 6, 2018 • edited Loading

ledvinap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supermerill Sep 6, 2018 • edited Loading

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

Choose a reason for hiding this comment

supermerill Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

supermerill Sep 11, 2018 • edited Loading

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

supermerill Sep 6, 2018 • edited Loading

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

supermerill Sep 6, 2018 • edited Loading

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

supermerill commented Sep 6, 2018 • edited Loading

ledvinap left a 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

Choose a reason for hiding this comment

supermerill commented Sep 7, 2018

ledvinap commented Sep 9, 2018

supermerill commented Sep 10, 2018 • edited Loading

ledvinap commented Sep 10, 2018

supermerill commented Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

ledvinap commented Sep 10, 2018

supermerill commented Sep 10, 2018

ledvinap commented Sep 11, 2018

supermerill commented Sep 14, 2018 • edited Loading

ledvinap commented Sep 14, 2018

lordofhyphens commented Sep 14, 2018

ledvinap commented Sep 17, 2018

supermerill commented Oct 4, 2018

Choose a reason for hiding this comment

lordofhyphens commented Oct 14, 2018

lordofhyphens commented Oct 15, 2018

VanessaE commented Dec 2, 2018

supermerill commented Dec 2, 2018 • edited Loading

VanessaE commented Dec 3, 2018

AppVeyorBot commented Feb 18, 2019

AppVeyorBot commented Feb 18, 2019

AppVeyorBot commented Feb 18, 2019

VanessaE commented Mar 25, 2019 • edited Loading

supermerill commented Mar 25, 2019

VanessaE commented Mar 25, 2019 • edited Loading

supermerill commented Mar 25, 2019 • edited Loading

VanessaE commented Mar 25, 2019

platsch commented Apr 3, 2019

supermerill commented Apr 5, 2019 • edited Loading

adaptative-width.t

testfill.cpp

AppVeyorBot commented Apr 6, 2019

supermerill commented Sep 6, 2018 •

edited

Loading

supermerill Sep 6, 2018 •

edited

Loading

supermerill Sep 6, 2018 •

edited

Loading

supermerill Sep 11, 2018 •

edited

Loading

supermerill Sep 6, 2018 •

edited

Loading

supermerill Sep 6, 2018 •

edited

Loading

supermerill commented Sep 6, 2018 •

edited

Loading

supermerill commented Sep 10, 2018 •

edited

Loading

supermerill commented Sep 10, 2018 •

edited

Loading

supermerill commented Sep 14, 2018 •

edited

Loading

supermerill commented Dec 2, 2018 •

edited

Loading

VanessaE commented Mar 25, 2019 •

edited

Loading

VanessaE commented Mar 25, 2019 •

edited

Loading

supermerill commented Mar 25, 2019 •

edited

Loading

supermerill commented Apr 5, 2019 •

edited

Loading