[CURA-7979] Lock outer wall in place and width as much as possible #1405

rburema · 2021-02-05T21:43:22Z

Outer walls react to what happens inside. While technically correct behaviour given the basic premise of Arachne, this can cause noticeable artefacts in what would otherwise be a straight wall. This would already be worthy of fixing, but it seems these artefacts can be exacerbated by the printing process, making it a at least a critical and probably even a blocker for release.

This commit sets up the basic premise. Add a parameter to adjust how close the outer walls are to their 'optimal' width (and, because they are outer walls therefore also placement, which is what is needed), with 1.0 meaning completely optimal (mostly ignore any width changes to the outer walls coming in, let the inner walls 'solve it as much as possible') and 0.0 being the old 'correct' behaviour (no compensation is applied to the previous strategy other than the 'relative widths for inner and outer' redistribution which was already happening).

~~(See also front-end PR: Ultimaker/Cura#9245)~~

Jelle says:
I have been thinking really hard how a beading strategy could be applied to two different wall regions (inner/outer) and still be true to the strategy and the difference in wall width specified, and this is basically the only way I see how.

I think of having the "Lock Wall 0" feature always on (no check-box in the front-end) just normal behavior.

Rational behind this:

If two different wall thicknesses are specified for outer and inner walls, the user wants that outer wall to have that dimension. Since the outer wall always consist of 1 wall there isn't anything to distribute. It either is the specified dimension or less if the thickness of the outline is less then (2 x the optimal outer wall width + 1 x minimal inner wall width).
Now it is entirely possible that the inner wall has more then 1 walls (even when 1 inner wall is specified, for the cases of converging walls) In this case the strategies define the distributed width.

The gif above shows how the outer wall will no longer react to changes on the inside, until it arrives at thickness which is less then twice the optimal outer width and once the minimum inner width (see image below) It will then keep the third minimum wall constant till there is only space for two beads.

Note 1:

I the divergence of the wall position in certain places is not part of this ticket (although I original thought it was, Sorry @rburema )
This is more likely related to CURA-7853 and CURA-7854

Note 2:

The center deviation will have some left over space in case of an even number of walls, this is a separate bug imo and I will create a separate ticket for this. Then over/under extrusion which I found during my own review for the distributed strategies were a part of this ticket tho.

Outer walls react to what happens inside. While technically correct behaviour given the basic premise of Arahcne, this can cause noticable artifacts in what would otherwise be a straight wall. This would already be worthy of fixing, but it seems these artifacts can be exacerbated by the printing process, making it a at least a critical and probably even a blocker for release. This commit sets up the basic premise. Add a parameter to adjust how close the outer walls are to their 'optimal' width (and, because they are outer walls therefore also placement, which is what is needed), with 1.0 meaning completely optimal (mostly ignore any width changes to the outer walls coming in, let the inner walls 'solve it as much as possible') and 0.0 being the old 'correct' behaviour (no compensation is applied to the previous strategy other than the 'relative widths for inner and outer' redistribution which was already happening). What still needs to be done is: - Add the parameter as a class parameter and expose it to the frontend (and add it to the frontend). - Handle the case where there where previously only 2 outer walls ... but a middle (inner) wall might be needed after compensation. - Handle the case where all(a?) middle wall(s) should be eliminated because the space for inner walls has become too small after the compensation for outer walls is applied. CURA-7979

CURA-7979

Even if there are only outer walls (perhaps especially so) they'll need to be locked in place (depending on the outer wall lock factor). In case there is one, there isn't much that can be changed. The same goes for, 'two, but closer together than the optimum'. With two and some room to play with, there are a number of possibilities. In any case, the outer walls themselves should be repositioned according to the outer wall lock factor. The hard part is to determine when a third wall needs the be inserted, since the outer walls will become narrower. The transition thickness from 0 walls to 1 seemed like a natural choice (this will actually call the parent strategy, so that's neat as well). CURA-7979

It makes everything more complicated, and the zero width signalling walls are now guaranteed to be handled _after_ the redistribution 'stage' (beading meta strategy). CURA-7979

Not _entirely_ sure if and when this can happen... If so, then the situation must've been happening in the 'old' situation as well perhaps. A bit of defensive programming is usually a good idea though. CURA-7979

CURA-7979

In retrospect it's really weird that it compiled, ran _and_ passed the tests without that setting here locally. CURA-7979

jellespijker

The code style and the intention are clear.
But the placement of the center line of inner walls seems to suffer from an accumulating error, which is more evident in certain cases when the wall count is increased to a high enough number to fill the shape completely.

I also see a problem when handling the width of the inner central wall in those cases.

CURA-7979_fix_outer_wall_artifacts

The weights were calculated twice, first to determine the total weight, then to determine the actual weight of the bead. These where a lot of unnecessary multiplications. Stored the weight in a vector, summed it up and called upon the weight later on. Contributes to CURA-7979

Contributes to CURA-7979

For some reason I don't notice a big difference between the locking and "old" behaviour. Probably because of the usage of resetToolPath function. Contributes to CURA-7979

Contributes to CURA-7979

I have been thinking really hard how a beading strategy could be applied to two different wall regions (inner/outer) and still be true to the strategy and the difference in wall width specified, and this is basically the only way I see how. I think of having the "Lock Wall 0" feature always on (no check-box in the front-end) just normal behavior. Rational behind this: > If two different wall thicknesses are specified for outer and inner walls, the user wants that outer wall to have that dimension. Since the outer wall always consist of 1 wall there isn't anything to distribute. It either is the specified dimension or less if the thickness of the outline is less then (2 x the optimal outer wall width + 1 x minimal inner wall width). Now it is entirely possible that the inner wall has more then 1 walls (even when 1 inner wall is specified, for the cases of converging walls) In this case the strategies define the distributed width. This would result in a cleaner code (since you basically don't have to re-distribute all walls according to ratio). Also After running multiple test prints with a constant outer wall I see a huge improvement in surface quality. Contributes to CURA-7979

Contributes to CURA-7979

…79_fix_outer_wall_artifacts

rburema

Other than the cosmetic changes I propose in my review, I think the code looks good.

If my component test(s) pass(es) as well, I'll make the cosmetic changes myself (otherwise I will of course put it back into Todo).

rburema · 2021-02-16T08:25:43Z

src/BeadingStrategy/InwardDistributedBeadingStrategy.cpp

 #include "InwardDistributedBeadingStrategy.h"
+#include "CenterDeviationBeadingStrategy.h"


Remove this import.

rburema · 2021-02-16T11:00:13Z

tests/WallsComputationTest.cpp

@@ -62,6 +62,7 @@ class WallsComputationTest : public testing::Test
        settings.add("wall_transition_length", "1");
        settings.add("wall_transition_threshold", "50");
        settings.add("wall_x_extruder_nr", "0");
+        settings.add("wall_0_lock_factor", "0.5");


Remove unused setting completely.

rburema · 2021-02-16T11:01:11Z

src/BeadingStrategy/RedistributeBeadingStrategy.cpp

-        // Actual count and thickness as represented by extant walls. Don't count any potential zero-width 'signalling' walls.
-        bead_count = std::count_if(ret.bead_widths.begin(), ret.bead_widths.end(), [](const coord_t width) { return width > 0; });
-        thickness -= ret.left_over;
+        // Outer wall is locked in size en position for wall regions of 3 and higher which have at least a


'size en position' -> 'size and position'

CURA-7979

…om/Ultimaker/CuraEngine into CURA-7979_fix_outer_wall_artifacts

Outdated. Changed afterwards by reviewer themselves.

rburema added 6 commits February 5, 2021 18:52

Rethink left-over width w.r.t. redistribution.

869cc4d

CURA-7979

Don't handle 0-width signalling walls.

aca8aeb

It makes everything more complicated, and the zero width signalling walls are now guaranteed to be handled _after_ the redistribution 'stage' (beading meta strategy). CURA-7979

Handle case where inner walls dissapear.

4928723

Not _entirely_ sure if and when this can happen... If so, then the situation must've been happening in the 'old' situation as well perhaps. A bit of defensive programming is usually a good idea though. CURA-7979

Expose 'Outer Wall Lock Factor' setting to frontend.

3f28e22

CURA-7979

rburema mentioned this pull request Feb 5, 2021

[CURA-7979] Add 'Outer Wall Lock Factor' setting. Ultimaker/Cura#9245

Closed

rburema marked this pull request as ready for review February 5, 2021 21:54

Add new setting to tests as well.

d13d08b

In retrospect it's really weird that it compiled, ran _and_ passed the tests without that setting here locally. CURA-7979

jellespijker self-requested a review February 8, 2021 11:04

jellespijker self-assigned this Feb 8, 2021

jellespijker previously requested changes Feb 8, 2021

View reviewed changes

jellespijker added 11 commits February 10, 2021 16:46

Rewrote outer wall lock factor usage.

21c93de

CURA-7979_fix_outer_wall_artifacts

Inner beads should also be processed

2e0af51

Contributes to CURA-7979

Make sure that all beads still adhere to minimum width

c1b75ab

Contributes to CURA-7979

Take into account filtered out beads

8c60b9a

Contributes to CURA-7979

Removed indention

3bd13c9

Contributes to CURA-7979

Reimplemented old distribution calculation.

667ef4a

For some reason I don't notice a big difference between the locking and "old" behaviour. Probably because of the usage of resetToolPath function. Contributes to CURA-7979

Force symmetry from and early start.

a12e989

Contributes to CURA-7979

Updated and added documentation

219a48c

Contributes to CURA-7979

Renamed parameter to minimum_width_inner for consistency

5b7e466

Contributes to CURA-7979

jellespijker changed the title ~~[CURA-7979] Possible to adjust how close to optimal-width/place outer walls are.~~ [CURA-7979] Lock outer wall in place and width as much as possible Feb 15, 2021

jellespijker requested a review from nallath February 15, 2021 12:30

jellespijker removed their assignment Feb 15, 2021

Merge remote-tracking branch 'origin/libArachne_rebased' into CURA-79…

82eb2b5

…79_fix_outer_wall_artifacts

rburema commented Feb 16, 2021

View reviewed changes

rburema added 2 commits February 16, 2021 12:24

Remove dead code. Correct English spelling.

4404c7e

CURA-7979

Merge branch 'CURA-7979_fix_outer_wall_artifacts' of https://github.c…

88e793e

…om/Ultimaker/CuraEngine into CURA-7979_fix_outer_wall_artifacts

rburema merged commit fb8d399 into libArachne_rebased Feb 16, 2021

rburema deleted the CURA-7979_fix_outer_wall_artifacts branch February 16, 2021 12:27

rburema mentioned this pull request Feb 26, 2021

[Arachne_engine_alpha] Blobs in skin on thin walls. Ultimaker/Cura#9075

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CURA-7979] Lock outer wall in place and width as much as possible #1405

[CURA-7979] Lock outer wall in place and width as much as possible #1405

rburema commented Feb 5, 2021 •

edited

Loading

jellespijker left a comment

rburema left a comment

rburema Feb 16, 2021

rburema Feb 16, 2021

rburema Feb 16, 2021

		#include "InwardDistributedBeadingStrategy.h"
		#include "CenterDeviationBeadingStrategy.h"

[CURA-7979] Lock outer wall in place and width as much as possible #1405

[CURA-7979] Lock outer wall in place and width as much as possible #1405

Conversation

rburema commented Feb 5, 2021 • edited Loading

jellespijker left a comment

Choose a reason for hiding this comment

rburema left a comment

Choose a reason for hiding this comment

rburema Feb 16, 2021

Choose a reason for hiding this comment

rburema Feb 16, 2021

Choose a reason for hiding this comment

rburema Feb 16, 2021

Choose a reason for hiding this comment

rburema commented Feb 5, 2021 •

edited

Loading