From 205de9ce633dba932e03c4d441fe074de09341a6 Mon Sep 17 00:00:00 2001 From: HanifKoh <76276251+HanifKoh@users.noreply.github.com> Date: Fri, 2 Oct 2026 20:13:06 +0800 Subject: [PATCH] Keep User Preset Values on Extruder Variants They Don't List (#16046) * Keep User Preset Values on Extruder Variants They Don't List A user preset stores the variant list its parent had when it was saved. When the parent later gains variants, update_diff_values_to_child_config matched variants by name only and left the new ones at the parent's value, so the user's settings were silently replaced there, and a re-save wrote the system values into the user's file. An unmatched parent variant now takes the child's first variant of the same extruder, the rule slicing already uses in get_config_index_base. A child without a variant list covers the parent's first extruder. The name match also no longer indexes the child's extruder ids when it has none. * Share One Variant Column Rule Between Slicing, User Presets and Projects Three places chose which variant column a value comes from, each with its own copy of "the same variant and owner, else the owner's first column": get_config_index_base when slicing, the user preset merge in update_diff_values_to_child_config, and normalize_filament_values_to_variants for projects and the CLI. find_variant_column now holds that rule and map_variant_columns applies it to a variant list, so a change to how missing variants are filled reaches all three. Each caller keeps its own copy step. There is no behaviour change: G-code is identical before and after. The one relaxation is that get_config_index_base no longer reads past a short id list when its two lists differ in length, which its assert already rules out. * Rename variant column helpers to variant index --------- Co-authored-by: SoftFever --- src/libslic3r/PrintConfig.cpp | 86 +++++++++++++++++--------- src/libslic3r/PrintConfig.hpp | 15 ++++- tests/libslic3r/test_config.cpp | 103 ++++++++++++++++++++++++++++++++ 3 files changed, 175 insertions(+), 29 deletions(-) diff --git a/src/libslic3r/PrintConfig.cpp b/src/libslic3r/PrintConfig.cpp index d4d0c90272..fbd72117cd 100644 --- a/src/libslic3r/PrintConfig.cpp +++ b/src/libslic3r/PrintConfig.cpp @@ -8,6 +8,7 @@ #include "format.hpp" #include "GCode/Thumbnails.hpp" +#include #include #include #include @@ -671,19 +672,46 @@ std::string get_extruder_variant_string(ExtruderType extruder_type, NozzleVolume return variant_string; } +int find_variant_index(const std::string& variant, int variant_id_1based, const std::vector& variant_list, const std::vector& variant_ids_1based) +{ + const int count = int(variant_list.empty() ? variant_ids_1based.size() : variant_list.size()); + if (count == 0) + return 0; + auto same_id = [&](int index) { + return variant_id_1based < 0 || variant_ids_1based.empty() || (index < int(variant_ids_1based.size()) && variant_ids_1based[index] == variant_id_1based); + }; + for (int index = 0; index < int(variant_list.size()); ++index) + if (variant_list[index] == variant && same_id(index)) + return index; + // Without this variant, use the id's own first variant (usually Standard), not variant index 0, + // which belongs to the first filament or extruder. + for (int index = 0; index < count; ++index) + if (same_id(index)) + return index; + return -1; +} + +std::vector map_variant_indices(const std::vector& variants, const std::vector& ids, + const std::vector& from_variants, const std::vector& from_ids) +{ + const size_t count = variants.empty() ? ids.size() : variants.size(); + std::vector variant_index(count); + for (size_t index = 0; index < count; ++index) { + if (!ids.empty() && index >= ids.size()) { + variant_index[index] = -1; + continue; + } + variant_index[index] = find_variant_index(index < variants.size() ? variants[index] : std::string(), + ids.empty() ? -1 : ids[index], from_variants, from_ids); + } + return variant_index; +} + int get_config_index_base(NozzleVolumeType volume_type, ExtruderType extruder_type, int variant_id_1based, const std::vector& variant_list, const std::vector& variant_ids_1based) { assert(variant_list.size() == variant_ids_1based.size()); - std::string extruder_variant = get_extruder_variant_string(extruder_type, volume_type); - for (int index = 0; index < int(variant_list.size()); ++index) { - if (extruder_variant == variant_list[index] && variant_ids_1based[index] == variant_id_1based) { return index; } - } - // Without this variant, use the id's own first variant (usually Standard), not variant index 0, - // which belongs to the first filament or extruder. - for (int index = 0; index < int(variant_list.size()); ++index) { - if (variant_ids_1based[index] == variant_id_1based) { return index; } - } - return 0; + const int index = find_variant_index(get_extruder_variant_string(extruder_type, volume_type), variant_id_1based, variant_list, variant_ids_1based); + return std::max(index, 0); } std::set get_extruder_supported_nozzle_volume_types(const DynamicPrintConfig &printer_config, int extruder_id) @@ -10741,14 +10769,23 @@ void normalize_filament_values_to_variants(DynamicPrintConfig &config) const int filament_count = *std::max_element(self_index->values.begin(), self_index->values.end()); if (filament_count <= 0 || size_t(filament_count) >= self_index->size()) return; + // The values are one per filament, without variant strings, or a single value for all of them. The + // variant strings do not change today's mapping; they are passed so a rule that reads them applies here too. + const auto *variants = config.option("filament_extruder_variant"); + const std::vector variant_list = variants && variants->size() == self_index->size() ? variants->values : std::vector(); + std::vector filament_ids(filament_count); + std::iota(filament_ids.begin(), filament_ids.end(), 1); + const std::vector from_filaments = map_variant_indices(variant_list, self_index->values, {}, filament_ids); + const std::vector from_single = map_variant_indices(variant_list, self_index->values, {}, {}); for (const std::string &key : filament_options_with_variant) { auto *opt = dynamic_cast(config.option(key)); if (opt == nullptr || (opt->size() != size_t(filament_count) && opt->size() != 1)) continue; - std::unique_ptr per_filament(opt->clone()); - // set_at() takes the first value for a filament past the end of a single-value vector + const std::vector &variant_index = opt->size() == size_t(filament_count) ? from_filaments : from_single; + std::unique_ptr source(opt->clone()); + // -1 and a single-value source both resolve to the first value through get_at() for (size_t variant = 0; variant < self_index->size(); ++variant) - opt->set_at(per_filament.get(), variant, self_index->values[variant] - 1); + opt->set_at(source.get(), variant, variant_index[variant]); } } @@ -11582,8 +11619,14 @@ void DynamicPrintConfig::update_diff_values_to_child_config(DynamicPrintConfig& else variant_index.resize(1, 0); + // A parent variant the child does not list (the parent gained it after the child was saved, or the + // child lists none) takes the child's first variant of the same extruder, as slicing does. + // Left unmatched, the parent's value would silently replace the user's. if (target_variant_count == 0) { - variant_index[0] = 0; + // The child's one value belongs to the extruder of the parent's first variant. + if (cur_variant_count > 0) + variant_index = map_variant_indices(cur_extruder_variants, cur_extruder_ids, {}, + cur_extruder_ids.empty() ? std::vector() : std::vector{cur_extruder_ids[0]}); } else if ((cur_extruder_ids.size() > 0) && cur_variant_count != cur_extruder_ids.size()){ //should not happen @@ -11595,19 +11638,8 @@ void DynamicPrintConfig::update_diff_values_to_child_config(DynamicPrintConfig& BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << boost::format(" size of %1% = %2%, not equal to size of %3% = %4%") %extruder_variant_name %target_variant_count %extruder_id_name %target_extruder_ids.size(); } - else { - for (int i = 0; i < cur_variant_count; i++) - { - for (int j = 0; j < target_variant_count; j++) - { - if ((cur_extruder_variants[i] == target_extruder_variants[j]) - &&(cur_extruder_ids.empty() || (cur_extruder_ids[i] == target_extruder_ids[j]))) - { - variant_index[i] = j; - break; - } - } - } + else if (cur_variant_count > 0) { + variant_index = map_variant_indices(cur_extruder_variants, cur_extruder_ids, target_extruder_variants, target_extruder_ids); } const t_config_option_keys &keys = new_config.keys(); diff --git a/src/libslic3r/PrintConfig.hpp b/src/libslic3r/PrintConfig.hpp index 3430de96fb..b1c407e3b6 100644 --- a/src/libslic3r/PrintConfig.hpp +++ b/src/libslic3r/PrintConfig.hpp @@ -556,8 +556,19 @@ enum PrimeVolumeMode { extern std::string get_extruder_variant_string(ExtruderType extruder_type, NozzleVolumeType nozzle_volume_type); -// Base slot lookup: scans a variant list (paired with its 1-based extruder/filament ids) for the -// entry matching the given extruder/volume type and id. Returns 0 when no entry matches. +// The variant index a value is taken from: in a variant list paired with its 1-based extruder or +// filament ids, the variant with the same variant string and id, else that id's first variant, else -1. +// variant_id_1based < 0 or empty variant_ids_1based match any id. A list without variant strings has +// one variant per id, and one with neither variant strings nor ids has a single variant. +extern int find_variant_index(const std::string& variant, int variant_id_1based, const std::vector& variant_list, const std::vector& variant_ids_1based); +// find_variant_index for every variant of a list paired with its ids, into from_variants/from_ids. +// A variant past the end of a shorter id list has no id and gets -1. +extern std::vector map_variant_indices(const std::vector& variants, const std::vector& ids, + const std::vector& from_variants, const std::vector& from_ids); + +// Variant index lookup: scans a variant list (paired with its 1-based extruder/filament ids) for the +// entry matching the given extruder/volume type and id, as find_variant_index. Returns 0 when the id +// has no variant. extern int get_config_index_base(NozzleVolumeType volume_type, ExtruderType extruder_type, int variant_id_1based, const std::vector& variant_list, const std::vector& variant_ids_1based); static std::set get_valid_nozzle_volume_type() { diff --git a/tests/libslic3r/test_config.cpp b/tests/libslic3r/test_config.cpp index 00c4d6d170..d7803f300b 100644 --- a/tests/libslic3r/test_config.cpp +++ b/tests/libslic3r/test_config.cpp @@ -421,6 +421,109 @@ SCENARIO("update_diff_values_to_child_config tolerates legacy machine-limit vect } } +TEST_CASE("A variant index comes from the same variant and id, else the id's first variant", "[Config][Variant]") { + const std::vector variant_list{"Direct Drive Standard", "Direct Drive High Flow", "Direct Drive Standard"}; + const std::vector variant_ids{1, 1, 2}; + + SECTION("same variant and id") { + CHECK(Slic3r::find_variant_index("Direct Drive High Flow", 1, variant_list, variant_ids) == 1); + CHECK(Slic3r::find_variant_index("Direct Drive Standard", 2, variant_list, variant_ids) == 2); + } + SECTION("a variant the id lacks falls back to the id's first variant") { + CHECK(Slic3r::find_variant_index("Bowden Standard", 1, variant_list, variant_ids) == 0); + CHECK(Slic3r::find_variant_index("Direct Drive High Flow", 2, variant_list, variant_ids) == 2); + } + SECTION("an id with no variants matches none") { + CHECK(Slic3r::find_variant_index("Direct Drive Standard", 3, variant_list, variant_ids) == -1); + } + SECTION("a negative id or a list without ids matches any id") { + CHECK(Slic3r::find_variant_index("Direct Drive High Flow", -1, variant_list, variant_ids) == 1); + CHECK(Slic3r::find_variant_index("Direct Drive High Flow", 2, variant_list, {}) == 1); + } + SECTION("a variant past a shorter id list gets no variant index") { + CHECK(Slic3r::map_variant_indices(variant_list, {1}, variant_list, variant_ids) == std::vector{0, -1, -1}); + } + SECTION("a list without variant strings has one variant per id, and an empty one a single variant") { + CHECK(Slic3r::map_variant_indices(variant_list, variant_ids, {}, {1, 2}) == std::vector{0, 0, 1}); + CHECK(Slic3r::map_variant_indices(variant_list, variant_ids, {}, {}) == std::vector{0, 0, 0}); + } +} + +SCENARIO("update_diff_values_to_child_config keeps a child's values on variants it does not list", + "[Config][Variant]") { + std::set no_keys; + auto variants = [](std::initializer_list names) { return new Slic3r::ConfigOptionStrings(names); }; + + GIVEN("A filament parent with three variants") { + Slic3r::DynamicPrintConfig parent; + parent.set_key_value("filament_extruder_variant", + variants({"Direct Drive Standard", "Bowden Standard", "Direct Drive High Flow"})); + parent.set_deserialize_strict("nozzle_temperature", "220,220,220"); + + WHEN("the child was saved when the parent had only its first variant") { + Slic3r::DynamicPrintConfig child; + child.set_key_value("filament_extruder_variant", variants({"Direct Drive Standard"})); + child.set_deserialize_strict("nozzle_temperature", "199"); + parent.update_diff_values_to_child_config(child, "", "filament_extruder_variant", + Slic3r::filament_options_with_variant, no_keys); + THEN("the child's value applies to every variant") { + REQUIRE(parent.opt_serialize("nozzle_temperature") == "199,199,199"); + } + } + WHEN("the child lists every variant, in another order") { + Slic3r::DynamicPrintConfig child; + child.set_key_value("filament_extruder_variant", + variants({"Bowden Standard", "Direct Drive High Flow", "Direct Drive Standard"})); + child.set_deserialize_strict("nozzle_temperature", "190,205,199"); + parent.update_diff_values_to_child_config(child, "", "filament_extruder_variant", + Slic3r::filament_options_with_variant, no_keys); + THEN("each variant keeps its own value") { + REQUIRE(parent.opt_serialize("nozzle_temperature") == "199,190,205"); + } + } + WHEN("the child lists no variants") { + Slic3r::DynamicPrintConfig child; + child.set_deserialize_strict("nozzle_temperature", "199"); + parent.update_diff_values_to_child_config(child, "", "filament_extruder_variant", + Slic3r::filament_options_with_variant, no_keys); + THEN("the child's value applies to every variant") { + REQUIRE(parent.opt_serialize("nozzle_temperature") == "199,199,199"); + } + } + } + + GIVEN("A two-extruder printer parent with two variants per extruder") { + Slic3r::DynamicPrintConfig parent; + parent.set_key_value("printer_extruder_variant", + variants({"Direct Drive Standard", "Direct Drive High Flow", "Direct Drive Standard", "Direct Drive High Flow"})); + parent.set_key_value("printer_extruder_id", new Slic3r::ConfigOptionInts({1, 1, 2, 2})); + parent.set_deserialize_strict("retraction_length", "0.8,0.8,0.8,0.8"); + + WHEN("the child lists only the Standard variant of each extruder") { + Slic3r::DynamicPrintConfig child; + child.set_key_value("printer_extruder_variant", variants({"Direct Drive Standard", "Direct Drive Standard"})); + child.set_key_value("printer_extruder_id", new Slic3r::ConfigOptionInts({1, 2})); + child.set_deserialize_strict("retraction_length", "1.1,2.2"); + parent.update_diff_values_to_child_config(child, "printer_extruder_id", "printer_extruder_variant", + Slic3r::printer_options_with_variant_1, + Slic3r::printer_options_with_variant_2); + THEN("each extruder's High Flow variant takes that extruder's value") { + REQUIRE(parent.opt_serialize("retraction_length") == "1.1,1.1,2.2,2.2"); + } + } + WHEN("the child lists no variants") { + Slic3r::DynamicPrintConfig child; + child.set_deserialize_strict("retraction_length", "1.1"); + parent.update_diff_values_to_child_config(child, "printer_extruder_id", "printer_extruder_variant", + Slic3r::printer_options_with_variant_1, + Slic3r::printer_options_with_variant_2); + THEN("only the first extruder's variants take the child's value") { + REQUIRE(parent.opt_serialize("retraction_length") == "1.1,1.1,0.8,0.8"); + } + } + } +} + // SCENARIO("DynamicPrintConfig JSON serialization", "[Config]") { // WHEN("DynamicPrintConfig is serialized and deserialized") { // auto now = std::chrono::high_resolution_clock::now();