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

[Feature] Enabled gap fill algorithm for all solid fill types #3412

Merged

Conversation

igiannakas
Copy link
Contributor

@igiannakas igiannakas commented Jan 1, 2024

Happy New Year!!!

So I've been debating whether to raise this PR or not, as there must be a reason it’s done this way (probably 🤣), so requesting feedback from someone a bit more skilled than me @SoftFever @Noisyfox @Ocraftyone. This fixes a particular annoyance of mine, which was the inconsistent handling of gap fill depending on the selected solid fill type as it was applied for some and not for others.

Also addresses issue: #2680 because gap fill for solid infill and top and bottom surface is now an option.

Summary:
This PR enables gap fill for all solid fill types. It:

  1. Moves the gap fill calculation logic to the parent FillBase fill_surface_extrusion function
  2. Removes the need for dedicated infill types that inherit FillBase and override the above
  3. Removes the need for duplicated code - gap fill is handled in the parent function with the inherited ones being responsible for simply generating the infill pattern. Arguably makes the code less convoluted and because gap fill is in a single place, easier to maintain
  4. This also fixes the issue where monotonic lines where not respecting the infill/wall overlap (because the monotonic lines function is now different) Bug: Corrected monotonic lines not respecting user infill / wall overlap setting #3388

Besides the above technical benefits, BBS has gap fill enabled only for the lines fill and no other infill type. This can result in tiny gaps especially in top and bottom surfaces if the user selects any other pattern

Feedback requested please:
I've noticed that SuperSlicer and Bambu slicer being the only two that have gap fill for solid surfaces implement this as an inherited class and I think replicate the gap detection code multiple times in their code base. This results in two variants of most infill types. Bambu implements gap fill only for the monotonic lines type while SuperSlicer I believe implements it for monotonic, monotonic lines and concentric.

Why would they do that and not move gap fill higher up in the inheritance chain and control where it's applied there? It seems such an obvious thing to do unless I am missing something?

Secondly, from a user perspective this is a change:

  1. You get gap fill on all infill types - if you want it disabled the user can set filter small gaps value to an appropriate number
  2. The above is a benefit, especially for models with high detail, like lettering / svg's etc as well as small features. Also helps with water tightness as solid infills have less gaps.
  3. Implemented options to enable gap fill for top and bottom surface and for solid infill and set “reasonable” defaults

What do you think of the above? To me it felt like a super obvious change, but I struggle to understand why the other two slicers have not implemented that. Let's not talk about prusa as they don't implement gap fill at all from what I see in the code and slicing outputs (I may be wrong here).

So i would appreciate some feedback on the above :)

Screenshots:
Before:
image
After:
Gap fill applied to monotonic
image

Before:
image
After: No change on the monotonic lines
image

Before:
image
After: Filled concentric. This I like the most as I use it all the time with organic models and was so frustrated by the gaps!
image

More:
(none of these have gap fill enabled in BBS/Orca)
Filled rectilinear:
image
Filled aligned rectilinear
image
Filled Archimedean chords
image
Filled octagram
image

Bottom surfaces are the same and so is internal solid infill.

fixes: #2680

@Eldenroot
Copy link
Contributor

Eldenroot commented Jan 1, 2024

You get gap fill on all infill types - if you want it disabled the user can set filter small gaps value to an appropriate number
The above is a benefit, especially for models with high detail, like lettering / svg's etc as well as small features. Also helps with water tightness as solid infills have less gaps.
Right now, I have not enabled an option tο selectively enable it on top/bottom and / or internal solid infill. I can see a case where this may be a desired option to speed up printing while maintaining "full" top and bottom surfaces. I have added an if statement that checks surface type in the code, so can implement a checkbox in the top/bottom shells and infill categories asking the user whether they want to "Enable gap fill"

Well, yeah, I noticed "gaps" sometimes on my prints, so this could be a solution.

I would like it to have it as an option with custom settings - selectively enabled on top/bottom and/or internal solid infill. This could be a really nice independent integration - if you need it, you can enable it and customize it on your own. §

@Ocraftyone
Copy link
Contributor

Only reason I could think for not having it enabled would be if it caused blemishes, but I also haven't gotten too deep into the actual slicing code other than merging other's changes.

