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

CURA-12324 random seam is not so random #2175

Merged
merged 4 commits into from
Nov 29, 2024

Conversation

wawanbreton
Copy link
Contributor

When using a random seam position, the scoring algorithm would still use the fallback criteria, which are useful for the "sharpest corner" strategy. We now don't use them, so that a really random point is always returned.
Also fixed a second bias where the best candidate could also not be returned to the profit of a nearly-best one.

CURA-12324

CURA-12324
When using random and shortest seam strategy, we don't care at all about the seam being aligned vertically, thus skip the fallback criterion which have a tendency of placing the seam at a certain position, which in case of random is really unexpected.
CURA-12324
When processing the last criterion pass, we used to keep all the best candidates in regard to the scoring threshold, then the first one of them would be returned, which is not always the one with the actual best score. Now we just early-out with the best candidate in case of the last pass. This is also more efficient.
Copy link
Member

@rburema rburema left a comment

Choose a reason for hiding this comment

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

Code looks good, though I have a couple of observations. I'll also need to look at why the unit-tests ❌ are failing (seems specifically the gcode export, but doesn't give much details beyond that), so I can't approve yet.

src/utils/scoring/BestElementFinder.cpp Show resolved Hide resolved
src/utils/scoring/BestElementFinder.cpp Outdated Show resolved Hide resolved
include/PathOrderOptimizer.h Show resolved Hide resolved
Copy link
Member

@rburema rburema left a comment

Choose a reason for hiding this comment

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

Turns out the unit-test errors aren't this PR's doings, but some other thing merged into main recently (see here).

Also the component test looks good, so approved!

@HellAholic HellAholic merged commit 1812a2e into main Nov 29, 2024
20 checks passed
@HellAholic HellAholic deleted the CURA-12324_random-seam-is-not-so-random branch November 29, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants