Ultimaker / CuraEngine

Powerful, fast and robust engine for converting 3D models into g-code instructions for 3D printers. It is part of the larger open source project Cura.
https://ultimaker.com/en/products/cura-software
GNU Affero General Public License v3.0
1.69k stars 886 forks source link

zig_zaggify_infill creates 1/2 linewidth gap between infill and perimeters #1265

Closed richfelker closed 4 years ago

richfelker commented 4 years ago

Without and with zig_zaggify_infill, all other settings same:

Screenshot_2020-06-05 G-Code Analyser - Analyse your 3D printing G-Code to provide accurate information such as print time Screenshot_2020-06-05 G-Code Analyser - Analyse your 3D printing G-Code to provide accurate information such as print time  (1)

Adding 1/2 line width (0.4/2 = 0.2) to the infill_overlap_mm suffices to work around this, but I think it's a bug that this is needed.

smartavionics commented 4 years ago

Hi, please provide a project file (File -> Save, zip or rename, attach) that illustrates this issue. Thanks.

richfelker commented 4 years ago

I'm not using the Cura gui, just CuraEngine. I'll try to make up a self-contained example (stl file and minimal json CuraEngine config file) and post it here but the problem does not seem to be hard to reproduce. It was happening all over the place and was the source of all my prints coming out britle wherever they had flat vertical walls (no support from the infill).

richfelker commented 4 years ago

Also, I checked whether there are any Python-expression values in fdmprinter.def.json that depend on the value of zig_zaggify_infill to derive other settings, where failure to duplicate that derivation might be the cause, but didn't find any. So it doesn't seem likely that this issue is specific to command line use of CuraEngine.

richfelker commented 4 years ago

@smartavionics: This issue is not present in your fork, and likely isn't present in upstream master either. I suspect it was a regression that was already fixed that just happens to be present in the version I had built, and I'm trying to ascertain exactly which revision that is. I'll update or close this as appropriate once I find out.

richfelker commented 4 years ago

Bug is present in e904e260 and current master, but was not present in some earlier build I have that I can't identify the exact commit it's from.

richfelker commented 4 years ago

This is reproducible with 4.6.1 tag and the above mentioned versions, using nothing but:

CuraEngine slice -j fdmprinter.def.json -s layer_height=0.2 \
 -s zig_zaggify_infill=true -l cube.stl -o cube.gcode

I have no idea why it's layer-height dependent, but it is.

cube.stl.txt

richfelker commented 4 years ago

4.3.0 lacks the bug, 4.4.1 has it.

richfelker commented 4 years ago

It seems to occur when infill_sparse_thickness != layer_height. And indeed it's a consequence of PR #1003. Before that, having infill_sparse_thickness set to some value lower than your layer height (and not adjusting it with layer height) worked fine. After that, there's a silent dependency on the Python expressions limiting infill_sparse_thickness to layer_height.

In src/infill.cpp, something like:

-const float width_scale = (mesh) ? (float)mesh->settings.get<coord_t>("layer_height") / mesh->settings.get<coord_t>("infill_sparse_thickness") : 1;
+float width_scale = (mesh) ? (float)mesh->settings.get<coord_t>("layer_height") / mesh->settings.get<coord_t>("infill_sparse_thickness") : 1;
+if (width_scale > 1) width_scale = 1;

should fix it.

richfelker commented 4 years ago

Confirmed, that fixes it.

smartavionics commented 4 years ago

Hi, sorry for the slow response, elsewhere..

How about this:

-        const float width_scale = (mesh) ? (float)mesh->settings.get<coord_t>("layer_height") / mesh->settings.get<coord_t>("infill_sparse_thickness") : 1;
+        const float width_scale = (mesh) ? std::min((float)mesh->settings.get<coord_t>("layer_height") / mesh->settings.get<coord_t>("infill_sparse_thickness"), 1.0f) : 1.0f;

BTW, why would one want the infill thickness to be less than the layer height?

richfelker commented 4 years ago

It's not that you want infill thickness to be less than the layer height. It's that #1003 introduced a new requirement that infill_sparse_thickness be updated (by Cura GUI frontend Python) to be no less than the layer height. Previously CuraEngine accepted infill_sparse_thickness values less than the layer height and they had no effect.

I'm not sure this is a bug strictly speaking, since it's not clear if usage of CuraEngine without front-end settings processing is considered supported. However it's a nasty gotcha. Ideally CuraEngine would ship a Python utility (or implement minimal equivalent expression handling) to apply all derivations in the input configuration like the Cura GUI would, so that this type of thing can't happen.

smartavionics commented 4 years ago

This issue stems from a lack of imagination on my part. It didn't occur to me that the infill thickness could be less than the layer height. I will submit a PR with the above fix and we can see what the UM devs think.

smartavionics commented 4 years ago

The UI currently lets one enter infill thickness values that are less than the layer height down to 1/2 of that height. Don't know the rationale behind that. Anyway, I'll do that PR soon.

richfelker commented 4 years ago

Weird, but indeed you're right. Also the documentation text says it "should be" a multiple of layer height and is "always rounded" (presumably to a multiple of layer height). So my reading of that is that settings less than layer height should work and behave identicallly to having it set to the layer height.

richfelker commented 4 years ago

Related: does this code work, or can it even be expected to work, with adaptive layer height?

smartavionics commented 4 years ago

I guess, strictly, the answer to your question is no but in practice it's more likely to be YMMV. i.e. it will work in the regions where the layer height is close to the nominal layer height but where the layer height is much smaller or larger than the nominal layer height, it's not going to be correct.

Apart from that, does it actually make sense to use an infill depth that is different from the layer height when adaptive layer height is activated?

richfelker commented 4 years ago

I think if you're using adaptive layer height to do 0.2 mm layers most places, but dropping to 0.12 or 0.1 or so where more surface detail is needed, it would be really nice to keep using 0.2 mm infill layers so the print doesn't get slowed down so much. So in the sense of being desirable to users, it makes sense. I don't know if CuraEngine's data model works well with it, though.

Ghostkeeper commented 4 years ago

I'm not sure this is a bug strictly speaking, since it's not clear if usage of CuraEngine without front-end settings processing is considered supported

We're supporting it as a debug tool for developers, so if it requires developer-like knowledge that is not necessarily considered a problem.

Ideally CuraEngine would ship a Python utility (or implement minimal equivalent expression handling) to apply all derivations in the input configuration like the Cura GUI would, so that this type of thing can't happen.

It's unlikely that we'll ever implement this. Computing the setting values is a huge part of the front-end code and also fairly complex to understand. We're not going to port all that to CuraEngine.

Related: does this code work, or can it even be expected to work, with adaptive layer height?

Infill Layer Thickness doesn't look at the adaptive layer height but pretends that all layers are the normal layer height except the first layer. So if you have 0.2mm layers and 0.4mm infill layer thickness, but the current layers get reduced to 0.15mm due to Adaptive Layers, you would get an infill layer thickness of 0.3mm there. This is one of the reasons why Adaptive Layers is still in experimental. In general, the Adaptive Layers implementation was just pasted on rather than integrated in the code, so all of the rest of the algorithms that involve vertical distance (skin thickness, support Z distance, overhang angle, rafts, etc) will not be correct if you enable that.

Ghostkeeper commented 4 years ago

The issue also occurs using the front-end by the way: image

The front-end limits the setting to a minimum of layer_height / 2 since the engine is expected to round it to a multiple of the layer height. Any lower and it would be rounded to 0.

Ghostkeeper commented 4 years ago

Looking deeper into it, I think that the entire premise of PR #1003 is wrong, actually.

The lines are visualised wider in layer view, but are modelled to drop down to fill the volume in layers below rather than spread out horizontally. So while layer view visualises the lines to be wider with a higher infill layer height, the actual infill line width is not expected to change in the print. So the premise of PR #1003 was that you'd get overextrusion because the lines are overlapping, but the lines are actually not overlapping and the "overextrusion" fills the space in the layer below. And so we should NOT offset the lines any differently to make it appear next to the walls in layer view, rather than on top of it. If anything, we should adjust layer view so that the lines are visualised correctly. That is a bit involved though for technical reasons (there is no notion of a line height in layer view, just a layer height).

I'll see if we can fix that.

smartavionics commented 4 years ago

Ignore the layer view, just consider the original issue that #1003 actually fixes.

smartavionics commented 4 years ago

It's the printed results that matter, not the visualisation.

Ghostkeeper commented 4 years ago

Yeah, sorry for the confusion. I misunderstood your analysis.

In any case, the position of the lines should not be affected by how many infill layers are combined, aside from breaking up the infill areas to have multiple thicknesses on one layer.

Currently (as of 4.6 and master) it is affected, so this is wrong. Here you can see a cube with the infill layer thickness at 1x layer height, no infill overlap: image The lines are exactly adjacent to the walls. That's correct since there is no weirdness going on with multiple layers extruding in one layer.

Contrast that with infill layer thickness at 2x layer height: image The wider lines indeed overlap the walls which is correct, but also the one layer at the top where a double layer doesn't fit any more gets moved a bit into the walls. There is overlap now even though the line's extrusion is the same. And in fact if you look at the coordinates, the wider lines are also too close to the walls.

This is because we're currently generating the infill with a different line width, and then compensating for that difference by performing offsets. The offset we were performing is a different formula than how the line width is actually calculated. So we have multiple problems here:

  1. We have this formula duplicated.
  2. Some of the duplications are slightly different. This is the most major reason to not have code duplicated.

Your fix addresses the 2nd problem, trying to bring the formula for the compensation-offset closer to the formula for the line width (although it's still not exactly the same).

I'm proposing a different fix: Let's adjust the flow instead. The line width is not affected and so the infill pattern generation should generate the same offset as if you're not using any infill combining, which eliminates this bug. It also makes the code more simple.

Ghostkeeper commented 4 years ago

All right, should be fixed in the upcoming release! The gap should no longer be affected by infill layer thickness now.