mirror of
https://github.com/OrcaSlicer/OrcaSlicer.git
synced 2026-10-10 17:21:10 +00:00
test: address self-review findings on [Variant] and [3mf][IMEX] coverage
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
b45d8a5b7b
commit
ee757dac20
@@ -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<Preset*> 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<ConfigOptionString>("imex_parallel_mode");
|
||||
auto *hfm = dst_plates[0]->config.option<ConfigOptionString>("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<ConfigOptionString>("imex_parallel_mode");
|
||||
auto *hfm = dst_plates[1]->config.option<ConfigOptionString>("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
|
||||
|
||||
@@ -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<Slic3r::ConfigOptionFloats>("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<Slic3r::ConfigOptionFloats>("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));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user