Cura icon indicating copy to clipboard operation
Cura copied to clipboard

Re-write of Time Lapse

Open GregValiant opened this issue 1 year ago • 9 comments

An update to the Time Lapse script.

  • Added "insertion frequency" so the user can decide how often to take an image.
  • Added support for relative extrusion.
  • Retract is now a boolean and the settings come from Cura. There won't be a retraction if there is already a retraction.

Description

Type of change

  • [ X] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Test Configuration:

  • Operating System: Windows 10 Pro (Intel processor)
  • Cura 4.13.1 and Cura 5.7.0

Checklist:

  • [ X] My code follows the style guidelines of this project as described in UltiMaker Meta and Cura QML best practices
  • [ X] I have read the Contribution guide
  • [ X] I have commented my code, particularly in hard-to-understand areas
  • [ X] I have uploaded any files required to test this change

GregValiant avatar Apr 20 '24 19:04 GregValiant

hi @GregValiant, I'm not on the cura team anymore. best to request a review of one of the other cura devs. Thanks

casperlamboo avatar Apr 22 '24 10:04 casperlamboo

@casperlamboo thanks for getting back.

I hope all goes well for you in the next phase.

Greg

GregValiant avatar Apr 22 '24 10:04 GregValiant

@wawanbreton could you take look at this next power hours, since @saumyaj3 is currently on holiday

jellespijker avatar May 13 '24 22:05 jellespijker

@wawanbreton The last commit adds support for firmware retraction.

GregValiant avatar May 14 '24 12:05 GregValiant

@wawanbreton Has there been any progress on this?

GregValiant avatar Jul 31 '24 14:07 GregValiant

Hi @GregValiant, sorry for not answering, I'm just back from holiday. I will create tickets to integrate this PR into our backlog for review, and also the one for the other plugin. However, please remember that the Cura team currently has very limited resources (team split up with other projects, and holiday) so this may take some time :grimacing:

wawanbreton avatar Aug 12 '24 14:08 wawanbreton

I understand completely and was not trying to jostle your elbow. I knew 5.8.0 was due to be released and I was just curious if either would get in under the deadline.

GregValiant avatar Aug 12 '24 20:08 GregValiant

No offense taken... It's actually a good thing that you regularly raise some flags, so that we don't forget we have PRs to look at :smiley:

wawanbreton avatar Aug 13 '24 07:08 wawanbreton

I have added the setting from #19556 but I have yet to update this PR as I am waiting to hear back from the other poster.

I'm getting "merge conflicts" so I'm sitting on the revised version.

GregValiant avatar Aug 22 '24 10:08 GregValiant

The 6th commit appears to be correct and includes the change proposed in 19556.

GregValiant avatar Sep 05 '24 02:09 GregValiant

Question: do potential bugs caused by bad behaving other post-processing scripts count as problems, especially when there's an easy fix? Line 173-175 (and 181-182, code reuse smells):

if " E" in line:
    last_e = line.split("E")[1]
    if float(last_e) < float(prev_e):

That assumes that E is the last parameter. In code produced by CuraEngine and Script.putValue() it will be, but if there's a script out there running before this which is crafting its own lines it might not be so polite. If "E" isn't the last parameter then the second item of the split() will be a string which will throw a ValueError when you try and cast it to a float.

The script does catch exceptions, but in doing so it'll stop processing that entire layer, including if there's supposed to be a shot at the end. What's wrong with this? We already know there's an E value, we checked for that.

last_e = float(self.getValue("E"))

Slashee-the-Cow avatar Oct 06 '24 14:10 Slashee-the-Cow

@wawanbreton, The extra spaces after units are needed when the post processor has a lot of settings and the vertical scroll bar is present. The spaces push the text to the left so the scrollbar doesn't occlude the text. I got in the habit of adding them "just in case". Feel free to box them up and send them back to me. (NOTE: Please be advised that I am philosophically opposed to paying for shipping "spaces" from the Netherlands to the US so please don't make them "cash on receipt" or I will refuse to accept them. We don't want to create an international incident.)

@Slashee-the-Cow I saw that but something else caught my eye and I forgot about it.

GregValiant avatar Oct 06 '24 22:10 GregValiant

The spaces push the text to the left so the scrollbar doesn't occlude the text.

Ok, good to know :slightly_smiling_face: if at some point we have to change the UI of the post-processing script, then we shall fix that !

wawanbreton avatar Oct 07 '24 05:10 wawanbreton

@wawanbreton So if I want to put my code where my mouth is and try and push request in a refactored version that I, in my completely biased opinion, think is better:

  1. I'm assuming I'm at least a couple of weeks too late?
  2. How would I do it anyway? Never actually used GitHub for anything but my own single person projects.

Slashee-the-Cow avatar Oct 09 '24 03:10 Slashee-the-Cow

  1. I'm assuming I'm at least a couple of weeks too late?

For 5.9, yes. But you can still start working on this now and it can be integrated in next release.

  1. How would I do it anyway? Never actually used GitHub for anything but my own single person projects.

Just make a fork of this Cura repository, then do whatevere you want on it, and when you are satisfied with your work, create a PR back to this repository

wawanbreton avatar Oct 09 '24 06:10 wawanbreton