diff --git a/localization/i18n/list.txt b/localization/i18n/list.txt index a36ecd5cab..d8bddd9ba5 100644 --- a/localization/i18n/list.txt +++ b/localization/i18n/list.txt @@ -167,6 +167,7 @@ src/slic3r/GUI/OptionsGroup.cpp src/slic3r/GUI/PrintOptionsDialog.cpp src/slic3r/GUI/SafetyOptionsDialog.cpp src/slic3r/GUI/ParamsPanel.cpp +src/slic3r/GUI/IMEXModesCtrl.cpp src/slic3r/GUI/PartPlate.cpp src/slic3r/GUI/Plater.cpp src/slic3r/GUI/Preferences.cpp diff --git a/src/libslic3r/GCode.cpp b/src/libslic3r/GCode.cpp index 60d3c716f4..cf7d18d956 100644 --- a/src/libslic3r/GCode.cpp +++ b/src/libslic3r/GCode.cpp @@ -3418,9 +3418,14 @@ void GCode::_do_export(Print& print, GCodeOutputStream &file, ThumbnailsGenerato const auto plate_head_map = parse_imex_head_filament_map( print.objects().front()->config().imex_head_filament_map.value); const ConfigOptionInts& pem = print.config().physical_extruder_map; - const int primary_physical = pem.values.empty() - ? -1 - : pem.get_at((int)initial_extruder_id); + // Bounds-check rather than get_at(), which clamps out-of-range to values.front(). The + // clamped value is used as a skip-primary sentinel below, so a filament id past the end + // of the map would suppress whichever head sits at pem[0]. -1 matches no head. + const int primary_physical = + ((int) initial_extruder_id >= 0 && + (int) initial_extruder_id < (int) pem.values.size()) + ? pem.values[(int) initial_extruder_id] + : -1; for (int logical : imex_secondary_logical_slots( get_imex_active_tools(print), primary_physical, plate_head_map, pem)) if (logical < (int)is_extruder_used.size()) @@ -3910,7 +3915,15 @@ void GCode::_do_export(Print& print, GCodeOutputStream &file, ThumbnailsGenerato // loop can skip the primary head (which emitted PA via the normal path). // Then pem-invert each active physical head back to its first routed filament // for the PA setting lookup. Guarded on non-empty pem above. - const int initial_physical = m_config.physical_extruder_map.get_at((int)initial_extruder_id); + // Bounds-check rather than get_at(), which clamps out-of-range to values.front(): + // a clamped initial_physical would make this loop skip whichever active head equals + // pem[0], leaving that head with no PA at all. Same reasoning as the second-layer + // temperature loop below. -1 matches no head, so every active head is emitted. + const int initial_physical = + ((int) initial_extruder_id >= 0 && + (int) initial_extruder_id < (int) m_config.physical_extruder_map.values.size()) + ? m_config.physical_extruder_map.values[(int) initial_extruder_id] + : -1; for (int tool_idx : get_imex_active_tools(print)) { // Unlike the second-layer temperature loop, the primary is skipped here: // set_extruder() above already emitted its PA with the pem tool qualifier. @@ -5937,9 +5950,17 @@ LayerResult GCode::process_layer( // transition at all. `tool_idx` is physical; the printing head uses this layer's // own filament, the parallel carriages resolve through the head map. const int num_filament_columns = (int)print.config().nozzle_temperature.values.size(); - const int initial_physical = m_config.physical_extruder_map.values.empty() - ? -1 - : m_config.physical_extruder_map.get_at((int)first_extruder_id); + // Bounds-check rather than get_at(): get_at() clamps to values.front(), which would + // make initial_physical the primary's head for any first_extruder_id past the end of + // the map. A secondary that happens to sit on that head would then take the + // "initial" branch below and be given the wrong filament's transition temperature, + // while never receiving its own. -1 matches no tool, so every head takes the + // resolved path instead. + const int initial_physical = + ((int) first_extruder_id >= 0 && + (int) first_extruder_id < (int) m_config.physical_extruder_map.values.size()) + ? m_config.physical_extruder_map.values[(int) first_extruder_id] + : -1; for (int tool_idx : get_imex_active_tools(print)) { const int logical = (tool_idx == initial_physical) ? (int)first_extruder_id diff --git a/src/libslic3r/IMEXHelpers.cpp b/src/libslic3r/IMEXHelpers.cpp index 6ffb17a420..6234863f46 100644 --- a/src/libslic3r/IMEXHelpers.cpp +++ b/src/libslic3r/IMEXHelpers.cpp @@ -60,7 +60,17 @@ int imex_pem_tool_for(int filament_id, const std::string& parallel_mode, const C const bool imex_parallel = !parallel_mode.empty() && parallel_mode != kImexPrimaryMode; if (!imex_parallel || pem.values.empty()) return -1; - return pem.get_at(filament_id); + // Bounds-check rather than get_at(), for the same reason imex_physical_heater_for() below + // does: get_at() CLAMPS to values.front(), so a filament id past the end of the map would + // silently address physical head pem[0] instead of reporting "no mapping". The map is one + // entry per NOZZLE (effective_physical_extruder_map), while filament ids index filament + // SLOTS, and nothing caps the slot count at the nozzle count -- set_num_filaments() takes + // its size from the project, so opening a project authored with more filaments than this + // printer has extruders leaves ids past the end. Emitting no tool qualifier is correct + // there; pinning PA onto the primary's carriage is not. + if (filament_id < 0 || filament_id >= (int) pem.values.size()) + return -1; + return pem.values[filament_id]; } int imex_physical_heater_for(bool is_imex, const ConfigOptionInts& pem, int logical_id) @@ -97,7 +107,7 @@ std::string imex_multicolor_block_reason(const std::string& parallel_mode, std::unordered_set seen_physicals; for (int filament : used_filaments_0b) { if (filament < 0 || filament >= (int)pem.values.size()) continue; - const int phys = pem.get_at(filament); + const int phys = pem.values[filament]; // bounds-checked above; get_at would clamp if (!seen_physicals.insert(phys).second) { return "Multi-color prints in IMEX parallel modes require each filament " "to have its own dedicated physical extruder. Two or more of the active " diff --git a/src/libslic3r/IMEXHelpers.hpp b/src/libslic3r/IMEXHelpers.hpp index e862b86b21..1bb08ad323 100644 --- a/src/libslic3r/IMEXHelpers.hpp +++ b/src/libslic3r/IMEXHelpers.hpp @@ -113,8 +113,12 @@ std::vector imex_mode_table(const ConfigBase& cfg); // resolve_filament_for_head(plate_head_filament_map, pem, physical_idx) // (or the simpler `first_filament_for_physical_head` if no per-plate override). // -// The reverse translation (logical → physical) is `pem.get_at(logical_idx)`, already -// encapsulated in `imex_pem_tool_for` for the per-tool-qualifier case. +// The reverse translation (logical → physical) is `imex_pem_tool_for`. Do NOT reach for +// `pem.get_at(logical_idx)`: get_at() CLAMPS an out-of-range index to values.front(), silently +// answering "physical head pem[0]" for a logical id that has no mapping at all. The map is one +// entry per nozzle while logical ids index filament slots, and nothing caps the slot count at +// the nozzle count, so out-of-range is reachable. Bounds-check and treat the miss as "no +// mapping" (-1 for a tool qualifier, which every caller renders as "emit none"). // // Past bugs in this class: // - GCode PA emission used the inline `pem.get_at(filament_id)` form at two diff --git a/src/slic3r/GUI/IMEXFilamentPickerPopover.cpp b/src/slic3r/GUI/IMEXFilamentPickerPopover.cpp index 6a3fbf8988..418765e2cb 100644 --- a/src/slic3r/GUI/IMEXFilamentPickerPopover.cpp +++ b/src/slic3r/GUI/IMEXFilamentPickerPopover.cpp @@ -39,6 +39,14 @@ void IMEXFilamentPickerPopover::popup_at_cursor() Popup(); } +void IMEXFilamentPickerPopover::OnDismiss() +{ + wxPopupTransientWindow::OnDismiss(); + // No CallAfter needed: wxPopupTransientWindowBase::Destroy() already defers by appending to + // wxPendingDelete, and guards a second call with a wxCHECK rather than double-freeing. + Destroy(); +} + void IMEXFilamentPickerPopover::build_row() { m_root_sizer->Clear(true); @@ -100,7 +108,10 @@ void IMEXFilamentPickerPopover::build_row() int sel = choice->GetSelection(); if (sel < 0 || sel >= (int)lane_logicals.size()) return; on_filament_selected(lane_logicals[sel] + 1); - Dismiss(); + // DismissAndNotify(), not Dismiss(): Dismiss() is only PopHandlers()+Hide() and never + // reaches OnDismiss(), so destroying from OnDismiss() would miss this path entirely -- + // which is the one users take on every successful pick. + DismissAndNotify(); }); m_root_sizer->Add(choice, 0, wxALIGN_CENTER_VERTICAL | wxALL, 6); diff --git a/src/slic3r/GUI/IMEXFilamentPickerPopover.hpp b/src/slic3r/GUI/IMEXFilamentPickerPopover.hpp index 43f190018c..85884d76a4 100644 --- a/src/slic3r/GUI/IMEXFilamentPickerPopover.hpp +++ b/src/slic3r/GUI/IMEXFilamentPickerPopover.hpp @@ -26,6 +26,13 @@ public: void popup_at_cursor(); private: + // wxPopupTransientWindow::Dismiss() only hides the window; it does not destroy it, and it + // does not call OnDismiss() either -- only DismissAndNotify() does. The ghost-click handler + // creates one popover per click and keeps no reference, so without destroying here every + // click would leak a live top-level window (with its BitmapComboBox, bitmaps and event + // bindings) parented to the GL canvas. The filament-selected handler therefore calls + // DismissAndNotify(), not Dismiss(), or it would bypass this entirely. + void OnDismiss() override; void build_row(); void on_filament_selected(int slot_1_based); diff --git a/src/slic3r/GUI/IMEXModesCtrl.cpp b/src/slic3r/GUI/IMEXModesCtrl.cpp index a07d95191c..d3bcf27629 100644 --- a/src/slic3r/GUI/IMEXModesCtrl.cpp +++ b/src/slic3r/GUI/IMEXModesCtrl.cpp @@ -408,20 +408,24 @@ void IMEXModesCtrl::add_row(const std::string& name, // raw_row = flip_y ? (n_rows-1-row) : row // raw_col = flip_x ? (n_cols-1-col) : col // - // row_start: anchor displayed rows to the gantry row containing the Primary - // assignment, so reducing gantry count keeps the meaningful row visible. - int primary_gantry_row = 0; - for (const auto& [idx, role] : tool_roles) { - if (role == ImexRole::Primary) { primary_gantry_row = idx / m_n_cols; break; } - } - int row_start = primary_gantry_row; // display rows [row_start .. row_start+m_n_rows-1] + // The displayed window is always the whole grid: m_n_rows is the gantry count, so valid tool + // indices are 0 .. m_n_rows*m_n_cols-1 and rows 0 .. m_n_rows-1. A window m_n_rows tall can + // only lie entirely inside the grid when it starts at row 0. + // + // This used to anchor the window's first row to the Primary's gantry row, to "keep the + // meaningful row visible" when the gantry count shrank. That cannot work: shifting a + // full-height window forward walks it off the end, so it drew tiles for tools that do not + // exist and hid real ones, and clicking a phantom tile wrote a tool index that + // compute_imex_zone_layout() (IMEXZones.cpp:76) then discards. A Primary sitting outside the + // current grid is a data problem -- only reachable from a hand-authored mode string, since + // the editor will not move Primary off tool 0 -- and the zone layout already answers it by + // producing no zones at all. Showing the real grid is the honest rendering of that state. bool flip_x = (m_layout == 1 || m_layout == 3); bool flip_y = (m_layout == 2 || m_layout == 3); for (int row = m_n_rows - 1; row >= 0; --row) { for (int col = 0; col < m_n_cols; ++col) { int raw_row = flip_y ? (m_n_rows - 1 - row) : row; - raw_row += row_start; // anchor to Primary's gantry row int raw_col = flip_x ? (m_n_cols - 1 - col) : col; int tool_idx = raw_row * m_n_cols + raw_col; std::optional role; // nullopt == Inactive diff --git a/src/slic3r/GUI/Jobs/ArrangeJob.cpp b/src/slic3r/GUI/Jobs/ArrangeJob.cpp index 8a769c9b57..0362008593 100644 --- a/src/slic3r/GUI/Jobs/ArrangeJob.cpp +++ b/src/slic3r/GUI/Jobs/ArrangeJob.cpp @@ -448,6 +448,22 @@ void ArrangeJob::prepare() Model::setExtruderParams(config, numExtruders); Model::setPrintSpeedTable(config, print_config); + // Snapshot the IMEX zones here, on the main thread, so process() never has to touch + // PartPlate's IMEX cache. See the members' declaration for why that matters. + m_imex_primary_zone_local.reset(); + m_imex_collision_zones_local.clear(); + if (PartPlate* curr_plate = m_plater->get_partplate_list().get_curr_plate()) { + if (auto pz = curr_plate->imex_primary_zone()) { + const Vec3d plate_origin = curr_plate->get_origin(); + const double ox = plate_origin.x(), oy = plate_origin.y(); + m_imex_primary_zone_local = BoundingBoxf(Vec2d(pz->min.x() - ox, pz->min.y() - oy), + Vec2d(pz->max.x() - ox, pz->max.y() - oy)); + for (const BoundingBoxf3& cz : curr_plate->imex_collision_zones()) + m_imex_collision_zones_local.emplace_back(Vec2d(cz.min.x() - ox, cz.min.y() - oy), + Vec2d(cz.max.x() - ox, cz.max.y() - oy)); + } + } + int state = m_plater->get_prepare_state(); if (state == Job::JobPrepareState::PREPARE_STATE_DEFAULT) { only_on_partplate = false; @@ -541,39 +557,34 @@ void ArrangeJob::process(Ctl &ctl) // When an IDEX/IQEX parallel mode is active, constrain auto-arrange to the primary zone only // and treat carriage collision strips as hard excluded regions. - // NOTE: m_imex_primary_zone_box and imex_collision_zones() are in global (world) coordinates - // because they are derived from m_shape which includes the plate origin offset. The arranger - // always works in plate-local space (origin = 0,0), so we subtract the plate origin here. - if (PartPlate* curr_plate = partplate_list.get_curr_plate()) { - if (auto pz = curr_plate->imex_primary_zone()) { - Vec3d plate_origin = curr_plate->get_origin(); - double ox = plate_origin.x(), oy = plate_origin.y(); - BoundingBoxf pz_local(Vec2d(pz->min.x() - ox, pz->min.y() - oy), - Vec2d(pz->max.x() - ox, pz->max.y() - oy)); - BoundingBox scaled_pz = scaled(pz_local); - bedpts = { - { scaled_pz.min.x(), scaled_pz.min.y() }, - { scaled_pz.max.x(), scaled_pz.min.y() }, - { scaled_pz.max.x(), scaled_pz.max.y() }, - { scaled_pz.min.x(), scaled_pz.max.y() }, - }; - for (const BoundingBoxf3& cz : curr_plate->imex_collision_zones()) { - Polygon poly({ - { scaled(cz.min.x() - ox), scaled(cz.min.y() - oy) }, - { scaled(cz.max.x() - ox), scaled(cz.min.y() - oy) }, - { scaled(cz.max.x() - ox), scaled(cz.max.y() - oy) }, - { scaled(cz.min.x() - ox), scaled(cz.max.y() - oy) }, - }); - arrangement::ArrangePolygon ap; - ap.poly.contour = poly; - ap.translation = Vec2crd(0, 0); - ap.rotation = 0.0; - ap.is_virt_object = true; - ap.bed_idx = current_plate_index; - ap.height = 1; - ap.name = "IMEXCollisionZone"; - m_unselected.emplace_back(std::move(ap)); - } + // NOTE: the plate's zone boxes are in global (world) coordinates because they derive from + // m_shape, which includes the plate origin offset. The arranger works in plate-local space + // (origin = 0,0), so the plate origin is subtracted when the snapshot is taken in prepare(); + // m_imex_primary_zone_local / m_imex_collision_zones_local are already plate-local here. + if (const auto& pz = m_imex_primary_zone_local) { + BoundingBox scaled_pz = scaled(*pz); + bedpts = { + { scaled_pz.min.x(), scaled_pz.min.y() }, + { scaled_pz.max.x(), scaled_pz.min.y() }, + { scaled_pz.max.x(), scaled_pz.max.y() }, + { scaled_pz.min.x(), scaled_pz.max.y() }, + }; + for (const BoundingBoxf& cz : m_imex_collision_zones_local) { + Polygon poly({ + { scaled(cz.min.x()), scaled(cz.min.y()) }, + { scaled(cz.max.x()), scaled(cz.min.y()) }, + { scaled(cz.max.x()), scaled(cz.max.y()) }, + { scaled(cz.min.x()), scaled(cz.max.y()) }, + }); + arrangement::ArrangePolygon ap; + ap.poly.contour = poly; + ap.translation = Vec2crd(0, 0); + ap.rotation = 0.0; + ap.is_virt_object = true; + ap.bed_idx = current_plate_index; + ap.height = 1; + ap.name = "IMEXCollisionZone"; + m_unselected.emplace_back(std::move(ap)); } } diff --git a/src/slic3r/GUI/Jobs/ArrangeJob.hpp b/src/slic3r/GUI/Jobs/ArrangeJob.hpp index 0c9f03de01..6039796133 100644 --- a/src/slic3r/GUI/Jobs/ArrangeJob.hpp +++ b/src/slic3r/GUI/Jobs/ArrangeJob.hpp @@ -6,6 +6,7 @@ #include "Job.hpp" #include "libslic3r/Arrange.hpp" +#include "libslic3r/BoundingBox.hpp" namespace Slic3r { @@ -26,6 +27,23 @@ class ArrangeJob : public Job std::map m_selected_groups; // groups of selected items for sequential printing std::vector m_uncompatible_plates; // plate indices with different printing sequence than global + // IMEX zone snapshot, taken on the main thread in prepare(). + // + // process() runs on the worker thread (see Job::process). PartPlate::imex_primary_zone() is + // non-const and calls ensure_imex_zones(), which reads wxGetApp().preset_bundle -- main-thread + // GUI state -- on every call, and on a cache miss calls calc_imex_zones(), which destroys and + // rebuilds std::vector members. ~GLModel issues glDeleteBuffers/glDeleteVertexArrays, + // and no GL context is current on the worker, while the GUI thread may be painting those same + // vectors from GLCanvas3D::on_paint. (imex_collision_zones() is itself const and merely reads + // the member -- but it is only valid once imex_primary_zone() has warmed the cache, so it + // cannot be moved off the main thread on its own.) Snapshotting plain geometry here keeps + // every one of those touches on the main thread. + // + // Both are stored already converted to plate-local coordinates, which is the space the + // arranger works in. + std::optional m_imex_primary_zone_local; + std::vector m_imex_collision_zones_local; + arrangement::ArrangeParams params; int current_plate_index = 0; Polygon bed_poly; diff --git a/src/slic3r/GUI/PartPlate.cpp b/src/slic3r/GUI/PartPlate.cpp index f0b99cf835..0eb047137e 100644 --- a/src/slic3r/GUI/PartPlate.cpp +++ b/src/slic3r/GUI/PartPlate.cpp @@ -2126,7 +2126,21 @@ void PartPlate::render_icons(bool bottom, bool only_name, int hover_id) render_icon_texture(m_imex_mode_icon.model, m_partplate_list->m_imex_mode_hovered_texture); std::string cur = get_imex_mode(); if (cur == kImexPrimaryMode) cur = _u8L("Primary"); - show_tooltip(_u8L("IDEX/IQEX mode: ") + cur + _u8L(" (left-click to cycle, right-click for menu)")); + // One format string, not two catalog fragments concatenated around a + // runtime value: translators need to move the mode name within the + // sentence, and the space-padded fragments were untranslatable alone. + // + // Guarded because this runs on the paint path: boost::format throws if a + // TRANSLATED string drops or malforms %1%, and an exception escaping here + // would take down the frame rather than show a wrong tooltip. The English + // literal is the fallback and cannot itself throw. + std::string imex_tip; + try { + imex_tip = (boost::format(_u8L("IDEX/IQEX mode: %1% (left-click to cycle, right-click for menu)")) % cur).str(); + } catch (const std::exception&) { + imex_tip = (boost::format("IDEX/IQEX mode: %1% (left-click to cycle, right-click for menu)") % cur).str(); + } + show_tooltip(imex_tip); } else { render_icon_texture(m_imex_mode_icon.model, m_partplate_list->m_imex_mode_texture); } diff --git a/tests/libslic3r/test_imex_helpers.cpp b/tests/libslic3r/test_imex_helpers.cpp index 05c98b9a5a..eb6ce1d5ab 100644 --- a/tests/libslic3r/test_imex_helpers.cpp +++ b/tests/libslic3r/test_imex_helpers.cpp @@ -1505,3 +1505,18 @@ TEST_CASE("imex_resolve_routing - routed_heads is sorted, deduplicated and never CHECK(all_out.routed_heads.empty()); CHECK(all_out.primary_unrouted); } + +TEST_CASE("imex_pem_tool_for - a filament id past the end of the map has no tool", "[IMEX]") { + // See the note on imex_pem_tool_for in IMEXHelpers.hpp for why get_at() is wrong here: it + // clamps an out-of-range id to values.front(), which would pin that filament's pressure + // advance onto the primary's carriage instead of reporting "no mapping". + // + // -1 differs deliberately from imex_physical_heater_for(), which returns the logical id + // unchanged when out of range: that one must still name SOME heater, while a PA qualifier + // can simply be omitted. + const auto pem = make_pem({0, 0, 1, 2}); // 4 nozzles; slots 0-1 share head 0 + + REQUIRE(imex_pem_tool_for(4, "copy_mode", pem) == -1); + REQUIRE(imex_pem_tool_for(9, "copy_mode", pem) == -1); + REQUIRE(imex_pem_tool_for(-1, "copy_mode", pem) == -1); +}