From c2492ccc474a8fa24d54ffd0d811d4f61edc29eb Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Fri, 24 Apr 2026 23:07:23 -0400 Subject: [PATCH] refactor(imex): extract imex_pem_tool_for helper + unit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The physical_extruder_map translation used by IMEX per-tool PA emission was inlined identically at two sites in GCode.cpp (tool-change and second-layer transition). Extract to IMEXHelpers so the routing rule ("parallel mode AND populated pem → physical index, else -1") is testable in isolation and the call sites read as intent rather than re-deriving the conditional. Production change is behavior-preserving: - Same predicate (`!mode.empty() && mode != "primary"`) - Same empty-pem short-circuit returning -1 - Same get_at() dispatch on hit - Both call sites replaced with a single call Four unit tests in [IMEX] cover the routing matrix: - non-IMEX ("") and primary mode short-circuit - parallel mode + empty pem short-circuits (defense-in-depth; get_at would throw on empty values otherwise) - identity pem (non-MMU IDEX) routes filament to itself - MMU collapse routes multiple logical slots to one physical (7-slot profile with 4-lane MMU on physical 0 and direct drives on 1/2/3) All IMEX + Variant regression suites pass post-refactor (133 assertions / 48 cases under libslic3r, 25 assertions / 6 cases under fff_print [PressureAdvance]). Co-Authored-By: Claude Opus 4.7 --- src/libslic3r/GCode.cpp | 14 ++--------- src/libslic3r/IMEXHelpers.cpp | 8 +++++++ src/libslic3r/IMEXHelpers.hpp | 8 +++++++ tests/libslic3r/test_imex_helpers.cpp | 34 +++++++++++++++++++++++++++ 4 files changed, 52 insertions(+), 12 deletions(-) diff --git a/src/libslic3r/GCode.cpp b/src/libslic3r/GCode.cpp index 2f10c40757..27352c9654 100644 --- a/src/libslic3r/GCode.cpp +++ b/src/libslic3r/GCode.cpp @@ -7589,13 +7589,7 @@ std::string GCode::set_extruder(unsigned int new_filament_id, double print_z, bo // In IMEX parallel modes each carriage needs an explicit tool address. // In primary mode (single active tool) regular tool changes handle PA // so no qualifier is needed — same as a non-IMEX printer. - // Guard the pem lookup: PrintApply populates pem when printer_extruder_id - // is set, but defense-in-depth prevents a throw from get_at on any - // empty-pem path that might slip through in exotic profiles. - const bool imex_parallel = !m_imex_parallel_mode.empty() && m_imex_parallel_mode != "primary"; - const int pa_tool = (imex_parallel && !m_config.physical_extruder_map.values.empty()) - ? m_config.physical_extruder_map.get_at((int)new_filament_id) - : -1; + const int pa_tool = imex_pem_tool_for((int)new_filament_id, m_imex_parallel_mode, m_config.physical_extruder_map); gcode += m_writer.set_pressure_advance(m_config.pressure_advance.get_at(new_filament_id), pa_tool); // Orca: Adaptive PA // Reset Adaptive PA processor last PA value @@ -7895,11 +7889,7 @@ std::string GCode::set_extruder(unsigned int new_filament_id, double print_z, bo gcode += m_ooze_prevention.post_toolchange(*this); if (m_config.enable_pressure_advance.get_at(new_filament_id)) { - // Empty-pem guard mirrors the earlier PA site; get_at throws on empty values. - const bool imex_parallel = !m_imex_parallel_mode.empty() && m_imex_parallel_mode != "primary"; - const int pa_tool = (imex_parallel && !m_config.physical_extruder_map.values.empty()) - ? m_config.physical_extruder_map.get_at((int)new_filament_id) - : -1; + const int pa_tool = imex_pem_tool_for((int)new_filament_id, m_imex_parallel_mode, m_config.physical_extruder_map); gcode += m_writer.set_pressure_advance(m_config.pressure_advance.get_at(new_filament_id), pa_tool); // Orca: Adaptive PA // Reset Adaptive PA processor last PA value diff --git a/src/libslic3r/IMEXHelpers.cpp b/src/libslic3r/IMEXHelpers.cpp index a5827b6c7a..2be6176823 100644 --- a/src/libslic3r/IMEXHelpers.cpp +++ b/src/libslic3r/IMEXHelpers.cpp @@ -33,6 +33,14 @@ ConfigOptionInts effective_physical_extruder_map(const PresetBundle& pb) return effective_physical_extruder_map(explicit_pem, pei); } +int imex_pem_tool_for(int filament_id, const std::string& parallel_mode, const ConfigOptionInts& pem) +{ + const bool imex_parallel = !parallel_mode.empty() && parallel_mode != "primary"; + if (!imex_parallel || pem.values.empty()) + return -1; + return pem.get_at(filament_id); +} + int first_filament_for_physical_head(const ConfigOptionInts& pem, int physical) { const auto& v = pem.values; diff --git a/src/libslic3r/IMEXHelpers.hpp b/src/libslic3r/IMEXHelpers.hpp index aa0489d268..291b909daa 100644 --- a/src/libslic3r/IMEXHelpers.hpp +++ b/src/libslic3r/IMEXHelpers.hpp @@ -30,6 +30,14 @@ ConfigOptionInts effective_physical_extruder_map(const ConfigOptionInts* explici // all agree on what the slicer will see. ConfigOptionInts effective_physical_extruder_map(const PresetBundle& pb); +// Returns the physical extruder index to emit as a per-tool qualifier (PA / temperature) +// for the given filament slot, or -1 when the current IMEX state does not warrant a +// per-tool qualification: non-IMEX / primary mode, or the pem is empty / unpopulated. +// Used at tool-change and second-layer transition call sites where IMEX parallel modes +// route emission through the physical extruder, while ordinary tool changes emit bare +// firmware commands like any non-IMEX printer. +int imex_pem_tool_for(int filament_id, const std::string& parallel_mode, const ConfigOptionInts& pem); + // Returns the lowest 0-based logical filament index L such that pem[L] == physical. // Returns -1 if no filament routes to `physical`. // Degenerate case: an empty pem returns 0 when `physical == 0` (identity-on-head-0 diff --git a/tests/libslic3r/test_imex_helpers.cpp b/tests/libslic3r/test_imex_helpers.cpp index 3f95a3e9d8..289e9baeab 100644 --- a/tests/libslic3r/test_imex_helpers.cpp +++ b/tests/libslic3r/test_imex_helpers.cpp @@ -49,6 +49,40 @@ TEST_CASE("effective_physical_extruder_map — IDEX ghost-color regression guard REQUIRE(first_filament_for_physical_head(pem, 1) == 1); // no longer -1 } +TEST_CASE("imex_pem_tool_for — non-parallel mode returns -1", "[IMEX]") { + // Empty mode string (not in IMEX) and "primary" (IMEX present but not parallel) both + // short-circuit to bare-firmware PA/temp emission. + auto pem = make_pem({0, 1, 2}); + REQUIRE(imex_pem_tool_for(0, "", pem) == -1); + REQUIRE(imex_pem_tool_for(1, "primary", pem) == -1); + REQUIRE(imex_pem_tool_for(2, "primary", pem) == -1); +} + +TEST_CASE("imex_pem_tool_for — parallel mode with empty pem returns -1", "[IMEX]") { + // Defense-in-depth for exotic profiles where pem never gets populated. get_at would + // throw on an empty pem; the helper must return -1 instead so the writer emits bare. + ConfigOptionInts empty_pem; + REQUIRE(imex_pem_tool_for(0, "copy_mode", empty_pem) == -1); + REQUIRE(imex_pem_tool_for(3, "mirror_mode", empty_pem) == -1); +} + +TEST_CASE("imex_pem_tool_for — identity pem routes filament to itself", "[IMEX]") { + // IDEX with printer_extruder_id = [1, 2] auto-derives pem = [0, 1]; non-MMU is identity. + auto pem = make_pem({0, 1}); + REQUIRE(imex_pem_tool_for(0, "copy_mode", pem) == 0); + REQUIRE(imex_pem_tool_for(1, "copy_mode", pem) == 1); +} + +TEST_CASE("imex_pem_tool_for — MMU collapse routes multiple logical slots to one physical", "[IMEX]") { + // 7-slot printer: first four slots share physical extruder 0 (a 4-lane MMU), + // slots 4/5/6 are independent on 1/2/3. Filament index is the logical slot; + // the helper returns the physical extruder carrying it. + auto pem = make_pem({0, 0, 0, 0, 1, 2, 3}); + REQUIRE(imex_pem_tool_for(0, "copy_mode", pem) == 0); + REQUIRE(imex_pem_tool_for(3, "copy_mode", pem) == 0); // MMU collapse: T3 logical → T0 physical + REQUIRE(imex_pem_tool_for(6, "copy_mode", pem) == 3); +} + TEST_CASE("first_filament_for_physical_head — identity pem", "[IMEX]") { auto pem = make_pem({0, 1, 2, 3}); REQUIRE(first_filament_for_physical_head(pem, 0) == 0);