Unretract before last travel move does not work

[richfelker]

Cura Version

5.7.0

Operating System

Any

Printer

Any

Reproduction steps

Slice a file/job with multiple disconnected parts and outer perimeters first, no infill or infill-after-perimeters, such that the first move of a layer is a travel to an outer perimeter.

Actual results

The travel move to the start of a new layer will unretract on the outer perimeter rather than on an inner one.

This happens as a consequence of my naive fix for Ultimaker/CuraEngine#1612 but without that fix, much worse things happen (the entire travel to the start of the new layer takes place unretracted!)

I don't understand this part of the code entirely but it looks like there is some merging of the path at the end of one layer and beginning of the next which breaks the logic.

Expected results

Unretract should take place on the inner perimeter before the final travel move to the outer perimeter.

[GregValiant]

Thanks for the report.
Please zip and post a project file that shows the problem.

[richfelker]

I don't use the frontend but CuraEngine directly. Would you be happy with an stl file and command line? Or should I go file the bug on the CuraEngine repo? My past experience was that reports posted there tended to get overlooked.

[GregValiant]

I print a lot and I know I don't have this problem but I always print Inside to Outside as I have experienced problems printing small holes when printing the outer wall first.
You have been making some changes to the code as well. In order to investigate any problem it has to be reproduceable so it can be chased down. Both retraction and Z-hops are affected by numerous settings so knowing how you set up the slice is important.

[richfelker]

On further investigation, unretracting on the inner wall has been fully broken since @jellespijker committed my fix. I can explain why the fix was wrong and must necessarily have broken it: final_travel_z_ and z_ are properties of the LayerPlan and are never equal except possibly for the last layer of the print. Changing the condition to path_idx != (paths.size() - 1) (i.e. not last path of the layer) fixes all but the first part in each layer. Apparently the initial travel of each layer has been merged into the last path of the previous layer.

[richfelker]

OK, the fix in the above comment (path_idx != (paths.size() - 1)) is correct but incomplete, because there's other logic intentionally breaking it.

The LayerPlan::addTravel bypass_combing code path is what's keeping the move to unretract inside the first part from happening. Removing that (never bypassing combing) would fix it, but LayerPlanBuffer::addConnectingTravelMove gratuitously asserts that there's no further travel at the start of the new layer. Removing those assertions seems to fix it with no adverse effect.

[jellespijker]

@wawanbreton and/or @rburema can either of you take a look at this?

[richfelker]

Updated the title because it wasn't working anywhere. My above attempt at fixing isn't quite right so I'll follow up with more in a bit.

[richfelker]

I have a new theory on why Ultimaker/CuraEngine#1612 happened and why my fix (which introduced the current bug) was needed.

The intent of the present code in CuraEngine was for LayerPlanBuffer::addConnectingTravelMove to be responsible for the travel path that would need "unretract before last travel" when the first part of the next layer is an outer perimeter. To do this, it calls prev_layer->addTravel to add a travel move to the start location of this perimeter. LayerPlan::addTravel then does the combing calculations to determine a point inside the combing boundary at the destination, and if that succeeds, marks the move as wanting an unretract before the last path component... but it did this calculation based on the layer-parts/comb-boundary of prev_layer, not the new layer being started! This means that the point it selects to unretract on may actually be outside the outer perimeter of the new layer.

I think the right fix is for addTravel to set first_travel_destination_ to the point inside the comb boundary it wants, and not skip adding an actual move for the first travel of the layer, but instead write a move to the final desired point with unretract_before_last_travel_ set on it. Also, addConnectingTravelMove should be revised to suppress unretract_before_last_travel_ on the path it adds to prev_layer, in case the combing miscalculation gets confused by using the wrong layer's parts for combing at the destination point.

I'll try to hack up a PoC for these changes and see if it works.

[richfelker]

