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] Only one perimeter on top/first layer #10648

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mjonuschat
Copy link

This PR backports the single perimeter on top surfaces feature of SuperSlicer. A single perimeter on the top layers allows more space for the infill to show on the outer surface and results in very clean looking fills.

All credit belongs to @supermerill and the contributors to SuperSlicer.

Implements the feature request in #7986

@Matszwe02
Copy link

Great feature, I'm happy that there's a pull request already for it :) . I hope it gets PS developers team's attention and maybe they'll add it soon.

@Eldenroot
Copy link

Oh, coooool! Please merge!

@Matszwe02
Copy link

for now - quick fix. You can add a modifier to every layer that has top surface and set perimeters to 1. PrusaSlicer is actually smart enough to fill non-top perimeters with remaining ones. So only top areas get 1 perimeter.
image

@LazaroFilm
Copy link

LazaroFilm commented Jun 6, 2023

Please merge this! This is the only reason I still use SuperSlicer over PrusaSlicer, even though its development has been lagging recently.

I would also add one bottom perimeter. I know it can be done with a layer modifier but a check box would be so much easier.

@TravisWilder
Copy link

+1

1 similar comment
@Sortexx
Copy link

Sortexx commented Jun 6, 2023

+1

@kieraneglin
Copy link

Please merge this! For me, this is the only reason to use orca or superslicer over PS

@jakub874
Copy link

this and fan ramp-up time are the only reasons i still use superslicer

@scottmudge
Copy link
Contributor

scottmudge commented Jun 22, 2023

Just FYI, the implementation you copied from SuperSlicer only works on the top-most surface, not all top-infill surfaces (like @Matszwe02 shows in his screenshot).

In order to get it to work with all top-surfaces, you have to first detect the layer types, then check if the current layer type is considered top-infill or not, rather than simply if there exists no layers above it.

Unfortunately the layer type classification doesn't happen until after the perimeter generation happens, so you have to make an early call to it if the setting is enabled.

Checkout how to fix it here: ad8cd62

--

Edit: Also, a lot of that code you added to process_classic() is probably unnecessary.

@mjonuschat
Copy link
Author

I'm not sure I understand where the problem is with this, the comment from @Matszwe02 was regarding how to get this with height range modifiers for the time being (without this patch). In my experiments it does work on other top surfaces as well, e.g. like this (single perimeter where needed on layer 20, single perimeters on layer 45:

Screenshot 2023-06-23 at 18 04 42
Screenshot 2023-06-23 at 18 05 03

I'll definitely take a look at your solution to see if it's more elegant (and agreed, there is a lot of cruft copied over that can be cleaned up before merging). I'm trying to limit the amount of work put in before there is any hint of this getting considered for merging.

@mjonuschat
Copy link
Author

@scottmudge I grabbed the implementation in your fork to compare the two approaches and right now I don't see the two solutions as equivalent, to me the results from the "original" SuperSlicer code look better. But please let me know if I missed some bits in your code, I needed to find bits and pieces from different commits. Here are a few side by side comparisons how they differ. Top image is your implementation, bottom image is the SuperSlicer code in this pull request:

Layer 20
scott-layer20
susl-layer20

Layer 35
scott-layer35
susl-layer35

Layer 45
scott-layer45
susl-layer45

The file from the examples can be found here: https://github.com/VoronDesign/Voron-0/blob/Voron0.2/STLs/Electronics/Fan_Mount_Bottom_x1.stl

@vovodroid
Copy link
Contributor

Well done!

Though it would be nice to get it working for Arachne, original code also has no full Arachne support. Anyway why to gray out this option for Arachne? It still works for the topmost surface, that could be usefull:

image

@vovodroid
Copy link
Contributor

And why not also add it for the first layer?

@mjonuschat
Copy link
Author

Personally I found it more confusing to have it only work on the topmost surface for Arachne and disabling will avoid bug reports for it.
Bottom layer: I’ve scratched my own itch with it the top layers for the moment. I’m happy to clean up code and/or implement bottom layers if there’s any hint of this being considered for merging.

@Matszwe02
Copy link

Personally I found it more confusing to have it only work on the topmost surface for Arachne and disabling will avoid bug reports for it.
Bottom layer: I’ve scratched my own itch with it the top layers for the moment. I’m happy to clean up code and/or implement bottom layers if there’s any hint of this being considered for merging.

I tested that height range modifier works surprisingly well, when I set number of perimeters to 1 at every layer that contains top surface, all other (than top surface) still force to have a normal number of perimeters. Maybe the algorithm could use this functionality? The only drawback will be that the perimeter direction isn't consistent in this single perimeter mode.

@Calcousin55
Copy link

Calcousin55 commented Aug 5, 2023

Tried the implementation again and am not seeing the issue that I was seeing before with the weird slicing.

@vovodroid
Copy link
Contributor

vovodroid commented Aug 23, 2023

Hi, after update to 2.6.1-rc1 build fail with errors

PerimeterGenerator.cpp(1514,29): error C2668: 'Slic3r::union_ex': ambiguous call
to overloaded function [D:\3D\Prusa\PrusaMaster\build\src\libslic3r\libslic3r.vcxproj]
PerimeterGenerator.cpp(1664,24): error C2668: 'Slic3r::union_ex': ambiguous call
to overloaded function [D:\3D\Prusa\PrusaMaster\build\src\libslic3r\libslic3r.vcxproj]

Line numbers could be different due to unrelated code changes.

This upstream update adds

Slic3r::ExPolygons union_ex(const Slic3r::ExPolygons &subject, const Slic3r::ExPolygons &subject2);
Slic3r::ExPolygons union_ex(const Slic3r::ExPolygons &subject, const Slic3r::Polygons &subject2);
Slic3r::ExPolygons union_ex(const Slic3r::Polygons &subject, const Slic3r::ExPolygons &subject2);

so you can remove your implementation.

@mjonuschat
Copy link
Author

Tried out the implementation and see some weird slicing. This is on layer 191 and also some artifacts on 190 in the same location. Using default 0.10mm 0.4nozzle V2 preset with 40% cubic infill

@Calcousin55 I don't have the exact profile you tested with, but with my profile I can't reproduce the issue (0.1mm layer height, 40% cubic infill):

Layer 190:
Screenshot 2023-08-24 at 21 07 09

Layer 191:
Screenshot 2023-08-24 at 21 06 43

Layer 192:
Screenshot 2023-08-24 at 21 06 48

@ndanyluk
Copy link

Love this! PS team please consider merging this :)

@ramosglauco
Copy link

With luck, maybe they can implement this in 1 or 2 years. Like the crappy seams that have been broken since version 2.4

@mjonuschat
Copy link
Author

With luck, maybe they can implement this in 1 or 2 years. Like the crappy seams that have been broken since version 2.4

Please keep the discussion on topic. This is not a forum to air your gripes about development velocity or unrelated bugs.

@vovodroid
Copy link
Contributor

Here is the case when object has only one layer (e.g. gasket or washer):

image

Should the first layer be considered as a top one?

@LazaroFilm
Copy link

Here is the case when object has only one layer (e.g. gasket or washer):

image

Should the first layer be considered as a top one?

The layer is touching the baseplate. for adhesion purposes it makes more sense for it to be treated as a bottom layer than a top.

@mjonuschat
Copy link
Author

Here is the case when object has only one layer (e.g. gasket or washer):

image

Should the first layer be considered as a top one?

This is an edge case that has no clear technical solutions and if you ask 10 people you'll probably and with 11 opinions on whether to treat this a top or bottom surface. This code is a 1:1 port of the SuperSlicer implementation that has been out in the wild for years and that implementation considers it a top surface.

@vovodroid
Copy link
Contributor

vovodroid commented Sep 27, 2023

with 11 opinions on whether to treat this a top or bottom surface

Make this random each slicing)))

By a way, is it feasible to implement this PR for Arachne?

@LazaroFilm
Copy link

SuperSlicer has this with arcane

@vovodroid
Copy link
Contributor

I know, but probably it's possible to improve it.

@LazaroFilm
Copy link

Or we could release Ross as is, THEN it...

@vovodroid
Copy link
Contributor

vovodroid commented Dec 26, 2023

is it feasible to implement this PR for Arachne?

Actually yes)) Here is my porting from Orca: Port single wall top feature for Arachne from Orca #2

image

@Eldenroot
Copy link

@vovodroid - maybe you should try to contribute to Orca slicer or Super slicer Github repository... because your ideas for new features and improvements are really clever and nice! Unfortunately, here in Prusa slicer repo your PRs will never be merged and your work will not be used for enhancing of user experience :) Just my 2 cents...

@vovodroid
Copy link
Contributor

vovodroid commented Dec 26, 2023

here in Prusa slicer repo your PRs will never be merged

One my idea was taken #10298 (comment) ;-)

maybe you should try to contribute to Orca slicer or Super slicer Github repository

Well, I've started once from using SS and contributing to it, Merill even took several my PRs. But eventually SuperSlicer become very obsolete and almost died. Recently Merill returned to project, but who knows what is SS future. Orca seems to be much more alive, but it's again only one man who manage the project (correct me if I'm wrong). And Ocra also is behind Prusa, for example Text modifier was added only today, and Organic support was ported to Orca after 7 month.

So I'm not sure is it worth to learn it's code (Orca's code is based on Prusa, but quite different. SS is more close to Prusa, so I'll wait until it's updated to Prusa 2.7 and will see what happens). As a bottom line currently I use Prusa with my own additions, some porting from SS, some from Orca, and with some other PRs merged.

@vovodroid
Copy link
Contributor

Hi,
what do you think about adding one perimeter in first layer to this PR? vovodroid@b9317f3#diff-036feb3dd0eb13cc15c00a940c7d77313164fd144b5eb1dd6f345f580c0aad70

I can create PR to you for this.

@mjonuschat
Copy link
Author

mjonuschat commented Feb 19, 2024

Hi, what do you think about adding one perimeter in first layer to this PR? vovodroid@b9317f3#diff-036feb3dd0eb13cc15c00a940c7d77313164fd144b5eb1dd6f345f580c0aad70

I can create PR to you for this.

No need for a PR, I already have it in my code base (minus the three or so lines for Arachne), happy to add it to this PR if there's interest.

@vovodroid
Copy link
Contributor

vovodroid commented Feb 19, 2024 via email

@mjonuschat mjonuschat changed the title [FEATURE] Only one top perimeter [FEATURE] Only one perimeter on top/first layer Feb 21, 2024
@@ -345,6 +345,8 @@ void ConfigManipulation::toggle_print_fff_options(DynamicPrintConfig* config)
toggle_field("min_feature_size", have_arachne);
toggle_field("min_bead_width", have_arachne);
toggle_field("thin_walls", !have_arachne);

toggle_field("min_width_top_surface", have_perimeters && config->opt_bool("only_one_perimeter_top"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add only_one_perimeter_first_layer and only_one_perimeter_top to for (auto el : { "extra_perimeters","extra_perimeters_on_overhangs", "thin_walls", "overhangs", loop.

vovodroid pushed a commit to vovodroid/PrusaSlicer that referenced this pull request Mar 24, 2024
vovodroid pushed a commit to vovodroid/PrusaSlicer that referenced this pull request Mar 26, 2024
vovodroid pushed a commit to vovodroid/PrusaSlicer that referenced this pull request Mar 29, 2024
vovodroid pushed a commit to vovodroid/PrusaSlicer that referenced this pull request Apr 18, 2024
vovodroid pushed a commit to vovodroid/PrusaSlicer that referenced this pull request Apr 23, 2024
vovodroid pushed a commit to vovodroid/PrusaSlicer that referenced this pull request May 7, 2024
vovodroid pushed a commit to vovodroid/PrusaSlicer that referenced this pull request May 7, 2024
vovodroid pushed a commit to vovodroid/PrusaSlicer that referenced this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet