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

Create MultiExtColorMix.py #16176

Closed
wants to merge 10 commits into from
Closed

Create MultiExtColorMix.py #16176

wants to merge 10 commits into from

Conversation

GregValiant
Copy link
Collaborator

@GregValiant GregValiant commented Jul 16, 2023

Description

2-in-1-out and 3-in-1-out Color Mixer. Supports both Constant and Gradient mixing using M163 and M164.
This for Geeetech multi-extruders using a shared heater and nozzle. Color mixes using M163 and M164.

This is a replacement for the current ColorMix post processor that supports only 2-in-1-out hot ends. This post includes an option to "Park and Purge".

10-3 Added functionality for 4 in 1 out hot ends. Created an exit for RepRap machines (they use M567).

This fixes... OR This improves... -->

Type of change

  • [ X] Update an existing post processor.
  • [ X] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Tested on virtual multi-extruder printers.

  • [X ] Tested with virtual multi-extruder printers based on Geeetech AxxT and M models.

Test Configuration:

  • Operating System: Win10 Home and Geeetech and Ender printers

Checklist:

  • [X ] My code follows the style guidelines of this project as described in UltiMaker Meta and Cura QML best practices
  • [ X] I have read the Contribution guide
  • [ X] I have commented my code, particularly in hard-to-understand areas
  • [ X] I have uploaded any files required to test this change

2-in-1-out and 3-in-1-out Color Mixer.  Supports constant and gradient mixing using M163 and M164.
@github-actions github-actions bot added the PR: Community Contribution 👑 Community Contribution PR's label Jul 16, 2023
@casperlamboo casperlamboo self-assigned this Aug 15, 2023
Copy link
Contributor

@casperlamboo casperlamboo left a comment

Choose a reason for hiding this comment

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

Hi @GregValiant, thanks for your contribution! I would be very nice to have support for color mixing extruders in Cura. I did have some remarks though, please review and apply/discuss further. From there we can test the changes on our side.

plugins/PostProcessingPlugin/scripts/MultiExtColorMix.py Outdated Show resolved Hide resolved
"maximum_value_warning": "100",
"enabled": "T1_include and mix_style == 'gradient'"
},
"invis_setting":
Copy link
Contributor

Choose a reason for hiding this comment

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

I donnot think invis_setting is very descriptive. Since we use descriptive setting names through out the code I would change this to something like

Suggested change
"invis_setting":
"sufficient_extruder_count":

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to "enable_3rd_extruder"

Comment on lines 40 to 44
if ext_cnt > 2:
enable_invis = True
else:
enable_invis = False
self._instance.setProperty("invis_setting", "value", enable_invis)
Copy link
Contributor

Choose a reason for hiding this comment

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

can be simplified to

Suggested change
if ext_cnt > 2:
enable_invis = True
else:
enable_invis = False
self._instance.setProperty("invis_setting", "value", enable_invis)
enable_invis = ext_cnt > 2
self._instance.setProperty("invis_setting", "value", enable_invis)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

super().initialize()

ext_cnt = Application.getInstance().getGlobalContainerStack().getProperty("machine_extruder_count", "value")
if ext_cnt > 2:
Copy link
Contributor

Choose a reason for hiding this comment

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

Cura can have up to 8 extruders, what happens when this script is enabled, and a 4th or even 5th extruder is enabled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a message and exit (at line 253) if Extruder Count > 3.

if "MultiExtColorMix" in pp_name:
multi_present += 1
if multi_present == 1:
Message(text = "Multi-Extruder Color Mixer:" + "\n" + "You must have a Blending Hot End. Extruders Share Heater' and 'Extruders Share Nozzle' must be enabled in the 'Printer Settings' in Cura. Open 'MultiExtColorMix.py' in a text editor and review the opening lines for more instructions." + "\n").show()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this message is a bit technical for the average cura user. We cannot expect users to open source code and read technical documentation.

Suggested change
Message(text = "Multi-Extruder Color Mixer:" + "\n" + "You must have a Blending Hot End. Extruders Share Heater' and 'Extruders Share Nozzle' must be enabled in the 'Printer Settings' in Cura. Open 'MultiExtColorMix.py' in a text editor and review the opening lines for more instructions." + "\n").show()
Message(text = "Multi-Extruder Color Mixer:" + "\n" + "You must have a Blending Hot End. Extruders Share Heater' and 'Extruders Share Nozzle' must be enabled in the 'Printer Settings' in Cura.).show()

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally such a message might be a bit verbose if a user is already aware of this requirement. Moving such a message in a less intrusive place might be better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed.
I have a pull request in for a post processor that simply opens a PDF document with instructions for all the post processors. Aldo Hoeben commented on the fact that it "doesn't do anything" and it is true that it has no function as a post processor but it is documentation on the post processor settings that is right there for a user. We'll see how that goes.

plugins/PostProcessingPlugin/scripts/MultiExtColorMix.py Outdated Show resolved Hide resolved
if T1_include:
T1_mix_start = int(self.getSettingValueByKey("T1_mix_start"))
T1_mix_end = int(self.getSettingValueByKey("T1_mix_end"))
T1_ext_incr = ((T1_mix_start - T1_mix_end) / (layer_span - 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
T1_ext_incr = ((T1_mix_start - T1_mix_end) / (layer_span - 1))
T1_ext_incr = (T1_mix_start - T1_mix_end) / (layer_span - 1)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

plugins/PostProcessingPlugin/scripts/MultiExtColorMix.py Outdated Show resolved Hide resolved
plugins/PostProcessingPlugin/scripts/MultiExtColorMix.py Outdated Show resolved Hide resolved
plugins/PostProcessingPlugin/scripts/MultiExtColorMix.py Outdated Show resolved Hide resolved
@GregValiant
Copy link
Collaborator Author

Casper,
My laptop is in for repair and this Win7 machine won't run Git Desktop so I'm stuck for a while.
I've made the changes except for the second to last. That will take a bit.
I had thought that splitting the two functions off would be neater but as a rank amateur at Python I went with what I knew how to do.
When I get the Win10 machine back I will get to work on this. You probably noticed that I have several pull requests in for new posts and updates to others. It's my "Make Nice" effort to address some requests that have been around for years, and to add some things that I have found useful for myself.
Be patient with me. Your talking to a guy that learned programming on analog computers and whose last actual computer class was Fortran II in 1971. I don't miss punch cards at all.

@casperlamboo
Copy link
Contributor

Hi @GregValiant

I had thought that splitting the two functions off would be neater but as a rank amateur at Python I went with what I knew how to do.
No worries, you overall quality is very neat! The suggestions I wrote were purely from a code-maintainance perspective; the aspect of code we as a large code base maintainer are most strict on.

Thanks for taking a look at the suggestions! I'm unable to see them though from my side. Did you already push the changes to the PR?

@GregValiant
Copy link
Collaborator Author

I don't have access to Git Desktop as my Win10 computer is in for repair.
This Win7 computer is 32 bit so there are issues with the Python version installed. I'll figure it out.

All your suggestions have been completed. I'm just waiting to get the laptop back so I can post the changed file.

@GregValiant
Copy link
Collaborator Author

I've put up the adjusted version with your suggested changes. See what you can make of it.

Changed some verbiage.  Allow the post to run if a printer has more than three extruders with a message that only 1, 2, and 3 will become mixed.
@GregValiant
Copy link
Collaborator Author

Casper,
I believe the last commit on this PR incorporates all the changes you requested.
Let me know if you wish further changes.
I have updated my other PR's so they also reflect some of the basic changes to bring them into line. "Advanced Cooling Profile" remains a piece-of-work.

Add functionality for 4-in-one-out hot ends.
Add an exit if the firmware is RepRap (M567 is not supported).

Update MultiExtColorMix.py

Change the variable names to start with lower case letter.  Altered messages to include a title.

Update MultiExtColorMix.py

More variable name changes.  Changed some string formats to templates.

Update MultiExtColorMix.py

merge 2
@GregValiant
Copy link
Collaborator Author

Casper,
When things sit too long I get these ideas. I've added functionality for 4-in-1-out hot ends (required adding a couple of variables)
I also put in a ban on RepRap machines as they use M567 which at this time is not supported. I'm looking into that.

@casperlamboo
Copy link
Contributor

Hi @GregValiant, thanks for updating the PR with the remarks.

Currently the PR contains some merge conflict messages, and thus we cannot merge. Example:
https://github.com/Ultimaker/Cura/pull/16176/files#diff-443ca6884a76820b08bdbb0a49453f23d6f36e40c3f9ba52baa794e0ccc5dc83R76
Could you resolve these merge conflicts? then I can move to the testing part of the review.

Let me know if you can use some help.

thanks,
Casper

@GregValiant
Copy link
Collaborator Author

GregValiant commented Oct 4, 2023

Along with help with Python I need help with Commits. I can't believe what a hash I've made of this. It wasn't just "changing computers in mid-stream" either.

Here is my latest and greatest revision. (The file I see in my last commit (which should have incorporated your suggestions) is different. I tried the "revert" to straighten this out but I seem to have made even more of a mess.)

One of your suggestions was in regard to this line:

if any(["MultiExtColorMix" in pp_name for pp_name in pp_name_list.split("\n")]):

My intent for that section of code was to keep the post from flooding the user with messages if there was more than one instance of the post running. With your suggested change I find it posting the message every time an additional instance of the post is enabled. That's what I was trying to avoid.

MultiExtColorMix.zip

Change concatenations to templates where possible.

Update MultiExtColorMix.py

Update

Update MultiExtColorMix.py

Update

Update MultiExtColorMix.py

Update2
@GregValiant GregValiant marked this pull request as draft November 29, 2023 15:27
@GregValiant GregValiant deleted the MultiColorMix branch December 19, 2023 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Community Contribution 👑 Community Contribution PR's
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants