New Tree Support: Coordinate Remaining Tasks

[rburema]

This set of discussions is meant to coordinate work between developers both in and external to UltiMaker, w.r.t. the New Tree Support, specifically as it relates to the release of the first Cura version that it's going to be in.

More precisely, we will post a few sub-discussions with the tasks that we still think we need to do. (From the UM side this will probably mean Jira tickets).

---

For a more end-user focussed discussion, and/or reporting new problems, please use either the normal Github bug-reporting flow, and/or this discussion about the alpha we already released with a version of the new tree support.

---

'Tree Support 2.0', or 'New Tree Support' represents a significant improvement to (tree) support, and we plan to replace the current version of Tree Support with this in 5.4.0., if all goes well.

(You can already try this out: There is an early alpha, based from 5.2.x and mildly confusingly called Cura 5.3.0-Alpha-Xmas.)

For the original Pull Request(s) related to this see here for the engine one. For settings added or changed in the front-end see this branch.

[rburema]

CURA-10062 Missing Support Interface

[jellespijker]

What tool are you using to format the code? My IDE seems to go for another style, especially with the parallel for loops.

We used to be not the most consistent in our formatting, which resulted in a couple of unnecessary discussions during code reviews, which were more about form that content.

To circumvent that we added clang-format and clang-tidy files. These files are not completely according to our style (yet). But you should still use them for formatting. If we discover during a code review that those tools don't provide the correct output we can change the configuration files in the PR according to our style.

Since our exiating code base is inconsistent in the formatting you should take care when using them to format existing files. Apply them only on the changed sections. If you introduce new files you should apply them from the start.

This is the most pragmatic way to move towards a fully automated format and statically analysed code base.

[ThomasRahm]

I do see some floating interface where no trees are generated.

As i now generate roof areas before i generate the tips, this is an issue that could occur more easily. I have added a function that should remove tips inside of support roof and tips that should support roof, but don't have any above it called removeUselessAddedPoints. This issue could also be related to it.

I got curious and looked at my favorite test model again (the hobgoblin) and while i could not see any non supported roofs, i found that a small rounding error could have caused very small roof areas to be not supported. I improved the error handling in that case a bit, but i would be surprised if this fixed your issue.
Happy to take a look if you can share the project file.

[rburema]

Happy to take a look if you can share the project file.

Thanks!

I tried the fix you made, but no dice. I don't think this is a small rounding error. It seems like the roof is overwriting the tips in some cases, causing tree generation to fail and only leave the interface behind. (Because if I slice it without interface, it does generate.)

UMS5_HMAL_3S.zip I've attached the model.

(In the mean time, I fixed a divide by zero error in the branch I'm working on now. https://github.com/Ultimaker/CuraEngine/tree/CURA-10415_update_tree (There's a front-end branch as well https://github.com/Ultimaker/Cura/tree/CURA-10415_update_tree )

[ThomasRahm]

Can reproduce it, found the cause and pushed a fix. Thank you :)

Forgot to ensure check that even if there is no overhang that has to be supported on a layer, the layer above may have roof that has to supported. Seems my test models have overhang on every layer.

[rburema]

Thanks again. That was even faster than you normally fix things! I'll have a look merge it in later on our side.

[rburema]

CURA-10065 Floating Islands Inside Support

[rburema]

We see it occurring far less now, since we merged in your solution. For the remaining issues, we're thinking of 'filling' the tree-support with (sparse) lightning-support-infill.

Which brings me to another point I'm not sure if I've mentioned yet; The lightning infill point generation could maybe also be repurposed (made to work with both) to generate the initial sites for the branches maybe. See the usage of the LightningDistanceField in generateNewTrees for an example.

[ThomasRahm]

I am currently trying to improve the way pointy overhang are supported, part of this is generating a high density line patterns inside of branches that support such pointy overhangs or interface (still having issues with larger areas that only touch interface being partially filled with said lines especially if the lines are to be drawn in more than one angle). This code is not yet pushed.

I will need to take a look at lightning infill to see if it solves this problem in a better way.

[ThomasRahm]

I opened a draft pull request with screenshots for the improvements for pointy overhangs: Ultimaker/CuraEngine#1880
I still do want to change some things, but it should be in a pretty good state to take a first look at.

It seems possible to use the LightningDistanceField to generate where tips are placed. What case do you think could be improved with it? I could see using it for small or thin overhangs to ensure more evenly tip placement, but i would need to look closer into it to be sure.

[rburema]

Thanks! I'll have a look at the PR.

As for the Lightning: We mostly think it would be a good idea to decouple the tree generation from (generic) infill, as it's not meant for that. This (also) has to do with maintenance; any change in the infill will have consequences for tree support -- conversely, any change we'll need to make to infill because of tree support will also need to work with infill. Additionally, infill isn't really made for the concept of 'try to find points to support this part', while the distance field already is.

However, on the other hand, I don't think this has high priority. If the current method works, we can also just leave it that way.

[ThomasRahm]

Supporting roof gets much more difficult without using the infill class. This includes the fake roof (similar to roof, but regular support) i use for large areas.

I think that the most issues caused by changes in the Infill class will be if parameters are added/removed or changed compared to how support/interface lines will be generated with it.
I agree having to changed them in two places is not optimal, but i don't see any easy way to prevent this (in the tree support code) without loosing the benefits.

[rburema]

CURA-10066 Weird Effect With Adaptive Layers

[ThomasRahm]

I cant find an issue related to this one, so i do not exactly know what this weird effect is. Can i get more information about it ?

[rburema]

Of course! I don't expect you to get very far with a ticket number you can't access and a vague title.

This is a screenshot:

This corresponds with the attached model.

adaptive (1).zip

To be honest, I'm not sure how high-priority this one is. For the initial release, we can consider disabling 'Adaptive Layer Height' when printing with trees.

[ThomasRahm]

Thank you.
I see the issue here. The code assumes that each layer has a certain height (here 0.1mm) to calculate values dependent on it (e.g. diameter_angle_scale_factor, maximum_move_distance). As here the actual layer-height can be 10% of the layer height setting, the effects are much more pronounced, causing branches to become larger and move at up to 20x rate. If reasonable settings are used (0.01mm-0.2mm layerheight is a pretty large range, with the upper limit being 20x the lower limit) the effect is much less dramatic.

I know that PrusaSlicer also has issues with the organic support and adaptive layer height, so there can be an argument made that i should look at it.

I see two possible solutions:

1. Making the code aware of layer-height, changing all variables that use layer-height to functions dependent on layer_idx.
2. Throwing away/unioning layers to calculate the tree support to achieve a constant distance between layers, and later extrapolate the position on the skipped layers.

The second option has the benefit that it may be also used to improve performance for small layer heights, but has a larger chance of causing issues with z distance.

I will evaluate how much work it would take to fix this as soon as i have updated my development setup.

[ThomasRahm]

I looked at possible solution 2: There are few cases that may require a good amount of changes to not cause issues. For now I will focus on the interface issue.

[rburema]

Please don't worry too much about it. I don't think supports and adaptive layer eight are used together that often.

[rburema]

CURA-10095 Slicing Crash At 100% (related to Tree Support?)

[ThomasRahm]

The corresponding issue seems to be #14113

The crash seems to happen when generating Support Infill lines. It occurs in SkeletalTrapezodiation.cppin constructFromPolygons when the vertex of the twin of vd_edge is NULL. Not sure what this means exactly, but it occurs when generating Tree Support, because it requests infill from the Infill class (Which should be fine).

In short, i think this bug is not in the TreeSupport code. I suspect it is the same bug that causes slicing failures, that can be avoided by moving the model around a bit.

[rburema]

Yes, a long standing issue known to us. We (still) think it has to do with a bug in boost-voronoi (or at least that doesn't help).

This is indeed isn't a Tree Support problem, if that's the case (though not sure why it would fail at 100% then, instead of in the middle.)

We do have two other crashes reported as well, in separate tickets, but I expect those to be much the same. (Probably at least.)

I'll attach the models here just in case it isn't.

CURA-10063: darth slicing failed.zip

CURA-10095: CCR10S_santa_claus_solid_v2 (2).zip

[Piezoid]

IME, this issue can be solved by scaling up the polygons before applying applying the opening/closing operation with a bigger offset. This removes more of the near-intersection ambiguities caused by snapping vertices to the grid. I've been doing that by globally scaling the unit system, but it might be easier to implement in the inputs/outputs of WallToolpath. This change allowed CuraEngine to slice every model that had this issue from the bug tracker, at least for the models that I tried.

Admittedly, this is more of a hack than a solution. The root cause may be in boost/polygon/voronoi, but CuraEngine shouldn't segfault when traversing a malformed graph. However, adding control flow for the checks to the current implementation is quite difficult.

Ideally, we should identify and fix the edge cases in the input polygons. Although, this requires understanding and reproducing the specific issue within boost/polygon/voronoi.

[rburema]

CURA-10091 Rounding Error That Causes Long Slicing Times

[ThomasRahm]

Corresponding issue seems to be #14133

Fixed with ThomasRahm/CuraEngine@201a12d

[rburema]

Yes, this was the ticket nr. made to pull that one in :-)

[rburema]

CURA-10161 Tree Branches Are Cut By Support Blockers

[ThomasRahm]

Assuming it is the issue i think it is, it is fixed with ThomasRahm/CuraEngine@1d62038

[ThomasRahm]

The corresponding issue seems to be #14208, but it is about wanting an option to allow tree support to pass though the support blocker, not about branches being cut/resting on support blocker.
This is not fixed as avoiding support blocker is an intentional behavior of the new tree support.
Is there a way to add a small checkbox in the support blocker UI like this and add a corresponding setting to the mesh?

Mock-up:

Otherwise the constant SUPPORT_TREE_AVOID_SUPPORT_BLOCKER is currently used to enable this feature, so if a global (not per mesh configurable) setting is preferable i could add it myself.

I don't think removing this behavior is beneficial, as being able to place support blocker in a hole to prevent branches from traveling though said hole can be very handy. I can provide examples/screenshots for situations where one wants the branches to avoid support blocker if you want.

[rburema]

The ticket is mostly about deciding what to actually do:

If there is no option to place a tree because of a support blocker, it just exists in the air.
There are mixed opinions about this.
We need to decide if we want to cull/remove these trees or give users the option to ignore the support blockers in these cases. 🤔🤔

... so we could leave it as is as well, we just need to make a clear decision.

[ThomasRahm]

The way i solved it in ThomasRahm/CuraEngine@1d62038 is that instead of using the collision when a branch has to non-graceful rest on the model, i calculate an avoidance for support blocker. That way branches that would have to rest on support blocker will not generate in the first place (the same way branches that would have to rest on the model, do not generate when the branches have to rest on the buildplate).
I think an UI option to allow branches to travel through support blocker would be most versatile.

[rburema]

CURA-10179 Tree Support Supporting Nothing In Particular Circumstance

[ThomasRahm]

Assuming it is related to an issue like #14302

Fixed with ThomasRahm/CuraEngine@1724f67

[rburema]

Yes, seems to be: This ticket was made from the forums (and you replied in that thread).

[rburema]

Code Style & Other Refactors

CURA-10302 Make 'Raw' Magic Numbers Constant & Consolidate
CURA-10303 Streamline various 'FUDGE_LENGTH' Constants
CURA-10304 Existing Constants: Keep, Consolidate, Move To Setting?

[DerGenaue]

Settings Default Parameters

I've got a few questions and suggestions regarding the default parameters that were chosen for the new Tree Support:

• Tree Support Rest Preference: The default value of this setting is currently set to On any flat surface.
  In my opinion the default should definitely be Buildplate to make the trees try to reach the buildplate if they can and only rest on the model if they must. I think that for most people the reduced scarring is one of the major reasons to chose Tree Supports and the slight increase in material usage compared to On any flat surface is perfectly justifiable.
• Tree Support Optimal Branch Range: This setting should in my opinion be given a higher default value. I personally even think that it should be highlighted in orange when it is smaller than Tree Support Trunk Diameter, because otherwise the tree trunks can overlap and this makes the Tree Supports much more prone to the floating island issue.

Are there any other opinions?
What are the motivations for the current default values?

[DerGenaue]

Regarding the Tree Support Rest Preference, I personally think that the best way would even be to integrate this into the Support Placement setting:
Three modes: Buildplate only, Prefer buildplate, Everywhere.
This might collide with other modes (non-tree), though...

[ThomasRahm]

I also think Tree Support Rest Preference should be set to Buildplate.

Tree Support Optimal Branch Range is very dependent on what is to be printed. E.g. for small miniatures i would disable it completely, but not for large models.

[ThomasRahm]

The settings related to Support Stair Step and Support Join Distance are visible when tree support is enabled, but do nothing.

[rburema]

We should decide to either make them work (probably a bit too much for for 5.4), or disable them outright (I don't think they work for our current tree-support either).

I think we can disable them, since with your tree support, it will be even less likely to need to rest on the model(s).

[ThomasRahm]

Enabling Connect Support Lines causes a drastic increase in tips being generated.

Enabled:

Disabled:

Project file:
TestOverhang.zip

[ThomasRahm]

Some profiles (E.g. https://github.com/Ultimaker/Cura/blob/main/resources/definitions/flyingbear_base.def.json) seem to have Minimum Support Area set to something larger than 0, which can cause confusion when tree supports are used.
Example where it has caused confusion:
#14150

Also some profiles (E.g. https://github.com/Ultimaker/Cura/blob/main/resources/definitions/creality_base.def.json) seem to have a default that the Support Top Distance is one layer-height more than Support Z Distance when tree support is used.
I don't think that this is applicable to the new tree support.

Should i make a pull request related to profiles? I had some profiles changed in my fork to accommodate for the tree support changes. For reference: ThomasRahm@3f77b9e and ThomasRahm@f41ed46 (this one just fixed missing " in the first commit)

[rburema]

You may if you like! We'll also go over at least fdm-printer and the ultimaker ones on our side.

( @pkuiper-ultimaker Might be good to take this into account when we update our profiles. )

[ThomasRahm]

Should i also document other bugs i fixed since the xmas-alpha that were not mentioned here?

[rburema]

I think that would be helpful, yes. We can determine if we missed anything important.

[ThomasRahm]

Some areas are sometimes not supported when Support Distance Priority is x/y overrides z if other overhangs on the same layer are placed further down because of x/y overrides z

Fixed with ThomasRahm/CuraEngine@2e5515d

[ThomasRahm]

Inital layer diameter not working reliably when rest preference is on any flat surface

Fixed with ThomasRahm/CuraEngine@e81807b and ThomasRahm/CuraEngine@4d65ae7

Very remotely related: There is a typo in Inital Layer Diameter, should be Initial Layer Diameter. The typo is not fixed.

[rburema]

The branches we're going to use for our 'exploratory' (and merge to main if we think it's close enough) are:

• For the engine; CURA-10415_update_tree
• For the front-end; CURA-10415_main_based_update_tree -- Note that this is a different branch name from the engine! The one that is the same as the engine was (also) based off-of the 'xmas alpha', and there where some changes we where dubious about in there, so we made a new branch that only has main and the tree changes in it.

(@ThomasRahm Jelle asked me to specifically tell you about the updated frontend-branch.)

[ThomasRahm]

Thank you. I noticed in the UI branch that quite a few changes were not migrated. I have done a pull request #15074. it also contains the profile changes i mentioned earlier. If any changes were that intentional that i this pull request would undo (e.g. Disabling minimum support area) please tell me.

[rburema]

Thanks for the additional fixes (not just the UI one I mean). We'll take another look at the settings with the Print Processing & Materials team when we have resolved some of the things we still want to fix.

[ThomasRahm]

Was informed about a bug related to roof generation:

I will be looking into this

Source: ThomasRahm/CuraEngine#11
Some settings have to be adjusted (e.g. support roof wall line count to 0) when trying to reproduce this one.

[ThomasRahm]

Fixed with ThomasRahm/CuraEngine@d046eaf

[ThomasRahm]

Roof lines that are directly next to support blocker are not supported.
Will be looking into this one. I expect a rounding error.

[ThomasRahm]

It was not a rounding error. It is fixed with ThomasRahm/CuraEngine@f20f751 though.

[rburema]

@ThomasRahm I'm not sure how closely you follow our branches, but we're currently working on CURA-10415_update_tree -- We're working on some bugs, some of which is due to the mess I created with my supposed divide-by-zero fix. (Way less support was generated under some circumstances.) This should be fixed however with the same set of changes that where meant to seep up the code, as I had to remove that fix anyway.

[ThomasRahm]

I found a bug: If config.getRadius(elem) is called while elem is a pointer the constructor used to initialize a new support element on the layer below is used to convert the pointer into an element.
So every time a pointer is used to call these functions it will be slightly too large (if it is part of a tip).
I made the TreeSupportElement constructor that takes a TreeSupportElement* explicit, and solved all compile errors de-referencing the pointers instead of passing them directly.
As i noticed this pretty far into me changing something else, it is not in a cherry-pickable commit ( ThomasRahm/CuraEngine@dc64f93 ).

Should i "backport" the fix for the current main branch and open another pull request?