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

Bugfix: Prevent overhang fan speeds from spilling into infill #11650

Closed
wants to merge 1 commit into from

Conversation

jschuh
Copy link
Contributor

@jschuh jschuh commented Nov 12, 2023

This is a small fix for #11648. There are a few different ways to fix it, but this seemed most correct in my limited testing.

@jschuh
Copy link
Contributor Author

jschuh commented Nov 13, 2023

@lukasmatena, I'm unsure who to ask about this PR fixing a regression introduced in 2.7. Godrak did the dynamic overhang work, but he seems to have moved on. The detailed explanation of the bug is in #11648.

@lukasmatena
Copy link
Collaborator

@jschuh No need to ask anyone, @hejllukas is already processing it :-)

@jschuh
Copy link
Contributor Author

jschuh commented Nov 13, 2023

@lukasmatena, Oh, great! Sorry for bugging you.

@hejllukas, Like I mentioned in the last comment of the bug report, the other way to fix this is to just have GCodeGenerator::_extrude wrap the assignment of ";_RESET_FAN_SPEED" in newlines, like it was before the regression from fa0986c. However, I felt like that would be more prone to future regressions, which is why I fixed it at the flag parsing stage. The counterargument to my approach is that fixing it in GCodeGenerator::_extrude would make the behavior identical to what it was before the regression.

FWIW, In my testing both approaches seem to produce identical results.

@hejllukas
Copy link
Collaborator

Thank you for your detailed investigation of the issue and proposed ways to fix it.

We are trying to ensure that there is just one string tag per line, and as you investigated in this case, it wasn't followed. I would rather resolve this issue by exporting just one string tag per line. To prevent possible issues in the future when someone forgets about this special case with two tags on the same line.
But also, you are right that fixing it in the parsing stage is more prone to regressions. I don't see any performance impact because most of g-code lines are without any tags, and for those lines, we have to try to parse all tags.

So, I modified your commit and appended newlines at the end of ;_BRIDGE_FAN_END and ;_EXTRUDE_END as follow:

    if (m_enable_cooling_markers)
        gcode += path_attr.role.is_bridge() ? ";_BRIDGE_FAN_END\n" : ";_EXTRUDE_END\n";

    if (dynamic_speed_and_fan_speed.second >= 0)
        gcode += ";_RESET_FAN_SPEED\n";

    this->set_last_pos(path.back().point);

So, both approaches will be applied.

I merged your PR (with the modification that I mentioned) into our private repository, so the commit will appear with the next release.
Thank you again.

@hejllukas hejllukas closed this Nov 14, 2023
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