From af8fe649ef8d908235d82b41b5360e141ff320f5 Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Fri, 24 Apr 2026 22:49:59 -0400 Subject: [PATCH 1/6] =?UTF-8?q?test(preset):=20expand=20[Variant]=20covera?= =?UTF-8?q?ge=20=E2=80=94=20stride=3D2,=20equal-size,=20non-variant=20guar?= =?UTF-8?q?d?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds three scenarios alongside the existing child>parent stride=1 regression test for update_non_diff_values_to_base_config: - stride=2 child>parent: machine_max_acceleration_x (size 4 vs 2) — confirms the truncation guard fires for the (normal,silent)-pair stride=2 path, not just stride=1. Catches a regression class the existing test would miss because stride=2 routes through normalize_stride2_floats and a different set_with_restore call site. - equal-size (2=2): exercises the path the guard does NOT short-circuit; asserts child per-extruder values survive set_with_restore's nil-restore merge. Catches any future change that breaks the equal-size merge — the fix's `cur > target ? skip` predicate could regress to `cur >= target` and silently override child values otherwise. - non-variant scalar: layer_height in `keys` and `different_keys` but absent from printer_options_with_variant_1/_2. Hits the is_scalar() / "nothing to do" branch and must remain untouched. Scopes the guard's blast radius. All four scenarios in the [Variant] tag pass: 15 assertions, 4 test cases. Co-Authored-By: Claude Opus 4.7 --- tests/libslic3r/test_config.cpp | 128 ++++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) diff --git a/tests/libslic3r/test_config.cpp b/tests/libslic3r/test_config.cpp index 2bee0d7d11..d6fad72508 100644 --- a/tests/libslic3r/test_config.cpp +++ b/tests/libslic3r/test_config.cpp @@ -265,6 +265,134 @@ SCENARIO("DynamicPrintConfig serialization", "[Config]") { } } +SCENARIO("update_non_diff_values_to_base_config does not truncate stride=2 child vectors when child has more extruders than parent", + "[Config][Variant]") { + GIVEN("A 2-extruder child with stride=2 machine limits inheriting from a 1-extruder parent") { + // Stride=2 keys store (normal, silent) pairs per variant: a 2-extruder child has size 4, + // a 1-extruder parent has size 2. The truncation guard must fire here too. + Slic3r::DynamicPrintConfig child; + Slic3r::DynamicPrintConfig parent; + + child.set_key_value("printer_extruder_id", new Slic3r::ConfigOptionInts({1, 2})); + child.set_key_value("printer_extruder_variant", new Slic3r::ConfigOptionStrings({"Direct Drive Standard", "Direct Drive Standard"})); + child.set_key_value("machine_max_acceleration_x", new Slic3r::ConfigOptionFloats({500.0, 200.0, 600.0, 300.0})); + + parent.set_key_value("printer_extruder_id", new Slic3r::ConfigOptionInts({1})); + parent.set_key_value("printer_extruder_variant", new Slic3r::ConfigOptionStrings({"Direct Drive Standard"})); + parent.set_key_value("machine_max_acceleration_x", new Slic3r::ConfigOptionFloats({1000.0, 400.0})); + + const Slic3r::t_config_option_keys keys = { + "machine_max_acceleration_x", "printer_extruder_id", "printer_extruder_variant" + }; + const std::set different_keys = { + "machine_max_acceleration_x", "printer_extruder_id", "printer_extruder_variant" + }; + + WHEN("update_non_diff_values_to_base_config is called") { + std::string id_name = "printer_extruder_id"; + std::string var_name = "printer_extruder_variant"; + child.update_non_diff_values_to_base_config( + parent, keys, different_keys, id_name, var_name, + Slic3r::printer_options_with_variant_1, + Slic3r::printer_options_with_variant_2); + + THEN("machine_max_acceleration_x retains size 4 (2 variants × 2 silent modes)") { + REQUIRE(child.option("machine_max_acceleration_x")->values.size() == 4); + } + THEN("machine_max_acceleration_x preserves both extruders' normal and silent values") { + auto* v = child.option("machine_max_acceleration_x"); + REQUIRE_THAT(v->values[0], Catch::Matchers::WithinAbs(500.0, 1e-9)); + REQUIRE_THAT(v->values[1], Catch::Matchers::WithinAbs(200.0, 1e-9)); + REQUIRE_THAT(v->values[2], Catch::Matchers::WithinAbs(600.0, 1e-9)); + REQUIRE_THAT(v->values[3], Catch::Matchers::WithinAbs(300.0, 1e-9)); + } + } + } +} + +SCENARIO("update_non_diff_values_to_base_config preserves child variant values when child and parent extruder counts match", + "[Config][Variant]") { + // The fix's guard is `cur > target ? skip`. The equal-size path must still run normally and + // preserve the child's per-extruder values via set_with_restore's nil-restore mechanism. + GIVEN("A 2-extruder child inheriting from a 2-extruder parent with different per-extruder values") { + Slic3r::DynamicPrintConfig child; + Slic3r::DynamicPrintConfig parent; + + child.set_key_value("printer_extruder_id", new Slic3r::ConfigOptionInts({1, 2})); + child.set_key_value("printer_extruder_variant", new Slic3r::ConfigOptionStrings({"Direct Drive Standard", "Direct Drive Standard"})); + child.set_key_value("retraction_length", new Slic3r::ConfigOptionFloats({1.5, 2.5})); + + parent.set_key_value("printer_extruder_id", new Slic3r::ConfigOptionInts({1, 2})); + parent.set_key_value("printer_extruder_variant", new Slic3r::ConfigOptionStrings({"Direct Drive Standard", "Direct Drive Standard"})); + parent.set_key_value("retraction_length", new Slic3r::ConfigOptionFloats({0.8, 0.8})); + + const Slic3r::t_config_option_keys keys = { + "retraction_length", "printer_extruder_id", "printer_extruder_variant" + }; + const std::set different_keys = { + "retraction_length", "printer_extruder_id", "printer_extruder_variant" + }; + + WHEN("update_non_diff_values_to_base_config is called") { + std::string id_name = "printer_extruder_id"; + std::string var_name = "printer_extruder_variant"; + child.update_non_diff_values_to_base_config( + parent, keys, different_keys, id_name, var_name, + Slic3r::printer_options_with_variant_1, + Slic3r::printer_options_with_variant_2); + + THEN("retraction_length retains size 2") { + REQUIRE(child.option("retraction_length")->values.size() == 2); + } + THEN("retraction_length preserves the child's per-extruder values, not the parent's") { + auto* rl = child.option("retraction_length"); + REQUIRE_THAT(rl->values[0], Catch::Matchers::WithinAbs(1.5, 1e-9)); + REQUIRE_THAT(rl->values[1], Catch::Matchers::WithinAbs(2.5, 1e-9)); + } + } + } +} + +SCENARIO("update_non_diff_values_to_base_config truncation guard does not affect non-variant scalar keys", + "[Config][Variant]") { + // The fix is scoped to options in printer_options_with_variant_1 / _2. A non-variant scalar + // listed in `keys` and `different_keys` should hit the "nothing to do" branch and remain + // untouched regardless of child/parent extruder count mismatch. + GIVEN("A 2-extruder child inheriting from a 1-extruder parent, with a non-variant scalar key in `keys`") { + Slic3r::DynamicPrintConfig child; + Slic3r::DynamicPrintConfig parent; + + child.set_key_value("printer_extruder_id", new Slic3r::ConfigOptionInts({1, 2})); + child.set_key_value("printer_extruder_variant", new Slic3r::ConfigOptionStrings({"Direct Drive Standard", "Direct Drive Standard"})); + child.set_key_value("layer_height", new Slic3r::ConfigOptionFloat(0.20)); + + parent.set_key_value("printer_extruder_id", new Slic3r::ConfigOptionInts({1})); + parent.set_key_value("printer_extruder_variant", new Slic3r::ConfigOptionStrings({"Direct Drive Standard"})); + parent.set_key_value("layer_height", new Slic3r::ConfigOptionFloat(0.28)); + + const Slic3r::t_config_option_keys keys = { + "layer_height", "printer_extruder_id", "printer_extruder_variant" + }; + const std::set different_keys = { + "layer_height", "printer_extruder_id", "printer_extruder_variant" + }; + + WHEN("update_non_diff_values_to_base_config is called") { + std::string id_name = "printer_extruder_id"; + std::string var_name = "printer_extruder_variant"; + child.update_non_diff_values_to_base_config( + parent, keys, different_keys, id_name, var_name, + Slic3r::printer_options_with_variant_1, + Slic3r::printer_options_with_variant_2); + + THEN("the non-variant scalar layer_height is left unchanged on the child") { + REQUIRE_THAT(child.option("layer_height")->value, + Catch::Matchers::WithinAbs(0.20, 1e-9)); + } + } + } +} + SCENARIO("update_non_diff_values_to_base_config preserves child vectors when child has more extruders than parent", "[Config][Variant]") { GIVEN("A 2-extruder child printer config inheriting from a 1-extruder parent") { From 5434a5217cd38e7e6776e39d4d3a60efc93cec9e Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Fri, 24 Apr 2026 22:53:24 -0400 Subject: [PATCH 2/6] test(gcode): per-firmware coverage for GCodeWriter::set_pressure_advance(tool) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Exercises the IMEX per-tool PA emission surface added in af59501f4a ("feat: firmware-agnostic per-tool PA emission for IMEX parallel modes"). Six scenarios cover the full routing matrix: - Negative PA returns empty across all flavors (early-exit guard). - Klipper: bare vs EXTRUDER=extruder vs EXTRUDER=extruderN. Asserts the tool=0 case emits the unsuffixed extruder name (first Klipper extruder is named "extruder", not "extruder0") — a subtle edge case easy to regress. - RRF: bare vs D0 vs DN. The D0 case matters: passing tool=0 explicitly must emit `D0`, not the current-tool fallback. - Marlin 2.x: bare vs T0 vs TN. - Marlin Legacy: tool index is silently dropped — verifies the fallback branch can't accidentally start emitting T qualifiers on firmware that doesn't support them. - BBL: flag wins over firmware flavor (Marlin 2 flavor + BBL flag emits the BBL-specific `M900 K... L1000 M10`) and BBL never emits a per-tool qualifier regardless of the tool argument. All 25 assertions across 6 cases pass under [PressureAdvance]. Co-Authored-By: Claude Opus 4.7 --- tests/fff_print/test_gcodewriter.cpp | 145 +++++++++++++++++++++++++++ 1 file changed, 145 insertions(+) diff --git a/tests/fff_print/test_gcodewriter.cpp b/tests/fff_print/test_gcodewriter.cpp index ef8fb58b41..e88009cfbe 100644 --- a/tests/fff_print/test_gcodewriter.cpp +++ b/tests/fff_print/test_gcodewriter.cpp @@ -68,6 +68,151 @@ SCENARIO("lift() is not ignored after unlift() at normal values of Z", "[GCodeWr } } +SCENARIO("set_pressure_advance emits nothing for negative PA", "[GCodeWriter][PressureAdvance]") { + GIVEN("A default GCodeWriter") { + GCodeWriter writer; + THEN("Negative PA returns empty regardless of firmware flavor") { + writer.config.gcode_flavor.value = gcfKlipper; + REQUIRE(writer.set_pressure_advance(-1.0).empty()); + writer.config.gcode_flavor.value = gcfRepRapFirmware; + REQUIRE(writer.set_pressure_advance(-0.001).empty()); + writer.config.gcode_flavor.value = gcfMarlinFirmware; + REQUIRE(writer.set_pressure_advance(-100.0).empty()); + } + } +} + +SCENARIO("set_pressure_advance emits Klipper form with optional EXTRUDER=extruder", "[GCodeWriter][PressureAdvance]") { + GIVEN("A Klipper-flavored GCodeWriter") { + GCodeWriter writer; + writer.config.gcode_flavor.value = gcfKlipper; + + WHEN("set_pressure_advance is called without a tool index") { + std::string out = writer.set_pressure_advance(0.05); + THEN("Output contains SET_PRESSURE_ADVANCE ADVANCE=0.05 with no EXTRUDER qualifier") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("SET_PRESSURE_ADVANCE")); + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("ADVANCE=0.05")); + REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring("EXTRUDER=")); + } + } + WHEN("set_pressure_advance is called with tool=0") { + std::string out = writer.set_pressure_advance(0.04, 0); + THEN("Output targets EXTRUDER=extruder (no trailing index)") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("EXTRUDER=extruder ")); + REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring("EXTRUDER=extruder0")); + } + } + WHEN("set_pressure_advance is called with tool=3") { + std::string out = writer.set_pressure_advance(0.06, 3); + THEN("Output targets EXTRUDER=extruder3") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("EXTRUDER=extruder3")); + } + } + } +} + +SCENARIO("set_pressure_advance emits RepRapFirmware form with optional D", "[GCodeWriter][PressureAdvance]") { + GIVEN("An RRF-flavored GCodeWriter") { + GCodeWriter writer; + writer.config.gcode_flavor.value = gcfRepRapFirmware; + + WHEN("set_pressure_advance is called without a tool index") { + std::string out = writer.set_pressure_advance(0.07); + THEN("Output is bare M572 S... with no D qualifier") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M572 S0.07")); + REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring(" D")); + } + } + WHEN("set_pressure_advance is called with tool=0") { + std::string out = writer.set_pressure_advance(0.08, 0); + THEN("Output contains D0 (explicit tool 0, not the current-tool fallback)") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M572 D0 S0.08")); + } + } + WHEN("set_pressure_advance is called with tool=2") { + std::string out = writer.set_pressure_advance(0.09, 2); + THEN("Output contains D2") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M572 D2 S0.09")); + } + } + } +} + +SCENARIO("set_pressure_advance emits Marlin 2.x form with optional T", "[GCodeWriter][PressureAdvance]") { + GIVEN("A Marlin 2-flavored GCodeWriter") { + GCodeWriter writer; + writer.config.gcode_flavor.value = gcfMarlinFirmware; + + WHEN("set_pressure_advance is called without a tool index") { + std::string out = writer.set_pressure_advance(0.10); + THEN("Output is bare M900 K... with no T qualifier") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M900 K0.1 ")); + REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring(" T")); + } + } + WHEN("set_pressure_advance is called with tool=0") { + std::string out = writer.set_pressure_advance(0.11, 0); + THEN("Output contains T0") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M900 K0.11 T0")); + } + } + WHEN("set_pressure_advance is called with tool=1") { + std::string out = writer.set_pressure_advance(0.12, 1); + THEN("Output contains T1") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M900 K0.12 T1")); + } + } + } +} + +SCENARIO("set_pressure_advance emits Marlin Legacy form without tool qualifier even when tool index is supplied", + "[GCodeWriter][PressureAdvance]") { + GIVEN("A Marlin Legacy-flavored GCodeWriter") { + GCodeWriter writer; + writer.config.gcode_flavor.value = gcfMarlinLegacy; + + WHEN("set_pressure_advance is called without a tool index") { + std::string out = writer.set_pressure_advance(0.05); + THEN("Output is bare M900 K...") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M900 K0.05")); + REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring(" T")); + } + } + WHEN("set_pressure_advance is called with tool=2 (a hypothetical IMEX secondary)") { + std::string out = writer.set_pressure_advance(0.06, 2); + THEN("Output is still bare M900 — Marlin Legacy has no per-tool LA") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M900 K0.06")); + REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring(" T2")); + } + } + } +} + +SCENARIO("set_pressure_advance emits BBL M900 L1000 M10 regardless of tool index", + "[GCodeWriter][PressureAdvance]") { + GIVEN("A BBL-flagged GCodeWriter (the flag overrides firmware flavor routing)") { + GCodeWriter writer; + writer.set_is_bbl_machine(true); + // Flavor intentionally set to something other than the BBL branch to prove the flag wins. + writer.config.gcode_flavor.value = gcfMarlinFirmware; + + WHEN("set_pressure_advance is called without a tool index") { + std::string out = writer.set_pressure_advance(0.05); + THEN("Output is the BBL-specific M900 Kx L1000 M10 form") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M900 K0.05 L1000 M10")); + } + } + WHEN("set_pressure_advance is called with a tool index") { + std::string out = writer.set_pressure_advance(0.05, 2); + THEN("BBL output is unchanged — no per-tool qualifier is emitted on BBL printers") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M900 K0.05 L1000 M10")); + REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring(" T2")); + REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring("EXTRUDER=")); + } + } + } +} + SCENARIO("set_speed emits values with fixed-point output.", "[GCodeWriter]") { GIVEN("GCodeWriter instance") { From cb15f35444c0027218502924bb79ddd61b30f70f Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Fri, 24 Apr 2026 22:58:19 -0400 Subject: [PATCH 3/6] test(3mf): round-trip coverage for per-plate IMEX state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validates that imex_parallel_mode and imex_head_filament_map survive a full store_bbs_3mf → load_bbs_3mf cycle — the same silent-state-loss bug class that produced the variant-vector truncation regression, applied to IMEX plate state which rides the same XML metadata path. - Positive round-trip: a plate with copy_mode + a non-trivial head filament map ("1:2,2:3") is saved and reloaded; both options land on the destination plate's config with the exact values preserved. - Guard scope: a plate with mode="primary" and empty head-filament-map does NOT emit metadata (per the serializer's short-circuit), and the reload leaves both options absent from the destination config. If the serializer ever regressed to writing primary-mode plates, the load path would surface phantom "primary" strings on plates that shipped clean — this catches that. Both scenarios call set_temporary_dir to point the BBS exporter's backup scaffolding at a writable per-process temp directory (by default it resolves under root at runtime, which fails for non-root test processes). All 27 assertions in 2 test cases pass under [3mf][IMEX]. Co-Authored-By: Claude Opus 4.7 --- tests/libslic3r/test_3mf.cpp | 135 +++++++++++++++++++++++++++++++++++ 1 file changed, 135 insertions(+) diff --git a/tests/libslic3r/test_3mf.cpp b/tests/libslic3r/test_3mf.cpp index 7d7593948e..bf9c697cfd 100644 --- a/tests/libslic3r/test_3mf.cpp +++ b/tests/libslic3r/test_3mf.cpp @@ -1,7 +1,9 @@ #include "libslic3r/Model.hpp" #include "libslic3r/Format/3mf.hpp" +#include "libslic3r/Format/bbs_3mf.hpp" #include "libslic3r/Format/STL.hpp" +#include "libslic3r/Utils.hpp" #include @@ -133,6 +135,139 @@ SCENARIO("Export+Import geometry to/from 3mf file cycle", "[3mf]") { } } +SCENARIO("BBS 3MF round-trips per-plate IMEX state (parallel mode + head filament map)", "[3mf][IMEX]") { + // Regression guard for the class of bug where per-plate state silently drops through + // save/load (the variant-truncation bug was the precipitating example; IMEX plate state + // rides the same XML metadata path and is equally vulnerable). + // BBS exporter scaffolds a backup dir under temporary_dir() for the project config file; + // point it at a writable location for the test process. + set_temporary_dir(boost::filesystem::temp_directory_path().string()); + + GIVEN("A Model with a single object on plate 0 and IMEX plate state set") { + Model src_model; + std::string src_file = std::string(TEST_DATA_DIR) + "/test_3mf/Prusa.stl"; + REQUIRE(load_stl(src_file.c_str(), &src_model)); + src_model.add_default_instances(); + + DynamicPrintConfig src_config; + + PlateDataPtrs src_plates; + auto *plate0 = new PlateData(); + plate0->plate_index = 0; + plate0->config.set_key_value("imex_parallel_mode", new ConfigOptionString("copy_mode")); + plate0->config.set_key_value("imex_head_filament_map", new ConfigOptionString("1:2,2:3")); + src_plates.push_back(plate0); + + WHEN("the model is saved to BBS 3MF and loaded back") { + std::string test_file = std::string(TEST_DATA_DIR) + "/test_3mf/imex_roundtrip.3mf"; + + StoreParams store_params; + store_params.path = test_file.c_str(); + store_params.model = &src_model; + store_params.plate_data_list = src_plates; + store_params.config = &src_config; + REQUIRE(store_bbs_3mf(store_params)); + + Model dst_model; + DynamicPrintConfig dst_config; + PlateDataPtrs dst_plates; + std::vector dst_presets; + bool is_bbl = false; + Semver file_version; + ConfigSubstitutionContext ctxt{ ForwardCompatibilitySubstitutionRule::Disable }; + bool loaded = load_bbs_3mf(test_file.c_str(), &dst_config, &ctxt, &dst_model, + &dst_plates, &dst_presets, &is_bbl, &file_version); + boost::filesystem::remove(test_file); + + THEN("load succeeds") { + REQUIRE(loaded); + } + THEN("the loaded plate list has the same number of plates") { + REQUIRE(dst_plates.size() == src_plates.size()); + } + THEN("imex_parallel_mode round-trips with its original value") { + REQUIRE(dst_plates.size() >= 1); + auto *mode_opt = dst_plates[0]->config.option("imex_parallel_mode"); + REQUIRE(mode_opt != nullptr); + REQUIRE(mode_opt->value == "copy_mode"); + } + THEN("imex_head_filament_map round-trips with its original value") { + REQUIRE(dst_plates.size() >= 1); + auto *hfm_opt = dst_plates[0]->config.option("imex_head_filament_map"); + REQUIRE(hfm_opt != nullptr); + REQUIRE(hfm_opt->value == "1:2,2:3"); + } + + release_PlateData_list(dst_plates); + } + + release_PlateData_list(src_plates); + } +} + +SCENARIO("BBS 3MF does not emit IMEX metadata when plate is in primary mode", "[3mf][IMEX]") { + // The serialization guard short-circuits when the mode is empty or "primary", so loading + // a plate that was saved in primary mode must not leave a stale imex_parallel_mode option + // on the plate's config. If this regressed, we'd see ghost "primary" strings appearing on + // plates that had no IMEX state at all. + set_temporary_dir(boost::filesystem::temp_directory_path().string()); + + GIVEN("A Model with plate 0 in primary mode and an empty head-filament map") { + Model src_model; + std::string src_file = std::string(TEST_DATA_DIR) + "/test_3mf/Prusa.stl"; + REQUIRE(load_stl(src_file.c_str(), &src_model)); + src_model.add_default_instances(); + + DynamicPrintConfig src_config; + PlateDataPtrs src_plates; + auto *plate0 = new PlateData(); + plate0->plate_index = 0; + plate0->config.set_key_value("imex_parallel_mode", new ConfigOptionString("primary")); + plate0->config.set_key_value("imex_head_filament_map", new ConfigOptionString("")); + src_plates.push_back(plate0); + + WHEN("the model is saved to BBS 3MF and loaded back") { + std::string test_file = std::string(TEST_DATA_DIR) + "/test_3mf/imex_primary_roundtrip.3mf"; + + StoreParams store_params; + store_params.path = test_file.c_str(); + store_params.model = &src_model; + store_params.plate_data_list = src_plates; + store_params.config = &src_config; + REQUIRE(store_bbs_3mf(store_params)); + + Model dst_model; + DynamicPrintConfig dst_config; + PlateDataPtrs dst_plates; + std::vector dst_presets; + bool is_bbl = false; + Semver file_version; + ConfigSubstitutionContext ctxt{ ForwardCompatibilitySubstitutionRule::Disable }; + bool loaded = load_bbs_3mf(test_file.c_str(), &dst_config, &ctxt, &dst_model, + &dst_plates, &dst_presets, &is_bbl, &file_version); + boost::filesystem::remove(test_file); + + THEN("load succeeds") { + REQUIRE(loaded); + } + THEN("the loaded plate has no imex_parallel_mode option set (primary is not serialized)") { + REQUIRE(dst_plates.size() >= 1); + auto *mode_opt = dst_plates[0]->config.option("imex_parallel_mode"); + REQUIRE(mode_opt == nullptr); + } + THEN("the loaded plate has no imex_head_filament_map option set (empty is not serialized)") { + REQUIRE(dst_plates.size() >= 1); + auto *hfm_opt = dst_plates[0]->config.option("imex_head_filament_map"); + REQUIRE(hfm_opt == nullptr); + } + + release_PlateData_list(dst_plates); + } + + release_PlateData_list(src_plates); + } +} + SCENARIO("2D convex hull of sinking object", "[3mf][.]") { GIVEN("model") { // load a model From c2492ccc474a8fa24d54ffd0d811d4f61edc29eb Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Fri, 24 Apr 2026 23:07:23 -0400 Subject: [PATCH 4/6] 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); From b45d8a5b7bc07de0e8413e7d918b1cc8a1d4b56b Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Fri, 24 Apr 2026 23:08:57 -0400 Subject: [PATCH 5/6] test(gcode): per-firmware coverage for GCodeWriter::set_temperature(tool) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four scenarios cover the temperature emission surface that IMEX layer-change handling routes through (Tier 1 of the deferred Target B test plan — pure-function-only, no fixture). - Per-flavor command routing: Marlin (M104), RRF (G10 — M104 is deprecated on RRF), Mach3/Machinekit (P-prefix for value instead of S). - Wait handling: Marlin emits M109, MakerWare/Sailfish silently drop wait requests (the firmware doesn't support blocking waits), Teacup and RRF both emit a separate M116 poll. - Per-tool qualifier for IMEX secondary carriages: Marlin and Klipper emit T, RRF uses P (same P override as its wait poll). This is exactly the path that lets IMEX set secondary-tool layer temperatures without a tool-change. - Instance overload's multi-extruder gating: a tool index passed to a single-extruder GCodeWriter is discarded (no spurious T0 on single-tool printers), but a multiple_extruders writer passes it through verbatim. All 24 assertions in 4 cases pass under [Temperature]. Co-Authored-By: Claude Opus 4.7 --- tests/fff_print/test_gcodewriter.cpp | 119 +++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/tests/fff_print/test_gcodewriter.cpp b/tests/fff_print/test_gcodewriter.cpp index e88009cfbe..321799c935 100644 --- a/tests/fff_print/test_gcodewriter.cpp +++ b/tests/fff_print/test_gcodewriter.cpp @@ -188,6 +188,125 @@ SCENARIO("set_pressure_advance emits Marlin Legacy form without tool qualifier e } } +SCENARIO("set_temperature per-flavor command routing", "[GCodeWriter][Temperature]") { + GIVEN("temperature=210, no tool index, no wait") { + WHEN("flavor is Marlin 2") { + std::string out = GCodeWriter::set_temperature(210, gcfMarlinFirmware, false, -1, std::string()); + THEN("output is M104 S210 with no tool qualifier") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M104 S210")); + REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring(" T")); + REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring("M109")); + } + } + WHEN("flavor is RepRapFirmware") { + std::string out = GCodeWriter::set_temperature(210, gcfRepRapFirmware, false, -1, std::string()); + THEN("output is G10 S210 (M104 is deprecated on RRF)") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("G10 S210")); + REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring("M104")); + } + } + WHEN("flavor is Mach3 or Machinekit") { + std::string mach3 = GCodeWriter::set_temperature(210, gcfMach3, false, -1, std::string()); + std::string machinekit = GCodeWriter::set_temperature(210, gcfMachinekit, false, -1, std::string()); + THEN("output uses P-prefix for the value instead of S") { + REQUIRE_THAT(mach3, Catch::Matchers::ContainsSubstring("M104 P210")); + REQUIRE_THAT(machinekit, Catch::Matchers::ContainsSubstring("M104 P210")); + REQUIRE_THAT(mach3, !Catch::Matchers::ContainsSubstring("S210")); + } + } + } +} + +SCENARIO("set_temperature wait=true handling per firmware", "[GCodeWriter][Temperature]") { + WHEN("flavor is Marlin 2 with wait") { + std::string out = GCodeWriter::set_temperature(210, gcfMarlinFirmware, true, -1, std::string()); + THEN("output is M109 S210 (blocking wait)") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M109 S210")); + REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring("M104")); + } + } + WHEN("flavor is MakerWare or Sailfish with wait") { + std::string mw = GCodeWriter::set_temperature(210, gcfMakerWare, true, -1, std::string()); + std::string sf = GCodeWriter::set_temperature(210, gcfSailfish, true, -1, std::string()); + THEN("output is empty — these flavors don't support blocking waits") { + REQUIRE(mw.empty()); + REQUIRE(sf.empty()); + } + } + WHEN("flavor is Teacup with wait") { + std::string out = GCodeWriter::set_temperature(210, gcfTeacup, true, -1, std::string()); + THEN("output emits M104 + a separate M116 poll (Teacup doesn't support M109)") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M104 S210")); + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M116")); + REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring("M109")); + } + } + WHEN("flavor is RepRapFirmware with wait") { + std::string out = GCodeWriter::set_temperature(210, gcfRepRapFirmware, true, -1, std::string()); + THEN("output emits G10 + M116 (same poll pattern as Teacup)") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("G10 S210")); + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M116")); + } + } +} + +SCENARIO("set_temperature per-tool qualifier routing for IMEX secondary carriages", + "[GCodeWriter][Temperature]") { + // IMEX secondary tools never go through a tool-change, so layer-change temperature + // for them is emitted via the tool-qualified static set_temperature overload. + GIVEN("temperature=220, tool=2, no wait") { + WHEN("flavor is Marlin 2") { + std::string out = GCodeWriter::set_temperature(220, gcfMarlinFirmware, false, 2, std::string()); + THEN("output contains T2 qualifier") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M104 S220 T2")); + } + } + WHEN("flavor is RepRapFirmware") { + std::string out = GCodeWriter::set_temperature(220, gcfRepRapFirmware, false, 2, std::string()); + THEN("output uses P-prefix for tool (RRF convention), not T") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("G10 S220 P2")); + REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring(" T2")); + } + } + WHEN("flavor is Klipper") { + std::string out = GCodeWriter::set_temperature(220, gcfKlipper, false, 1, std::string()); + THEN("output contains T1 qualifier (Klipper layer-change temperature uses T)") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M104 S220 T1")); + } + } + } +} + +SCENARIO("set_temperature instance overload forces tool=-1 on single-extruder writers", + "[GCodeWriter][Temperature]") { + // Guards against spuriously emitting `T0` on printers that only have one extruder. + // The instance overload discards the tool argument when !multiple_extruders. + GIVEN("A default GCodeWriter (multiple_extruders=false)") { + GCodeWriter writer; + writer.config.gcode_flavor.value = gcfMarlinFirmware; + + WHEN("set_temperature is called with tool=2") { + std::string out = writer.set_temperature(210, false, 2); + THEN("output has no T qualifier despite the caller passing tool=2") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M104 S210")); + REQUIRE_THAT(out, !Catch::Matchers::ContainsSubstring(" T")); + } + } + } + GIVEN("A GCodeWriter with multiple_extruders=true (not SEMM)") { + GCodeWriter writer; + writer.config.gcode_flavor.value = gcfMarlinFirmware; + writer.multiple_extruders = true; + + WHEN("set_temperature is called with tool=2") { + std::string out = writer.set_temperature(210, false, 2); + THEN("tool argument passes through — output contains T2") { + REQUIRE_THAT(out, Catch::Matchers::ContainsSubstring("M104 S210 T2")); + } + } + } +} + SCENARIO("set_pressure_advance emits BBL M900 L1000 M10 regardless of tool index", "[GCodeWriter][PressureAdvance]") { GIVEN("A BBL-flagged GCodeWriter (the flag overrides firmware flavor routing)") { From ee757dac20d970cc5828a53d67541a7c77522375 Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Fri, 24 Apr 2026 23:29:01 -0400 Subject: [PATCH 6/6] test: address self-review findings on [Variant] and [3mf][IMEX] coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review found two weaknesses in the preceding test commits: 1) The equal-size [Variant] scenario claimed to distinguish the truncation guard's `cur > target` predicate from a regression to `cur >= target`, but both paths yield identical child values in practice: when extruder_variant names match, set_with_restore's variant_index is fully populated (no -1 slots) and the merge path restores every position from backup — producing the same {1.5, 2.5} output as the skip path. The test passes in both guard states. Rewritten to use mismatched variant names between child and parent. variant_index then has -1 slots, and set_with_restore overwrites those positions with parent values. Now the merge path yields {0.8, 0.8} and the skip path yields {1.5, 2.5} — observably different. Verified: - `cur > target` (correct): 4 scenarios pass, 15 assertions - `cur >= target` (regressed): equal-size scenario fails with "1.5 is within 0.000000001 of 0.80000000000000004" - Guard removed entirely: child>parent + stride=2 both fail with truncation ("1 == 2" / "2 == 4") 2) The [3mf][IMEX] round-trip only covered a single plate. A plate- indexing regression (IMEX metadata landing on the wrong plate, or bleeding across plates on reload) would not have been caught. Added a multi-plate scenario: two plates with distinct mode and head-filament-map values. Asserts both land on their respective destination plates after reload. Load-bearing verified: - With IMEX serialization intact: 3 scenarios pass, 45 assertions - With IMEX serialization disabled: positive + multi-plate fail (both "nullptr != nullptr"); primary-mode passes (expects nullptr) - With primary-mode short-circuit removed: primary-mode scenario fails ("0x... == nullptr") because primary modes now serialize Co-Authored-By: Claude Opus 4.7 --- tests/libslic3r/test_3mf.cpp | 78 +++++++++++++++++++++++++++++++++ tests/libslic3r/test_config.cpp | 25 +++++++---- 2 files changed, 95 insertions(+), 8 deletions(-) diff --git a/tests/libslic3r/test_3mf.cpp b/tests/libslic3r/test_3mf.cpp index bf9c697cfd..e856883f0f 100644 --- a/tests/libslic3r/test_3mf.cpp +++ b/tests/libslic3r/test_3mf.cpp @@ -205,6 +205,84 @@ SCENARIO("BBS 3MF round-trips per-plate IMEX state (parallel mode + head filamen } } +SCENARIO("BBS 3MF round-trips distinct IMEX state across multiple plates", "[3mf][IMEX]") { + // Guards against a plate-indexing regression where IMEX metadata lands on the wrong + // plate or bleeds across plates on reload. Each plate carries distinct mode + head- + // filament-map values; the reload must reproduce them in the same order. + set_temporary_dir(boost::filesystem::temp_directory_path().string()); + + GIVEN("A Model with two plates each carrying different IMEX state") { + Model src_model; + std::string src_file = std::string(TEST_DATA_DIR) + "/test_3mf/Prusa.stl"; + REQUIRE(load_stl(src_file.c_str(), &src_model)); + src_model.add_default_instances(); + + DynamicPrintConfig src_config; + + PlateDataPtrs src_plates; + auto *plate0 = new PlateData(); + plate0->plate_index = 0; + plate0->config.set_key_value("imex_parallel_mode", new ConfigOptionString("copy_mode")); + plate0->config.set_key_value("imex_head_filament_map", new ConfigOptionString("1:2")); + src_plates.push_back(plate0); + + auto *plate1 = new PlateData(); + plate1->plate_index = 1; + plate1->config.set_key_value("imex_parallel_mode", new ConfigOptionString("mirror_mode")); + plate1->config.set_key_value("imex_head_filament_map", new ConfigOptionString("2:4,3:5")); + src_plates.push_back(plate1); + + WHEN("the model is saved to BBS 3MF and loaded back") { + std::string test_file = std::string(TEST_DATA_DIR) + "/test_3mf/imex_multiplate_roundtrip.3mf"; + + StoreParams store_params; + store_params.path = test_file.c_str(); + store_params.model = &src_model; + store_params.plate_data_list = src_plates; + store_params.config = &src_config; + REQUIRE(store_bbs_3mf(store_params)); + + Model dst_model; + DynamicPrintConfig dst_config; + PlateDataPtrs dst_plates; + std::vector dst_presets; + bool is_bbl = false; + Semver file_version; + ConfigSubstitutionContext ctxt{ ForwardCompatibilitySubstitutionRule::Disable }; + bool loaded = load_bbs_3mf(test_file.c_str(), &dst_config, &ctxt, &dst_model, + &dst_plates, &dst_presets, &is_bbl, &file_version); + boost::filesystem::remove(test_file); + + THEN("load succeeds and both plates are returned") { + REQUIRE(loaded); + REQUIRE(dst_plates.size() == 2); + } + THEN("plate 0 retains its own IMEX state (copy_mode, 1:2) and does not inherit plate 1's") { + REQUIRE(dst_plates.size() == 2); + auto *mode = dst_plates[0]->config.option("imex_parallel_mode"); + auto *hfm = dst_plates[0]->config.option("imex_head_filament_map"); + REQUIRE(mode != nullptr); + REQUIRE(hfm != nullptr); + REQUIRE(mode->value == "copy_mode"); + REQUIRE(hfm->value == "1:2"); + } + THEN("plate 1 retains its own IMEX state (mirror_mode, 2:4,3:5) and does not inherit plate 0's") { + REQUIRE(dst_plates.size() == 2); + auto *mode = dst_plates[1]->config.option("imex_parallel_mode"); + auto *hfm = dst_plates[1]->config.option("imex_head_filament_map"); + REQUIRE(mode != nullptr); + REQUIRE(hfm != nullptr); + REQUIRE(mode->value == "mirror_mode"); + REQUIRE(hfm->value == "2:4,3:5"); + } + + release_PlateData_list(dst_plates); + } + + release_PlateData_list(src_plates); + } +} + SCENARIO("BBS 3MF does not emit IMEX metadata when plate is in primary mode", "[3mf][IMEX]") { // The serialization guard short-circuits when the mode is empty or "primary", so loading // a plate that was saved in primary mode must not leave a stale imex_parallel_mode option diff --git a/tests/libslic3r/test_config.cpp b/tests/libslic3r/test_config.cpp index d6fad72508..00cb68d58b 100644 --- a/tests/libslic3r/test_config.cpp +++ b/tests/libslic3r/test_config.cpp @@ -310,16 +310,21 @@ SCENARIO("update_non_diff_values_to_base_config does not truncate stride=2 child } } -SCENARIO("update_non_diff_values_to_base_config preserves child variant values when child and parent extruder counts match", +SCENARIO("update_non_diff_values_to_base_config runs the merge path in the equal-size case", "[Config][Variant]") { - // The fix's guard is `cur > target ? skip`. The equal-size path must still run normally and - // preserve the child's per-extruder values via set_with_restore's nil-restore mechanism. - GIVEN("A 2-extruder child inheriting from a 2-extruder parent with different per-extruder values") { + // Distinguishes the fix's `cur > target` guard from a stricter `cur >= target`. + // With `cur > target` (correct): equal-size does NOT fire the guard; merge runs via + // set_with_restore, which builds variant_index by matching (extruder_variant, extruder_id) + // pairs between child and parent. When the variants don't match, variant_index positions + // stay at -1, and set_with_restore overwrites those child positions with parent values. + // With `cur >= target` (regressed): guard fires; merge is skipped; child values stay intact. + // Using mismatched variants makes the two outcomes observably different. + GIVEN("A 2-extruder child and parent with matching extruder counts but mismatched variant names") { Slic3r::DynamicPrintConfig child; Slic3r::DynamicPrintConfig parent; child.set_key_value("printer_extruder_id", new Slic3r::ConfigOptionInts({1, 2})); - child.set_key_value("printer_extruder_variant", new Slic3r::ConfigOptionStrings({"Direct Drive Standard", "Direct Drive Standard"})); + child.set_key_value("printer_extruder_variant", new Slic3r::ConfigOptionStrings({"Bowden Standard", "Bowden Standard"})); child.set_key_value("retraction_length", new Slic3r::ConfigOptionFloats({1.5, 2.5})); parent.set_key_value("printer_extruder_id", new Slic3r::ConfigOptionInts({1, 2})); @@ -344,10 +349,14 @@ SCENARIO("update_non_diff_values_to_base_config preserves child variant values w THEN("retraction_length retains size 2") { REQUIRE(child.option("retraction_length")->values.size() == 2); } - THEN("retraction_length preserves the child's per-extruder values, not the parent's") { + THEN("retraction_length gets parent values — proves the merge ran (guard did not fire)") { + // If the guard regressed to `cur >= target`, this path would be skipped and + // retraction_length would remain {1.5, 2.5}. The correct `cur > target` guard + // does not fire for equal-size, the merge proceeds, and with mismatched + // variants the child positions receive parent values. auto* rl = child.option("retraction_length"); - REQUIRE_THAT(rl->values[0], Catch::Matchers::WithinAbs(1.5, 1e-9)); - REQUIRE_THAT(rl->values[1], Catch::Matchers::WithinAbs(2.5, 1e-9)); + REQUIRE_THAT(rl->values[0], Catch::Matchers::WithinAbs(0.8, 1e-9)); + REQUIRE_THAT(rl->values[1], Catch::Matchers::WithinAbs(0.8, 1e-9)); } } }