Account for skirt when arranging parts on bed

[jschuh]

This PR fixes issue #3477 by subtracting the skirt region from the build plate area when arranging or filling the bed. The file arrange_test_better.3mf in arrange_tests.zip demonstrates where this patch performs better by removing a model that cannot fit on the bed without pushing the skirt outside the printable area. You can test this by arranging and then slicing the file with patched and unpatched versions to see the difference.

This approach has the following tradeoffs:

1. Because the arrange algorithm uses 2D projections, it can lose some space for parts that would overhang but not intersect the skirt area in 3D space. The file arrange_test_worse.3mf in arrange_tests.zip demonstrates a case where the old approach can fit all the parts on the bed, but this new approach unnecessarily removes a part. This hasn't been a problem for my use, and I've found the tradeoff to be quite a bit better than the current behavior, but other people may differ on that call.
2. The skirt can still run outside the printable area if the minimum length setting requires additional loops for multiple extruders. This isn't a change from the current behavior, but it seems worth mentioning that this change doesn't fix that problem.

I've been using this for a bit and haven't run into any issues. That stated, even after several hours of reading through the arrange code I can't say I know it well enough to be confident that there isn't a better approach to solving this.

[lukasmatena]

@jschuh Thanks for the pull request. This is not a perfect solution to the problem (does not account for brim and supports for example), but much better than nothing. It is a good idea to do it. However,

bedpts = offset(Polygon(bedpts), scaled(-skirt_inset)).front().points; will crash for large values of skirt_inset. offset will eat the whole polygon, return an empty vector and the .front()...

I'm not entirely sure if we need the compensation in bed_stride:

double bed_stride(const Plater *plater) {
    double bedwidth = plater->build_volume().bounding_volume().size().x() - 2 * get_skirt_inset(plater);
    return scaled<double>((1. + LOGICAL_BED_GAP) * bedwidth);
}

It might make more sense to use the unaltered value. It would also remove the bad feeling I have about what might happen if bedwidth ends up negative. What is the reason for the change here?

[jschuh]

Thanks @lukasmatena, I'm happy to fix the issues below and post an updated CL in the next few days. (It would be sooner, but it's a busy week for me and I don't anticipate having any time in front of my dev box until the weekend.)

@jschuh Thanks for the pull request. This is not a perfect solution to the problem (does not account for brim and supports for example), but much better than nothing. It is a good idea to do it. However,

Yeah, forgot to mention the brim and supports, but it's another case where it doesn't change the status quo.

bedpts = offset(Polygon(bedpts), scaled(-skirt_inset)).front().points; will crash for large values of skirt_inset. offset will eat the whole polygon, return an empty vector and the .front()...

That is deeply embarrassing on my part. It's an easy fix, and I certainly should know better than to index a vector without checking it's size first.

I'm not entirely sure if we need the compensation in bed_stride:

double bed_stride(const Plater *plater) {
    double bedwidth = plater->build_volume().bounding_volume().size().x() - 2 * get_skirt_inset(plater);
    return scaled<double>((1. + LOGICAL_BED_GAP) * bedwidth);
}

It might make more sense to use the unaltered value. It would also remove the bad feeling I have about what might happen if bedwidth ends up negative. What is the reason for the change here?

I simply didn't know the code well enough and assumed that the stride should match, even though I never saw a difference in my own testing without this. Since you don't see it as necessary, I'll just remove it.

[jschuh]

@lukasmatena, I just updated the PR to address the issues you identified.

[bubnikv]

Hello Justin.

Thanks for your contribution. We were considering it for 2.4.1, but then we postponed it to 2.5.0. We would rather use the new BuildVolume for getting an analytic shrunk circular bed definition to limit the numerical issues of arranging with an imprecise shape and then getting collisions from the collision check.

We also identified a thread safety bug in our code where plater->config() is called from a worker thread while it could very rarely be modified by the main thread. Also plater->fff_print() is data that the back-end thread works on. Some of the fff_print() data is assigned by the main thread, while other is assigned by the backend thread. It is not clearly structured / documented which data is changed by which thread, the architecture may be better. Thus it may be safter to calculate the skirt width from the configuration data, which is the source to the configuration data of the back end.

@tamasmeszaros will integrate your contribution into 2.5.0 together with the BuildVolume refactoring and we will also strive to integrate the brim & support expansion into the arrangement.

[jschuh]

@bubnikv, thanks for all the context. Would you like me to take a crack at any of the changes you mentioned, or is at all being actively worked on by other people?

[IllicLanthresh]

@bubnikv Hi, I've been following this PR and I see that the issue is still present in 2.5.0, is that intended, or was this forgotten?

[jschuh]

Closing this because 2c64312 added a parameter that allows manually specifying a distance from the edge of the bed.