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

Add pause-at-layer functionality for 3092 #4177

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

goofdad
Copy link

@goofdad goofdad commented Oct 31, 2017

Added 2 new configuration parameters: pause_gcode and pause_heights.

pause_gcode is under the Custom G-Code section of Printer Configuration.
pause_heights is under Layers and perimeters section of Print Configuration.

also added a test case.

Copy link
Member

@lordofhyphens lordofhyphens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that a test was added, but there are a few usability questions/issues.

Philosophical Questions

With custom conditional gcode, this is basically a shortcut to adding an (if [layer_num] == x) to before_layer_gcode (or after_layer_gcode) statements.

Does it come up enough to warrant yet another custom Gcode configuration option?

UI

While pausing at layer height X will probably be the most common use-case for this, it is more generically a before_layer_gcode that is only applied to specified Z heights. I think the configuration option name should suit this.

@@ -355,6 +357,8 @@ class GCodeConfig : public virtual StaticPrintConfig
}

virtual ConfigOption* optptr(const t_config_option_key &opt_key, bool create = false) {
OPT_PTR(pause_gcode);
OPT_PTR(pause_heights);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move these so that they're in alphabetical order in the list.

@@ -314,6 +314,8 @@ class PrintRegionConfig : public virtual StaticPrintConfig
class GCodeConfig : public virtual StaticPrintConfig
{
public:
ConfigOptionString pause_gcode;
ConfigOptionString pause_heights;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move these so that they're in alphabetical order in the list.

@@ -453,6 +453,16 @@ sub process_layer {
$pp->set('layer_z' => $layer->print_z);
$gcode .= Slic3r::ConditionalGCode::apply_math($pp->process($self->print->config->layer_gcode) . "\n");
}
if ($self->print->config->pause_gcode && $self->print->config->pause_heights) {
foreach my $height (split / /, $self->print->config->pause_heights) {
if ($height && $height eq $layer->print_z) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be appropriate to round the effective height (either up or down, down may be the safer option) if the current layer doesn't land on the prescribed height?

It's something that can occur often with adaptive slicing turned on or if there's some oddball floating point math going on internally.

Obviously you'd want to extend the tooltip appropriately.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to play with how this interacts with Adaptive Slicing, but my gut says no. I also work with ideamaker, which has this feature and does such rouding ... and the end result is that I have to manually open my gcode after export to make sure it paused where I wanted it to. They have multiple bugs open because of this. (now ... I have to open the gcode anyway because they don't allow me to customize the code inserted, but that's a different story).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since logically this is intended to be a before-layer gcode, using floor() would seem to be safe (as opposed to round(), which could go up or down and definitely could confuse people).

That is, if layer 5 spans z=0.1 and z=0.3 and z=0.2 was requested, it would be inserted at 0.1. If z=0.299999999 was requested, it'd still be at z=0.1.

@lordofhyphens
Copy link
Member

Implementing #4178 is another route to getting this PR with this functionality in ;)

@goofdad
Copy link
Author

goofdad commented Nov 1, 2017

I'll look at some changes in the morning after coffee (never code before coffee!).
I'll also take a look at #4178 because I agree ... the UI is getting cluttered!

@goofdad
Copy link
Author

goofdad commented Nov 1, 2017

In response to your Philosophical Question:
For me, the workflow warrants being separate and not an if statement. Philosophically, the actual code being inserted is printer specific, and thus belongs in the "Printer" config, while the layers needed is part of this specific print, and thus belongs in the "Print" config. I wouldn't want to need to go mucking about with my Printer config for each and every print where I wanted a pause.

@lordofhyphens
Copy link
Member

lordofhyphens commented Nov 1, 2017

@goofdad I did write #4178 with your use-case in mind ;)

I also just edited the main description to put some notes on some places that would need to be touched to implement this (not an exhaustive list).

I may have been thinking about collapsing Custom GCode into one UI element for months now (at least how it would work from the user's perspective).

@ledvinap
Copy link
Collaborator

ledvinap commented Nov 1, 2017

One possibility is to replace pause_heights with mark_heights and allow G-code scripting to access mark status (possibly with set of mark values).
Then before_layer_gcode would just use {if [layer_mark]} (or {if [layer_mark] == 2} / {if [layer_mark] & 0x04})
Similar mechanism could be used for example in other g-code parts (toolchange gcode - pause before toolchange on specific layer)

@goofdad
Copy link
Author

goofdad commented Nov 1, 2017

I actually like the layer mark idea, but I'd like it to be more general. Now I'm contemplating syntax between what belongs on the "Printer" side and what belongs on the "Print" side.

@lordofhyphens
Copy link
Member

lordofhyphens commented Nov 1, 2017 via email

@lordofhyphens
Copy link
Member

@goofdad prod

@goofdad
Copy link
Author

goofdad commented Dec 21, 2017 via email

@lordofhyphens
Copy link
Member

lordofhyphens commented Dec 21, 2017 via email

@lordofhyphens
Copy link
Member

@goofdad still need those changes made to accept.

@lordofhyphens
Copy link
Member

@goofdad :)

@tcurdt
Copy link

tcurdt commented Sep 9, 2018

@goofdad any plans on making those changes? or should someone else take over?

@lordofhyphens
Copy link
Member

@tcurdt Probably should take a swing and/or make PRs against his branch.

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.

4 participants