Fix wall ordering #1562

BagelOrb · 2022-01-24T11:56:09Z

This PR builds on #1554
That PR should be merged first - or it will be automatically merged with this one.

This PR comes with an accompanying frontend PR: Ultimaker/Cura#11334
Please merge both simultaneously.

This PR solves a number of bugs all relating to the order in which walls are printed:

Arachne splits wall polygons into several segments with extra starts/stops when Optimize Wall Printing Order is enabled, i.e. when Order Walls by Inset is disabled. (JIRA ticket CURA-8896)
Arachne then printed the separate segments belonging to each outline/hole region together, rather than printing whole polygons. It enforced ordering by region harder than stitching wall lines into polygons.
The order in which walls are printed had some bugs in 4.13 and in Arachne which could cause some polygons to be printed out of order (inner to outer vs outer to inner)
The gap filler walls were printed after the skin in 4.13, but in Arachne even before some of the walls when order was set to Outer to Inner.
Arachne would sometimes leave tiny travel gaps in between a single wall 'gap filler', because stitching wasn't implemented properly.
Wall lines were sometimes printed in the wrong direction (counter clockwise instead of clockwise).

Code changes:

This PR applies the PolylineStitcher introduced in Polyline handling fixes #1554 to form closed wall polygons before simplifying them en passing them to the optimizer.
Because a stitched ExtrusionLine can have multiple region_id s associated with it, the InsetOrderOptimizer needed to be overhauled.
I've implemented a proper weak order based on the adjacency between inset polygons, which is computed in InsetOrderOptimizer and used in PathOrderOptimizer.

Behavior changes:

Gap fillers should always be printed after their enclosing wall, and depending on whether we use Center Last ordering, after all walls.
Optimize wall order enabled / Order walls by inset disabled now strictly enforces the Inner-to-outer order per region, while wall polygons pertaining to two regions are not cut up, but printed before/after the other walls pertaining to both those regions.

Benefits:

The quality of surfaces is better for complex layers with holes in them and/or with thin parts.
The travel time is reduced.
Alternate Wall Direction works again
Order Inner Wals by Inset is re-renamed back to Optimize Wall Printing Order and the behavior is reflipped to the old behavior

Possibly affected behavior:

All wall ordering, z seam config etc.
of each wall feature type: Walls, infill walls, skin walls, support walls, support bottom walls, support infill walls, raft walls

Before:

After:

UM3_santa.3mf.renamed.zip

Before:

After:

UM3_perimeter_hole_connections.3mf.renamed.zip

The following items are not addressed by this PR, but came up during review of the surrounding code and during testing:

The description of Wall Transitioning Angle is wholly incorrect. The Transitioning Angle controls how often Arachne will introduce odd walls. A small transitioning angle means that transitioning between even and odd number of wall lines only happens for very sharp geometry. Reducing the Transitioning Angle reduces the number and length of odd gap filler segments. The Transitioning Angle has nothing to do with the angle of a transition; that angle is fully determined by the Wall Transition Length. The Wall Transitioning Angle is one of the most important Arachne settings, because it determines how much of the toolpaths are determined by Arachne, and how much uses normal constant width offsets.
ExtrusionLine.simplify() only works correctly for open polylines, not for closed polygons. Also you might want to remove the code duplication with Polygons.simply().
The Z seam config is currently used for all walls, rather than only the outer wall - as it was in 4.13.
PathOrderOptimizer::isLoopingPolyline returns true for polylines which have a total length smaller than coincident_point_distance
Wall Ordering Center Last can now become its own setting. Inner to Outer and Outer to Inner should be able to be configured separately from Print Odd Gap Filler Walls After All Even Walls
The starts of polylines are not communicated to the frontend. They don't show up as white squares in the layer view.
The gap filler walls don't appear yellow in the layer view any more, but maybe that's for the better.
The idea to stitch odd polylines to the polygonal walls at a Split reduces the number of starts/stops even further, but this feature has not been implemented.
In the original Arachne library the ends of non-stitched lines were chopped off so that there would be less overextrusion at the 3-way intersection of a split. This functionality was lost when the library got merged into Cura. This feature has not yet been reimplemented.

BagelOrb · 2022-01-25T08:04:16Z

Fixes:
#1495
Ultimaker/Cura#10957

BagelOrb · 2022-01-25T15:05:27Z

Results of tests on Applicatas dataset (assembled from models from the internet). Tested on S5 with Generic PLA at 0.1mm layer height.

Travel times:
Benchy:
4.13: 0:09
Arachne beta 2: 0:11
this PR: 0:10

ACAM:
4.13: 2:04
Arachne beta 2: 1:18
this PR: 1:32

airhose splitter
4.13: 0:11
Arachne beta 2: 0:11
this PR: 0:09

Alahamra
4.13: 0:02
Arachne beta 2: 0:04
this PR: 0:03

David:
4.13: 0:53
Arachne beta 2: 1:03
this PR: 0:58

Decking fixture:
4.13: 0:26
Arachne beta 2: 0:29
this PR: 0:27

Jet engine Combustion_and_HPT_Casing
4.13: 0:32
Arachne beta 2: 0:39
this PR: 0:36

In all cases this PR reduced the Arachne travel time; in some cases this PR reduced travel time even more than before Arachne.

rburema · 2022-01-26T09:17:07Z

Sort of the same story for this one, except in this case the build-server is down for some reason. Can you run the test manually & report back? Otherwise we can do that, but it might slow down the process a bit.

BagelOrb · 2022-01-26T15:40:59Z

Here's the order on a complex model with a lot of internal holes: (Outer to Inner, Optimize Wall Ordering, Gap fillers always (perhaps immediately) after enclosing wall)

Outer and inner walls are printed intermittently, but the ordering is preserved everywhere:

Now we can print some of the small islands of inner walls:

Here wall 2 is printed on the right side, then it takes a gap filler of index 2 to the left side, and then continues to print inset 3 for both islands, before moving on to a gap filler of index 2 farther away:

Notice how the gap filler of index 3 is printed together with the pieces of wall with index 3:

BagelOrb · 2022-01-26T15:53:38Z

With the above model
UMS5_Stand.3mf.renamed.zip

These are the travel times:
Inside to Outside: 0:49
Outside to Inside: 0:49
Center Last: 0:42
Arachne beta 2, Center Last: 0:53
4.13, Inner before outer: 0:41

richfelker · 2022-02-03T02:03:14Z

Is this PR in a state where it's suitable for testing? I'm the reporter of #1495 and would like to test if so.

BagelOrb · 2022-02-03T13:44:28Z

Yes it is. If you are able to test from source then we would be very interested in your findings.

Before it will be merged some developers will review my code and some more tests will be performed, but there's always a chance for bugs to fall through, so I would be very happy if you could help our testing efforts.

richfelker · 2022-02-04T21:34:50Z

OK, I can confirm at least for my test case in that report that the problem is fixed.

richfelker · 2022-02-04T21:42:28Z

Unfortunately it segfaulted trying to slice a Benchy.

richfelker · 2022-02-04T22:00:00Z

The crash occurs with skin_monotonic=true which I always use. Here's the backtrace (sorry probably lacks sufficient debug info I didn't build with):
curacrash.txt

BagelOrb · 2022-02-04T22:16:54Z

Could you please save your project as a 3mf file, put it in a zip and upload it here?

richfelker · 2022-02-04T22:56:40Z

I don't use or have the Cura gui, just CuraEngine. I can probably make you a minimal reproducing testcase with that.

richfelker · 2022-02-05T01:05:21Z

OK, all that's required to reproduce the issue is skin_monotonic=true and top_bottom_pattern_0=zigzag with the baseline fdmprinter.def.json and the standard Benchy STL.

BagelOrb · 2022-02-05T10:27:53Z

Thanks. This is a problem on master. Fixed in fa196db

richfelker · 2022-02-05T13:50:16Z

OK, good to know. It was not present in master but appeared when checking out your PR branch which is a ways behind master and apparently doesn't merge cleanly. I'll try the merge or wait for you to get it merged upstream.

BagelOrb · 2022-02-05T14:07:56Z

The bug was in master since somehwere last week, so perhaps you hadn't pulled he bug in yet. I'll merge master into this branch soon.

…all lines when the outline and hole regions interact then the ordering is more strict than need be. This is because adjacency information is either expensive to compute, or requires a lot of score-keeping in arachne

casperlamboo

As discussed part I would submit (the incomplete) review earlier. Expect part 2/2 of the review soon.

src/utils/PolylineStitcher.h

casperlamboo · 2022-02-01T15:03:08Z

src/WallToolPaths.cpp

+        if (wall_idx >= wall_lines.size())
+        {
+            wall_lines.resize(wall_idx + 1);
+        }


As wall_lines was just cleared before it's size is always 0, making the statement wall_idx >= wall_lines.size() always true

Suggested change

if (wall_idx >= wall_lines.size())

{

wall_lines.resize(wall_idx + 1);

}

wall_lines.resize(wall_idx + 1);

Additionally I also don't really get why you would want to resize the vector to wall_idx. Seems like the vector needs a size of closed_polygons.size()

I can't find that code and github can't find the commit. Looks like you're looking at outdated commits. Idk.

src/WallToolPaths.cpp

