Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arachne "one top wall" fix and refactor #6236

Merged
merged 6 commits into from
Sep 23, 2024

Conversation

vovodroid
Copy link
Contributor

This PR supersedes Reduce one top perimeter offset #6086 and fixes #6000 One wall on top causes wrong wall generation in Arachne and fixes #3313 Perimeters offset too much for one wall on top surfaces.

See previous discussion in Reduce one top perimeter offset #6086.

Changes:

  1. Arachne one top level wall code was replaced by ported from Prusa, while keeping "One wall threshold" parameter.
  2. Classic - perimeter offset was reduced to produce good result.

@igiannakas @Eldenroot FYI

@vovodroid
Copy link
Contributor Author

@igiannakas I applied min with top to Prusa code, you can test it.

@vovodroid vovodroid changed the title One top wall fix pr One top wall fixes Jul 24, 2024
@igiannakas
Copy link
Contributor

Think its still not 100% there, sorry :(

Project file from the last post in the previous PR. While it has fixed the nozzle crashing over the perimeters, even with 0 gap you still get walls coming through the top perimeters

image

Also the one wall threshold seems to be over compensating

image
Both above are at 300% threshold - left is the old version, right is the new.

The old 300% is closer to the new 100% (see below)

image

@igiannakas
Copy link
Contributor

igiannakas commented Jul 24, 2024

I think we're missing the re-expansion of the top surface after here:

  double min_width_top_surface = std::max(double(ext_perimeter_spacing / 2. + 10), scale_(config->min_width_top_surface.get_abs_value(unscale_(perimeter_width))));

             // Get top ExPolygons from current infill contour.
             Polygons upper_slices_clipped = ClipperUtils::clip_clipper_polygons_with_subject_bbox(*upper_slices, infill_contour_bbox);
             upper_slices_clipped          = offset(upper_slices_clipped, min_width_top_surface);

Maybe here we need to expand by the threshold?

top_expolygons = offset2_ex(top_expolygons, -top_surface_min_width, top_surface_min_width + float(perimeter_width));

But I may be wrong

@vovodroid
Copy link
Contributor Author

Maybe here we need to expand by the threshold?

I didn't see its effect, but here is something else.

Actually this code
double min_width_top_surface = std::max(double(ext_perimeter_spacing / 2. + 10), scale_(config->min_width_top_surface.get_abs_value(unscale_(perimeter_width))));

prevents setting to min_width_top_surface to low value specified in printing profile.

So I tried
double min_width_top_surface = scale_(config->min_width_top_surface.get_abs_value(unscale_(perimeter_width))));

i.e. WYSIWYG)). You text model looks good with 0.06mm (so min_width_top_surface is 60000, vs previous limit 202841):

No holes
image

neither nozzle collisions:
image

Lesser value causes holes, large collisions.

Benchy also feels good with absolute zero)).

Try updated PR.

@igiannakas
Copy link
Contributor

Hm, still not there yet I think

image
image

vs:

image

Basically the threshold shouldn't split a continuous polygon into multiple top surfaces - it's there to filter out small total top surface areas (i.e. smaller than X% of nozzle size/line width because very small extrusions cannot be reliably printed)

@igiannakas
Copy link
Contributor

`Hence why in the code before the polygon was expanded after the "trimming" by the threshold area, so the top surface can be "re-unified" again into a single polygon, I think

@vovodroid
Copy link
Contributor Author

Here is your benchy with latest code and zero threshold

image

Did you check latest commit vovodroid@80207d5?

@igiannakas
Copy link
Contributor

Try with 300% threshold (that’s the default profile value) and you’ll see what I mean :) vs classic or the old Arachne

@vovodroid
Copy link
Contributor Author

Try with 300% threshold

Why use 300? 14% produce good result for text model. Probably it's worth to change default value to zero or something like 15%.

@igiannakas
Copy link
Contributor

igiannakas commented Jul 24, 2024

The original approach works by expanding the upper polygons with a uniform offset, and it tends to merge adjacent top surface polygons.

The Prusa approach ensures that each top surface polygon meets the minimum width independently. However, the filtering process can break up a single larger polygon into multiple smaller polygons if parts of it do not meet the width requirement, which is not the case in the old approach or the classic perimeter generator approach.

Basically the old approach merges polygons to create larger, unified top surfaces, while the new approach enforces the minimum width on each polygon segment individually, leading to more separated top surfaces. This explains why a top surface polygon spanning two perimeters is treated as one in the old code and as two in the new code.

I think this is happening because of this line here. The offsetting applied here involves both inward and outward adjustments, which can result in breaking up larger polygons into smaller segments if they do not meet the width criteria.

top_expolygons = offset2_ex(top_expolygons, -top_surface_min_width, top_surface_min_width + float(perimeter_width));

Replacing it with the below should correct for this but I haven't finished compiling the code yet to test it with that one line change. Theoretically this offsetting approach expands the upper polygons, ensuring that any top surface is at least the specified minimum width.
top_expolygons = offset_ex(top_expolygons, top_surface_min_width);

Edit: nope that was not it. Ignore the above.

@vovodroid
Copy link
Contributor Author

Actually what is problem with test projects in latest commit with adjusted threshold? Both cases look good for me.

@igiannakas
Copy link
Contributor

Actually what is problem with test projects in latest commit with adjusted threshold? Both cases look good for me.

It's what the setting means - basically it runs the risk of splitting surfaces when it doesnt need to. For all intents and purposes you can set a much higher value to filter small area polygons before without affecting large contiguous surfaces at all. For example

image
Look on the side walls of the benchy (where it slopes up). The old 300% threshold didn't generate top fill for all as the polygon was smaller than the threshold. The new approach with 14% does. Because they are small, they will over extrude.

@igiannakas
Copy link
Contributor

Setting the new approach to 300% breaks them up like this:

image

Even setting to 100% creates half filled top surfaces

image

@vovodroid
Copy link
Contributor Author

vovodroid commented Jul 24, 2024

Because they are small, they will over extrude.

I guess linear/pressure advance can handle it, in conjunction with "Small area flow compensation". Chimney top also has small area.

At least new version doesn't produce things like these
image
image

What we can try to do, is to find some good default threshold value. Probably even set it zero. It's good for most cases without very small details, like text.

@igiannakas
Copy link
Contributor

igiannakas commented Jul 24, 2024

Im afraid it can't handle it fully. In general you want to avoid very small extrusion volumes for this reason - there must be a better way.

I'm trying to compile it now with a filtering change, checking the size of the polygon separately instead of using the threshold value to expand and contract the perimeters to create the top surfaces. TBC if this works, will post back.

            // Filter out small polygons
            ExPolygons filtered_top_expolygons;
            for (const auto &poly : top_expolygons) {
                BoundingBox bbox = get_extents(poly);
                double bbox_width = bbox.max.x - bbox.min.x;
                double bbox_height = bbox.max.y - bbox.min.y;
                if (bbox_width >= min_width_top_surface && bbox_height >= min_width_top_surface) {
                    filtered_top_expolygons.push_back(poly);
                }
            }
            top_expolygons = filtered_top_expolygons;

@igiannakas
Copy link
Contributor

Yeah I know that's why I'm also so keen on getting this implemented, but it has to be right with the threshold handling :S

@igiannakas
Copy link
Contributor

igiannakas commented Jul 24, 2024

Using the radius filter instead of the split with the underlying paths is promising:

image
image

Now testing instead of filtering with bbox radius, to simply filter by the max as below:

                float top_surface_min_width = std::max<float>(float(ext_perimeter_spacing) / 4.f + scaled<float>(0.00001), float(perimeter_width) / 4.f);
                top_surface_min_width = std::max(min_width_top_surface,top_surface_min_width);

@vovodroid
Copy link
Contributor Author

Are you straggling to get such picture - internal perimeter here and top surface in other places?

image

@igiannakas
Copy link
Contributor

igiannakas commented Jul 24, 2024

Are you straggling to get such picture - internal perimeter here and top surface in other places?

image

That's expected as the width there is smaller than the threshold. The radius check doesnt create this artefact though. I'm testing the above changes as well as this change here:

Arachne::WallToolPaths inner_wall_tool_paths(not_top_polygons, perimeter_width, perimeter_width, coord_t(inner_loop_number + 1), 0, layer_height, input_params_tmp);

As the inner wall tool paths were generated using the perimeter spacing as the nominal line width which may explain the lettering issue.

Will revert back.

@igiannakas
Copy link
Contributor

igiannakas commented Jul 24, 2024

Getting closer:

With this change
const float top_surface_min_width = std::max<float>(float(ext_perimeter_spacing) / 4.f + scaled<float>(0.00001), float(scale_(config->min_width_top_surface.get_abs_value(unscale_(perimeter_width)))) / 4.f);

Old - new at 300%

image

Old - New at 300%

New handles perimeters better:

image

Will revert back once final build is done and I'll post the revised code segment for your review

@igiannakas
Copy link
Contributor

OK it worked perfectly.

This commit here contains the changes I've made:
igiannakas@4694448

I'll post screenshots in a little while.

@igiannakas
Copy link
Contributor

igiannakas commented Jul 24, 2024

Screenshots:

Before left / After Right
Identical handling of one wall threshold
image

Improved handling of one wall top surface when subsequent layer is followed by thin walls
image

Improved handling of irregular shapes:
image

Improved handling of top surface before lettering:
image

Further tweaking and tuning:

top_expolygons = offset2_ex(top_expolygons, -top_surface_min_width, top_surface_min_width + float(perimeter_width/2));

The above contracts and expands the top surface polygon to, in effect, filter out the small areas. In addition in expands the polygon slightly (perimeter_width/2) to tuck the top surface under the next perimeter on top. Prusa slicer has the expansion increase to be equal to perimeter_width. This can cause nozzle collisions as thin features surrounded by top surface polygons are in effect "expanded into". Setting this value to a lower % (in this case perimeter_width/2) allows for more room for thin features to print, without the nozzle colliding over them.

However - this does result in minor artefacting like the below because the top polygon is clipped with the perimeter next to it.
image

Potentially a value to try would be 80% of perimeter width - experiment in progress.

@vovodroid thank you for doing the hard work of porting this over and bearing with me for debugging!

@igiannakas
Copy link
Contributor

Also the code still fixes the below issues:

image
image

@igiannakas
Copy link
Contributor

igiannakas commented Jul 24, 2024

OK turns out 75% is the magic value - no more defects

image

Updated commit here: f192509

@vovodroid
Copy link
Contributor Author

Great finding, so PS code does the same, just need to be configured and tuned.

turns out 75% is the magic value - no more defects

I found 0.85 is better. No holes:

image

No nozzle crossing and artifacts:
image

From my POV benchy is also looking good
image

Regarding concern about small area of left strip - it's the same as others.

But 0.85 fits internal perimeters better than 0.75
image

Probably this
image

was caused by mismatch between threshold and coefficient, as they work together.

So I would leave it 0.85. Or add new parameter to configuration.

By a way, PS divides threshold by four, while in classic it's used "as is". May be not divide it for Arachne to make both methods closer one to another?

@igiannakas
Copy link
Contributor

The threshold division in Bambu is because it’s expanding the polygon by that distance as a whole - so divide by 4 to get the expansion on each edge.

Yeap let’s try 0.85 and see how it goes. I don’t feel that another parameter for this is needed, just need to test and find the right one and maybe tune with some user feedback. More options tend to confuse newer users as they are not familiar with what to tune for what (I’ve seen so many horror stories in users profiles over the past year or so!!).

Would you mind updating the PR with the changes from my commit last night? Thank you for the work btw, this is working very very well 🎉

@vovodroid
Copy link
Contributor Author

vovodroid commented Jul 25, 2024

How does it look with 80% overlap?

The same. The only value preventing nozzle crossing and producing better underlying result is 0.55
image

Internal perimeters offset with 0.55:
image

image

So we have two dimensional parametric space, and lock one parameter could be not optimal for certain case. I agree that too much parameters could be intimidating)), but there is no choice if one needs perfect result.

@igiannakas
Copy link
Contributor

Leave it at 0.85 then - the small redundant wall line won’t be an issue and is an edge case compared to the bigger issue of not setting this correctly (as it will fragment walls - tested 0.55 yesterday).

@vovodroid
Copy link
Contributor Author

Probably @SoftFever would like to add parameter. Now we can only wait for him.

@discip
Copy link
Contributor

discip commented Jul 25, 2024

@vovodroid
@igiannakas

Thank you for your effort on that one! 😃👍

ps:
I know this is off topic,
but would you mind having a look on the handling of the wall printing order when Extra perimeters on overhangs is activated.
Regardless of the Walls printing order these walls should always start on the inside. Otherwise it absolutely misses the point.
I'm aware of this being no easy task, but maybe you can think of a solution. 😊

thanks in advance

@jeremytodd1
Copy link

jeremytodd1 commented Jul 26, 2024

Does this pull request also resolve the issue shown in this post?
: #5738 (comment)

@vovodroid
Copy link
Contributor Author

@discip

I'm aware of this being no easy task

Nothing terrible, I've already made some changes in wall order, but they are based on my wall direction refactoring PR.

@jeremytodd1
could please give test project?

@jeremytodd1
Copy link

@vovodroid

You can find the model here:
https://www.printables.com/model/141746-the-office-sign

@vovodroid
Copy link
Contributor Author

Does this pull request also resolve the issue shown in this post?

No, it's for another things.

@jeremytodd1
Copy link

Got it.
The issue in that linked post might be another thing to look into, as it is another big issue on how Orca handles top layers in some cases. There is currently no way to tune for that issue in Orca.

@igiannakas
Copy link
Contributor

igiannakas commented Jul 29, 2024

OK found an issue with this option and how it interacts with precise wall. When both are enabled you can see artefacts in the benchy like the below. (open the image below in full screen to see it better).

image

They look like perimeter calculation inaccuracies.

With precise wall disabled - tool path deviations are not present.
image

Unfortunately these are visible on the final print and they look like wall artefacts

I'm busting my head to figure out where these come from but I'm drawing blanks right now.

Project file:
One wall inaccuracies.3mf.zip

I won't be able to look into this issue for a little while but hope the above helps.

@vovodroid
Copy link
Contributor Author

Can you highlight areas in question? I don't really see the difference)))

@igiannakas
Copy link
Contributor

Yeah sure hope this helps:

IMG_4226

@vovodroid
Copy link
Contributor Author

Fixed (I hope)), updated. Though I don't see meaning in precise wall option. Use "outer-inner" for it.

image

@igiannakas
Copy link
Contributor

It increases the spacing between the first internal and the external perimeter when using inner outer mode. It helps slightly reduce surface issues when you’re forced to use inner outer (like some of the Voron parts for example)…

I’ll give it a test at some point and let you know.

@vovodroid
Copy link
Contributor Author

you’re forced to use inner outer (like some of the Voron parts for example)…

For instance? By a way, I implemented "Print overhang first", so one can use "outer-inner" for all, but overhangs.

@igiannakas
Copy link
Contributor

Changing wall order sequence between two layers from outer inner to inner outer will result in layer surface discontinuities - so it’s generally preferable to stick to one wall order mode throughout the print.

that being said, which pr is it? Keen to test it!

@vovodroid
Copy link
Contributor Author

Changing wall order sequence between two layers from outer inner to inner outer will result in layer surface discontinuities

We are talking about steep overhangs anyway, so surface discontinuities is not issue here.

@vovodroid
Copy link
Contributor Author

that being said, which pr is it?

This. Added new commit to it.

@igiannakas
Copy link
Contributor

Some feedback - have been using it for the past 3 weeks or so after the latest commit and it has been working great ;)

@vovodroid
Copy link
Contributor Author

Now waiting for SoftFever ))

@SoftFever
Copy link
Owner

@vovodroid @igiannakas
Thank you so much!

I can't repro the issue reported in #6000
But I do see there's improvements for #3313 and thin texts

Copy link
Owner

@SoftFever SoftFever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thank you!

@SoftFever SoftFever merged commit 69f521f into SoftFever:main Sep 23, 2024
15 checks passed
@vovodroid vovodroid deleted the one-top-wall-fix-pr branch September 23, 2024 11:36
@discip
Copy link
Contributor

discip commented Oct 4, 2024

@vovodroid

@discip

I'm aware of this being no easy task

Nothing terrible, I've already made some changes in wall order, but they are based on my wall direction refactoring PR.

Did you mean to look into this?
Or do you even have already a solution to this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants