From e309029b28a1484b6dc5992b07a9849c3cf5d8d9 Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Sat, 5 Sep 2026 01:02:28 -0400 Subject: [PATCH] Fix extruder-map bounds, popover lifetime, and arrange thread safety Review findings on the IDEX/IQEX parallel printing code, all in paths the feature owns. - physical_extruder_map lookups used ConfigOptionVector::get_at(), which clamps an out-of-range index to values.front() rather than reporting a miss. The map holds one entry per nozzle while filament ids index slots, and nothing caps the slot count at the nozzle count, so a project authored with more filaments than the printer has extruders silently addressed the primary's head: pressure advance pinned to the wrong carriage, and skip-primary loops suppressing whichever head sat at pem[0]. Bounds-check at all four sites and treat the miss as "no mapping" (-1). Covered by a new imex_pem_tool_for test; the header note now warns against get_at here. - IMEXFilamentPickerPopover leaked a top-level window per ghost click: wxPopupTransientWindow::Dismiss() only hides, and never reaches OnDismiss(). Destroy from an OnDismiss() override and dismiss the picker through DismissAndNotify(), which is the path a successful pick takes. - ArrangeJob read PartPlate's IMEX zone cache from the worker thread, where a cache miss rebuilds GLModel members with no GL context current while the GUI thread may be painting them. Snapshot the zones in prepare(), on the main thread, already converted to plate-local coordinates. - The mode grid anchored its row window to the Primary's gantry row. A window as tall as the grid can only start at row 0, so this drew tiles for tools that do not exist and hid real ones. Render the whole grid instead; a Primary outside it is a data problem the zone layout already reports. - Build the mode tooltip from one format string rather than two catalog fragments concatenated around a runtime value, so translators can move the mode name within the sentence, and register IMEXModesCtrl.cpp for string extraction. Co-Authored-By: Claude Opus 5 (1M context) --- localization/i18n/list.txt | 1 + src/libslic3r/GCode.cpp | 35 +++++++-- src/libslic3r/IMEXHelpers.cpp | 14 +++- src/libslic3r/IMEXHelpers.hpp | 8 +- src/slic3r/GUI/IMEXFilamentPickerPopover.cpp | 13 +++- src/slic3r/GUI/IMEXFilamentPickerPopover.hpp | 7 ++ src/slic3r/GUI/IMEXModesCtrl.cpp | 20 +++-- src/slic3r/GUI/Jobs/ArrangeJob.cpp | 77 +++++++++++--------- src/slic3r/GUI/Jobs/ArrangeJob.hpp | 18 +++++ src/slic3r/GUI/PartPlate.cpp | 16 +++- tests/libslic3r/test_imex_helpers.cpp | 15 ++++ 11 files changed, 170 insertions(+), 54 deletions(-) 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); +}