src/utils/polygon.cpp

src/WallToolPaths.h

src/utils/polygon.cpp

casperlamboo · 2022-02-16T14:33:01Z

src/utils/polygon.cpp

+            auto it = node_to_index.find(child);
+            assert(it != node_to_index.end() && "Each PolyNode should be mapped to the corresponding Polygon index!");
+            ret[index].emplace_back(it->second);


I think this part is deserving of a comment. Why shoudn't it equal node_to_index.end(), and why push it->second to ret[index]

I'm not sure. This exact line doesn't appear anymore in the latest branch. Please check if the new code is still unclear to you.

src/PathOrderOptimizer.h

Old code was less intelligible.

Ghostkeeper · 2022-02-17T13:08:51Z

Fixes #1552.

Simply use a SparseGrid to see which lines are adjacent to each other, rather than using nesting informtoin and info from region_idx. This makes the code simpler to read, the algorithm more reobust to when stitching hasn't been accomplished where it should have, while have negligle impact in slicing time.

We didn't check the distance between two lines properly, so wall lines which weren;t adjacent could end up with an order constraint. This effectively meant that in some cases Optimize Wall Printing Order acted as if it wasn't enabled.

Fixes merge comflicts with ca83659 Contributes to CURA-8067

Fixes an issue with code duplication introduced in ca83659 Contributes to CURA-8067

The test assumed that the pat hwas closed. Only for closed polygons is a size of 2 or 1 not allowed.

src/FffGcodeWriter.cpp

src/InsetOrderOptimizer.cpp

src/utils/ExtrusionLine.cpp

src/SkeletalTrapezoidation.h

src/utils/PolylineStitcher.h

src/WallToolPaths.cpp

src/SkeletalTrapezoidation.cpp

…erse direction Polygonal walls with even index should always be closed into polygons, but if they somehow weren't closed, we should at least retain their printing direction.

Is better for finding neighboring walls in uncommon cases where the vertices of consectuive walls are not next to each other.

BagelOrb · 2022-02-22T11:03:38Z

Mostly just code conventions and documentation.

Please pay special attention when reviewing the last two commits, since they are behavioral changes/fixes.

An extrusion junction lies on an edge of the MAT, but since each edge has two half edges which may belong to a different region, the extrusion junction should have recorded two region_ids. The region_id recorded in the junction was therefore not reliable, and code depending on it was very bug-prone.

It's not used anymore.

BagelOrb · 2022-03-09T08:25:13Z

src/TopSurface.cpp

@@ -38,13 +39,14 @@ void TopSurface::setAreasFromMeshAndLayerNumber(SliceMeshStorage& mesh, size_t l
    }
 }

-bool TopSurface::ironing(const SliceMeshStorage& mesh, const GCodePathConfig& line_config, LayerPlan& layer) const
+bool TopSurface::ironing(const SliceDataStorage& storage, const SliceMeshStorage& mesh, const GCodePathConfig& line_config, LayerPlan& layer, const FffGcodeWriter& gcode_writer) const


I didn't touch the order of ironing lines, only of the walls of ironing.

BagelOrb mentioned this pull request Jan 25, 2022

Variable walls stitching Ultimaker/Cura#11334

Merged

BagelOrb force-pushed the variable_walls_stitching branch from 24a94e2 to a526a46 Compare January 25, 2022 10:49

BagelOrb force-pushed the variable_walls_stitching branch 2 times, most recently from e3177f7 to 1409d46 Compare January 27, 2022 10:24

BagelOrb added 10 commits February 7, 2022 11:52

prevent stitching from reversing wall printing direction

8b0455c

stitch variable width walls

e3a8620

fix inset index and only stitch polygonal wall lines

68e63a4

Move wall stitching to separate function

decf0bd

stitch odd gap filling walls

a5726b5

don't stitch odd wall polylines to even wall polygons

7619882

Function to get the nesting information from a Polygons object

663f2f1

helper functions

e7afd76

unit test for WallToolPaths::getWeakOrder

7f549a7

casperlamboo reviewed Feb 16, 2022

View reviewed changes

BagelOrb added 3 commits February 17, 2022 10:52

clarify test condition

0f6e26d

Old code was less intelligible.

remove debugging code

eabfeae

lil code style and debug check

4b0fa03

Ghostkeeper mentioned this pull request Feb 17, 2022

Arachne regression: Seam settings are not honored #1552

Closed

BagelOrb added 6 commits February 21, 2022 14:05

remove unused polygon::getNesting(.)

8c33703

fix order between too far away lines

e745b8f

We didn't check the distance between two lines properly, so wall lines which weren;t adjacent could end up with an order constraint. This effectively meant that in some cases Optimize Wall Printing Order acted as if it wasn't enabled.

