diff --git a/localization/i18n/list.txt b/localization/i18n/list.txt index d8bddd9ba5..fdb21093c7 100644 --- a/localization/i18n/list.txt +++ b/localization/i18n/list.txt @@ -214,6 +214,7 @@ src/libslic3r/ExtrusionEntity.cpp src/libslic3r/Flow.cpp src/libslic3r/Format/AMF.cpp src/libslic3r/miniz_extension.cpp +src/libslic3r/IMEXHelpers.cpp src/libslic3r/Preset.cpp src/libslic3r/Print.cpp src/libslic3r/PrintBase.cpp diff --git a/src/libslic3r/GCode.cpp b/src/libslic3r/GCode.cpp index cf7d18d956..6abaae9f5b 100644 --- a/src/libslic3r/GCode.cpp +++ b/src/libslic3r/GCode.cpp @@ -3408,7 +3408,7 @@ void GCode::_do_export(Print& print, GCodeOutputStream &file, ThumbnailsGenerato // pollute is_extruder_used with the *first* slot routed to the primary's // physical extruder, which is generally not the slot the user assigned to // the printing object. Same skip-primary pattern as the IMEX PA emission - // path (GCode.cpp:3917). + // path in _do_export(). // // For secondaries: translate physical -> logical via the per-plate // imex_head_filament_map (set by the IMEX ghost picker), with @@ -3418,9 +3418,8 @@ 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; - // 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. + // Bounds-checked, not get_at() -- see IMEXHelpers.hpp. The value is a skip-primary + // sentinel below, so a clamp would suppress whichever head sits at pem[0]. const int primary_physical = ((int) initial_extruder_id >= 0 && (int) initial_extruder_id < (int) pem.values.size()) @@ -3915,22 +3914,29 @@ 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. - // 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. + // Bounds-checked, not get_at() -- see the note on imex_pem_tool_for in + // IMEXHelpers.hpp. A clamped initial_physical would skip whichever active head + // equals pem[0], leaving it with no PA at all. 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; + const int num_pa_filament_columns = std::min( + (int) print.config().enable_pressure_advance.values.size(), + (int) print.config().pressure_advance.values.size()); 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. if (tool_idx == initial_physical) continue; const int logical = resolve_filament_for_head( m_imex_head_filament_map, m_config.physical_extruder_map, tool_idx); - if (logical < 0) continue; + // resolve_filament_for_head() answers in pem's index space -- one entry per + // NOZZLE -- while these two options are indexed per FILAMENT SLOT. The spaces + // diverge when the printer has more nozzles than the project has filaments, and + // get_at() would clamp a past-the-end index to filament 0 and pin its PA onto a + // secondary carriage. Same guard the second-layer temperature loop uses. + if (logical < 0 || logical >= num_pa_filament_columns) continue; if (!print.config().enable_pressure_advance.get_at(logical)) continue; file.write(m_writer.set_pressure_advance( print.config().pressure_advance.get_at(logical), @@ -5950,12 +5956,9 @@ 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(); - // 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. + // Bounds-checked, not get_at() -- see IMEXHelpers.hpp. A clamp would hand the + // "initial" branch below to whichever secondary sits on pem[0], giving it the wrong + // filament's transition temperature and never its own. const int initial_physical = ((int) first_extruder_id >= 0 && (int) first_extruder_id < (int) m_config.physical_extruder_map.values.size()) diff --git a/src/libslic3r/IMEXHelpers.cpp b/src/libslic3r/IMEXHelpers.cpp index 6234863f46..f1a93f1619 100644 --- a/src/libslic3r/IMEXHelpers.cpp +++ b/src/libslic3r/IMEXHelpers.cpp @@ -11,9 +11,13 @@ #include #include "libslic3r/ClipperUtils.hpp" +#include "libslic3r/I18N.hpp" #include "libslic3r/PresetBundle.hpp" #include "libslic3r/PrintConfig.hpp" +// Mark string for localization and translate. +#define L(s) Slic3r::I18N::translate(s) + namespace Slic3r { namespace { @@ -60,14 +64,9 @@ 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; - // 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. + // Bounds-checked, not get_at() -- see the note in IMEXHelpers.hpp for why the two index + // spaces diverge. Emitting no tool qualifier is correct for a miss; 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]; @@ -109,11 +108,11 @@ std::string imex_multicolor_block_reason(const std::string& parallel_mode, if (filament < 0 || filament >= (int)pem.values.size()) continue; 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 " - "filaments are routed to the same physical head via the printer's " - "physical extruder map (an MMU/AFC manifold), which the slaved gantry " - "cannot follow."; + return L("Multi-color prints in IDEX/IQEX parallel modes require each filament " + "to have its own dedicated physical extruder. Two or more of the active " + "filaments are routed to the same physical head via the printer's " + "physical extruder map (an MMU/AFC manifold), which the slaved gantry " + "cannot follow."); } } } @@ -121,9 +120,9 @@ std::string imex_multicolor_block_reason(const std::string& parallel_mode, // Determine which physical head is the primary in this mode. const int primary_physical = imex_primary_tool_for_mode(active_tools_str); if (primary_physical < 0) { - return "The active IMEX mode does not define a primary tool, so multi-color " - "printing cannot be scheduled. Open the printer settings IMEX Modes " - "editor and assign a Primary role to one tool."; + return L("The active IDEX/IQEX mode does not define a primary tool, so multi-color " + "printing cannot be scheduled. Open the printer settings IDEX/IQEX Modes " + "editor and assign a Primary role to one tool."); } // Walk the active tools once: collect the set of distinct gantries spanned by @@ -149,11 +148,11 @@ std::string imex_multicolor_block_reason(const std::string& parallel_mode, // (use Primary mode for that), and the user's mode_gcode would still emit // parallel-print firmware setup that doesn't apply here. if (active_gantries.size() < 2) { - return "Multi-color in this mode isn't a parallel-print scenario — all of the " - "active tools sit on a single gantry, so there's no second gantry being " - "copied or mirrored to. Switch the plate to Primary mode for multi-color " - "printing on a single gantry, or define an IMEX mode that includes " - "tools on a second gantry."; + return L("Multi-color in this mode isn't a parallel-print scenario — all of the " + "active tools sit on a single gantry, so there's no second gantry being " + "copied or mirrored to. Switch the plate to Primary mode for multi-color " + "printing on a single gantry, or define an IDEX/IQEX mode that includes " + "tools on a second gantry."); } // Dual-gantry mode but no Span tool on the primary's gantry — the slicer would @@ -162,11 +161,11 @@ std::string imex_multicolor_block_reason(const std::string& parallel_mode, // copies (each gantry tool prints its own object) which is incompatible with // mid-print multicolor in a parallel mode. if (!span_on_primary_gantry) { - return "Multi-color prints in IMEX parallel modes require a Span tool on the " - "primary's gantry — without one, the mode doesn't declare a within-gantry " - "multicolor partner. Either reduce the print to a single filament, switch to " - "Primary mode, or open the printer settings IMEX Modes editor and mark a " - "tool on the primary's gantry as Span."; + return L("Multi-color prints in IDEX/IQEX parallel modes require a Span tool on the " + "primary's gantry — without one, the mode doesn't declare a within-gantry " + "multicolor partner. Either reduce the print to a single filament, switch to " + "Primary mode, or open the printer settings IDEX/IQEX Modes editor and mark a " + "tool on the primary's gantry as Span."); } return {}; } @@ -507,8 +506,8 @@ int resolve_filament_for_head(const std::map& plate_map, auto it = plate_map.find(physical); if (it != plate_map.end()) { const int zero_based = it->second - 1; - // pem has one entry per logical filament slot, so its size IS the slot count and - // an override outside it names a filament that does not exist. Bounding here rather + // pem is indexed by filament slot here, so an override outside it names a filament + // this printer cannot route. Bounding here rather // than at the parse site is deliberate: the parser is handed a raw string with no // notion of how many filaments the project has, while every consumer of this // function's result indexes a per-filament array. The picker only ever offers diff --git a/src/libslic3r/IMEXHelpers.hpp b/src/libslic3r/IMEXHelpers.hpp index 1bb08ad323..9c58ca7029 100644 --- a/src/libslic3r/IMEXHelpers.hpp +++ b/src/libslic3r/IMEXHelpers.hpp @@ -118,7 +118,16 @@ std::vector imex_mode_table(const ConfigBase& cfg); // 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"). +// mapping" (-1). +// +// The same nozzle-vs-slot divergence bites in the other direction: anything derived from pem +// -- `resolve_filament_for_head`, `first_filament_for_physical_head` -- answers in NOZZLE index +// space, so bound it against the filament array you are about to index before using it as a +// filament id. +// +// -1 as a tool qualifier reaches GCodeWriter::set_pressure_advance, which omits the qualifier +// on Klipper, Marlin and BBL but substitutes the historical `D0` on RepRapFirmware +// (GCodeWriter.cpp, set_pressure_advance) -- deliberate, not a miss. // // 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.hpp b/src/slic3r/GUI/IMEXFilamentPickerPopover.hpp index 85884d76a4..fd81b0e66d 100644 --- a/src/slic3r/GUI/IMEXFilamentPickerPopover.hpp +++ b/src/slic3r/GUI/IMEXFilamentPickerPopover.hpp @@ -26,12 +26,9 @@ 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. + // Destroys the popover: the ghost-click handler creates one per click and keeps no + // reference, so every click would otherwise leak a live top-level window parented to the + // GL canvas. Only DismissAndNotify() reaches this -- plain Dismiss() just hides. 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 d3bcf27629..19c7c7eded 100644 --- a/src/slic3r/GUI/IMEXModesCtrl.cpp +++ b/src/slic3r/GUI/IMEXModesCtrl.cpp @@ -408,18 +408,10 @@ 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 // - // 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. + // Render the whole grid: m_n_rows is the gantry count, so the valid tool indices are + // 0 .. m_n_rows*m_n_cols-1 and a window this tall only fits inside the grid at row 0. + // A Primary outside the grid is a data problem the zone layout already reports by + // producing no zones (compute_imex_zone_layout, IMEXZones.cpp). bool flip_x = (m_layout == 1 || m_layout == 3); bool flip_y = (m_layout == 2 || m_layout == 3); diff --git a/tests/libslic3r/test_imex_helpers.cpp b/tests/libslic3r/test_imex_helpers.cpp index eb6ce1d5ab..6db20c936d 100644 --- a/tests/libslic3r/test_imex_helpers.cpp +++ b/tests/libslic3r/test_imex_helpers.cpp @@ -1520,3 +1520,19 @@ TEST_CASE("imex_pem_tool_for - a filament id past the end of the map has no tool REQUIRE(imex_pem_tool_for(9, "copy_mode", pem) == -1); REQUIRE(imex_pem_tool_for(-1, "copy_mode", pem) == -1); } + +TEST_CASE("resolve_filament_for_head answers in nozzle index space, not filament slots", "[IMEX]") { + // The contract that callers get wrong: the returned index is bounded by pem's length -- + // one entry per NOZZLE -- and NOT by the number of filaments the project has. A caller that + // feeds this straight into a per-filament option must bound it first, or get_at() clamps + // the overflow onto filament 0 (see GCode.cpp's IMEX pressure-advance loop, which does). + const auto pem = make_pem({0, 1, 2, 3}); // 4 nozzles, identity routing + + // Head 3 resolves to index 3 even for a 2-filament project: nothing here knows the + // filament count, so the result can legitimately exceed it. + REQUIRE(resolve_filament_for_head({}, pem, 3) == 3); + REQUIRE(resolve_filament_for_head({}, pem, 2) == 2); + + // Only a head with no routing at all yields -1, so "-1 means safe to index" is false. + REQUIRE(resolve_filament_for_head({}, pem, 9) == -1); +}