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

Add fan speed visualization in preview #2922

Closed
wants to merge 11 commits into from

Conversation

jschuh
Copy link
Contributor

@jschuh jschuh commented Sep 15, 2019

This adds an option to preview fan speed as requested in issue #1491.
I needed this to verify that patch #2921 generates correct output.

This adds an option to preview fan speed as requested in issue prusa3d#1491.
I needed this to verify that patch prusa3d#2781 generates correct output.
@jschuh
Copy link
Contributor Author

jschuh commented Sep 15, 2019

Checkout was in a messed up state when I uploaded this, so I have a to fix a few things and then I'll re-post it.

@jschuh jschuh closed this Sep 15, 2019
Removed some default arguments for Metadata and GCodeMove constructors to prevent future errors.
@jschuh jschuh reopened this Sep 15, 2019
@jschuh
Copy link
Contributor Author

jschuh commented Sep 15, 2019

Seems correct now. Sorry for the noise. I'm just learning Github's workflow (and am far from an expert on git in general).

Makes M106 and M107 consistent if a P parameter is present.
This could still provide an incorrect visualization if the fan that's changed is not the current tool, but we later switch to it. However, that seems worth ignoring to keep the code simple for now.
This adds an option to preview fan speed as requested in issue prusa3d#1491.
I needed this to verify that patch prusa3d#2781 generates correct output.
Removed some default arguments for Metadata and GCodeMove constructors to prevent future errors.
Makes M106 and M107 consistent if a P parameter is present.
This could still provide an incorrect visualization if the fan that's changed is not the current tool, but we later switch to it. However, that seems worth ignoring to keep the code simple for now.
enricoturri1966 added a commit that referenced this pull request Sep 23, 2019
@bubnikv
Copy link
Collaborator

bubnikv commented Sep 30, 2019

thanks, merged into master.
I may refactor the fan speed and color change ID away from ExtrusionPath into either a separate structure just for the final G-code preview, or for the same, but derived from ExtrusionPath.

@bubnikv bubnikv closed this Sep 30, 2019
@jschuh jschuh deleted the view_fan_speed branch September 30, 2019 17:15
@jschuh
Copy link
Contributor Author

jschuh commented Sep 30, 2019

Thanks, glad it was useful.

@rongith
Copy link
Contributor

rongith commented Mar 13, 2020

@jschuh, in the function void GCodeAnalyzer::_processM106(const GCodeReader::GCodeLine& line) I would set
_set_fan_speed((100.0f / 255.0f) * new_fan_speed);
then 100% fan speed is 100 and not 99.609375

@jschuh
Copy link
Contributor Author

jschuh commented Apr 25, 2020

Sorry, been busy with other things for the last few months. Answer inline...

@jschuh, in the function void GCodeAnalyzer::_processM106(const GCodeReader::GCodeLine& line) I would set
_set_fan_speed((100.0f / 255.0f) * new_fan_speed);
then 100% fan speed is 100 and not 99.609375

Ah, that is a bug. The divisor should be 255 rather than 256, otherwise the percentage is off by a small fraction. However, the float promotion is implicit, so you don't need to change the type of the divisor (unless you just think it's clearer code).

Unfortunately, I don't have a functional checkout right now, but it's a very simple patch if you want to submit it as a PR. Otherwise, I'll circle back to it once I get a chance.

@rongith
Copy link
Contributor

rongith commented Apr 26, 2020

@jschuh, I just changed a 6 to a 5 and Prusa already committed it. Perhaps someone with extra super high color sensitivity could have been annoyed.... It is more about perfection.

@jschuh
Copy link
Contributor Author

jschuh commented Apr 27, 2020

@rongith, that's awesome. Thanks for getting it fixed.

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