From ee757dac20d970cc5828a53d67541a7c77522375 Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Fri, 24 Apr 2026 23:29:01 -0400 Subject: [PATCH] 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)); } } }