[CURA-10268] Fix wipe for walls.

[rburema]

For wiping in the walls (as opposed to infill) there was an extra requirement for the path not to be a linked path, that is to say, we don't wipe for open paths where there could be another of its exact type right after. -- In order to detect whether something is a 'linked path', we need to determine if a path is closed or not. Previously, a path was closed if and only if the end-vertex was the same as the start-vertex. That equivalence has been dropped. Fortunately, is_closed was added way before that, so it can now be used to determine the is_linked status of the path.

[github-actions[bot]]

Unit Test Results

25 tests   25 :heavy_check_mark:  13s :stopwatch:   1 suites    0 :zzz:   1 files      0 :x:

Results for commit 169fd583.

:recycle: This comment has been updated with latest results.

[rburema]

@casperlamboo That would be nice, expect that doesn't work :-p

First something that you might not be aware of: This part of the code (where the changes where made) used to work fine in 5.2.x, when the definitions where equivalent. With that out of the way:

W.r.t. the pattern, it's true that this is a very common occurrence, and that might be the reason the loop was originally written this way, but it's clear to me that hasn't been the case for a long time: The 'old' end doesn't get used anywhere else in the loop. Since this is the same code that used to work, I think we can reasonably safely assume that this was the intended order: Get the start and end of the current path, then check for a loop; if it's not a loop, it's an open path and can thus be linked.

Also, the is_closed property was implemented later (it's younger than the line I originally replaced), if we didn't change the end != start definition (either on purpose or accidentally) for looping paths, it should be equivalent to what I did here (plus maybe your suggested change). It's not however.

Whether the definition of a closed path is or isn't start != end anymore, this should work in any case. Come to think of it, I think the start/end can be removed, since they where only needed for linked_path.

It would be a good idea to bring the change to end != start for loop-detection to the eCCB, if we have an answer there, we could also either change the comments, or go on a hunt to see why it doesn't work like that anymore.

[jellespijker]

breakage of this functionality might also be related to the wipe config now being able to set per-mesh. This also broke the purge configuration when switching nozzles (the so called poopies)

[rburema]

@jellespijker I currently don't think that's the reason, and that my original fix suffices for restoring the wipe in particular.

[casperlamboo]

Ah I see, wasn't aware about the changes regarding closed paths.

This just came to mind

    cura::Point p_end{ 0, 0 };
    for (const PathOrdering<const ExtrusionLine*>& path : order_optimizer.paths)
    {
        ...

        const bool linked_path = p_start != p_end;

        if (path.is_closed)
        {
            p_end = path.vertices->front().p;
        }
        else
        {
            p_end = path.backwards ? path.vertices->back().p : path.vertices->front().p;
        }

        ...
   }

Would that also work? might be better inline with the "intent" of the variable (to the extend in which we can interpret the original intent).

[rburema]

Ah I see, wasn't aware about the changes regarding closed paths.

Well, I'm not sure the changes are intentional... (that's what I hope to find out at either the stand-up or the eCCB).

However:

• If they are not intentional, then this change at least can't hurt (and fixes the problem at least for the wipe), since then is_closed was meant to convey the same, but more conveniently in a single variable. In that case, maybe the error that lead to this should be fixed either instead of as well.
• If they are intentional, then end == start doesn't mean the same thing (we relied on) anymore and it should be replaced by is_closed at least in this case.

Also, the whole is_linked thing is the only variable that still previously used the start/end. So I removed them in the next commit (I made after your first comment). The way it was using those, back when the code was correct (so in like 5.2, 5.1, or before) is consistent with ! is_closed.