@igiannakas
Copy link
Contributor Author

Cool, I'll add the two check boxes and lets take it from there :)

@qwewer0
Copy link

qwewer0 commented Jan 1, 2024

Only reason I could think for not having it enabled would be if it caused blemishes, but I also haven't gotten too deep into the actual slicing code other than merging other's changes.

I agree, I always try to minimize the use of gap fill to reduce small extrusions and retractions, as it tends to result in a worse surface finish on my prints.

And too much retraction in a set amount of time causes clogging in my hotend, but that is a different issue... :/

@igiannakas
Copy link
Contributor Author

igiannakas commented Jan 1, 2024

Done - PR updated with 2x tickboxes added to enable gap fill for the corresponding top/bottom and internal solid infill areas.

I've set gap fill for top and bottom surfaces as enabled by default (as this results in a better finish with no gaps) and the internal solid infill as turned off (as this saves time). This is now my default setup on my print profiles.

@igiannakas igiannakas changed the title ENH: Enabled gap fill algorithm for all solid fill types [Feature] Enabled gap fill algorithm for all solid fill types Jan 12, 2024
@igiannakas
Copy link
Contributor Author

@SoftFever I've implemented the enum. I've set it to apply gap fill everywhere as the default option, which is consistent with current slicer behaviour. I've also added the option to apply gap fill to top/bottom only and nowhere (which disables gap fill). When nowhere is selected, the filter out tiny gaps option is hidden as no gap fill is applied.

Ready for review.

@SoftFever
Copy link
Owner

@SoftFever I've implemented the enum. I've set it to apply gap fill everywhere as the default option, which is consistent with current slicer behaviour. I've also added the option to apply gap fill to top/bottom only and nowhere (which disables gap fill). When nowhere is selected, the filter out tiny gaps option is hidden as no gap fill is applied.

Ready for review.

Awesome!
Looks all good now.

Could you take a look at the conflicts? it's ready to merge once the conflicts is solved.

@igiannakas
Copy link
Contributor Author

@SoftFever I've implemented the enum. I've set it to apply gap fill everywhere as the default option, which is consistent with current slicer behaviour. I've also added the option to apply gap fill to top/bottom only and nowhere (which disables gap fill). When nowhere is selected, the filter out tiny gaps option is hidden as no gap fill is applied.
Ready for review.

Awesome! Looks all good now.

Could you take a look at the conflicts? it's ready to merge once the conflicts is solved.

Done ;)

@SoftFever SoftFever merged commit fe14851 into SoftFever:main Jan 18, 2024
12 checks passed
@igiannakas igiannakas deleted the pr-gap-fill-enablement-for-all-infill-types branch January 18, 2024 14:55
@Festivejelly
Copy link

@igiannakas Is this working as expected? When I disable the gap infill for everywhere I still see the gap infill being added

image

@igiannakas
Copy link
Contributor Author

@igiannakas Is this working as expected? When I disable the gap infill for everywhere I still see the gap infill being added

image

What you’re seeing is wall gap fill. This is because of the classic wall generator. This option controls gap fill for infill and solid top and bottom surfaces

@Festivejelly
Copy link

Ahh I see. The problem now is that I cant actually disable the gap infill on classic because the speed doesnt allow a non zero value which is how I disabled it before.

@halofx
Copy link

halofx commented Mar 18, 2024

So can Wall Gap Fill not be disabled? I would love a checkbox to be displayed when Wall Generator is Classic for Gap Fill, and when unchecked, all Gap Fill is disabled, regardless of where it is used. Then enable options for other various types when it is checked. Gap fill causes way more trouble than it ever solves for me.

@igiannakas
Copy link
Contributor Author

So can Wall Gap Fill not be disabled? I would love a checkbox to be displayed when Wall Generator is Classic for Gap Fill, and when unchecked, all Gap Fill is disabled, regardless of where it is used. Then enable options for other various types when it is checked. Gap fill causes way more trouble than it ever solves for me.

What you can do is set the gap fill threshold to a value that is really high , say 9999 and it will disable all gap fill at the walls. But I don’t know why you’d want that with classic mode as you’ll end up with gaps between your walls.

