From e47e487fe22e9b2c48f116f8c15b8cb479256e74 Mon Sep 17 00:00:00 2001 From: Slic3rPE Date: Wed, 11 Mar 2020 09:10:25 +0100 Subject: [PATCH] Fix of a crash on OSX after changing a layer range value and then pressing the "add layer" button. --- src/slic3r/GUI/GUI_ObjectLayers.cpp | 70 +++++++++--------- src/slic3r/GUI/GUI_ObjectLayers.hpp | 20 +++++- src/slic3r/GUI/GUI_ObjectList.cpp | 106 ++++++++++++++++------------ src/slic3r/GUI/GUI_ObjectList.hpp | 17 ++++- src/slic3r/GUI/Mouse3DHandlerMac.mm | 5 +- 5 files changed, 132 insertions(+), 86 deletions(-) diff --git a/src/slic3r/GUI/GUI_ObjectLayers.cpp b/src/slic3r/GUI/GUI_ObjectLayers.cpp index d209214ae..d62bb2727 100644 --- a/src/slic3r/GUI/GUI_ObjectLayers.cpp +++ b/src/slic3r/GUI/GUI_ObjectLayers.cpp @@ -58,7 +58,7 @@ void ObjectLayers::select_editor(LayerRangeEditor* editor, const bool is_last_ed } } -wxSizer* ObjectLayers::create_layer(const t_layer_height_range& range) +wxSizer* ObjectLayers::create_layer(const t_layer_height_range& range, PlusMinusButton *delete_button, PlusMinusButton *add_button) { const bool is_last_edited_range = range == m_selectable_range; @@ -79,8 +79,8 @@ wxSizer* ObjectLayers::create_layer(const t_layer_height_range& range) // Add control for the "Min Z" - auto editor = new LayerRangeEditor(this, double_to_string(range.first), etMinZ, - set_focus_data, [range, update_focus_data, this](coordf_t min_z, bool enter_pressed) + auto editor = new LayerRangeEditor(this, double_to_string(range.first), etMinZ, set_focus_data, + [range, update_focus_data, this, delete_button, add_button](coordf_t min_z, bool enter_pressed, bool dont_update_ui) { if (fabs(min_z - range.first) < EPSILON) { m_selection_type = etUndef; @@ -89,10 +89,14 @@ wxSizer* ObjectLayers::create_layer(const t_layer_height_range& range) // data for next focusing coordf_t max_z = min_z < range.second ? range.second : min_z + 0.5; - const t_layer_height_range& new_range = { min_z, max_z }; + const t_layer_height_range new_range = { min_z, max_z }; + if (delete_button) + delete_button->range = new_range; + if (add_button) + add_button->range = new_range; update_focus_data(new_range, etMinZ, enter_pressed); - return wxGetApp().obj_list()->edit_layer_range(range, new_range); + return wxGetApp().obj_list()->edit_layer_range(range, new_range, dont_update_ui); }); select_editor(editor, is_last_edited_range); @@ -100,8 +104,8 @@ wxSizer* ObjectLayers::create_layer(const t_layer_height_range& range) // Add control for the "Max Z" - editor = new LayerRangeEditor(this, double_to_string(range.second), etMaxZ, - set_focus_data, [range, update_focus_data, this](coordf_t max_z, bool enter_pressed) + editor = new LayerRangeEditor(this, double_to_string(range.second), etMaxZ, set_focus_data, + [range, update_focus_data, this, delete_button, add_button](coordf_t max_z, bool enter_pressed, bool dont_update_ui) { if (fabs(max_z - range.second) < EPSILON || range.first > max_z) { m_selection_type = etUndef; @@ -110,9 +114,13 @@ wxSizer* ObjectLayers::create_layer(const t_layer_height_range& range) // data for next focusing const t_layer_height_range& new_range = { range.first, max_z }; + if (delete_button) + delete_button->range = new_range; + if (add_button) + add_button->range = new_range; update_focus_data(new_range, etMaxZ, enter_pressed); - return wxGetApp().obj_list()->edit_layer_range(range, new_range); + return wxGetApp().obj_list()->edit_layer_range(range, new_range, dont_update_ui); }); select_editor(editor, is_last_edited_range); @@ -120,9 +128,8 @@ wxSizer* ObjectLayers::create_layer(const t_layer_height_range& range) // Add control for the "Layer height" - editor = new LayerRangeEditor(this, - double_to_string(m_object->layer_config_ranges[range].option("layer_height")->getFloat()), - etLayerHeight, set_focus_data, [range, this](coordf_t layer_height, bool) + editor = new LayerRangeEditor(this, double_to_string(m_object->layer_config_ranges[range].option("layer_height")->getFloat()), etLayerHeight, set_focus_data, + [range, this](coordf_t layer_height, bool, bool) { return wxGetApp().obj_list()->edit_layer_range(range, layer_height); }); @@ -141,30 +148,27 @@ wxSizer* ObjectLayers::create_layer(const t_layer_height_range& range) return sizer; } - + void ObjectLayers::create_layers_list() { for (const auto layer : m_object->layer_config_ranges) { const t_layer_height_range& range = layer.first; - auto sizer = create_layer(range); - - auto del_btn = new ScalableButton(m_parent, wxID_ANY, m_bmp_delete); + auto del_btn = new PlusMinusButton(m_parent, m_bmp_delete, range); del_btn->SetToolTip(_(L("Remove layer range"))); - - sizer->Add(del_btn, 0, wxRIGHT | wxLEFT, em_unit(m_parent)); - - del_btn->Bind(wxEVT_BUTTON, [range](wxEvent &) { - wxGetApp().obj_list()->del_layer_range(range); - }); - - auto add_btn = new ScalableButton(m_parent, wxID_ANY, m_bmp_add); + auto add_btn = new PlusMinusButton(m_parent, m_bmp_add, range); add_btn->SetToolTip(_(L("Add layer range"))); + auto sizer = create_layer(range, del_btn, add_btn); + sizer->Add(del_btn, 0, wxRIGHT | wxLEFT, em_unit(m_parent)); sizer->Add(add_btn); - add_btn->Bind(wxEVT_BUTTON, [range](wxEvent &) { - wxGetApp().obj_list()->add_layer_range_after_current(range); + del_btn->Bind(wxEVT_BUTTON, [del_btn](wxEvent &) { + wxGetApp().obj_list()->del_layer_range(del_btn->range); + }); + + add_btn->Bind(wxEVT_BUTTON, [add_btn](wxEvent &) { + wxGetApp().obj_list()->add_layer_range_after_current(add_btn->range); }); } } @@ -204,7 +208,7 @@ void ObjectLayers::update_layers_list() if (type & itLayerRoot) create_layers_list(); else - create_layer(objects_ctrl->GetModel()->GetLayerRangeByItem(item)); + create_layer(objects_ctrl->GetModel()->GetLayerRangeByItem(item), nullptr, nullptr); m_parent->Layout(); } @@ -255,7 +259,7 @@ void ObjectLayers::msw_rescale() { wxSizerItem* b_item = item->GetSizer()->GetItem(btn); if (b_item->IsWindow()) { - ScalableButton* button = dynamic_cast(b_item->GetWindow()); + auto button = dynamic_cast(b_item->GetWindow()); if (button != nullptr) button->msw_rescale(); } @@ -275,7 +279,7 @@ LayerRangeEditor::LayerRangeEditor( ObjectLayers* parent, const wxString& value, EditorType type, std::function set_focus_data_fn, - std::function edit_fn + std::function edit_fn ) : m_valid_value(value), m_type(type), @@ -293,13 +297,13 @@ LayerRangeEditor::LayerRangeEditor( ObjectLayers* parent, m_enter_pressed = true; // If LayersList wasn't updated/recreated, we can call wxEVT_KILL_FOCUS.Skip() if (m_type&etLayerHeight) { - if (!edit_fn(get_value(), true)) + if (!edit_fn(get_value(), true, false)) SetValue(m_valid_value); else m_valid_value = double_to_string(get_value()); m_call_kill_focus = true; } - else if (!edit_fn(get_value(), true)) { + else if (!edit_fn(get_value(), true, false)) { SetValue(m_valid_value); m_call_kill_focus = true; } @@ -319,13 +323,13 @@ LayerRangeEditor::LayerRangeEditor( ObjectLayers* parent, #endif // not __WXGTK__ // If LayersList wasn't updated/recreated, we should call e.Skip() if (m_type & etLayerHeight) { - if (!edit_fn(get_value(), false)) + if (!edit_fn(get_value(), false, dynamic_cast(e.GetWindow()) != nullptr)) SetValue(m_valid_value); else m_valid_value = double_to_string(get_value()); e.Skip(); } - else if (!edit_fn(get_value(), false)) { + else if (!edit_fn(get_value(), false, dynamic_cast(e.GetWindow()) != nullptr)) { SetValue(m_valid_value); e.Skip(); } @@ -387,4 +391,4 @@ void LayerRangeEditor::msw_rescale() } } //namespace GUI -} //namespace Slic3r \ No newline at end of file +} //namespace Slic3r diff --git a/src/slic3r/GUI/GUI_ObjectLayers.hpp b/src/slic3r/GUI/GUI_ObjectLayers.hpp index c0de3be4c..08b594910 100644 --- a/src/slic3r/GUI/GUI_ObjectLayers.hpp +++ b/src/slic3r/GUI/GUI_ObjectLayers.hpp @@ -43,7 +43,8 @@ public: const wxString& value = wxEmptyString, EditorType type = etUndef, std::function set_focus_data_fn = [](EditorType) {;}, - std::function edit_fn = [](coordf_t, bool) {return false; } + // callback parameters: new value, from enter, dont't update panel UI (when called from edit field's kill focus handler for the PlusMinusButton) + std::function edit_fn = [](coordf_t, bool, bool) {return false; } ); ~LayerRangeEditor() {} @@ -69,8 +70,23 @@ public: ObjectLayers(wxWindow* parent); ~ObjectLayers() {} + + // Button remembers the layer height range, for which it has been created. + // The layer height range for this button is updated when the low or high boundary of the layer height range is updated + // by the respective text edit field, so that this button emits an action for an up to date layer height range value. + class PlusMinusButton : public ScalableButton + { + public: + PlusMinusButton(wxWindow *parent, const ScalableBitmap &bitmap, std::pair range) : ScalableButton(parent, wxID_ANY, bitmap), range(range) {} + // updated when the text edit field loses focus for any PlusMinusButton. + std::pair range; + }; + void select_editor(LayerRangeEditor* editor, const bool is_last_edited_range); - wxSizer* create_layer(const t_layer_height_range& range); // without_buttons + // Create sizer with layer height range and layer height text edit fields, without buttons. + // If the delete and add buttons are provided, the respective text edit fields will modify the layer height ranges of thes buttons + // on value change, so that these buttons work with up to date values. + wxSizer* create_layer(const t_layer_height_range& range, PlusMinusButton *delete_button, PlusMinusButton *add_button); void create_layers_list(); void update_layers_list(); diff --git a/src/slic3r/GUI/GUI_ObjectList.cpp b/src/slic3r/GUI/GUI_ObjectList.cpp index 961fc2077..4db7b06fe 100644 --- a/src/slic3r/GUI/GUI_ObjectList.cpp +++ b/src/slic3r/GUI/GUI_ObjectList.cpp @@ -2881,76 +2881,84 @@ static double get_max_layer_height(const int extruder_idx) return max_layer_height; } -void ObjectList::add_layer_range_after_current(const t_layer_height_range& current_range) +void ObjectList::add_layer_range_after_current(const t_layer_height_range current_range) { const int obj_idx = get_selected_obj_idx(); - if (obj_idx < 0) return; + if (obj_idx < 0) + // This should not happen. + return; const wxDataViewItem layers_item = GetSelection(); t_layer_config_ranges& ranges = object(obj_idx)->layer_config_ranges; + auto it_range = ranges.find(current_range); + assert(it_range != ranges.end()); + if (it_range == ranges.end()) + // This shoudl not happen. + return; - const t_layer_height_range& last_range = (--ranges.end())->first; - - if (current_range == last_range) + auto it_next_range = it_range; + bool changed = false; + if (++ it_next_range == ranges.end()) { + // Adding a new layer height range after the last one. take_snapshot(_(L("Add Height Range"))); + changed = true; - const t_layer_height_range& new_range = { last_range.second, last_range.second + 2. }; + const t_layer_height_range new_range = { current_range.second, current_range.second + 2. }; ranges[new_range] = get_default_layer_config(obj_idx); add_layer_item(new_range, layers_item); } - else + else if (const std::pair &next_range = it_next_range->first; current_range.second <= next_range.first) { - const t_layer_height_range& next_range = (++ranges.find(current_range))->first; - - if (current_range.second > next_range.first) - return; // range division has no sense - const int layer_idx = m_objects_model->GetItemIdByLayerRange(obj_idx, next_range); - if (layer_idx < 0) - return; - - if (current_range.second == next_range.first) + assert(layer_idx >= 0); + if (layer_idx >= 0) { - const auto old_config = ranges.at(next_range); + if (current_range.second == next_range.first) + { + // Splitting the currnet layer heigth range to two. + const auto old_config = ranges.at(next_range); + const coordf_t delta = (next_range.second - next_range.first); + if (delta >= get_min_layer_height(old_config.opt_int("extruder"))/*0.05f*/) { + const coordf_t midl_layer = next_range.first + 0.5 * delta; + t_layer_height_range new_range = { midl_layer, next_range.second }; - const coordf_t delta = (next_range.second - next_range.first); - if (delta < get_min_layer_height(old_config.opt_int("extruder"))/*0.05f*/) // next range division has no sense - return; + Plater::TakeSnapshot snapshot(wxGetApp().plater(), _(L("Add Height Range"))); + changed = true; - const coordf_t midl_layer = next_range.first + 0.5 * delta; - - t_layer_height_range new_range = { midl_layer, next_range.second }; + // create new 2 layers instead of deleted one + // delete old layer - Plater::TakeSnapshot snapshot(wxGetApp().plater(), _(L("Add Height Range"))); + wxDataViewItem layer_item = m_objects_model->GetItemByLayerRange(obj_idx, next_range); + del_subobject_item(layer_item); - // create new 2 layers instead of deleted one + ranges[new_range] = old_config; + add_layer_item(new_range, layers_item, layer_idx); - // delete old layer + new_range = { current_range.second, midl_layer }; + ranges[new_range] = get_default_layer_config(obj_idx); + add_layer_item(new_range, layers_item, layer_idx); + } + } + else + { + // Filling in a gap between the current and a new layer height range with a new one. + take_snapshot(_(L("Add Height Range"))); + changed = true; - wxDataViewItem layer_item = m_objects_model->GetItemByLayerRange(obj_idx, next_range); - del_subobject_item(layer_item); - - ranges[new_range] = old_config; - add_layer_item(new_range, layers_item, layer_idx); - - new_range = { current_range.second, midl_layer }; - ranges[new_range] = get_default_layer_config(obj_idx); - add_layer_item(new_range, layers_item, layer_idx); + const t_layer_height_range new_range = { current_range.second, next_range.first }; + ranges[new_range] = get_default_layer_config(obj_idx); + add_layer_item(new_range, layers_item, layer_idx); + } } - else - { - take_snapshot(_(L("Add Height Range"))); - - const t_layer_height_range new_range = { current_range.second, next_range.first }; - ranges[new_range] = get_default_layer_config(obj_idx); - add_layer_item(new_range, layers_item, layer_idx); - } } - changed_object(obj_idx); + if (changed) + changed_object(obj_idx); + // The layer range panel is updated even if this function does not change the layer ranges, as the panel update + // may have been postponed from the "kill focus" event of a text field, if the focus was lost for the "add layer" button. // select item to update layers sizer select_item(layers_item); } @@ -2996,7 +3004,7 @@ bool ObjectList::edit_layer_range(const t_layer_height_range& range, coordf_t la return false; } -bool ObjectList::edit_layer_range(const t_layer_height_range& range, const t_layer_height_range& new_range) +bool ObjectList::edit_layer_range(const t_layer_height_range& range, const t_layer_height_range& new_range, bool dont_update_ui) { const int obj_idx = get_selected_obj_idx(); if (obj_idx < 0) return false; @@ -3012,17 +3020,21 @@ bool ObjectList::edit_layer_range(const t_layer_height_range& range, const t_lay ranges.erase(range); ranges[new_range] = config; changed_object(obj_idx); - + wxDataViewItem root_item = m_objects_model->GetLayerRootItem(m_objects_model->GetItemById(obj_idx)); // To avoid update selection after deleting of a selected item (under GTK) // set m_prevent_list_events to true m_prevent_list_events = true; m_objects_model->DeleteChildren(root_item); - if (root_item.IsOk()) + if (root_item.IsOk()) { // create Layer item(s) according to the layer_config_ranges for (const auto& r : ranges) add_layer_item(r.first, root_item); + } + + if (dont_update_ui) + return true; select_item(sel_type&itLayer ? m_objects_model->GetItemByLayerRange(obj_idx, new_range) : root_item); Expand(root_item); diff --git a/src/slic3r/GUI/GUI_ObjectList.hpp b/src/slic3r/GUI/GUI_ObjectList.hpp index edb7d800e..fa39c8b0c 100644 --- a/src/slic3r/GUI/GUI_ObjectList.hpp +++ b/src/slic3r/GUI/GUI_ObjectList.hpp @@ -320,13 +320,26 @@ public: // Remove objects/sub-object from the list void remove(); void del_layer_range(const t_layer_height_range& range); - void add_layer_range_after_current(const t_layer_height_range& current_range); + // Add a new layer height after the current range if possible. + // The current range is shortened and the new range is entered after the shortened current range if it fits. + // If no range fits after the current range, then no range is inserted. + // The layer range panel is updated even if this function does not change the layer ranges, as the panel update + // may have been postponed from the "kill focus" event of a text field, if the focus was lost for the "add layer" button. + // Rather providing the range by a value than by a reference, so that the memory referenced cannot be invalidated. + void add_layer_range_after_current(const t_layer_height_range current_range); void add_layer_item (const t_layer_height_range& range, const wxDataViewItem layers_item, const int layer_idx = -1); bool edit_layer_range(const t_layer_height_range& range, coordf_t layer_height); + // This function may be called when a text field loses focus for a "add layer" or "remove layer" button. + // In that case we don't want to destroy the panel with that "add layer" or "remove layer" buttons, as some messages + // are already planned for them and destroying these widgets leads to crashes at least on OSX. + // In that case the "add layer" or "remove layer" button handlers are responsible for always rebuilding the panel + // even if the "add layer" or "remove layer" buttons did not update the layer spans or layer heights. bool edit_layer_range(const t_layer_height_range& range, - const t_layer_height_range& new_range); + const t_layer_height_range& new_range, + // Don't destroy the panel with the "add layer" or "remove layer" buttons. + bool suppress_ui_update = false); void init_objects(); bool multiple_selection() const ; diff --git a/src/slic3r/GUI/Mouse3DHandlerMac.mm b/src/slic3r/GUI/Mouse3DHandlerMac.mm index eaf580908..b2d07a4e0 100644 --- a/src/slic3r/GUI/Mouse3DHandlerMac.mm +++ b/src/slic3r/GUI/Mouse3DHandlerMac.mm @@ -152,8 +152,9 @@ static void DeviceAdded(uint32_t unused) static void DeviceRemoved(uint32_t unused) { BOOST_LOG_TRIVIAL(info) << "3dx device removed\n"; - assert(m_connected); - assert(! m_device_str.empty()); +// not accessible in a free function +// assert(m_connected); +// assert(! m_device_str.empty()); mouse_3d_controller->disconnected(); }