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-12352 different minimum layer time for layers that contain overhangs #2187

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

wawanbreton
Copy link
Contributor

@wawanbreton wawanbreton commented Dec 16, 2024

CURA-12352

⚠️ Depends on CURA-11966, so don't review until merged

Requires Ultimaker/Cura#20039

Copy link
Contributor

github-actions bot commented Dec 16, 2024

Test Results

27 tests  ±0   27 ✅ ±0   5s ⏱️ ±0s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit c468d03. ± Comparison against base commit a7e6fa0.

♻️ This comment has been updated with latest results.

@wawanbreton wawanbreton changed the title Cura 12352 different minimum layer time for layers that contain overhangs CURA-12352 different minimum layer time for layers that contain overhangs Dec 16, 2024
wawanbreton and others added 18 commits January 9, 2025 12:05
…ang' into CURA-12352_different-minimum-layer-time-for-layers-that-contain-overhangs
…er-time-for-layers-that-contain-overhangs' into CURA-12352_different-minimum-layer-time-for-layers-that-contain-overhangs
…fferent-minimum-layer-time-for-layers-that-contain-overhangs
Even if the overhang wall speed is set to 100%, and thus wouldn't normally change, the minimum layer time for overhanging walls (split off from the minimum layer time, which is the whole point of this ticket) still should take effect -- which we can't detect if the merged regions are just one big everything blob.

part of CURA-12352
Since the speed regions can 'too' small as well, we need to make sure we don't create microsegments -- While there was already a mitigation earlier, Ihad to partially undo that to fix a bug, and now it's done in a more guaranteed, explicit way.

part of CURA-12352
@wawanbreton wawanbreton marked this pull request as ready for review January 20, 2025 09:45
Copy link
Contributor Author

@wawanbreton wawanbreton left a comment

Choose a reason for hiding this comment

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

Micro-segment filtering is a good addition, although I have doubts on the second part. We should take some time to discuss it.

| ranges::views::chunk_by(
[](const auto& region_a, const auto& region_b)
{
return region_a.chunk && region_b.chunk && region_a.speed_factor == region_b.speed_factor;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sure there is a good reason for not merging the internal area, but I would be curious to know it 😃

}

// (Also) Avoid microsegments: Filter out pairs of intersections that are too close to each other.
// This should be possible because the intersections happen in the same region.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really understand why this should work. According to me, it shouldn't because in the next step, you always assume that you are moving from a region to an adjacent one. But not you filter our intersections, which means you can move from a region to any other.

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