PoC fix (against my tree, hopefully it doesn't have gratuitous conflicts):

diff --git a/src/LayerPlan.cpp b/src/LayerPlan.cpp
index e046fef16..bdc395817 100644
--- a/src/LayerPlan.cpp
+++ b/src/LayerPlan.cpp
@@ -399,7 +399,18 @@ GCodePath& LayerPlan::addTravel(const Point2LL& p, const bool force_retract, con
             path->retract = true;
             path->perform_z_hop = true;
         }
-        forceNewPathStart(); // force a new travel path after this first bogus move
+        if (is_inside_) {
+            // Find a suitable point inside the comb boundary for the connecting travel move to
+            // go to, and comb to the final destination here. This is necessary because the
+            // connecting travel move takes place in the previous layer and uses the previous
+            // layer's combing boundary, which might be outside the new layer's.
+            last_planned_position_ = p;
+            moveInsideCombBoundary(MM2INT(0.4), std::nullopt, path);
+            first_travel_destination_ = last_planned_position_;
+            bypass_combing = false;
+        } else {
+            forceNewPathStart(); // force a new travel path after this first bogus move
+        }
     }
     else if (force_retract && last_planned_position_ && ! shorterThen(*last_planned_position_ - p, retraction_config.retraction_min_travel_distance))
     {
@@ -2210,7 +2221,7 @@ void LayerPlan::writeGCode(GCodeExport& gcode)
                 {
                     gcode.writeTravel(path.points[point_idx], speed);
                 }
-                if (path.unretract_before_last_travel_move && final_travel_z_ == z_)
+                if (path.unretract_before_last_travel_move)
                 {
                     // We need to unretract before the last travel move of the path if the next path is an outer wall.
                     gcode.writeUnretractionAndPrime();
diff --git a/src/LayerPlanBuffer.cpp b/src/LayerPlanBuffer.cpp
index 7c6b69db7..2a3686b36 100644
--- a/src/LayerPlanBuffer.cpp
+++ b/src/LayerPlanBuffer.cpp
@@ -90,7 +90,6 @@ void LayerPlanBuffer::addConnectingTravelMove(LayerPlan* prev_layer, const Layer
 
     Point2LL first_location_new_layer = new_layer_destination_state->first;
 
-    assert(newest_layer->extruder_plans_.front().paths_[0].points.size() == 1);
     assert(newest_layer->extruder_plans_.front().paths_[0].points[0] == first_location_new_layer);
 
     // if the last planned position in the previous layer isn't the same as the first location of the new layer, travel to the new location
@@ -98,13 +97,18 @@ void LayerPlanBuffer::addConnectingTravelMove(LayerPlan* prev_layer, const Layer
     {
         const Settings& mesh_group_settings = Application::getInstance().current_slice_->scene.current_mesh_group->settings;
         const Settings& extruder_settings = Application::getInstance().current_slice_->scene.extruders[prev_layer->extruder_plans_.back().extruder_nr_].settings_;
-        prev_layer->setIsInside(new_layer_destination_state->second);
+        // Wherever we are travelling to, it is not inside any layer part of the previous layer,
+        // only potentially inside part of the new layer. Thus, for combing purposes within the
+        // previous layer, we are always moving to somewhere "outside" the layer parts.
+        prev_layer->setIsInside(false);
         const bool force_retract = extruder_settings.get<bool>("retract_at_layer_change")
                                 || (mesh_group_settings.get<bool>("travel_retract_before_outer_wall")
                                     && (mesh_group_settings.get<InsetDirection>("inset_direction") == InsetDirection::OUTSIDE_IN
                                         || mesh_group_settings.get<size_t>("wall_line_count") == 1)); // Moving towards an outer wall.
         prev_layer->final_travel_z_ = newest_layer->z_;
         GCodePath& path = prev_layer->addTravel(first_location_new_layer, force_retract);
+        // this must be cleared because it was calculated based on the wrong layer's combing boundaries
+        path.unretract_before_last_travel_move = false;
         if (force_retract && ! path.retract)
         {
             // addTravel() won't use retraction if the travel distance is less than retraction minimum travel setting

I have not run extensive tests on this, but based on my dive into reading/understanding this code, I think the concept is very sound, and it produces correct output for basic smoke tests.

[richfelker]

Note: I forgot to remove the hard-coded 0.4 mm from testing. Obviously that should be fixed to match the move-inside logic elsewhere based on innermost line width.

[wawanbreton]

Hi @richfelker, thanks for the report.
You seem to have a very good overview of the issue 😃 Would it be possible for you to open a PR with the fix, or shall I do it ?

[richfelker]

@wawanbreton I will when it's ready, but I think we should hold off a little. The above changes do the right thing in the cases I'm trying to address and most care about, but they have some potentially unwanted consequences like preventing the ability to do unretracted travel to the start of the next layer when the travel would take place entirely within a single part (and in practice is often very short).

I think I'd like to break this into 2 or more parts. First, fixing the "unretract before last travel move" to work as it was originally intended for all travels except the connecting move between layers. This is a simple, self-contained fix and should be uncontroversial.

Then, in order to allow the functionality to work at layer change, I want to introduce a better version of the above that chooses a point inside the comb boundary as the "first location of new layer", and emit an explicit move from there to the wall start that can have the "unretract before last travel move" flag set.

Finally, I'm not convinced that the functionality I've been trying to fix is entirely a good thing. I've been carrying a patch to suppress it when there's only one wall, on account of this bug I reported: Ultimaker/CuraEngine#1704. However, I think in general that moving inside the "preferred comb boundary" is probably the wrong heuristic. Depending on a number of settings, the point selected can be significantly too far inside, possibly compromising seam integrity. Moreover, on prints in transparent material, the entire feature gives rather ugly zits visible inside the seam that accentuate it rather than concealing it as intended.

What I'd like to do is make it the whole feature optional, either with an on/off setting or a configurable distance inward from the outer perimeter (rather than from preferred comb boundary) whose default could be zero if there's only one perimeter, and otherwise the outer wall line width. I know there's a hesitance to add too many unnecessary switches, but this seems like a place where there are compelling reasons to prefer either behavior depending on material, priority of air/water-tightness requirements vs dimensional accuracy requirements of the part, etc.

[richfelker]

FWIW there's another issue I have open (and for which I'm carrying a local patch) related to this functionality: Ultimaker/CuraEngine#1712. It'd be nice to get that fixed too.

[wawanbreton]

What I'd like to do is make it the whole feature optional

This in indeed what we do when we have features for which we are not really sure they will make everyone happy. It probably doesn't help for advertising them, but at least when someone is in need, we can say "just enable this" !

Take your time, I don't have the feeling that this issue is highly harmful. I mean, it may impact overall quality, but won't fail the print :)
However, please be aware that we also do modifications on such similar topics. We are currently working on a few improvements regarding seam and retraction/unretraction moves to make them smoother. So we may also change similar parts of the code, possibly (de-)emphasizing this issue.

[richfelker]

Sure.

The main thing that seems actively harmful to print quality with the current upstream code (which, per my old patch that got adopted, completely suppresses the early unretract) is that the travel route on inward sloping parts will often gratuitously travel through a point outside the perimeter, where, depending on acceleration and z-hop settings, it may be more likely to deposit zits than a direct travel move to the desired starting position on the perimeter would do.

Otherwise, fixing this is mostly about getting the intended benefits of the unretract-inside improvement my patch unintentionally suppressed.

I've been experimenting a little bit more, and ran into my above approach causing a sort of "double Z hop" at layer change if Z-hop is enabled: doing a Z-hop, travelling to the destination, then hopping again and only descending on the final exrusion start point. This greatly increases zits/surface defects. I've now been able to get it not to do that, but I'm not sure the logic follows the intend of the existing code, so I will post it for review at some point once I'm confident it's at least producing good output.

[wawanbreton]

Thanks for you effort. If you want to, you can also have a look at this PR: Ultimaker/CuraEngine#2075 It is currently in an early experimental state, but in the coming weeks we will have to implement it properly, which means touching a lot of code related to travel moves, retraction and unrectration, and Z-hop. I just hope we don't work in parrallel and end up with conflicting changes on both sides. And if I understand correctly, having this feature work properly would make your changes obsolete. So maybe just wait for it to be done, and make a review on your side once we are ready ?

[richfelker]

@wawanbreton Thanks for the reference. If there are people working on this already, I don't want to step on their toes. I'll keep and continue using my changes against 5.7 and report any important observations. I don't see any way my code changes will be directly useful to the upstream work moving forward, but what I would like to happen is:

1. For the folks working on it to read and understand my latest patches (which I'll post) and the problems they are intended to solve, so that whatever they come up with doesn't reproduce the same problems, and

2. For me to read their WIP code and provide some feedback on whether it looks like it's doing anything that will introduce new problems in this area or lock-in (make it harder to fix) existing problems.

From what I skimmed of the PR so far, though, it looks like they're aware of some of the same things I've been fighting with, so I think this is all moving in a good direction!

[wawanbreton]

For the folks working on it to read and understand my latest patches (which I'll post) and the problems they are intended to solve, so that whatever they come up with doesn't reproduce the same problems, and

I must admit I didn't get deep into your patch yet, but I will add this as a prerequisite to working on the topic.

For me to read their WIP code and provide some feedback on whether it looks like it's doing anything that will introduce new problems in this area or lock-in (make it harder to fix) existing problems.

Well you can read the code as we regularly push it, but I will try to remember to send you a notification when the code is in a more stable state. It is very likely that I will be working on this, so it should help 😃

From what I skimmed of the PR so far, though, it looks like they're aware of some of the same things I've been fighting with, so I think this is all moving in a good direction!

Good to know, thanks for taking the time to look at it !