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

Reduce one top perimeter offset #6086

Closed
wants to merge 1 commit into from

Conversation

vovodroid
Copy link
Contributor

@vovodroid vovodroid commented Jul 10, 2024

Reduce offset of internal perimeters inward for one top perimeter. Current value seems to be too large causing excessive shift and artifacts. 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 **

Current value:
image
image

New value:
image
image

Both Arache and Classib are affected.

Obviously this value could be configurable, though it seems this value is good enough.

@igiannakas
Copy link
Contributor

I think I had experimented with this a while ago but I found that the offset was needed at least with my printer to “tuck in” the top solid infill under the walls of the next layer. What has your experience been? Admittedly it was before implementing the independent top and bottom overlap % though.

@vovodroid
Copy link
Contributor Author

With suggested offset it's almost "tucked in"

image

Not prefect, but I guess it's better than having completely broken perimeter. As I mentioned this coefficient can be configurable, or rather offset formula could be improved in general. For now I prefer reduced offset.

@igiannakas
Copy link
Contributor

Maybe 1x would cover the delta nicely?

Need to try the PR with more complex models with circular tops and also some of the Voron parts like the stealth burner cover that didn’t like one wall top surface before…

@vovodroid
Copy link
Contributor Author

vovodroid commented Jul 11, 2024

Maybe 1x would cover the delta nicely?

Yes, but it still breaks the round model from screenshot round.zip

@igiannakas
Copy link
Contributor

igiannakas commented Jul 11, 2024

Maybe 1x would cover the delta nicely?

Yes, but it still breaks the round model from screenshot round.zip

How does prusas implementation behave? Maybe we can port that over if it’s better?

Edit: as in I think they have rewritten the feature

@vovodroid
Copy link
Contributor Author

I think they have rewritten the feature

I guess they based on my PR of potting from Orca))) Bamboo also works good.

@igiannakas
Copy link
Contributor

OK I've done a bit of testing on this and I think it's ok to be a bit inset. By the way I've also seen this aberration with the feature enabled - 0.16LH - 0.25 first layer height, layer 199.

image

Have you ever seen this before? Project file below:

one wall issue.3mf.zip

@igiannakas
Copy link
Contributor

igiannakas commented Jul 18, 2024

Unfortunately it is end up causing ever so slight perimeter gaps as the overlap of the top surface is affected by it :(

left after - right before
IMG_4140
Bottom after, top before
IMG_4141
The prusa implementation looks a bit different - haven’t had time to dissect it but it may be worth doing so

@vovodroid
Copy link
Contributor Author

Here is layer 199:

image

What's wrong here?

I think they have rewritten the feature

Yes, I took a look on they code - it's different.

@igiannakas
Copy link
Contributor

Here is layer 199:

image

What's wrong here?

I think they have rewritten the feature

Yes, I took a look on they code - it's different.

It’s generating solid infill instead of the walls. That messes with wall ordering algos like IoI

@vovodroid
Copy link
Contributor Author

It’s generating solid infill instead of the walls. That messes with wall ordering algos like IoI

I'll try to port code from Prusa.

@vovodroid
Copy link
Contributor Author

@igiannakas

Here is result of porting Prusa algorithm:

image

Your benchy:

image

As a side effect one top wall is always enabled. I hope it could be fixed)))

@igiannakas
Copy link
Contributor

Looks good I think?

@vovodroid
Copy link
Contributor Author

vovodroid commented Jul 22, 2024

@igiannakas
would you like to test https://github.com/vovodroid/OrcaSlicer/tree/one-top-wall-fix ?

Changes:

  1. Port Arachne code from Prusa
  2. Set top perimeter offset for Classic to 0.9. It seems producing good results.

Option "One top wall" also works.

@igiannakas
Copy link
Contributor

Would you mind pushing the changes to this PR so I can cherry pick it into my Dev branch to test together with all the features I've pushed recently too?

BTW this looks excellent!

@vovodroid
Copy link
Contributor Author

Would you mind pushing the changes to this PR

It's another branch with other meaning. I think you can create PR to yourself and merge to yours.

@igiannakas
Copy link
Contributor

Ah yes indeed! Merged in the dev branch. Running a build now and will test. Doesnt this supersede this PR or am I misunderstanding it?

@igiannakas
Copy link
Contributor

Ok tested - it works but it breaks the top/bottom solid infill - wall overlap for surfaces that are not the topmost/bottom most. It only changes overlap based on the infill/wall overlap value for any intermediate top surfaces.

@vovodroid
Copy link
Contributor Author

Could you provide an example project?

@vovodroid
Copy link
Contributor Author

Doesnt this supersede this PR

Yes, it's new different code.

@igiannakas
Copy link
Contributor

Could you provide an example project?

Yeap

How its supposed to work (exaggerated for effect)
image

vs

image
Only top most surface adjusted

Project file: Overlap.3mf.zip

@igiannakas
Copy link
Contributor

I think its because this piece of code is missing:

        if (!top_fills.empty()) {
            infill_exp = union_ex(infill_exp, offset_ex(top_fills, double(top_inset)));
        }

But without delving deeper in the code I'm not sure where the new top surface polygons are now held

@vovodroid
Copy link
Contributor Author

vovodroid commented Jul 22, 2024

Yes, I see.

because this piece of code is missing:

top_fills was filled in deleted call to split_surfaces, I'll restore it for this purpose and test. Or will try to find polygons in new code))

@vovodroid
Copy link
Contributor Author

Ok, I found top poligons in Prusa code

image

New commit added to the same branch. If it seems to be good I'll create new PR.

@vovodroid vovodroid marked this pull request as draft July 23, 2024 06:22
@igiannakas
Copy link
Contributor

Awesome thank you!! I’ll compile it shortly and let you know but the change seems correct so don’t see a reason it wouldn’t work :)

@igiannakas
Copy link
Contributor

igiannakas commented Jul 23, 2024

Tested - looks good!

A bit of feedback below:

  1. I dont believe the one wall threshold parameter works any more with the prusa code.

image

The below code is not evaluated (replaced with the prusa code)

    // skip if the exposed area is smaller than "min_width_top_surface"
    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 the Polygons upper the polygon this layer
    Polygons upper_polygons_series_clipped = ClipperUtils::clip_clipper_polygons_with_subject_bbox(*this->upper_slices, last_box);
    upper_polygons_series_clipped          = offset(upper_polygons_series_clipped, min_width_top_surface);

[...]

// get the not-top surface, from the "real top" but enlarged by external_infill_margin (and the
    // min_width_top_surface we removed a bit before)
    ExPolygons temp_gap = diff_ex(top_polygons, fill_clip);
    ExPolygons inner_polygons =
        diff_ex(orig_polygons,
                offset_ex(top_polygons, offset_top_surface + min_width_top_surface - double(ext_perimeter_spacing / 2.)),
                ApplySafetyOffset::Yes);

There is a question whether this is needed any more as I've noticed that this approach is far less sensitive to errors with text embossed on top surfaces
(left old, right new)
image

  1. However I did notice that Prusa's approach will result in nozzle collisions over thin wall surfaces, like letters

image

Notice that the outline is drawn with an internal perimeter but the top surface doesnt segment around it. This will result in a nozzle collision over the letter.

image

Prusa slicer has the exact same problem:

image

Whereas with the old version there were artefacts (tiny holes) but no nozzle collisions.
(0% one wall threshold)
image
(300% one wall threshold)
image

I suspect this is happening due to the difference in shrinking/expanding the polygons between the prusa approach and the old Orca approach.

Project file: bbb.3mf.zip

  1. However the prusa approach (and the port to orca) fixes another long standing issue, where the first internal perimeter when a one wall top surface was printed, had the wrong line width. See below (left old - right new) - look at the thickness of the first wall.

image

So in summary it's almost perfect, if only we could figure out the segmentation for the lettering issue. Everything else seems to be working better than the current implementation.

@vovodroid
Copy link
Contributor Author

Does threshold value prevent nozzle collision in current version? I could try to use it.

@igiannakas
Copy link
Contributor

Does threshold value prevent nozzle collision in current version? I could try to use it.

I think so but I’m not 100% sure

@vovodroid
Copy link
Contributor Author

New PR One top wall fix pr #6236 open.

@vovodroid vovodroid closed this Jul 24, 2024
@vovodroid vovodroid deleted the reduce-one-top-offset-pr branch July 24, 2024 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants