[CURA-13024] Spike: merge thin parts of floor/roof/skin#2316
[CURA-13024] Spike: merge thin parts of floor/roof/skin#2316
Conversation
… vice-versa. Spike/Test/Work in progress/etc. CURA-13024
…e TODOs). part of spike CURA-13024
…onstant. Also take a short-cut if the value is 0. part of spike CURA-13024
Test Results31 tests 31 ✅ 5s ⏱️ Results for commit 0c83978. ♻️ This comment has been updated with latest results. |
wawanbreton
left a comment
wawanbreton
left a comment
There was a problem hiding this comment.
The global implementation is very elegant and should indeed work without edge-cases. I have a few suggestions/requests though, to be challenged of course 🙂
Most notably, rewrite mergeThinOverlap a bit to do a mostly inplace alteration of the polygons, have some extra early-out(s) and move the method to where we can re-use it later should the need arise. part of spike CURA-13024
wawanbreton
left a comment
wawanbreton
left a comment
There was a problem hiding this comment.
Just one more possible optimization, otherwise looks good !
CURA-13024 The previous method allowed re-growing a thin part over which we had previously grown with a higher precedence area. Now we don't allow growing the thin area with less precedence to try and get a natural result. The merging code also has been moved to the generateSkinRoofingFlooringFill method to do it as soon as possible and avoid using inconsistent areas throughout the code.
rburema
left a comment
rburema
left a comment
There was a problem hiding this comment.
Two very small remarks (only one of which isn't a complete nitpick). I'm passing this along to QA again now since they're just small refactors, not functional changes, in any case.
| } | ||
|
|
||
| // If necessary, remove the thin parts of the source to not allow them to grow (using the same principle as above) | ||
| const Shape source_grow_part = allow_thin_areas_grow ? source : source.intersection(source.offset(-max_dist).offset(max_dist + EPSILON)); |
There was a problem hiding this comment.
I don't think it's worth it (since it would become less readable), but if we really wanted to, we could prevent the copy of source to source_grow_parts in the case allow_thin_areas_grow is true (and instead make it a reference).
There was a problem hiding this comment.
Well, I completely agree, don't like this copy either, but I can't find a way to optimize it without making an ugly code...
| } | ||
|
|
||
| // Now calculate the actual growing area, which is the intersection of the offset source with the allowed growing area | ||
| const Shape actual_grow_area = source_grow_part.offset(max_dist).intersection(allow_grow_area).offset(EPSILON); |
There was a problem hiding this comment.
Since both source_grow_part and allow_grow_area already include EPSILON, I think you can add the epsilon to the max_dist instead here, thus avoiding the final offset operation (as those aren't the cheapest).
There was a problem hiding this comment.
Hmm, I like the idea, but I think that would defeat the purpose of this offset. If I do it before the intersection, the final shape will not go outside the allow_grow_area that it intersects with, which is required to properly make the subsequent union with the source.
3 participants
Merge roof/floor into skin or vice-versa, given a distance, where anything under this is considered 'too thin' and will be merged to the other areas when next to each other.
See frontend PR for the accompanying setting: Ultimaker/Cura#21515