@halofx
Copy link

halofx commented Mar 19, 2024

So apply gap fill, and then set the filter out tiny gaps high, to eliminate gap fill. It works, but highly counter intuitive.

As for why, I have 2 parts of a box held together with magnets, the walls are only a few mm thick, and have indents to hold magnets, where the wall is only just over a mm thick. Arcachne swells the line, and shows a blemish on the outside of the part. Gap fill in the same area also can leave a blemish, But leaving a small gap between the walls in that area does not. Normally I would just use PrusaSlicer, but I am playing with scarf seams, which are perfect for these parts that don't have sharp corners.
gap
fill
arachne

@igiannakas
Copy link
Contributor Author

It is counter intuitive because the feature is not really designed to disable gap fill on walls. This can change if there is enough of a use case - the current code in classic perimeter generation only checks for gap fill length and filters the gap fill based on that. It could also check for whether the user desires it as enabled - there is nothing limiting this.

But on the other hand adding more features like this risks novice users or ones that don’t understand what this feature does to create slicing conditions where wall gaps are created, which is undesirable. For example try slicing a benchy with 99999 filter gap fill and you’ll see what I mean 😢

Also in most cases there is an alternative way to solving this problem.

For example, in your case this gap fill could be eliminated by changing the external perimeter line width to be slightly larger (say 0.45 instead of 0.42). Then you wouldn’t create gap fill in that area as it wouldn’t be needed and you would also get properly bonded walls with a consistent finish instead of using Arachne.

@Festivejelly
Copy link

The problem though is that the ability to remove gap fill altogether was essentially removed to make way for these changes. Previously we could set gap fill speed to 0. Its the only reason im still using bambu slicer. Sure there are lots of different workarounds we could try but ultimately people just want to be able to disable gap infill and they really should be able to.

@igiannakas
Copy link
Contributor Author

The problem though is that the ability to remove gap fill altogether was essentially removed to make way for these changes. Previously we could set gap fill speed to 0. Its the only reason im still using bambu slicer. Sure there are lots of different workarounds we could try but ultimately people just want to be able to disable gap infill and they really should be able to.

You’re replacing one workaround (setting speed to 0) with another (filter gap to 9999). How is that not acceptable? 🤔🤔

setting the gap fill speed to 0 caused other issues (over extrusion in overlapping lines in sharp corners) so not recommended in bambu slicer either.

@Festivejelly
Copy link

The problem though is that the ability to remove gap fill altogether was essentially removed to make way for these changes. Previously we could set gap fill speed to 0. Its the only reason im still using bambu slicer. Sure there are lots of different workarounds we could try but ultimately people just want to be able to disable gap infill and they really should be able to.

You’re replacing one workaround (setting speed to 0) with another (filter gap to 9999). How is that not acceptable? 🤔🤔

setting the gap fill speed to 0 caused other issues (over extrusion in overlapping lines in sharp corners) so not recommended in bambu slicer either.

Fair point, I didnt consider the downsides. Ultimately I think a checkbox would be best but as you already said probably outside of the scope of this change. I'll give it a try setting filter gap to 9999 thanks.

@igiannakas
Copy link
Contributor Author

The problem though is that the ability to remove gap fill altogether was essentially removed to make way for these changes. Previously we could set gap fill speed to 0. Its the only reason im still using bambu slicer. Sure there are lots of different workarounds we could try but ultimately people just want to be able to disable gap infill and they really should be able to.

You’re replacing one workaround (setting speed to 0) with another (filter gap to 9999). How is that not acceptable? 🤔🤔
setting the gap fill speed to 0 caused other issues (over extrusion in overlapping lines in sharp corners) so not recommended in bambu slicer either.

Fair point, I didnt consider the downsides. Ultimately I think a checkbox would be best but as you already said probably outside of the scope of this change. I'll give it a try setting filter gap to 9999 thanks.

I may implement this as a PR at some point probably for v2.1 but need to also show a warning to the user when enabling this as it can bork some models quite badly (and I’ve seen some weird print profiles recently beyond belief :))

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

Successfully merging this pull request may close these issues.

Unable to disable Gap Fill
7 participants