fix support infill wall

04ebf5f

Fixes merge comflicts with ca83659 Contributes to CURA-8067

remove printing of infill walls which are never generated

ae378b2

Fixes an issue with code duplication introduced in ca83659 Contributes to CURA-8067

fix unit test for ExtrusionLine

39e4d71

The test assumed that the pat hwas closed. Only for closed polygons is a size of 2 or 1 not allowed.

casperlamboo reviewed Feb 22, 2022

View reviewed changes

BagelOrb added 6 commits February 22, 2022 11:52

lil code style and doc

f99f911

make alternate_walls code more explicit

3962373

remove superfluous double assignment

5a2f5dd

lil refactor for readability

e6e114f

prevent walls which should have been closed from being printed in rev…

63ec958

…erse direction Polygonal walls with even index should always be closed into polygons, but if they somehow weren't closed, we should at least retain their printing direction.

increase searching radius of getRegionOrder a little

5cc439e

Is better for finding neighboring walls in uncommon cases where the vertices of consectuive walls are not next to each other.

BagelOrb added 4 commits February 22, 2022 13:52

lil brackets to prevent compile warning

766c239

make arachne stitch prevention choice more clear

5f06212

remove edge_t::region

d837179

It's not used anymore.

casperlamboo merged commit 39ed434 into master Feb 22, 2022

casperlamboo deleted the variable_walls_stitching branch February 22, 2022 14:32

This was referenced Feb 22, 2022

Arachne regression: inset_direction not always honored #1495

Closed

Arachne regression: warts on outside of print #1448

Open

BagelOrb commented Mar 9, 2022

View reviewed changes

BagelOrb mentioned this pull request Mar 25, 2022

Support gap filler fix #1595

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wall ordering #1562

Fix wall ordering #1562

BagelOrb commented Jan 24, 2022 •

edited

Loading

BagelOrb commented Jan 25, 2022 •

edited

Loading

BagelOrb commented Jan 25, 2022 •

edited

Loading

rburema commented Jan 26, 2022

BagelOrb commented Jan 26, 2022

BagelOrb commented Jan 26, 2022

richfelker commented Feb 3, 2022

BagelOrb commented Feb 3, 2022

richfelker commented Feb 4, 2022

richfelker commented Feb 4, 2022

richfelker commented Feb 4, 2022

BagelOrb commented Feb 4, 2022

richfelker commented Feb 4, 2022

richfelker commented Feb 5, 2022

BagelOrb commented Feb 5, 2022

richfelker commented Feb 5, 2022

BagelOrb commented Feb 5, 2022

casperlamboo left a comment

casperlamboo Feb 1, 2022

casperlamboo Feb 1, 2022

BagelOrb Feb 17, 2022

casperlamboo Feb 16, 2022

BagelOrb Feb 17, 2022

Ghostkeeper commented Feb 17, 2022

BagelOrb commented Feb 22, 2022

BagelOrb Mar 9, 2022

Fix wall ordering #1562

Fix wall ordering #1562

Conversation

BagelOrb commented Jan 24, 2022 • edited Loading

BagelOrb commented Jan 25, 2022 • edited Loading

BagelOrb commented Jan 25, 2022 • edited Loading

rburema commented Jan 26, 2022

BagelOrb commented Jan 26, 2022

BagelOrb commented Jan 26, 2022

richfelker commented Feb 3, 2022

BagelOrb commented Feb 3, 2022

richfelker commented Feb 4, 2022

richfelker commented Feb 4, 2022

richfelker commented Feb 4, 2022

BagelOrb commented Feb 4, 2022

richfelker commented Feb 4, 2022

richfelker commented Feb 5, 2022

BagelOrb commented Feb 5, 2022

richfelker commented Feb 5, 2022

BagelOrb commented Feb 5, 2022

casperlamboo left a comment

Choose a reason for hiding this comment

casperlamboo Feb 1, 2022

Choose a reason for hiding this comment

casperlamboo Feb 1, 2022

Choose a reason for hiding this comment

BagelOrb Feb 17, 2022

Choose a reason for hiding this comment

casperlamboo Feb 16, 2022

Choose a reason for hiding this comment

BagelOrb Feb 17, 2022

Choose a reason for hiding this comment

Ghostkeeper commented Feb 17, 2022

BagelOrb commented Feb 22, 2022

BagelOrb Mar 9, 2022

Choose a reason for hiding this comment

BagelOrb commented Jan 24, 2022 •

edited

Loading

BagelOrb commented Jan 25, 2022 •

edited

Loading

BagelOrb commented Jan 25, 2022 •

edited

Loading