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

Contact Z distance cannot be changed to values other than the defaults #2752

Closed
CNCKitchen opened this Issue Mar 22, 2015 · 8 comments

Comments

Projects
None yet
4 participants
@CNCKitchen

It is not possible to use other values for the "contact Z distance" for support materials. If I put in a value different to the defaults it is changed back to 0 as soon as a change enter another option.
(1.2.6 on Windows 7 x64)

@alexrj

This comment has been minimized.

Show comment
Hide comment
@alexrj

alexrj Mar 23, 2015

Member

What value are you entering for example?

Member

alexrj commented Mar 23, 2015

What value are you entering for example?

@umfan92

This comment has been minimized.

Show comment
Hide comment
@umfan92

umfan92 Mar 23, 2015

I experienced the same issue. I was just playing around with the settings. I tried everything from 0.10 to 0.18. I also tried to leave the gap equal to one layer height. So if my layer height was 0.16, I tried to leave the gap as 0.16mm as well. After saving the settings and slicing the object, it seems to revert to 0 or 0.2 mm. I'm also using 1.2.6 on Windows 7 x64.

umfan92 commented Mar 23, 2015

I experienced the same issue. I was just playing around with the settings. I tried everything from 0.10 to 0.18. I also tried to leave the gap equal to one layer height. So if my layer height was 0.16, I tried to leave the gap as 0.16mm as well. After saving the settings and slicing the object, it seems to revert to 0 or 0.2 mm. I'm also using 1.2.6 on Windows 7 x64.

@CNCKitchen

This comment has been minimized.

Show comment
Hide comment
@CNCKitchen

CNCKitchen Mar 23, 2015

I set it up just how umfan92 explained. I tried using different distances to assess the change on the support material adhesion and peelability. I used values between 0.1 to 0.3mm but it always reset after leving the tab.

I set it up just how umfan92 explained. I tried using different distances to assess the change on the support material adhesion and peelability. I used values between 0.1 to 0.3mm but it always reset after leving the tab.

@alexrj

This comment has been minimized.

Show comment
Hide comment
@alexrj

alexrj Mar 23, 2015

Member

I suspect this might be a Windows-specific issue.

Member

alexrj commented Mar 23, 2015

I suspect this might be a Windows-specific issue.

@alexrj alexrj added this to the 1.2.7 milestone Mar 23, 2015

@alexrj alexrj added the OS: Windows label Mar 23, 2015

@markwal

This comment has been minimized.

Show comment
Hide comment
@markwal

markwal Mar 24, 2015

Contributor

Repros on Ubuntu 14.04 for me though I had to back off Wx to version 0.9923 (Build.PL only requires 0.9918, but cpanm tried to install 0.9926 which doesn't compile on my machine). I could imagine that it is something to do with the widgets, perhaps.

OS: Ubuntu 14.04
Version 1.2.7-dev: 13b63d0
config.ini: https://www.dropbox.com/s/pelthf400dw1gpv/testconfig.ini?dl=0

Steps:

  • choose Print Settings.Support material
  • type a value other than 0 or 0.2 in Contact Z distance
  • hit "tab" three times

Result:
Contact Z distance snaps to 0 when the control focus leaves pattern spacing. These aren't the only steps that produce the result, but they're a quick and reliable way to get it to happen.

Expected:
No change to contact Z distance.

I'll take a gander and see if I can figure it out.

Contributor

markwal commented Mar 24, 2015

Repros on Ubuntu 14.04 for me though I had to back off Wx to version 0.9923 (Build.PL only requires 0.9918, but cpanm tried to install 0.9926 which doesn't compile on my machine). I could imagine that it is something to do with the widgets, perhaps.

OS: Ubuntu 14.04
Version 1.2.7-dev: 13b63d0
config.ini: https://www.dropbox.com/s/pelthf400dw1gpv/testconfig.ini?dl=0

Steps:

  • choose Print Settings.Support material
  • type a value other than 0 or 0.2 in Contact Z distance
  • hit "tab" three times

Result:
Contact Z distance snaps to 0 when the control focus leaves pattern spacing. These aren't the only steps that produce the result, but they're a quick and reliable way to get it to happen.

Expected:
No change to contact Z distance.

I'll take a gander and see if I can figure it out.

@markwal

This comment has been minimized.

Show comment
Hide comment
@markwal

markwal Mar 24, 2015

Contributor

It's in Slic3r::GUI::OptionsGroup::Field::NumericChoice::set_value:

sub set_value {
    my ($self, $value) = @_;

    $self->disable_change_event(1);

    my $field = $self->wxWindow;
    if ($self->option->gui_flags =~ /\bshow_value\b/) {
        $field->SetValue($value);
    } else {
        if ($self->option->values) {
            # check whether we have a value index
            my $value_idx = first { $self->option->values->[$_] eq $value } 0..$#{$self->option->values};
            if (defined $value_idx) {
                $field->SetSelection($value_idx);
                $self->disable_change_event(0);
                return;
            }
        }
        if ($self->option->labels && $value <= $#{$self->option->labels}) {
#------- next line stomps on the users input
            $field->SetValue($self->option->labels->[$value]);
            $self->disable_change_event(0);
            return;
        }
        $field->SetValue($value);
    }

    $self->disable_change_event(0);
}

I think what is happening here is that it's trying to use the labels (since !show_value) but it's using the value as an index, but the value is actually the measurement in mm, not the index. It rounds any fractional measurement to 0 and you git the first label.

For this field, that whole if seems unnecessary since the SetSelection above will already cover the case where the value matches a label in SetSelection, but perhaps there is a different kind of NumericChoice where there isn't a values array and $value is an index? If so, then the fix would be to make that if an else if? I'll poke around a little more.

Contributor

markwal commented Mar 24, 2015

It's in Slic3r::GUI::OptionsGroup::Field::NumericChoice::set_value:

sub set_value {
    my ($self, $value) = @_;

    $self->disable_change_event(1);

    my $field = $self->wxWindow;
    if ($self->option->gui_flags =~ /\bshow_value\b/) {
        $field->SetValue($value);
    } else {
        if ($self->option->values) {
            # check whether we have a value index
            my $value_idx = first { $self->option->values->[$_] eq $value } 0..$#{$self->option->values};
            if (defined $value_idx) {
                $field->SetSelection($value_idx);
                $self->disable_change_event(0);
                return;
            }
        }
        if ($self->option->labels && $value <= $#{$self->option->labels}) {
#------- next line stomps on the users input
            $field->SetValue($self->option->labels->[$value]);
            $self->disable_change_event(0);
            return;
        }
        $field->SetValue($value);
    }

    $self->disable_change_event(0);
}

I think what is happening here is that it's trying to use the labels (since !show_value) but it's using the value as an index, but the value is actually the measurement in mm, not the index. It rounds any fractional measurement to 0 and you git the first label.

For this field, that whole if seems unnecessary since the SetSelection above will already cover the case where the value matches a label in SetSelection, but perhaps there is a different kind of NumericChoice where there isn't a values array and $value is an index? If so, then the fix would be to make that if an else if? I'll poke around a little more.

@markwal

This comment has been minimized.

Show comment
Hide comment
@markwal

markwal Mar 24, 2015

Contributor

@alexrj If I understand PrintConfig.cpp correctly, there are only three options of this type: "extruder", "fill_density" and "support_material_contact_distance". "fill_density" uses show_value, so takes the first if. "extruder" only has labels, not values, so I conclude that the label lookup by index is for that case. So I propose the following:

diff --git a/lib/Slic3r/GUI/OptionsGroup/Field.pm b/lib/Slic3r/GUI/OptionsGroup/Field.pm
index 13b86cd..b5c8981 100644
--- a/lib/Slic3r/GUI/OptionsGroup/Field.pm
+++ b/lib/Slic3r/GUI/OptionsGroup/Field.pm
@@ -296,7 +296,7 @@ sub set_value {
                 return;
             }
         }
-        if ($self->option->labels && $value <= $#{$self->option->labels}) {
+        elsif ($self->option->labels && $value <= $#{$self->option->labels}) {
             $field->SetValue($self->option->labels->[$value]);
             $self->disable_change_event(0);
             return;

This also matches the logic of the constructor.

While testing out the extruder box before and after this change I found a couple of other bugs, one Windows only, the other in both Ubuntu and Windows, but since they both repro without the fix, I think they aren't regressions of my proposal above. I'll report them separately.

Contributor

markwal commented Mar 24, 2015

@alexrj If I understand PrintConfig.cpp correctly, there are only three options of this type: "extruder", "fill_density" and "support_material_contact_distance". "fill_density" uses show_value, so takes the first if. "extruder" only has labels, not values, so I conclude that the label lookup by index is for that case. So I propose the following:

diff --git a/lib/Slic3r/GUI/OptionsGroup/Field.pm b/lib/Slic3r/GUI/OptionsGroup/Field.pm
index 13b86cd..b5c8981 100644
--- a/lib/Slic3r/GUI/OptionsGroup/Field.pm
+++ b/lib/Slic3r/GUI/OptionsGroup/Field.pm
@@ -296,7 +296,7 @@ sub set_value {
                 return;
             }
         }
-        if ($self->option->labels && $value <= $#{$self->option->labels}) {
+        elsif ($self->option->labels && $value <= $#{$self->option->labels}) {
             $field->SetValue($self->option->labels->[$value]);
             $self->disable_change_event(0);
             return;

This also matches the logic of the constructor.

While testing out the extruder box before and after this change I found a couple of other bugs, one Windows only, the other in both Ubuntu and Windows, but since they both repro without the fix, I think they aren't regressions of my proposal above. I'll report them separately.

@alexrj

This comment has been minimized.

Show comment
Hide comment
@alexrj

alexrj Mar 27, 2015

Member

@markwal, thank you for investigating this. Your patch is indeed correct. I also made a minor fix in the constructor. Maybe this fixes your other issues?

Member

alexrj commented Mar 27, 2015

@markwal, thank you for investigating this. Your patch is indeed correct. I also made a minor fix in the constructor. Maybe this fixes your other issues?

@alexrj alexrj closed this Mar 27, 2015

@alexrj alexrj added Fixed and removed OS: Windows labels Mar 27, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment