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-11978 Retract and unretract in a travel #2204

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

Conversation

wawanbreton
Copy link
Contributor

@wawanbreton wawanbreton commented Feb 5, 2025

Applies retraction and priming during travel move, partially or completely according to the given settings.

Most calculations are done according to durations, because we want to respect both the travel speed and the retraction/prime speed. If we don't have enough time to process the retraction/prime during travel, we will make a stationary retraction/prime irregardless of the setting, because there is no other way to respect the speeds.

Requires Ultimaker/Cura#20240

CURA-11978

Copy link
Contributor

github-actions bot commented Feb 5, 2025

Test Results

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

Results for commit cfdf96b. ± Comparison against base commit eb0d2d7.

♻️ This comment has been updated with latest results.

Copy link
Member

@jellespijker jellespijker left a comment

Choose a reason for hiding this comment

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

Overal it looks good and well thought out, a couple of minor remarks and suggestions. But I do suggest adding some UT
given the complexity of splitting travel moves into multiple phases, maybe add unit and integration tests that cover:

  • A travel move with enough duration for all retraction/priming to happen during travel.
  • A travel move where extra stationary time is required.
  • Edge cases with only one travel segment.
  • Both “forward” (retraction) and “reversed” (priming) computations.

Copy link
Member

@jellespijker jellespijker left a comment

Choose a reason for hiding this comment

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

LGTM

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