search

BirthT / BeltPrinterSlicing

This is a plugin for using a belt printer with the latest Cura.
GNU Lesser General Public License v3.0
19 stars 11 forks source link

Support in Ultimaker Cura Marketplace #9

Open Ghostkeeper opened 3 years ago

Ghostkeeper commented 3 years ago

Hi BirthT! I must apologise for the weird process that happened after you submitted this plug-in for review. Someone accepted it, but they did not follow our procedures. None of the Cura developers had reviewed it yet. We're investigating internally, because that could've been a security issue. We found out about it a few hours later and had to unpublish the plug-in. I did a cursory review of the code then and found a few issues that would normally have blocked the plug-in from the Marketplace. When reviewing plug-ins we look for two things:

This issue is a code review requesting some changes before we can publish this. The main goal is to prevent the plug-in from interfering with Cura's normal behaviour. I intend to help you to suggest changes you could make to fix them.

Nozzle model

This plug-in changes the nozzle node to make it point diagonally when printing with a belt printer. However this deletes the node entirely when the plug-in is disabled. I would suggest that you don't adjust the nozzle mesh at all if the plug-in is disabled. A simpler structure would be to simply rotate the nozzle mesh whenever it is enabled and rotate it back whenever it is disabled.

Update checker

This plug-in explicitly disables the Update Checker plug-in. This will never be allowed. Remove that code. This might be a remnant of wherever you copied that code from. @fieldOfView might recognise it? It says that the update checker would check the wrong version, but I don't see how you could come to that conclusion.

USB Printing

This plug-in explicitly disables USB printing, sabotaging third-party printers. This will never be allowed. Please remove that code, as it is anti-competitive. If the printer doesn't support USB printing, it will not have a USB port or will not respond to g-code commands coming in from there.

Patches

Your plug-in patches a number of existing internal functions of Cura. Even if the copies are currently correct and working well, Cura developers will change these functions around in future releases, yet your version was copied from some fixed version in the past. Cura will depend on these changes, and break if they are not applied.

This concerns:

There are two better solutions to these patches that I can think of, that wouldn't have this problem.

Setting and profile adjustments

In the StartSlicingJob you're adjusting the user's settings.

Adjusting settings should be done by printer definitions and profiles, not by a plug-in. For instance, you're disabling Support Enable setting and the adhesion type setting. If support and adhesion are not supported by a belt printer, their printer definition should set the setting to "enabled": "false" and its default to false or none. It should not allow the user to change settings but then secretly change them back once they start slicing. The same goes for adjusting the layer height and flow. If the user needs to have a different layer height for slicing with belt printers, please let the profiles change the layer height.

I suggest that you provide a .def.json for the printer(s) you are targetting which adjusts those settings and disables the ones that are not available for belt printing. This would also allow your plug-in to automatically enable/disable depending on if the belt printer is currently the active global container stack.

These setting changes are currently preventing the Ultimaker 3 profile from slicing at all, even if belt printing is disabled. I'm supposing the same goes for various other printers too.

Support for Linux

The plug-in crashes on Linux. You don't give any warning about it. There's also no real reason for it, since it crashes due to never having imported Trimesh, which is a pure Python library and supports Linux fine. We ask that you either:

Further, non-blocking issues

A few things I noticed during the review which I think you might want to fix, but don't pose an issue for Cura itself.

KobayashiRui commented 3 years ago

Hi Ghostkeeper ! Thanks for the code review🙇‍♂️ That was very helpful !!

I would like to confirm one thing.

Are you sure you want to create a new printer profile and set the values there, instead of using addPreference to set them?

In that case, is there any way to add def.json from the plugin? Or would it be better to ask plugin users to copy def.json to the resources directory?

I'm not very good at English and I use online translators a lot, so I'm sorry if my sentences sound weird.

Ghostkeeper commented 3 years ago

Your sentences are fine. No worries! I'll try to use shorter sentences too. Those are easier for online translators.

Are you sure you want to create a new printer profile and set the values there, instead of using addPreference to set them?

No, you misunderstood. You are correctly using addPreference for all of your current preferences, like BeltPlugin/on_plugin, BeltPlugin/view_depth, etc. This is okay. No problem.

I was only talking about slicing settings: support_enable, adhesion_type, layer_height, etc. You are editing those here:

https://github.com/BirthT/BeltPrinterSlicing/blob/50234ab39142b75846ebc4f86fdcfe429552e434/StartSliceJob.py#L293-L300

and also:

https://github.com/BirthT/BeltPrinterSlicing/blob/50234ab39142b75846ebc4f86fdcfe429552e434/StartSliceJob.py#L320-L323

Those are a problem. They would not behave as the user expects. For instance, the user can still enable the brim, but then won't see a brim.

In that case, is there any way to add def.json from the plugin? Or would it be better to ask plugin users to copy def.json to the resources directory?

It would be easier for the user, if .def.json files are automatically installed. It'd be less work for the user. Other plug-ins in the Marketplace do this as well. They simply copy the file from the plug-in folder to the resource folder upon first launch. Something like this:

from UM.Resources import Resources
from UM.PluginRegistry import PluginRegistry
definition_dir = Resources.getStoragePathForType(Resources.DefinitionContainers)
my_resource_dir = os.path.join(PluginRegistry.getInstance().getPluginPath("BeltPrinterSlicing"), "resources")
for definition_file in ("creality_cr30.def.json", "creality_cr30_extruder0.def.json", "blackbelt3d.def.json", "blackbelt3d_extruder0.def.json"):
    old_definition_path = os.path.join(my_resource_dir, definition_file)
    new_definition_path = os.path.join(definition_dir, definition_file)
    shutil.copyfile(old_definition_path, new_definition_path)

You'd have to do something similar for any nozzle profiles and such if necessary.

You can also look at examples from different plug-ins on the Marketplace:

Some plug-ins just install the files at first run. Some have a button that the user needs to press. Note that .def.json files are loaded during start-up. You have to install them before the resources are loaded (e.g. on the CuraApplication.pluginsLoaded signal) or ask the user to re-start Cura when they are installed.

We don't consider it important that resource files remain installed if the user uninstalls the BeltPrinterSlicing plug-in.

KobayashiRui commented 3 years ago

Thanks to your reply, I now understand what I misunderstood👌 And thanks for the link to the example.

We will modify the code. I will contact you if I have any further questions.

Thank you!

fieldOfView commented 3 years ago

Hey, creator of Blackbelt Cura here. It seems to liberally "borrowed" my code, but removed my copyright from it. As far as I know - but I am not a lawyer -, you are not allowed to do this according to the LGPL v3 license the code was originally published under. Please add back my original copyright statements! I recommend Ultimaker not publish this code in the Marketplace until you at least comply with the license you use the code under.

I also can't help but wonder why you butchered the code to use global preferences to change what should be a printer setting - and was easily settable per printer in the settings in BLACKBELT Cura.

KobayashiRui commented 3 years ago

Hi @fieldOfView I respect BlackBelt Cura, which you have developed, and I respect you as well. Therefore, I am very sorry about the removal of the license. We will add it right away. I will be careful in the future to avoid this kind of situation. I would appreciate it if you could tell me if there is anything wrong.

I also can't help but wonder why you butchered the code to use global preferences to change what should be a printer setting - and was easily settable per printer in the settings in BLACKBELT Cura.

This was due to my ignorance of how to use .def.json. Now I'm thinking to add .def.json of the belt printer from the plugin and configure it, since Ghostkeeper taught me. So, I will eventually stop using global preferences. Thank you for your valuable input.

fieldOfView commented 3 years ago

Thanks for putting the copyright back in.

I have given this matter some thought since I have first been made aware of it last friday. This plugin does not sit well with me. It is 95% my code, and the part that is not my code is already causing issues. Given that you cannot explain to @Ghostkeeper why certain in-place patches to Cura are necessary, I don't think you understand the code well enough to maintain these patches if (and when) Cura changes to make updates to the patches necessary. Also my work has been paid for by BLACKBELT 3D, and you have unceremoniously removed their name from the project (except for the readme).

I propose that this version of my work does not get published to the Marketplace (since it currently has major issues), but I get back in touch with BLACKBELT 3D and ask for their blessing to create a plugin out of the work I did for them. That way I can keep the patches up to date, work with Ultimaker to keep them happy about what I do with their code, and BLACKBELT 3D at least gets the community recognition that they deserve.

If you want, I will include a printer definition and quality profiles for your printer in that version of the plugin. The plugin will of course remain licensed under the LGPLv3 and you will be able to create pull requests against it to request changes either specific to your printer or belt-style slicing in general.

KobayashiRui commented 3 years ago

The fieldOfView suggestion makes a lot of sense and I am very grateful for it. I would like to contact BLACKBELT 3D immediately. I hope we can proceed with the project in the format you and BLACKBELT 3D want.

fieldOfView commented 3 years ago

I have reached out to BLACKBELT 3D, and will inform you when they get back to me.

fieldOfView commented 3 years ago

I have their blessing, with some conditions about liability.

Development of the plugin shall take place here: https://github.com/fieldOfView/Cura-BlackBeltPlugin

It is currently private until I have some copyrights and disclaimers in place.

KobayashiRui commented 3 years ago

Thank you for your report. It's very nice. I'll definitely try it as soon as it's available. I'm looking forward to its release. Please let me know if there are any modifications you would like to see in this repository.

Thank you again.

Ghostkeeper commented 3 years ago

I think, legally, anyone has the right to re-publish that code as long as the license is preserved and possibly the copyright, due to the LGPL license. I'm not a lawyer either, of course.

But we are all humans here and in the spirit of honour and cooperation I understand that KobayashiRui agrees to hold off their version?

Ultimaker is not likely to take party in the discussion of copyright and licensing of this plug-in (as long as it adheres to the license and the Dutch law), but I hope it can be resolved between you.

The re-submission we received on the 27th in any case still has most of the aforementioned problems, so we'll reject it for those reasons.

23TNC commented 2 years ago

@fieldOfView I was wondering if you are still working on https://github.com/fieldOfView/Cura-BlackBeltPlugin

I was playing around with this plugin for a bit yesterday and was able to get it to run in Ubuntu and made a simple PR with those changes. I'd prefer to hold one copy of trimesh, and after reading the above issues from Ghostkeeper, holding one copy of trimesh does seem to be the correct method I am uncertain why we changed to mac and win versions they seem very similar if not identical from the simple diff I ran against them.

I did have to make one change to trimesh to get this to function in ubuntu. There is a line in inertia.py that includes util from trimesh, but it cannot find trimesh because it's path is one level lower then it's expecting, I changed to a relative path to fix this. from . import util

I think the original trimesh comes from here? https://github.com/mikedh/trimesh/blob/main/trimesh/inertia.py

I am guessing there is a better method to handle this dependency, as I think I could pip install trimesh. I am uncertain the method cura plugins use to acquire dependencies and that is another thing I'd like to look into, as it would be nice to remove trimesh from this plugin all together and just pull it.

I am uncertain if I solved the circular dependency correctly, and need to dig into that a bit deeper, and honestly after reading Ghostkeepers issues above I'd like to try and remove the Cura patches all together.

I just thought I'd ask if there was any movement on fieldOfViews plugin before I started playing around with this one. I'd really like to solve Ghostkeepers issues above and get a belt plugin into the market place.

KingBain commented 2 years ago

I have their blessing, with some conditions about liability.

Development of the plugin shall take place here: https://github.com/fieldOfView/Cura-BlackBeltPlugin

It is currently private until I have some copyrights and disclaimers in place.

@fieldOfView is there any way I could contribute to your project ?

KingBain commented 2 years ago

any update on this @fieldOfView ? Are you actually working on this ?

jimmieclark3 commented 2 years ago

I would also love to contribute, @fieldOfView . I tried reaching out to you on your website, but the spam filter error is preventing me from contacting you. I would LOVE to help anyway I can, to make this a thing. It's hard to use belt printers without proper software.

Thanks Jimmie

KingBain commented 1 year ago

a bit of sour grapes on my part, but it looks like @fieldOfView is ghosting this project and its community.

However, looking online it looks like he has moved onto a different cura belt project https://github.com/Autodrop3d/BeltEngine

jimmieclark3 commented 1 year ago

I am contemplating tackling this solution on my own, or leading the effort at least. The community needs a good slicer, and nothing on the market really does what we need (or at least that I have found).

I have modified the cura octoprint plugin, and an auto print plugin for octoprint to allow queuing of many jobs into the belt printer. It will just autostart the next job after it finishes one. Works really really well.

Anyway, hope we can get something working. I can do most prints with the Cura plugin, but it has many issues and really fails around supports. It needs a ton of work there, at a minimum.

KingBain commented 1 year ago

@jimmieclark3 tbh I haven't done much printing with supports but when I compare what BlackBelt generates for supports, it seems identical to what the plugin generates. IDK if you had noticed but I started working on a plugin that would support v5 of CURA.

I would say most of the replacement of deprecated libraries has been done.... and its able to actually slice now. https://github.com/BirthT/BeltPrinterSlicing/issues/19#issuecomment-1370484587

Which printer were you working with ?

jimmieclark3 commented 1 year ago

CR30. I will take a look at your work. I have not looked at how to create a v5 plugin, but need to. I wanted to try and even get it to support tree supports. I have some ideas. Once i get my printer going again, I plan on jumping into this more.

Thanks Jimmie

KingBain commented 1 year ago

Doesnt seen much has changed in the architecture of the plugins, going through the old plugoin and updating the code was more a matter of replaceing deprecated libraries and updating menu's to meet the new QT syntax.

Seems the jump from 4 to 5 was a major change :D

getting tree supports would be very interesting.

jimmieclark3 commented 1 year ago

Yeah, that's what I meant. I went through and was trying to diff various repositories to see what made them v5 vs v4. Wasn't clear. Been a while, I'll get back into it soon.

KingBain commented 1 year ago

Funny enough, since @fieldOfView is the author of the original implementation of Blackbelt you can see a lot of his styling/mannerisms in the code. If you wanted to see an actual example where @fieldOfView upgraded a plugin from Cura v4 support to Cura v5. Take alook, its very similar . https://github.com/fieldOfView/Cura-MeshTools

KingBain commented 1 year ago

to raise an old topic, probably worth figuring out which code is which and restore the software licenses of the original authors.