-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Exclude Objects #4716
Exclude Objects #4716
Conversation
A reference implementation of the preprocessor is available at https://github.com/kageurufu/cancelobject-preprocessor and works for Cura, IdeaMaker, and Slic3r-family. Other slicers are fairly trivial to implement at this point, and I plan on adding a conversion for M486 -> our syntax. The benefit over macros are getting inferred POLYGON and CENTER. Integration into moonraker is also in progress, to simplify the overall workflow. Native slicer support is also in discussion with a few slicer developers. EDIT: The preprocessor is now on pypi as well, for ease of distribution and integration into other projects EDIT: A implementation of Moonraker with cancellation processing built in is available at https://github.com/kageurufu/moonraker/tree/work-cancel-object |
Thanks. Seems like useful functionality. Alas, it'll likely take me some time before I'll be able to review it. What is the current status - is this mergeable and fully useble now, or are there some dependencies needed first? -Kevin |
The Klipper-side module is complete, and fully functional through raw G-Code. @meteyou has the Mainsail support ready to go as well, and we have a working Fluidd fork. The preprocessor itself is feature-complete enough, I haven't released official binaries on Github yet, but there are builds on the github actions. Moonraker support is still in the works: its functional, but we are rewriting things to move the preprocessing out of the main metadata generation task to smooth out the process. I've also tested this PR against the Python 3 branch, and everything works fine there! EDIT: Just to note- the Moonraker support is not required for Mainsail/Fluidd to be fully functional. |
|
module: Exclude Objects Adding Klipper functionality to support cancelling objects while printing. This module keeps track of motion in and out of objects and adjusts movements as needed. It also tracks object status and provides that to clients. The Klipper module is relatively simple, and only provides one piece of the workflow. Support from Moonraker is underway to pre-process gcode files from various slicers with the extended gcode commands supplied by this module. UI's such as Fluidd and Mainsail will provide the user facing controls. There has been a small group sharing code. In addition to the Moonraker work, I have Fluidd code that will be submitted shortly. Mainsail support is also underway. Signed-off-by: Troy Jacobson <troy.d.jacobson@gmail.com> Co-authored-by: Franklyn <voron@tackitt.net> Co-authored-by: Eric Callahan <arksine.code@gmail.com>
I'm going to try to do a detailed review of the module soon. One thing I would recommend is to rebase your branch against master so the PR doesn't have 173 commits, it makes the review a bit easier. |
7ed239d
to
89acce7
Compare
if self.next_transform: | ||
logging.debug('Disabling ExcludeObject as a move transform') | ||
self.gcode_move.set_move_transform(self.next_transform, force=True) | ||
self.next_transform = None |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
klippy/extras/exclude_object.py
Outdated
|
||
def get_position(self): | ||
self.last_position[:] = self.next_transform.get_position() | ||
self.last_delta = [0., 0., 0., 0.] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The self.last_delta
attribute doesn't seem to be used anywhere.
klippy/extras/exclude_object.py
Outdated
def _normal_move(self, newpos, speed): | ||
self.last_position_extruded[:] = newpos | ||
self.last_position[:] = newpos | ||
self.next_transform.move(newpos, speed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how this works without tracking and offsetting the amount of extrusion that has been excluded. I can see that you offset extrusion in _move_from_excluded_region()
, but I would think that the next "normal move" would significantly jump forward in extrusion.
I left a few comments, I'm mostly unsure about the extrusion offset. Once I have a good understanding of how your approach is working, there are some other things to consider:
|
Based on feedback from Arksine, reworked the move, get_position and related methods to track using an offset. Signed-off-by: Troy Jacobson <troy.d.jacobson@gmail.com>
Signed-off-by: Troy Jacobson <troy.d.jacobson@gmail.com>
Thanks. What's the current state of this PR? -Kevin |
Sorry for the delay. It's pretty well ready to go as far as I've been aware, Moonraker has full support for processing uploaded G-Code for cancellation, the python module there is fully functional standalone for processing within your slicer. Cancellation works correctly in testing, and the fork has been tested by a variety of users to great success so far. I believe @Arksine was intending to review this as well when they have the time |
There is currently a problematic interaction between this module and |
klippy/extras/exclude_object.py
Outdated
|
||
import logging | ||
import json | ||
from datetime import datetime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there is an unused import here.
klippy/extras/exclude_object.py
Outdated
self.max_position_excluded) | ||
logging.debug("EXO: New position: %s", " ".join(str(x) for x in newpos)) | ||
logging.debug("EXO: Offset: %s", " ".join(str(x) for x in offset)) | ||
self.log_next_moves = 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a heads up, you are going to need to remove the debug logging per the contributing guidelines.
# differences between the last object printed and excluded one. | ||
self.extruder_adj = self.max_position_excluded \ | ||
- self.last_position_excluded[3] \ | ||
- (self.max_position_extruded - self.last_position_extruded[3]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I know what is happening here, let me verify that I have it correct.
- If the extruder is retracted before we entered the object, but not retracted before we exit, we are adding an additional retract on the next normal move that isn't a z hop.
- If the extruder is not retracted before we enter the object, and retracted before the exit, we are going to detract (ie: prime) on the next normal move that isn't a z hop.
I'm guessing we are making certain assumptions how slicers behave. My concern is what will happen if the user cancels the object while its in progress? Could it result in an undesirable prime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is the intended logic around retraction, and the intent is to ensure that the filament is in the expected position in the extruder prior to moving to the next object.
You're correct that this is assuming a particular behavior on the part of the slicers, but this behavior has been consistent between slicers so far. This is relatively simple logic and I'm comfortable with that it's doing. If there's a slicer or something that needs different behavior, that could probably be addressed in preprocessing.
Sorry it took me a while to get to this. I went ahead and looked at the updates. I think we are close, and I know its testing well. I had some minor comments and the question about a "what if" when attempting to compensate for retraction. Once that is hammered out we can rebase this and it will be approved from my perspective. |
Hey everyone. Sorry for the lapse in communication. I haven't had as much time to dedicate to this since the holidays. Here's the current status:
|
Ok, I'll wait on pending feedback for the retraction issue. IIRC, you mentioned that this was to accommodate SuperSlicer. I'm curious, does this affect PrusaSlicer as well? If not, should we just consider it a bug in SuperSlicer and request that it get fixed there rather than attempt to compensate in Klipper?
The issue we are bumping into is that Tuning Tower is registered before Exclude Object is registered, then unregistered before Cancel Object is unregistered. This is effectively "breaking the chain", because when Tuning Tower is removed Exclude Object's |
We could also just make |
TBH, its probably not valid to enable def is_testing(self):
return self.normal_transform is not None Before setting the transform in |
That sounds fine to me. We could add a more flexible transform registration/unregistration process, but that's likely overkill right now. -Kevin |
This commit mainly addresses conflicts between Exclude Object and the Tuning Tower module register and unregister process. Some workflows that include both modules resulted in Klipper crashes. The overall issue is that the current process for registering move trasnforms in Klipper requires that transforms be unregistered in the reverse order they were registered. As it currently stands, these two modules cannot guarantee that behavior. This change gives priority to Tuning Tower by not enabling the exclude object features if a tuning tower test is in progress. It also will not unregister the exclude object move transform if that transform is not at the front of the list. There are some logging cleanup changes as well. Signed-off-by: Troy Jacobson <troy.d.jacobson@gmail.com>
Just pushed changes to address the Tuning Tower issue. The change is basically a) don't load the exclude_object transform if a tuning tower test is active and b) don't unload the exclude_object transform if it's not the at the head of the list. I think this both fixes the Klipper crashes and maintains the expected behavior for users. |
I'm running into performance issues excluding an object on a relatively simple plate: For every new kind of filament I'm printing I run a small plate of samples consisting of a filament swatch and a Voron test cube. I wanted to print another Voron cube, so I just reran the plate excluding the filament swatch. However I'm seeing significant stalling on layer change: VID_20220227_202326Zsmall.mp4Here is the SS project: I can provide the GCode if it's of any help. Running Klipper on python 3.10 on RPi 4B 2GB. |
That is typical behavior of exclude object. The gcode on the excluded portion of the layer must still be processed up through the |
@Arksine out of curiosity: have you considered fast-forwarding sdcard position from start to end object macro? |
I'm not the lead dev on this, but I don't think that is feasible. You couldn't fast forward as you still need to check each line of G-Code to determine when the object ends. It is also necessary to process the excluded gcode moves so we can track how much extrusion is being excluded and offset the tool appropriately. |
@CODeRUS We have to do some processing of skipped G-Code to handle some edge cases, mostly around toolchanging and absolute extrusion Fast-forwarding could be done in controlled situations, although it would probably require native slicer integration to get "right". If the preprocessor included the jump and extrusion offsets in the I'll experiment sometime, but the priority right now is getting this working in general. |
@Arksine sounds like something can be done on moonraker preprocessor side, end_object macro could contain all necessary values itself, with no need to process chunks in realtime again. @kageurufu no problem, just my silly question? mostly out of context :) |
btw, @Arksine is it possible to have macro for calling object processing for file manually? i have couple of scenarios. 1. is when long file is saved by chunks, object processing may be called to early. 2. is when i dont want to have exculude object being active for all my prints for time savings and etc., so i can call processing manually per file. having separate slicing profiles can be pita in this case. |
Moonraker imports @kageurufu's library to handle the processing. That said, even if the preprocessor made these calculations and provided the correct location to jump to, there are still outstanding issues:
Its not possible to manually call the preprocessor from Moonraker, the purpose of adding that support was to automate the process. It should be possible to work with the library directly if you want to determine when and what files to process. |
@Arksine thanks, just found debug output with correct command
can work with that. asked that question because i already had issues with not properly processed files, when processing was started while file is still uploading manually. |
@CODeRUS no problem, I'm always glad to entertain requests. Helps keep you from boxing yourself into one mindset while building something You can call the preprocessor manually, it's installed to |
I wonder if it would make sense to be able to define things like optional E- and Z-offsets to apply and undo when object exclusion starts and ends respectively, e.g. raise the nozzle a millimeter and retract the filament some to prevent oozing during stalls and the nozzle melting neighboring plastic depending on geometry. |
@wlhlm I would like to see the gcode file, if you still have it. That's a lot more of a pause than I'm used to seeing. |
@glowtape I don't think it should be necessary to have extra moves like that. There could be a situation, though, where we should tweak the preprocessing in order to clean up the between object moves for an individual slicer. I feel that this portion in klipper should be as simple as possible. If we need to provide hints or slicer specific work-arounds, they should be directed by the preprocessor. Although I did take a peak at the config in the file you attached. The settings are for a slightly longer and slower retraction that I'm using, so that's probably what's happening. It may be something we'd like to look at for a super slicer specific tweak. |
Sure thing. |
@JRHeaton If you're interested in trying this out, the best way is through Mainsail. There is documentation from the beta cycle that includes directions for klipper and mainsail. The currently released Mainsail has the UI side of the functionality. It's best to discuss any install questions there. |
I've been processing the comments here for a while, and feel like I have a good path forward for all of the outstanding items except around the sd card reset. Here are some of my current thoughts and things that influenced the implementation. I'm hesitant to delegate this to the preprocessor. If someone is in the middle of configuration, tuning, slicer changes, etc. they could easily run into a situation where the UI could be showing object information that is from an older print. That's why I looked for an automated solution. I had thought about having the UI handle this, but then starting a print from an LCD could cause issues. From a user's perspective, it's nice not to reset the state (the object lists, etc.) at the end of a print. It lets them review what happened. Especially if the print was cancelled. That state can be handled independently from the transform. And if I can be pointed to a reliable way to detect the end of a print (including it being cancelled), I'd like to unregister the transform at that time but keep the other state for the user. I'm not sure if this has any bearing on the reset implementation, but I'd like to have flexibility to tie into the Moonraker metadata in the future. One example could be reprinting only the cancelled objects. |
Okay. I don't object to an automated reset on an sdcard As previously mentioned though, the event should use "virtual_sdcard" prefix and it would be preferable for the change to be in a separate commit. Thanks. |
Perhaps it would still be a good idea to add a G-Code command that resets the transform. That way |
Alright, I have a updated branch for this PR ready at https://github.com/kageurufu/klipper/tree/exclude-object-pr, I think Troy got a bit busy, would it be acceptable to open a new PR, referencing this one? |
@kageurufu Did a fantastic job rewriting the commit history, formatting the documentation, and renaming the commands. I've been testing these changes for the past couple of days, including his preprocessor changes that use the new commands. I opened a new pull request with these updates: #5326 |
did this get abandoned ? |
No its moved to #5413 |
PR #5413 was merged. -Kevin |
This ticket is being closed because the underlying issue is now thought to be resolved. Best regards, PS: I'm just an automated script, not a human being. |
module: Exclude Objects
Adding Klipper functionality to support cancelling objects while printing.
This module keeps track of motion in and out of objects and adjusts movements as needed. It also
tracks object status and provides that to clients.
The Klipper module is relatively simple, and only provides one piece of the workflow. Support from Moonraker is underway to pre-process gcode files from various slicers with the extended gcode commands supplied by this module. UI's such as Fluidd and Mainsail will provide the user facing controls.
There has been a small group sharing code. In addition to the Moonraker work, I have Fluidd code that will be submitted shortly. Mainsail support is also underway.
Signed-off-by: Troy Jacobson troy.d.jacobson@gmail.com
Co-authored-by: Franklyn voron@tackitt.net
Co-authored-by: Eric Callahan arksine.code@gmail.com