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

Added BIQU machine base files as well as BIQU B1 machine files. #8276

Merged
merged 10 commits into from
Sep 30, 2020

Conversation

looxonline
Copy link
Contributor

This PR adds a base profile for BIQU printers and on top of that it adds the first printer, the B1. More will follow.

There is also a change required to the PLA 175 materials file which will be committed shortly. I'll reference this PR in that one.

@nallath
Copy link
Member

nallath commented Sep 1, 2020

Is there a specific reason that you added an inbetween base profile?

You also don't need to exclude the 2.85 materials in the machine definition. Those are automatically excluded.

@looxonline
Copy link
Contributor Author

Is there a specific reason that you added an inbetween base profile?

You also don't need to exclude the 2.85 materials in the machine definition. Those are automatically excluded.

BIQU have many printers that are similar in design and will therefore share mostly the same settings with just a few variations per machine. While I've only added one machine in this PR there will be others to come and having a base definition made the most sense to me since I would therefore only need to edit the deltas between machines. Please let me know if I have misunderstood this point.

Regarding the materials I'll just allow anything through and leave Cura to filter out the 2.85 batch. Thanks for the tip. New commit incoming.

@nallath
Copy link
Member

nallath commented Sep 7, 2020

Please let me know if I have misunderstood this point.

You didn't, but it's a mistake I've seen other people make before, hence me mentioning it.

…nts. Adjusted materials in quality profiles to refer to the 175 profiles directly.
@looxonline
Copy link
Contributor Author

Please let me know if I have misunderstood this point.

You didn't, but it's a mistake I've seen other people make before, hence me mentioning it.

I've fixed the author in the variants and the comments in the quality profiles. I also adjusted the quality profiles to refer directly to the 175 generic materials instead of to the 2.85 generic materials. Please let me know if you see any issue with this adjustment. I know that Cura automatically makes the change but I figured that there would be no harm in referencing the correct file directly.

@looxonline
Copy link
Contributor Author

Hi @Ghostkeeper and @nallath. I just want to check up on this to see whether there is anything left that is expected of me before this can be merged. Thanks gents.

@looxonline
Copy link
Contributor Author

Hi @Ghostkeeper and @nallath. I just want to check up on this to see whether there is anything left that is expected of me before this can be merged. Thanks gents.

I was reviewing the CR6SE PR and I saw that you recommended a settings version of 16 for 4.8 merge candidates. I'll bump these and push the changes shortly.

@Ghostkeeper
Copy link
Collaborator

Since you made your PR before we bumped up the settings version, I would normally do the version upgrade myself upon merging. It's really our fault for being slow then.

I think you're okay. It's just us being slow.

@tomburns
Copy link

Would be awesome to get this merged; let me know if I can help!

@looxonline
Copy link
Contributor Author

Would be awesome to get this merged; let me know if I can help!

I'm sure @Ghostkeeper and @nallath will get around to it. The 4.8 release must just be keeping them super tied up. They did approve my other PR to the FDM materials so there has been some movement 👍💪😀

@konskarm
Copy link
Contributor

Hey @looxonline. I noticed that you made changes to this PR up until recently. Is it finalized now?

@looxonline
Copy link
Contributor Author

Hey @looxonline. I noticed that you made changes to this PR up until recently. Is it finalized now?

Hey there. Yup, I've just been tweaking and finessing but none of it was essential. It's good to be merged now. Thanks as always guys.

@konskarm
Copy link
Contributor

konskarm commented Sep 30, 2020

Both machines slice fine. Also the generic_pla_175 is modified to override some material settings for these printers. As a result, I am accepting this PR and the printers will become available from Cura 4.8 onwards.

@konskarm konskarm merged commit ecf32a2 into Ultimaker:master Sep 30, 2020
@looxonline looxonline deleted the biqu_base_and_b1 branch September 30, 2020 15:10
@gsdaemon
Copy link

Hey @konskarm. Any chance to see a nightly build of the master or Cura 4.8 (beta) containing this pull? 🙃

@tomburns
Copy link

Y'all are the best, thanks for landing this for 4.8 <3

@looxonline
Copy link
Contributor Author

Hey @konskarm. Any chance to see a nightly build of the master or Cura 4.8 (beta) containing this pull? 🙃

If you guys are really that eager for it then join the B1 facebook group and you will find a post there where I have all the files and instructions on how to integrate them into the current Cura for testing purposes.

@Ghostkeeper
Copy link
Collaborator

We found a few more problems with these profiles, after the fact:

  • The quality profiles had the same ID as the Creality profiles, causing the Creality profiles to not load any more.
  • The definitions contained duplicate JSON keys. Only one of them gets loaded by Python then.
  • The definitions were defining value properties for enum-type settings. The value property needs to be a Python expression (if it's not a string in JSON it'll directly convert it to a string that results in the same Python expression). But since it didn't add any quotes in the Python expression it was thinking the value was actually a variable, and the variable doesn't exist.
  • There was a default_value for a float-type property that was using a Python expression. Only value can have a Python expression. As a result it was basically setting this setting just to a string value.

I've fixed the IDs here: a73cd96
I've fixed the other issues here: 9b5005f

@looxonline
Copy link
Contributor Author

We found a few more problems with these profiles, after the fact:

  • The quality profiles had the same ID as the Creality profiles, causing the Creality profiles to not load any more.
  • The definitions contained duplicate JSON keys. Only one of them gets loaded by Python then.
  • The definitions were defining value properties for enum-type settings. The value property needs to be a Python expression (if it's not a string in JSON it'll directly convert it to a string that results in the same Python expression). But since it didn't add any quotes in the Python expression it was thinking the value was actually a variable, and the variable doesn't exist.
  • There was a default_value for a float-type property that was using a Python expression. Only value can have a Python expression. As a result it was basically setting this setting just to a string value.

I've fixed the IDs here: a73cd96
I've fixed the other issues here: 9b5005f

Thanks for picking up on these points. The profiles were strongly based on the Creality methods and original profiles due to the machine similarities which explains the quality overlap. I figured that the internal definition name change would be enough to allow the parser to differentiate. I didn't realize that the filename could also not be duplicated. Thanks again.

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.

6 participants