-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
@igiannakas I applied min with top to Prusa code, you can test it. |
I think we're missing the re-expansion of the top surface after here:
Maybe here we need to expand by the threshold?
But I may be wrong |
I didn't see its effect, but here is something else. Actually this code prevents setting to min_width_top_surface to low value specified in printing profile. So I tried i.e. WYSIWYG)). You text model looks good with 0.06mm (so min_width_top_surface is 60000, vs previous limit 202841): Lesser value causes holes, large collisions. Benchy also feels good with absolute zero)). Try updated PR. |
`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 |
Here is your benchy with latest code and zero threshold Did you check latest commit vovodroid@80207d5? |
Try with 300% threshold (that’s the default profile value) and you’ll see what I mean :) vs classic or the old Arachne |
Why use 300? 14% produce good result for text model. Probably it's worth to change default value to zero or something like 15%. |
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.
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. Edit: nope that was not it. Ignore the above. |
Actually what is problem with test projects in latest commit with adjusted threshold? Both cases look good for me. |
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.
|
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 |
Using the radius filter instead of the split with the underlying paths is promising: Now testing instead of filtering with bbox radius, to simply filter by the max as below:
|
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:
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. |
Getting closer: With this change Old - new at 300% Old - New at 300% New handles perimeters better: Will revert back once final build is done and I'll post the revised code segment for your review |
OK it worked perfectly. This commit here contains the changes I've made: I'll post screenshots in a little while. |
Screenshots: Before left / After Right Improved handling of one wall top surface when subsequent layer is followed by thin walls Improved handling of irregular shapes: Improved handling of top surface before lettering: Further tweaking and tuning:
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. 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! |
OK turns out 75% is the magic value - no more defects Updated commit here: f192509 |
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 🎉 |
Got it. |
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). They look like perimeter calculation inaccuracies. With precise wall disabled - tool path deviations are not present. 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: I won't be able to look into this issue for a little while but hope the above helps. |
Can you highlight areas in question? I don't really see the difference))) |
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. |
For instance? By a way, I implemented "Print overhang first", so one can use "outer-inner" for all, but overhangs. |
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! |
We are talking about steep overhangs anyway, so surface discontinuities is not issue here. |
This. Added new commit to it. |
Some feedback - have been using it for the past 3 weeks or so after the latest commit and it has been working great ;) |
Now waiting for SoftFever )) |
@vovodroid @igiannakas I can't repro the issue reported in #6000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you!
Did you mean to look into this? |
but I didn't open PR, as this change is based on |
Would you mind sharing this build? 😅 |
It's included in my Orca fork. |
I was not able to find it in your fork. 🤷♂️ |
https://github.com/vovodroid/OrcaSlicer/commits/vvd/ Commit "Overhangs after" |
Ah, I see, I was hoping you had a compiled version. |
Sure, I use my fork for slicing. |
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:
@igiannakas @Eldenroot FYI