[CURA-13287] Re-fix random dots without introducing microsegments. - #2357
Open
Remco Burema (rburema) wants to merge 3 commits into
Open
[CURA-13287] Re-fix random dots without introducing microsegments.#2357Remco Burema (rburema) wants to merge 3 commits into
Remco Burema (rburema) wants to merge 3 commits into
Conversation
Originally, an issue (see #2089 / C-9399) was fixed, where we needed to re-insert some ignored local maxima. This fix was intended to be applied only for very small areas. Large areas didn't need to be corrected for, certainly not ones large enough to be a problem w.r.t non-convexness. (And non-connected shapes won't be treated as a single area w.r.t. SkeletalTrapezoidation.) -- We found, while doing some other work, that this could cause points to appear in the middle of nowhere (see commets in #2346 / C-13250). This was fixed by taking into account that averaged points should only be merged when close togehter. -- The real problem was however that the fix was also applied to large areas (otherwise the middle of nowhere behaviour wouldn't be possible at all). If we restrict the fix to only be for cases where the total length of the covered lines is very small, we get the originally inteded behaviour of the hack (and this is not a problem, because there can't be multiple areas within a single trapezoidation other than holes, and areas _otherwise_ large enough to contain multiple clearly separate local maxima are not the intended target for the original and now curent fix anyway). CURA-13287
I actually wanted to use an optional, but that turned out to work a bit ugly with the += operator. But then I just deleted the 'does this actually exist' check, instead of replacing it with this. Fixed. part of CURA-13287
Contributor
Test Results31 tests 31 ✅ 5s ⏱️ Results for commit bb861bc. ♻️ This comment has been updated with latest results. |
Erwan MATHIEU (wawanbreton)
approved these changes
Aug 7, 2026
Erwan MATHIEU (wawanbreton)
left a comment
Erwan MATHIEU (wawanbreton)
left a comment
Contributor
There was a problem hiding this comment.
Just a minor remark. Code looks good, but I must admit I don't get the global meaning...
| @@ -2212,11 +2212,22 @@ void SkeletalTrapezoidation::generateLocalMaximaSingleBeads() | |||
| Point2LL p_; | |||
| coord_t width_; | |||
| size_t acc_; | |||
Contributor
There was a problem hiding this comment.
I know this is not part of this PR, but could this variable be renamed with a more explicit name ? Out of the context, I don't know if this is acceleration, accumulation or something else 😄
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Originally, an issue (see #2089 / C-9399) was fixed, where we needed to re-insert some ignored local maxima.
This fix was intended to be applied only for very small areas. Large areas didn't need to be corrected for, certainly not ones large enough to be a problem w.r.t non-convexness. (And non-connected shapes won't be treated as a single area w.r.t. SkeletalTrapezoidation.)
We found, while doing some other work, that this could cause points to appear in the middle of nowhere (see comments in #2346 / C-13250). This was fixed by taking into account that averaged points should only be merged when close together.
The real problem was however that the fix was also applied to large areas (otherwise the middle of nowhere behaviour wouldn't be possible at all). If we restrict the fix to only be for cases where the total length of the covered lines is very small (as posited, but not actually implemented, in the first mentioned PR), we get the originally intended behaviour of the hack.
... and this is not a problem, because there can't be multiple areas within a single trapezoidation (other than holes), and areas otherwise large enough to contain multiple clearly separate local maxima are not the intended target for the original and now current fix anyway). (Even though the input for the trapezoidation is a Shape, not a SimpleShape ... but the outline that it receives as an input via SlicePart is, at the very least for the walls.)
P.S. I wanted the
combined_local_maxima_pointto be an optional, but that turned out to work a bit ugly with the +-operator.