-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Broken PostProcessing when script changes out-filename #6042
Comments
@foreachthing are you sure your scripts were executing fine before my changes? It could be that your scripts were always producing errors, but previously file was saved in the temp dir, then copied onto SD card, and only then post-processed. So if your scripts were failing, you could not notice that. Can you please send me some very basic script that reproduces your problem, I will try to fix that. I also have windows, few postprocessing scripts and they all work fine in temp directory. |
Yes, my scripts used to work flawlessly... :-/ Note, that I always create a backup. But that is not required and could be removed. That was for debugging, mainly. I've been using this https://github.com/foreachthing/Slic3rPostProcessing/releases for years now and it began to fail yesterday after a new pull/build) . |
@xorza how do you process the gcode file after post processing? |
@foreachthing from what I see https://github.com/foreachthing/Slic3rPostProcessing/blob/master/SPP-Python/Slic3rPostProcessor.py you actually overwrite the original file, and that is exactly how my scripts work. Before my PR behavior was
My pr made it:
If your scripts remove post-processed file, that is our problem. PS fails to copy it from the temp to the destination and throws an exception. |
@foreachthing I am not sure what you are doing with the script, but I suppose you want the final name of the file to make a backup. Should we add it to the environment variables of the script? |
@bubnikv with my c# post processing scrip (pps) I create a file with a counter-prefix: some.gcode => 000123_some.gcode. And then I need to do that "Cura-Move", because the way the Ultimakers 2 work.They have those clips on the build-plate to hold the glass plate in place. If PS slices, I cannot control the entry point and ever so often it happens that the nozzle crashes in the clip. So, first XY, then Z, not Z first! As discussed in #2931 and #4065. So it's quite crucial for me that the pps work.
Maybe so, but PS failes to copy the gcode to the correct location. So, maybe I need to change my script to accommodate your workflow. Maybe you can show me your pps, so I can learn how it should work now?! |
Yeah current flow will fail if script renames the file. In my vision - it is the correct behavior, output filename should be exactly same which you specified after pressing 'Export G-code' button. I'm sorry that these changes broke you work, though. Can you change your C# script to make a backup of the input file, then postprocess it and overwrite the original file? I can take a look at your script and try to help, if needed |
@xoan, well, this renaming thing was the whole purpose of my script. I may have one print setting that prints ok, improve it save it again, without worrying about overwriting the previous file (and keep track of changes). Now I'm back to square one ... |
I try and change behavior so name of the temp file will have an incrementing numeric postfix |
Other thing I can do: if any post-processing needed, I can make one more temp copy, that will be used for postprocessing |
What exactly will that do to my filename at the final location? Backup file won't be necessary. I'll remove that "feature" soon. That was for debugging only. |
@foreachthing I still don't understand your problem. The post-processing script was always assumed to do the change in place. If it does not, then the OctoPrint integration would not work. The situation now is the same as before, only the file is named with a temp name. If you need the final name, we may accommodate that somehow, maybe through an environment variable. |
@xorza We are not going to integrate a one trick pony. I still don't understand why @foreachthing cannot adjust his script to the new behavior and what would be the minimal support from PrusaSlicer to enable that. |
I can tell :-)
I don't use OctoPrint, All I want to do is rename the output gcode file with a prefix counter ->
I have the final name. Not the issue at hand.
Sure. If I can somehow rename my files after postprocessing!! I can handle the new temp-file. Only the new way the temp files are being process leaves me standing in the cold.
And I cannot rename the file AFTER it has been copied to the "save-to" location. I hope that now everything is clear as mud ;-) |
I think I understand your workflow. The ways of PrusaSlicer users are mysterious. If you leave the original file there, you may create your new file and the original file will be copied. I don't think that having the final unmodified file copied somewhere (maybe to a temp directory?) is an issue. |
Oh, come on! ;-)
No, no issue except my new file will remain buried in %app-data% and the unmodified on my SD-Card. That way I lose the ability to use the save-as dialog - I'd need to hardcode something. It used to work soooo well...! Could you pass the save-as path to the postprocessing script as an additional parameter? First parameter: the temp file, second parameter the "real" path. |
We can do that. I wonder whether that may break somebody's script. I just checked that we allow the "script" line to contain not just the script, but also its optional parameters. The G-code path is being appended to the end of the optional user's script parameters. If somebody counts the number of parameters, she/he may get surprised. We can optionally save the name of the target G-code file into an environment variable. I am not sure which one is better. |
Nobody cared when merging the changes to store the gcode in the temp directory... why now?
Why? It just needs to be properly documented and if someone can create a post processing script, he/she can adjust the script for another additional parameter.
I like this idea. That way one could choose what (optional) parameters are being passed. |
Because the person doing the merge (me) showed insufficient imagination of what could break (how people are using our software). We actually do care, even if it may not seem to. |
This came across wrong. Should have used a ;-). |
1) New environment variable SLIC3R_PP_HOST contains one of "File", "PrusaLink", "Repetier", "SL1Host", "OctoPrint", "FlashAir", "Duet", "AstroBox" ... 2) New environment variable SLIC3R_PP_OUTPUT_NAME contains the name of the G-code file including path (for SLIC3R_PP_HOST == "File") or a name of the file after upload to the host (PrusaLink, Octoprint ...) 3) The post-processing script may suggest a new output file name (likely based on SLIC3R_PP_OUTPUT_NAME) by saving it as a single line into a new "output name" temp file. The "output name" file name is created by suffixing the input G-code file name with ".output_name". Please note that the G-code viewer visualizes G-code before post-processing. Fixes Broken PostProcessing when script changes out-filename #6042
Implemented with cf32b56
Please test it. |
@bubnikv, this looks promising!!! :-) Questions:
Thanks! |
Example shell script:
The notification about the successful export still contains the old name, actually selected in the file dialog. That is too complicated to change. In fact, we may soon regret implementing this at all. |
Then don't! Seriously! If it's a pain to maintain and prone to break other stuff ... remove it! |
My vote goes to: Revert that change. |
This is a royal pain to work around. My post-processing script may produce multiple additional files next to the gcode file. These now end up in a temp directory and are lost. I can fix that by detecting this There are other problems however. The post-processing script can optionally be run inside a Windows WSL environment (or whatever they call the integrated Linux thing in WIndows these days). To do this, the user has to configure a simple batch script as the post-processing script in PrusaSlicer. This script then invokes the actual post-processing script through WSL. I haven't tested this, but I seriously doubt that the environment variables of the batch script will carry over to the WSL environment. If this new post-processing mechanism will be enforced, I will simply have to drop support for using the script through WSL. Apparently the idea behind this change is that some people save their gcode files directly to SD cards or slow USB sticks or the like, and it isn't a terribly good idea to run post-processing scripts on those. If you really deem this a common use case, I would vote to make this new post-processing method optional so the old direct method can still be used. The new method may be the default for all I care, as long as there is some option that I can toggle inside the config bundle that I distribute with my post-processing script. |
we seem to have a lot of hacky workarounds and changes to PrusaSlicer and post-processing scripts due to this merge, which sounds like it was made to address an issue that could be fixed simply by a change in workflow: save locally |
Maybe you should try it first? |
It should exist.
That does not sound as a "royal pain" to me. What you are asking for is to add an additional logic, to maintain it, to test it, so that three people could make use of it, while 150 thousand users will just wonder why the checkbox is there. |
The guy who provided the original pull request? For each feature in PrusaSlicer there is somebody somewhere in the universe who will want it to work the opposite way. We cannot handle the exponential explosion. |
But so far the one guy who wants the PR is outvoted by those who would rather it stayed as it was, and that's just out of alpha testers |
The problem is that those who are happy are keeping quiet. Only those unhappy are loud. |
I have just tested it and unsurprisingly, none of the environment from the Windows BAT makes it to the Linux runtime. As @sej7278 says, 2.4.0 is not even out of alpha phase and you're already getting complaints due to this change. I'm probably not the only one who normally doesn't even look at alpha releases (I only investigated this because I was notified that things were broken about the post-processing scripts I distribute). Those three people (actually at least four if I look at who posted in this issue) are likely just the tip of the iceberg. It wouldn't surprise me if even more complaints will come rolling in when going beta, and yet more after the final release. Sure, you could try to carefully document this new post-processing system and its env var, but it will remain non-intuitive and there will always be people who don't read all the documentation. |
@DrLex0: I'm trying to use Python to get it to work - but still fail in renaming the output-file. So much for "post" processing. |
BTW I don't understand your scenario. You receive a file name on the command line. You post process it and save it under the same name. IMHO you don't need access to $SLIC3R_PP_OUTPUT_NAME. |
@bubnikv maybe it would help if you could explain why you seem so against reverting the change? do you suspect the people who would benefit would outweigh those who would be inconvenienced; or is there another feature that is likely to rely on this that you're thinking of or somesuch? would be nice if we had some way to do a community vote to get some sort of idea of the fallout when this goes final - or if more than one person would benefit from it. blog/forum post maybe? |
https://github.com/DrLex0/FFCP-GCodeSnippets The main purpose of the script is to produce an extra X3G file because that is the only format this type of printer accepts. Next to this, the script also works around #2210, and can invoke other scripts before it does the X3G conversion. It may produce additional extra files with warnings or debugging info in them, which are particularly useful to troubleshoot problems during post-processing. Therefore I would absolutely need the env var to create the additional files in the target location instead of the tmp dir. Preserving the WSL workflow may be possible by fiddling with this |
Add a configurable counter to the filename and I'll close this issue. This issue is going in circels anyway. One guy wants it, many others don't. |
About 3 and half, possibly 4 man days were already spent on this topic by Prusa Research developers. That is quite a lot for what it is worth and still counting. Frankly I am inclined to just remove the feature if this continues. We need our development resources to fix supports, improve path planning, implement variable width extrusion etc. I personally would really love to spend my time on geometric algorithms to apply my expertise where it is most worthy. The primary issue I see is that you guys are misusing a "post processing script" for deployment purposes. We at Prusa Research have no way to know how people in the world are using or misusing some versatile or too versatile feature without collecting usage statistics, which is another risky proposition. We find out by the (mostly negative) feedback once we change something. You certainly agree that running a post processing script on an SD card does not make sense. SD card is slow and wears out. Thus we have accepted a pull request that made sense to us. There were other reasons to rework how the post processing script is integrated: OctoPrint integration and "export finished" notifications. Both of these features should be aware of the final name of the G-code or whatever file name is being exported. Certainly one can come up with a nicer interface to call the script, possibly passing some of the parameters as the target file name on the command line, however our implementation strives to be backward compatible wrt. to the scenarios we expected. I have discussed this topic with two other in house developers and we frankly don't know what would be the best solution. IMHO you could help yourself with the tools that we gave you, but you oppose, because it requires a bit of work from your side. Which is difficult to understand to me, because the post processing script is a "write once, run many times" task. I am considering to add another "deployment script" that would only be called when exporting a G-code and not when sending to OctoPrint, but I am not yet convinced to do that. What seems to you a single liner is a burden that the development, testing |
Regarding integration of Perl post processing scripts on Windows, we support |
@bubnikv I agree to a certain degree. I agree that you should be doing what you're doing best. But also understand that I spent many, many, many hours to get my script back to work (https://github.com/foreachthing/Slic3rPostProcessing/blob/master/SPP-Python/Slic3rPostProcessor.py) which still does not - and I don't know why. --> "write once, run many times" does not apply in this case! Back to my issue: I'm stuck with the renaming the temp file to my name-pattern. If you got some spare time (which is probably rare), please have a look at my script. Use in in PS: PS save as: Expected: |
@foreachthing
seems suspicious. I would expect something like
You will probably have to pythonize the syntax and everything. The name of the file is the temp filename appended with |
@foreachthing So I decided to test it after all and this works (tested with Python 2.7.12). Like I said, I don't know Python, there might be some (possibly serious) mistakes, but it illustrates what needs to be done.
Post edited to correctly handle file extension. |
@lukasmatena , I feel really stupid now. I didn't understand, that the new name should be content of the file with "output_name" suffix. Thank you so much, Lukáš! |
@KnatteAnka, in case you're interested: I've added a revers counter to my script. |
WSL is just one possible way to invoke my scripts, which can be convenient if it's already available because Perl and Python normally ‘just work’ inside a Linux environment. I do recommend to the average Windows users to simply install a Perl runtime instead.
Of course, as usual it took quite a while to figure out what this one-liner should be, and verify that it works and doesn't break in older versions. It would make sense to add a “deployment script” config option that is intended for doing things with the gcode file after it has been processed. |
Hence the name post processing. I'm outta here! 😁 @DrLex0 I have not tested it with Linux, but the Python's |
Version
version_2.3.0-248-g9c568a543
Operating system type + version
Windows 10
Behavior
CGode file is first created in "c:\Users\USERNAME\AppData\Local\Temp.16312.gcode", instead of "save-as" location.
So, original "save-to" location is lost while post processing. This is most annoying!!!!
Got a Post Processing Script defined.
Python script (creates backup and changes original)
C# script (copies and renames "save-as" gcode file)
So, now all my post processing has gone down the drain....
How is post processing supposed to work nowadays?
The text was updated successfully, but these errors were encountered: