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

Bug fix: Avoid crossing walls feature removes some retraction wipes #6518

Conversation

igiannakas
Copy link
Contributor

@igiannakas igiannakas commented Aug 20, 2024

Description

Avoid crossing walls was erasing the wipe paths even though a retraction was performed as part of the travel move on some occasions.

That is inconsistent with the existing approach when avoid crossing walls is disabled (where a retraction move always has a wipe if paths are available and the user has enabled it in the extruder settings/filament settings).

Fixes #6266

Screenshots/Recordings/Graphs

Before:
image

After:
image

Avoid crossing walls disabled:
image

Tests

Tested with avoid crossing walls - wipe happens with retractions as expected
Disabled wipe in the extruder settings - wipe is also disabled when avoid crossing walls
Remaining logic is untouched so disabling avoid crossing walls retains existing behaviour as the code path is unchanged

@discip
Copy link
Contributor

discip commented Aug 20, 2024

Yes it stops removing retraction wipes, but it unfortunately also removes retracts. 😅

no-retract

@igiannakas
Copy link
Contributor Author

Yes it stops removing retraction wipes, but it unfortunately also removes retracts. 😅

no-retract no-retract

Post me the project file? It doesn’t make sense this is happening 🤔🤨

@igiannakas
Copy link
Contributor Author

igiannakas commented Aug 21, 2024

Can you also try with avoid crossing walls off? I have a suspicion that the retraction move is finishing during the wipe hence it’s not visible in the gcode viewer as there is no need to retract and the start of the wipe.

if the behaviour with avoid crossing walls enabled and disabled is identical, it’s working as expected

This may be happening because the wipe distance is long enough to allow the retraction to finish while wiping. In my screenshot test in the first post the retractions are happening, so I believe it’s just the intended behaviour.

post the project file when you have a chance and I’ll double check it.

@discip
Copy link
Contributor

discip commented Aug 21, 2024

@igiannakas
Will do, as soon as I'm home again. 😊

@igiannakas
Copy link
Contributor Author

Thank you :)

Looks like it is working as expected on my side.

Tested with the below settings and indeed a retraction is not shown visually in the gcode preview, but its done as part of the wipe move as expected.

image

vs. setting a smaller wipe distance and larger retraction requirement forces a retraction to happen before the wipe operation as the wipe move is not long enough to finish the retraction while wiping.

image

All of the above are with avoid crossing walls enabled.

@bernarden
Copy link

@igiannakas The issue seems to be fixed on the retraction test. (2.1.1 on the left side and PR build on the right side)
retraction_test

I wonder what is going to be an impact on models/situations that don't have enough space to do a full wipe but now we are trying to do at least some of it. For example, orca tolerance test around Test word:
tolerance_test

I also tried to look for any regressions around generated travel paths, but they seem to only differ in a few places due to additions of wipe operations:
tolerance_test_travel

@igiannakas
Copy link
Contributor Author

There isn't an issue if the wipe path is smaller than the available path - in that case it will retract faster to complete the wipe within the available distance. If however, the faster retraction exceeds the retraction speed, it will retract the excess before the wipe and then retract and wipe with the rest.

Basically what wipe does today when avoid crossing perimeters is disabled.

@discip
Copy link
Contributor

discip commented Aug 21, 2024

@igiannakas
After reading your thorough explanation and seeing the tests, I'm pretty sure this is good to be merged in! 😃👍

Thank you very much!

@igiannakas
Copy link
Contributor Author

@igiannakas After reading your thorough explanation and seeing the tests, I'm pretty sure this is good to be merged in! 😃👍

Thank you very much!

Please do post the project file just in case, I can check and confirm..

@discip
Copy link
Contributor

discip commented Aug 21, 2024

Of course, here you go:
cephalopod.3mf.txt

@igiannakas
Copy link
Contributor Author

Of course, here you go: cephalopod.3mf.txt

Yeap working as expected
image
(you can see the retraction during wipe in the gcode preview :))
With avoid crossing walls disabled, again no retractions as expected
image

If I force it to use higher speeds, meaning there is not enough time to retract while respecting your retraction speed
image

So looks good all in all!

@discip
Copy link
Contributor

discip commented Aug 21, 2024

Thank you for the investigation! 👍🏻

igiannakas and others added 2 commits August 24, 2024 00:45
…ing-walls-removing-wipe-moves-when-retraction-was-happening
m_wipe.reset_path();
// ORCA: Fix scenario where wipe is disabled when avoid crossing perimeters was enabled even though a retraction move was performed.
// This replicates the existing behaviour of always wiping when retracting
/*if (m_config.reduce_crossing_wall && could_be_wipe_disabled)
Copy link
Owner

@SoftFever SoftFever Aug 26, 2024

Choose a reason for hiding this comment

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

Any idea why the static bool need_wipe(...) function give wrong result?
Ideally that function should only disable wipe if and only if the coming travel is inside on island to save print time.

@igiannakas
Copy link
Contributor Author

Honestly, no idea. I did a code diff between orca and prusa and the functions in avoid crossing walls are identical (bar formatting and definition deltas). So I’m puzzled.

however I think orca handles wipe differently - if you do have a retraction, when avoid crossing walls is off, it always wipes. So removing this code segment just makes it identical, when avoid crossing walls is on. I think at least…

I can do a bit more digging but I’m not sure where the fault lies - also it’s gotten worse over the versions but that makes absolutely no sense :S as the code there has been untouched.

Copy link
Owner

@SoftFever SoftFever left a comment

Choose a reason for hiding this comment

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

Honestly, no idea. I did a code diff between orca and prusa and the functions in avoid crossing walls are identical (bar formatting and definition deltas). So I’m puzzled.

however I think orca handles wipe differently - if you do have a retraction, when avoid crossing walls is off, it always wipes. So removing this code segment just makes it identical, when avoid crossing walls is on. I think at least…

I can do a bit more digging but I’m not sure where the fault lies - also it’s gotten worse over the versions but that makes absolutely no sense :S as the code there has been untouched.

I see.
No worries, we can live with this workaround ;)
The root cause might from somewhere else but for this specific issue, I don't foresee any issues.

@SoftFever SoftFever merged commit 1ff5424 into SoftFever:main Aug 27, 2024
15 checks passed
@igiannakas igiannakas deleted the Bug-Fix-Avoid-crossing-walls-removing-wipe-moves-when-retraction-was-happening branch August 28, 2024 15:19
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.

Avoid crossing walls feature removes some retraction wipes
4 participants