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

Tuned quality profiles #16481

Merged
merged 7 commits into from
Sep 20, 2023
Merged

Tuned quality profiles #16481

merged 7 commits into from
Sep 20, 2023

Conversation

goofoo3d
Copy link
Contributor

@goofoo3d goofoo3d commented Aug 15, 2023

Description

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Printer definition file(s)
  • Translations

Checklist:

@github-actions github-actions bot added the PR: Community Contribution 👑 Community Contribution PR's label Aug 15, 2023
@MariMakes MariMakes added the PR: Printer Definitions 🏭 A PR that introduces or changes settings and printer definitions label Aug 15, 2023
@MariMakes
Copy link
Contributor

👋 Hey @goofoo3d,

That's a huge PR. 😮
I've done my best to analyze it, but I'll need to ask for a second opinion by one of the developers in the team.
This is what I've learned analyzing your PR, especially for our testers.

This PR is going to impact at least 18 printers available in Cura + the new one that will be added.
They can be divided in 4 categories.

Goofoo_Open will only impact Goofoo E-one
Goofoo_Small will impact 5 printers

  • Cube One
  • Renkforge Basic 3
  • Cube
  • Tiny
  • Tiny+

Goofoo_Far will impact 3 printers

  • T-one
  • Gemini
  • Pro7 Dual

Goofoo_Near will impact 9 printers

  • Max
  • Mido
  • Plus
  • Nova
  • Pro3
  • Giant
  • Mini+
  • Pro 6+
  • Pro 6

All of these printers have 5 available nozzle sizes: 0.2, 0.4, 0.6, 0.8 and 1.0

There are resolution definitions specifically for GooFoo Materials, but depending on the material there are either 2, 3 or 4 different layerheights available, and not all materials are available to all printers.

Goofoo Material goofoo_far goofoo_near goofoo_small goofoo_open
ABS 2 2 - -
ASA 2 2 - -
HIPS 4 4 - 4
PA 2 2 - -
PA_CF 2 2 - -
PC 4 4 - 4
PEEK 4 4 - -
PETG 2 2 2 2
PLA (5x) 4x5 4x5 4x5 4x5
PVA 4 4 - 4
TPE 4 4 - -
TPU 87a 3 3 - -
TPU 95a 4 4 - -

This PR introduces a significant amount of new files, and renames a bunch to align them with a naming standard. That must have been a bunch of work to create 😅 👑

Personally I'm not seeing any odd setting definitions and our printer linters doesn't seem to throw any errors aswell. I'll show your work to the team to see if we can introduce it in Cura 5.5. Fingers crossed 🤞

Can you confirm if I made the correct assumptions?

@goofoo3d
Copy link
Contributor Author

goofoo3d commented Sep 1, 2023

Your understanding is absolutely correct! Thank you for validating my work - I really appreciate it. If there are no other issues, I hope my pull request can be merged into Cura 5.5! Thank you very much!

@MariMakes
Copy link
Contributor

Hey @goofoo3d,

I had one of the developers take a look and they spotted a potential problem.
A number of the already existing filenames have been renamed. This could cause some trouble for users who upgrade but have
the configuration still selected.

For example
/goofoo_far_0.40_abs_standard.inst.cfg → /goofoo_far_0.4_abs_standard.inst.cfg

Do you have a strong reason why you would like to rename the files? Because our developers have to write an upgrade script for people to start using these new settings and that might take some time. If it's not too important and too much work you can remove the renames, and then I could probably merge the change and we could include it in the 5.5 release.

How would you like to proceed?

@MariMakes MariMakes added the Status: Needs Info Needs more information before action can be taken. label Sep 8, 2023
@goofoo3d
Copy link
Contributor Author

goofoo3d commented Sep 9, 2023

Hello @MariMakes ,
There is no particular reason, I just thought having a consistent naming format seemed more formal! If it causes any potential issues due to the renaming, then I'll remove it, since the renaming itself is unimportant!

@goofoo3d
Copy link
Contributor Author

goofoo3d commented Sep 9, 2023

Hey @MariMakes ,
Can you help me remove the commits about the renaming ?

@goofoo3d
Copy link
Contributor Author

Hello,
When will my pull request be merged? If there is anything I need to do, please let me know! Thanks!

These file names are already present in Cura, and will cause some broken configurations if they are renamed without a script
These files are renamed, back to how they are already in Cura, so it won't break configurations.
Hopefully these are the last renames before merging
@MariMakes MariMakes merged commit 8ccca7f into Ultimaker:main Sep 20, 2023
4 checks passed
@MariMakes
Copy link
Contributor

Hey @goofoo3d,

It took a couple of tries, but I've been able to rename the files that we expected would cause issues with an upgrade script.
I tested a number of material, printer, core combinations and it's looking good 😄

I'll merge these changes so we can ship them with the upcoming Cura 5.5 release.

@MariMakes MariMakes removed the Status: Needs Info Needs more information before action can be taken. label Sep 20, 2023
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 PR: Printer Definitions 🏭 A PR that introduces or changes settings and printer definitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants