Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thin wall improvements (v3) #4790

Open
wants to merge 36 commits into
base: master
Choose a base branch
from
Open

Thin wall improvements (v3) #4790

wants to merge 36 commits into from

Conversation

supermerill
Copy link
Collaborator

@supermerill supermerill commented Apr 18, 2019

As it seems like the previous one is now a bit buggy, I redo the pr here on this branch.

Before : it

  • use the output of the voronoi and
  • remove everything at 0 width
  • extends the line to the end of the surface.

now it

  • create a simplied version of surface, store it in expolygon
  • use the output of the voronoi and
  • fusion little polylines created (by voronoi) on the external side of a curve inside the main polyline.
  • fusion polylines created by voronoi, where needed.
  • like fusion_curve but for sharp angles like a square corner.
  • extends the polylines inside bounds, use extends_line on both end
  • extends the polylines inside bounds (anchors)
  • remove too thin bits at start & end of polylines
  • instead of keeping polyline split at each corssing, we try to create long strait polylines that can cross each other.
  • remove bits around points that are too thin (can be inside the polyline)
  • delete polylines that are too short
  • be sure we didn't try to push more plastic than the volume defined by surface * height can receive. If overextruded, reduce all widths by the correct %.
  • if nozzle_diameter > min_width, grow bits that are < width(nozzle_diameter) to width(nozzle_diameter) (don't activate that for gapfill)
  • taper the ends of polylines (don't activate that for gapfill)

supermerill added 30 commits September 6, 2018 17:18
 - 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.
* 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)
add to cmakelist
add needed function (mostly copy from slic3rPE)
…same orientation'

to 'all medial axis segments of a semicircumference have the same orientation (but the 2 end points)'
correct a bug with offset parameters.
some bugfix on the algo of thin_walls.
…_width (can be toggled when calling expolygon.medial_axis).
 - reworked thin_variable_width (discretize into segments of constant width)
 - bugfix taper_ends
 - add setting thin_walls_overlap to control the perimeter/thin wall overlap
 - add point.interpolate(%,point) : point method
 * 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.
 - taper_ends fix
 - empty_vector.back() bugfix
 - more accurate min width from nozzle diameter & layer height
 - add epsilon offset when trying to see if points are inside a polygon
 - failsafe not safe (do not fusion with a too high width).
 - add remove_collinear_points() in the simplify function.
adding comments
@VanessaE
Copy link
Collaborator

VanessaE commented Sep 1, 2019

bump @supermerill I know time is short, but please, let's get this finished before it dies.

@supermerill
Copy link
Collaborator Author

I just found 2-3 more bugs, have corrected them, will push that shortly (with a new test).

@lordofhyphens
Copy link
Member

I am paying attention :)

@VanessaE
Copy link
Collaborator

VanessaE commented Oct 3, 2019

17 days to push a few bug fix commits? Man, that's a slow pipe. 😉

@supermerill
Copy link
Collaborator Author

Problem is, i moved things and the computer that has the compiling environment isn't easily accessible anymore. Also i'm sick so motivation is low to re-setup a new one.

@AppVeyorBot
Copy link

Build Slic3r 1.3.0-master-2368 completed (commit 3cdb10c243 by @)

@VanessaE
Copy link
Collaborator

VanessaE commented Oct 23, 2019

OK, I pulled to HEAD today and applied this PR. Clean build, no errors or warnings that I noticed.

Everything looks good on the face... lines are well-sized and placed, but there's a major problem:

I use autospeed mode; when thin wall detection is enabled, all print moves (perimeters, sparse infill, solid infill, you name it) have their speeds just set to F0.000 in the exported file, instead of speeds consistent with the profile.

Lines that are thin enough to hit the configured speed cap (such as what I assume to be gap fill) get properly-set to the cap. Since Marlin just ignores a speed of 0, and the only other valid speeds in most of the file are those set to the aforementioned cap, the whole print just runs at that speed.

In the print profile I'm using (attached below), print moves should target a volumetric rate of 5.0 mm³/s.
Most regular print moves in this profile should have speeds of around 70 mm/s, and indeed they are when thin wall mode is disabled. I have the maximum speed capped at 300 mm/s. So, with thin walls mode enabled, the whole print just runs at 300 mm/s.

She cannae take anymore; she'll blow, captain! 😁

Here's an example of bad g-code, generated with thin walls enabled, showing speeds of 0 on normal print moves, interspersed with speeds matching my 300 mm/s cap (note: travels are also set to 300 mm/s in this profile):

Show the code...
G11
G92 E0
G1 F0.000
G1 X106.419 Y144.642 E0.05105
G1 X106.572 Y145.012 E0.06235
G1 X104.902 Y145.703 E0.11340
G1 X104.772 Y145.389 E0.12301
G1 X105.129 Y145.208 F18000.000
G10
G92 E0
G1 X109.553 Y143.344 F18000.000
G11
G92 E0
G1 F0.000
G1 X111.318 Y142.613 E0.05395
G1 X111.471 Y142.983 E0.06525
G1 X109.707 Y143.713 E0.11920
G1 X109.576 Y143.399 E0.12881
G1 X109.933 Y143.216 F18000.000
G10
G92 E0
G1 X114.377 Y141.346 F18000.000
G11
G92 E0
G1 F0.000
G1 X116.263 Y140.564 E0.05768
G1 X116.416 Y140.934 E0.06898
G1 X114.530 Y141.715 E0.12667
G1 X114.400 Y141.401 E0.13627
G1 X114.756 Y141.217 F18000.000
G10
G92 E0
G1 X111.703 Y138.557 F18000.000
G11
G92 E0
G1 F0.000

And here's code generated with thin wall mode disabled. I'm pretty sure this is on a different layer than the above, but the lines are obviously identical:

Show the code...
G11
G92 E0
G1 F4200.743
G1 X106.419 Y144.642 E0.05105
G1 X106.572 Y145.012 E0.06235
G1 X104.902 Y145.703 E0.11340
G1 X104.772 Y145.389 E0.12301
G1 X105.129 Y145.208 F18000.000
G10
G92 E0
G1 X109.553 Y143.344 F18000.000
G11
G92 E0
G1 F4200.743
G1 X111.318 Y142.613 E0.05395
G1 X111.471 Y142.983 E0.06525
G1 X109.707 Y143.713 E0.11920
G1 X109.576 Y143.399 E0.12881
G1 X109.933 Y143.216 F18000.000
G10
G92 E0
G1 X114.377 Y141.346 F18000.000
G11
G92 E0
G1 F4200.743
G1 X116.263 Y140.564 E0.05768
G1 X116.416 Y140.934 E0.06898
G1 X114.530 Y141.715 E0.12667
G1 X114.400 Y141.401 E0.13627
G1 X114.756 Y141.217 F18000.000
G10
G92 E0
G1 X111.703 Y138.557 F18000.000
G11
G92 E0
G1 F4200.743

Configuraton and model that produced that g-code:
slic3r-config-20191023.ini.zip
ctrlV_3D_test-v3.stl.zip

Couple of UI problems One minor UI issue. Enable thin-wall mode, note how it appears in the sidebar right of the 3d/preview section:
image
...I'm pretty sure there should be some more text in there.

Now click the red (-), then go look in the print profile settings. It shows thin wall mode to still be enabled, but you can disable it there without issue.

@supermerill
Copy link
Collaborator Author

weird.
I didn't change anything outside of medial axis / thin walls.
And i don't know what in the merge could ave done that.

@VanessaE
Copy link
Collaborator

VanessaE commented Oct 24, 2019

I suppose it's possible that the problem exists outside of (or predates) your thin wall code, but it still seems like it ought to be fixed here, otherwise this PR is... kinda useless. 😁

Does the issue I described not happen for you?

@supermerill
Copy link
Collaborator Author

supermerill commented Oct 25, 2019

I didn't have the time to perl-compile before leaving.
As you can see, i didn't even have the time to check all the tests.

@VanessaE
Copy link
Collaborator

Just to follow-up, while still at commit 4f5b935, but without this PR applied, the speeds behave as they should in thin wall mode. So... "you bwoke it 😢" 😁

@lordofhyphens
Copy link
Member

Build errors for the tests from Travis:

                                                         unshare
/home/travis/build/slic3r/Slic3r/src/test/libslic3r/test_thin.cpp:441:76: error: expression cannot be used as a function
                     for (Line &l : polyline.lines()) sum += std::abs(l.b.y() - l.a.y());
                                                                            ^
/home/travis/build/slic3r/Slic3r/src/test/libslic3r/test_thin.cpp:441:86: error: expression cannot be used as a function
                     for (Line &l : polyline.lines()) sum += std::abs(l.b.y() - l.a.y());
                                                                                      ^
/home/travis/build/slic3r/Slic3r/src/test/libslic3r/test_thin.cpp:442:86: error: expression cannot be used as a function
                     coord_t expected_y = expolygon.contour.bounding_box().center().y();
                                                                                      ^
In file included from /home/travis/build/slic3r/Slic3r/src/test/libslic3r/test_thin.cpp:4:0:
/home/travis/build/slic3r/Slic3r/src/test/libslic3r/test_thin.cpp:443:45: error: ‘class Slic3r::Polyline’ has no member named ‘size’
                     REQUIRE((sum / polyline.size()) - expected_y < SCALED_EPSILON);
                                             ^
/home/travis/build/slic3r/Slic3r/src/test/libslic3r/test_thin.cpp:443:45: error: ‘class Slic3r::Polyline’ has no member named ‘size’
                     REQUIRE((sum / polyline.size()) - expected_y < SCALED_EPSILON);
                                             ^
/home/travis/build/slic3r/Slic3r/src/test/libslic3r/test_thin.cpp:447:46: error: expression cannot be used as a function
                 if (polyline.first_point().x() > polyline.last_point().x()) polyline.reverse();
                                              ^
/home/travis/build/slic3r/Slic3r/src/test/libslic3r/test_thin.cpp:447:74: error: expression cannot be used as a function
                 if (polyline.first_point().x() > polyline.last_point().x()) polyline.reverse();
                                                                          ^
In file included from /home/travis/build/slic3r/Slic3r/src/test/libslic3r/test_thin.cpp:4:0:
/home/travis/build/slic3r/Slic3r/src/test/libslic3r/test_thin.cpp:451:54: error: expression cannot be used as a function
                     REQUIRE(polyline.first_point().x() == polyline_bb.min.x());
                                                      ^
/home/travis/build/slic3r/Slic3r/src/test/libslic3r/test_thin.cpp:451:77: error: expression cannot be used as a function
                     REQUIRE(polyline.first_point().x() == polyline_bb.min.x());
                                                                             ^

@VanessaE
Copy link
Collaborator

@supermerill please sort out the build errors and finish up this PR so we can merge it.

@supermerill
Copy link
Collaborator Author

ok

@AppVeyorBot
Copy link

Build Slic3r 1.3.0-master-2381 completed (commit 5bc62e25b9 by @)

@VanessaE
Copy link
Collaborator

VanessaE commented Mar 3, 2020

Another thing I've meant to point out:

Don't try to draw a thin wall if doing so creates a line or series of line segments with no anchor points and nothing (or very little) under it. Maybe "under it" should be governed by the same rules that dictate whether a regular line gets drawn normally or as a bridge.

For example, a Benchy has two holes on either side of its bow. With thin walls enabled, the bottom edge of one of the holes looks like this (even with a 0.4 mm perimeter line width):

image

That tiny little bead has nothing under it or at either end, and just becomes a tiny blob on the wall below the hole.

Turning thin walls off eliminates this little bead, but then we get back to the issue with perimeters being printed over one another where a thick gap fill line should be used. For example, part way up in the Benchy's smokestack:

image

Here you can see two perims overlapping.

@ruevs
Copy link

ruevs commented Mar 19, 2020

#4554 does not happen with the AppVeyor build artifact:
https://ci.appveyor.com/project/lordofhyphens/slic3r/builds/31012289

@supermerill
Copy link
Collaborator Author

@VanessaE good catch, i'll see what i can do for that.
Currently, i'm surveying the flow of thick extrusion, as the amount is right in my tests but the real-world result are quite not thick enough sometimes.

Also i don't have access to my linux computer for some time now, i'll merge the last changes & correct problem when i'll have access to it again.

@AppVeyorBot
Copy link

@lordofhyphens
Copy link
Member

@supermerill in your tests, are you expecting a Point.x and Point.y to be methods?

//} };
WHEN("1 nozzle, 0.2 layer height") {
const coord_t nozzle_diam = scale_(1);
ExPolygon anchor = union_ex(ExPolygons{ tooth }, intersection_ex(ExPolygons{ base_part }, offset_ex(tooth, nozzle_diam / 2)), true)[0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version of union_ex doesn't seem to exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the x() it's something from prusa, i have to remove the () when converting for here.

I added this union. Basically, adding all expoly into a vector and returning the unionex of it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should check to see if wrapping it in an accessor is worth converting code too or if it was just something their fork did because reasons

@AppVeyorBot
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants