From df4009e7342bcee0712e58835999bdbd5c4ca306 Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Fri, 7 Aug 2026 10:40:40 -0400 Subject: [PATCH] fix(imex): size physical_extruder_map from the nozzle count physical_extruder_map has one entry per logical extruder -- the index space of nozzle_diameter -- and its consumers size their own arrays from that count. It was being derived from printer_extruder_id, which is indexed by variant slot: one entry per extruder+variant pair. An X1 Carbon has one nozzle and printer_extruder_id {1,1}; an H2D 0.4 has two nozzles and {1,1,2,2,2}. The two spaces coincide only when every extruder declares a single variant. The visible effect was on the standby cool-down. set_extruder skips it when the outgoing and incoming filaments share a physical extruder, and that check is not gated on IMEX. With the map built from the wrong array, two filaments on a dual-nozzle machine read as sharing one hotend and the cool-down was dropped -- caught by "Toolchange temperature commands are unchanged when the wipe tower wait is off", which failed on all five CI platforms with the ;cooldown line missing. Derive the identity over the nozzle count instead, the same fallback Plater.cpp already applies where a profile authors no map. A profile counts as authoring one only when its length matches the nozzle count, so the single-element PrintConfig default is replaced rather than read as a one-extruder machine. Authored maps pass through untouched, including the {1,0} numbering permutation the BBL dual-nozzle profiles ship. Tests pin the four branches and the length invariant the consumers depend on. Co-Authored-By: Claude Opus 5 (1M context) --- src/libslic3r/IMEXHelpers.cpp | 21 ++++++----- src/libslic3r/IMEXHelpers.hpp | 29 +++++++------- src/libslic3r/PrintApply.cpp | 10 ++--- tests/libslic3r/test_imex_helpers.cpp | 54 +++++++++++++++++---------- 4 files changed, 62 insertions(+), 52 deletions(-) diff --git a/src/libslic3r/IMEXHelpers.cpp b/src/libslic3r/IMEXHelpers.cpp index 4c81e20a7f..0f728f97f7 100644 --- a/src/libslic3r/IMEXHelpers.cpp +++ b/src/libslic3r/IMEXHelpers.cpp @@ -1,6 +1,7 @@ #include "libslic3r/IMEXHelpers.hpp" #include +#include #include #include #include @@ -12,17 +13,17 @@ namespace Slic3r { -ConfigOptionInts effective_physical_extruder_map(const ConfigOptionInts* explicit_pem, - const ConfigOptionInts* printer_extruder_id) +ConfigOptionInts effective_physical_extruder_map(const ConfigOptionInts* explicit_pem, int nozzle_count) { - if (explicit_pem && explicit_pem->values.size() >= 2) + // One entry per logical extruder, so a profile has authored a map only when its length + // matches. The single-element PrintConfig default does not, and is replaced rather than + // treated as a one-extruder machine. + if (explicit_pem && nozzle_count > 0 && (int) explicit_pem->values.size() == nozzle_count) return *explicit_pem; + ConfigOptionInts derived; - if (printer_extruder_id) { - derived.values.reserve(printer_extruder_id->values.size()); - for (int v : printer_extruder_id->values) - derived.values.push_back(v - 1); - } + derived.values.resize(std::max(nozzle_count, 1)); + std::iota(derived.values.begin(), derived.values.end(), 0); return derived; } @@ -31,8 +32,8 @@ ConfigOptionInts effective_physical_extruder_map(const PresetBundle& pb) const ConfigOptionInts* explicit_pem = pb.project_config.option("physical_extruder_map"); if (!explicit_pem || explicit_pem->values.size() < 2) explicit_pem = pb.printers.get_edited_preset().config.option("physical_extruder_map"); - const ConfigOptionInts* pei = pb.printers.get_edited_preset().config.option("printer_extruder_id"); - return effective_physical_extruder_map(explicit_pem, pei); + const auto* nozzles = pb.printers.get_edited_preset().config.option("nozzle_diameter"); + return effective_physical_extruder_map(explicit_pem, nozzles ? (int) nozzles->values.size() : 0); } int imex_pem_tool_for(int filament_id, const std::string& parallel_mode, const ConfigOptionInts& pem) diff --git a/src/libslic3r/IMEXHelpers.hpp b/src/libslic3r/IMEXHelpers.hpp index 21b3b26bb2..06e03c382b 100644 --- a/src/libslic3r/IMEXHelpers.hpp +++ b/src/libslic3r/IMEXHelpers.hpp @@ -49,8 +49,8 @@ inline constexpr const char* kImexPrimaryMode = "primary"; // 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 just `pem.get_at(filament_id)`, -// already encapsulated in `imex_pem_tool_for` for the per-tool-qualifier case. +// The reverse translation (logical → physical) is `pem.get_at(logical_idx)`, already +// encapsulated in `imex_pem_tool_for` for the per-tool-qualifier case. // // Past bugs in this class: // - GCode PA emission used the inline `pem.get_at(filament_id)` form at two @@ -65,21 +65,18 @@ inline constexpr const char* kImexPrimaryMode = "primary"; // route through one of the helpers below. // ============================================================================= -// Returns the effective physical_extruder_map given an optionally-explicit map and -// the printer's `printer_extruder_id`. If `explicit_pem` has size >= 2 the caller -// authored one, and it is returned verbatim. Otherwise the map is auto-derived -// from `printer_extruder_id` by converting each 1-indexed value to 0-indexed. -// Returns an empty ConfigOptionInts if neither source yields any values. -// Slice-time (PrintApply) and GUI ghost-color paths both call this so a printer -// profile without an explicit pem still gets a consistent mapping. -ConfigOptionInts effective_physical_extruder_map(const ConfigOptionInts* explicit_pem, - const ConfigOptionInts* printer_extruder_id); +// physical_extruder_map has one entry per LOGICAL extruder -- the index space of +// nozzle_diameter -- with each value a physical extruder index. It is NOT derivable from +// printer_extruder_id, which is indexed by variant slot (one entry per extruder+variant pair): +// an X1 Carbon has one nozzle and printer_extruder_id {1,1}, an H2D two nozzles and {1,1,2,2,2}. +// +// A profile has authored a map only when its length matches nozzle_count; otherwise the identity +// is returned, which is the same default upstream applies (see Plater.cpp's extruder_map). +ConfigOptionInts effective_physical_extruder_map(const ConfigOptionInts* explicit_pem, int nozzle_count); -// GUI overload: resolves the effective pem from a live PresetBundle using the -// project_config → printer preset fallback, then derives from printer_extruder_id -// if neither holds a user-authored map (size >= 2). Use this instead of open-coding -// the lookup at ghost-color, tooltip, click-gate, and cache-key call sites so they -// all agree on what the slicer will see. +// GUI overload: resolves the effective pem from a live PresetBundle using the project_config +// → printer preset fallback. Use this instead of open-coding the lookup at ghost-color, +// tooltip, click-gate and cache-key call sites so they all agree with what the slicer sees. ConfigOptionInts effective_physical_extruder_map(const PresetBundle& pb); // Returns the physical extruder index to emit as a per-tool qualifier (PA / temperature) diff --git a/src/libslic3r/PrintApply.cpp b/src/libslic3r/PrintApply.cpp index c7408326b9..027c93bd52 100644 --- a/src/libslic3r/PrintApply.cpp +++ b/src/libslic3r/PrintApply.cpp @@ -1230,15 +1230,13 @@ Print::ApplyStatus Print::apply(const Model &model, DynamicPrintConfig new_full_ } } - // Derive physical_extruder_map (0-indexed) from printer_extruder_id (1-indexed) when the - // map hasn't been explicitly configured in the printer profile (size <= 1 = default). - // This gives all firmware code a consistent slot → physical-extruder translation, - // including AFC/MMU setups where multiple tool slots share one physical extruder. + // Fill in physical_extruder_map when the printer profile has not authored one. It has one + // entry per logical extruder, so it is sized from the nozzle count -- not from + // printer_extruder_id, which is indexed by variant slot. { auto* pem = new_full_config.option("physical_extruder_map", true); - const auto* pei = new_full_config.option("printer_extruder_id"); if (pem) { - pem->values = effective_physical_extruder_map(pem, pei).values; + pem->values = effective_physical_extruder_map(pem, extruder_count).values; // m_ori_full_print_config was snapshotted above, before this derivation, and the // selector write-back path rebuilds m_full_print_config from that snapshot. Without // mirroring the derived map into it, m_full_print_config keeps the unexpanded default diff --git a/tests/libslic3r/test_imex_helpers.cpp b/tests/libslic3r/test_imex_helpers.cpp index e1744010e8..9c66bb871b 100644 --- a/tests/libslic3r/test_imex_helpers.cpp +++ b/tests/libslic3r/test_imex_helpers.cpp @@ -13,38 +13,52 @@ static ConfigOptionInts make_pem(std::vector v) { return o; } -TEST_CASE("effective_physical_extruder_map - explicit wins", "[IMEX]") { +TEST_CASE("effective_physical_extruder_map - authored map wins", "[IMEX]") { + // An AFC manifold on a 7-extruder machine: logical 0-3 all feed physical 0. Not derivable, + // so it must be honoured verbatim. auto explicit_pem = make_pem({0, 0, 0, 0, 1, 2, 3}); - auto pei = make_pem({1, 2, 3, 4}); // would derive to {0,1,2,3} - auto out = effective_physical_extruder_map(&explicit_pem, &pei); + auto out = effective_physical_extruder_map(&explicit_pem, 7); REQUIRE(out.values == std::vector{0, 0, 0, 0, 1, 2, 3}); } -TEST_CASE("effective_physical_extruder_map - default pem falls back to pei derive", "[IMEX]") { - auto default_pem = make_pem({0}); // size 1, the PrintConfig default - auto pei = make_pem({1, 2}); // 1-indexed IDEX - auto out = effective_physical_extruder_map(&default_pem, &pei); - REQUIRE(out.values == std::vector{0, 1}); // 1-indexed → 0-indexed +TEST_CASE("effective_physical_extruder_map - a permutation is honoured", "[IMEX]") { + // Shipping BBL dual-nozzle profiles author {1,0}: a slicer/firmware numbering swap, not a + // sharing map. Deriving over it would silently renumber both extruders. + auto explicit_pem = make_pem({1, 0}); + auto out = effective_physical_extruder_map(&explicit_pem, 2); + REQUIRE(out.values == std::vector{1, 0}); } -TEST_CASE("effective_physical_extruder_map - null explicit, pei present", "[IMEX]") { - auto pei = make_pem({1, 2, 3}); - auto out = effective_physical_extruder_map(nullptr, &pei); - REQUIRE(out.values == std::vector{0, 1, 2}); +TEST_CASE("effective_physical_extruder_map - unauthored derives the identity", "[IMEX]") { + // The PrintConfig default is a single element, which is not an authored map on a 2-extruder + // machine. Identity is what upstream itself falls back to. + auto default_pem = make_pem({0}); + auto out = effective_physical_extruder_map(&default_pem, 2); + REQUIRE(out.values == std::vector{0, 1}); } -TEST_CASE("effective_physical_extruder_map - both absent yields empty", "[IMEX]") { - auto out = effective_physical_extruder_map(nullptr, nullptr); - REQUIRE(out.values.empty()); +TEST_CASE("effective_physical_extruder_map - result is always one entry per extruder", "[IMEX]") { + // The defining property: consumers index this map by logical extruder and size their own + // arrays from nozzle_diameter, so a shorter map is an out-of-bounds read in several of them. + for (int n : { 1, 2, 4, 7 }) { + DYNAMIC_SECTION("nozzle count " << n) { + REQUIRE((int) effective_physical_extruder_map(nullptr, n).values.size() == n); + auto stale = make_pem({0}); // wrong length: must not be mistaken for authored + REQUIRE((int) effective_physical_extruder_map(&stale, n).values.size() == n); + } + } +} + +TEST_CASE("effective_physical_extruder_map - degenerate nozzle count still yields a usable map", "[IMEX]") { + // Never hand back an empty map: several consumers index it as a raw vector. + REQUIRE(effective_physical_extruder_map(nullptr, 0).values == std::vector{0}); } TEST_CASE("effective_physical_extruder_map - IDEX ghost-color regression guard", "[IMEX]") { - // Printer with printer_extruder_id = [1, 2] and no explicit pem (default {0}). - // Before the GUI fix, this scenario produced a black ghost on T1 because - // first_filament_for_physical_head({0}, 1) == -1. + // Dual extruder, no authored map. Before the GUI fix this produced a black ghost on T1 + // because first_filament_for_physical_head({0}, 1) == -1. auto default_pem = make_pem({0}); - auto pei = make_pem({1, 2}); - auto pem = effective_physical_extruder_map(&default_pem, &pei); + auto pem = effective_physical_extruder_map(&default_pem, 2); REQUIRE(first_filament_for_physical_head(pem, 0) == 0); REQUIRE(first_filament_for_physical_head(pem, 1) == 1); // no longer -1 }