From 36e5d4770b2cb833ed73f121b7fcd4f5ebc74476 Mon Sep 17 00:00:00 2001 From: Lam Wei Lun Date: Fri, 21 Aug 2026 19:04:10 +0800 Subject: [PATCH] Code cleanup and fixes --- src/libslic3r/PresetBundle.cpp | 56 ++++++++++++++----- src/libslic3r/PublishSettings.cpp | 44 +++++++++------ tests/libslic3r/test_3mf.cpp | 33 +++++++++++ .../libslic3r/test_preset_bundle_loading.cpp | 41 +++++++++++++- 4 files changed, 141 insertions(+), 33 deletions(-) diff --git a/src/libslic3r/PresetBundle.cpp b/src/libslic3r/PresetBundle.cpp index fffd9c7fa4..d29dbdf46f 100644 --- a/src/libslic3r/PresetBundle.cpp +++ b/src/libslic3r/PresetBundle.cpp @@ -4834,10 +4834,26 @@ void PresetBundle::load_config_file_config(const std::string &name_or_path, bool // receiver may have a different extruder count than the author. Out-of-range // indices are skipped (set_at would otherwise resize the receiver's vector). if (key.size() > base_key.size()) { - const size_t idx = static_cast(std::atoi(key.c_str() + base_key.size() + 1)); - if (idx >= static_cast(src_opt)->size() || + // Strict numeric suffix parse: a malformed variant ("#abc", "#1x") + // must be reported as skipped, not silently applied as element 0. + const std::string suffix = key.substr(base_key.size() + 1); + size_t idx = 0; + bool valid = !suffix.empty(); + for (const char c : suffix) { + if (c < '0' || c > '9') { + valid = false; + break; + } + idx = idx * 10 + size_t(c - '0'); + if (idx > 1000000) { // overflow guard; real vector sizes are tiny + valid = false; + break; + } + } + if (!valid || + idx >= static_cast(src_opt)->size() || idx >= static_cast(dst_opt)->size()) - continue; // out-of-range variant: cannot apply; reported as skipped + continue; // malformed or out-of-range variant: cannot apply; reported as skipped } else if (static_cast(src_opt)->size() != static_cast(dst_opt)->size()) { // Whole-vector base key: the receiver must have a matching vector size, @@ -4894,15 +4910,18 @@ void PresetBundle::load_config_file_config(const std::string &name_or_path, bool // Grow the receiver's slots only as far as the highest published slot (never // shrink, never pull filler materials for unpublished slots). bool has_published_entries = false; - size_t target_slots = this->filament_presets.size(); + size_t grow_target = 0; for (const PublishedMaterialEntry &entry : published_config->material_keys) { has_published_entries = true; if (entry.slot >= 0) - target_slots = std::max(target_slots, size_t(entry.slot) + 1); + grow_target = std::max(grow_target, size_t(entry.slot) + 1); } if (has_published_entries) { - // Defensive cap: never exceed the file's own filament count. - target_slots = std::min(target_slots, num_filaments); + // Defensive cap: growth never exceeds the file's own filament count. The + // receiver's current slot count is a floor: neither the preset list nor the + // project vectors are ever shrunk, even when the file carries fewer filaments + // than the receiver has slots. + const size_t target_slots = std::max(this->filament_presets.size(), std::min(grow_target, num_filaments)); // Slots carrying published content, steering the initial preset selection of // newly grown slots. std::set published_slots; @@ -5122,13 +5141,22 @@ void PresetBundle::load_config_file_config(const std::string &name_or_path, bool ConfigOptionInts *proj_nozzle_map = this->project_config.opt("filament_nozzle_map"); ConfigOptionInts *proj_volume_map = this->project_config.opt("filament_volume_map"); const size_t old_colour_count = (proj_colour != nullptr) ? proj_colour->values.size() : 0; - if (proj_colour) proj_colour->resize(target_slots); - if (proj_multi_colour) proj_multi_colour->values.resize(target_slots); - if (proj_colour_type) proj_colour_type->values.resize(target_slots); - if (proj_map) proj_map->values.resize(target_slots, 1); - if (proj_nozzle_map) proj_nozzle_map->values.resize(target_slots, 0); - if (proj_volume_map) proj_volume_map->values.resize(target_slots, static_cast(NozzleVolumeType::nvtStandard)); - this->ams_multi_color_filment.resize(target_slots); + // Grow-only: an already-larger project vector is left untouched (the receiver's + // slot count never shrinks below its own setup). + if (proj_colour && proj_colour->values.size() < target_slots) + proj_colour->resize(target_slots); + if (proj_multi_colour && proj_multi_colour->values.size() < target_slots) + proj_multi_colour->values.resize(target_slots); + if (proj_colour_type && proj_colour_type->values.size() < target_slots) + proj_colour_type->values.resize(target_slots); + if (proj_map && proj_map->values.size() < target_slots) + proj_map->values.resize(target_slots, 1); + if (proj_nozzle_map && proj_nozzle_map->values.size() < target_slots) + proj_nozzle_map->values.resize(target_slots, 0); + if (proj_volume_map && proj_volume_map->values.size() < target_slots) + proj_volume_map->values.resize(target_slots, static_cast(NozzleVolumeType::nvtStandard)); + if (this->ams_multi_color_filment.size() < target_slots) + this->ams_multi_color_filment.resize(target_slots); for (size_t slot = old_colour_count; slot < target_slots; ++slot) { std::string seed; for (const PublishedMaterialEntry &entry : published_config->material_keys) diff --git a/src/libslic3r/PublishSettings.cpp b/src/libslic3r/PublishSettings.cpp index e212770fd8..ac25d815f1 100644 --- a/src/libslic3r/PublishSettings.cpp +++ b/src/libslic3r/PublishSettings.cpp @@ -5,6 +5,8 @@ #include "PrintConfig.hpp" #include "MaterialType.hpp" +#include + #include #include @@ -188,17 +190,20 @@ DynamicPrintConfig filter_published_config( } } - // Mask non-published vector slots with the option default; keys without a default stay - // unmasked (whole vector, matching partial-publish behavior). + // Masking restores every non-published slot of a vector option with the option default, so + // a partial publish does not leak unrelated slot data. can_mask_slots reports whether a key + // is maskable at all (vector option plus a registered default of the same type); an + // unmaskable key is dropped from the payload entirely instead of shipping the author's + // whole vector. + auto can_mask_slots = [](const ConfigOption &opt, const ConfigOptionDef *def) -> bool { + if (def == nullptr || !def->default_value || def->default_value->type() != opt.type()) + return false; + const auto *vec = dynamic_cast(&opt); + const auto *default_vec = dynamic_cast(def->default_value.get()); + return vec != nullptr && vec->size() > 0 && default_vec != nullptr && !default_vec->empty(); + }; auto mask_slots = [](ConfigOption &opt, const ConfigOptionDef *def, const std::set &keep_slots) { auto *vec = dynamic_cast(&opt); - if (vec == nullptr || vec->size() == 0 || def == nullptr || !def->default_value) - return; - if (def->default_value->type() != opt.type()) - return; - const auto *default_vec = dynamic_cast(def->default_value.get()); - if (default_vec == nullptr || default_vec->empty()) - return; for (size_t idx = 0; idx < vec->size(); ++idx) if (keep_slots.count(static_cast(idx)) == 0) vec->set_at(def->default_value.get(), idx, 0); @@ -206,15 +211,20 @@ DynamicPrintConfig filter_published_config( // Copy the selected options from full_config into the filtered config. for (const std::string &key : base_keys_to_include) { - if (const ConfigOption *opt = full_config.option(key)) { - ConfigOption *cloned = opt->clone(); - if (mask_exempt_keys.count(key) == 0) { - const auto it = slot_mask_map.find(key); - if (it != slot_mask_map.end() && !it->second.empty()) - mask_slots(*cloned, print_config_def.get(key), it->second); - } - filtered.set_key_value(key, cloned); + const ConfigOption *opt = full_config.option(key); + if (opt == nullptr) + continue; + const auto mask_it = slot_mask_map.find(key); + const bool needs_masking = mask_exempt_keys.count(key) == 0 && mask_it != slot_mask_map.end() && !mask_it->second.empty(); + if (needs_masking && !can_mask_slots(*opt, print_config_def.get(key))) { + BOOST_LOG_TRIVIAL(warning) << "publish: dropping unmaskable key \"" << key + << "\" from the published payload (no usable option default)"; + continue; } + ConfigOption *cloned = opt->clone(); + if (needs_masking) + mask_slots(*cloned, print_config_def.get(key), mask_it->second); + filtered.set_key_value(key, cloned); } return filtered; diff --git a/tests/libslic3r/test_3mf.cpp b/tests/libslic3r/test_3mf.cpp index 6a20613a79..8898dae2fe 100644 --- a/tests/libslic3r/test_3mf.cpp +++ b/tests/libslic3r/test_3mf.cpp @@ -841,6 +841,39 @@ SCENARIO("Partial-publish entries mask the other slots like full entries", "[3mf } } +// A key needing slot masking that cannot be masked (no registered option default of the same +// type) is dropped from the payload entirely instead of shipping the author's whole vector. +SCENARIO("Unmaskable keys are dropped from the published payload instead of leaking", "[3mf]") { + GIVEN("a config carrying a synthetic def-less vector key and a maskable one") { + DynamicPrintConfig full_cfg = DynamicPrintConfig::full_print_config(); + full_cfg.opt("filament_diameter")->values = { 1.75, 1.75 }; + full_cfg.opt("filament_colour")->values = { "#111111", "#222222" }; + // Not a PrintConfig key: print_config_def has no default to mask with. + full_cfg.set_key_value("orca_synthetic_setting", new ConfigOptionFloats({ 9.9, 8.8 })); + full_cfg.opt("filament_flow_ratio", true)->values = { 1.02, 0.98 }; + + PublishedMaterialEntry partial_entry; + partial_entry.slot = 1; + partial_entry.keys = { "orca_synthetic_setting", "filament_flow_ratio" }; + + WHEN("filtering with a partial entry for slot 1") { + DynamicPrintConfig filtered_cfg = filter_published_config(full_cfg, {}, { partial_entry }); + + THEN("the unmaskable synthetic key is not published") { + REQUIRE(filtered_cfg.option("orca_synthetic_setting") == nullptr); + } + THEN("the maskable key is present, author slot kept, other slot masked") { + REQUIRE(filtered_cfg.opt("filament_flow_ratio") != nullptr); + REQUIRE(filtered_cfg.opt("filament_flow_ratio")->values[1] == 0.98); + REQUIRE(filtered_cfg.opt("filament_flow_ratio")->values[0] == 1.0); + } + THEN("the identity keys stay present") { + REQUIRE(filtered_cfg.option("filament_colour") != nullptr); + } + } + } +} + // The extended per-entry fields (full dump list, published type and colour) travel inside the // published_material_keys metadata and round-trip unchanged. SCENARIO("Published 3MF round-trips the extended material metadata", "[3mf]") { diff --git a/tests/libslic3r/test_preset_bundle_loading.cpp b/tests/libslic3r/test_preset_bundle_loading.cpp index b68fafbc2a..3a344e0fdc 100644 --- a/tests/libslic3r/test_preset_bundle_loading.cpp +++ b/tests/libslic3r/test_preset_bundle_loading.cpp @@ -1890,6 +1890,41 @@ TEST_CASE("Published 3MF grows the receiver's slots only as far as the published REQUIRE(bundle.filament_presets.size() == 3); CHECK(bundle.filament_presets[1] == "My PLA"); } + + // A receiver with more slots than the file's filament count keeps its setup: neither the + // preset list nor the project-level vectors are shrunk to the file's smaller size. + { + PresetBundle bundle; + add_pla_preset(bundle); + bundle.filament_presets = { "My PLA", "My PLA", "My PLA" }; + // Distinct project colours make a shrink observable. + bundle.project_config.opt("filament_colour")->values = { "#111111", "#222222", "#333333" }; + bundle.project_config.opt("filament_multi_colour")->values = { "#111111", "#222222", "#333333" }; + + PublishedConfig pub; + pub.published = true; + pub.material_keys = { make_color_entry(0) }; // highest published slot: 0 + DynamicPrintConfig config = DynamicPrintConfig::full_print_config(); + // A one-filament file: num_filaments (1) is below the receiver's slot count (3). + config.opt("filament_diameter")->values = { 1.75 }; + config.opt("filament_self_index")->values = { 1 }; + config.opt("filament_extruder_variant")->values = { "Direct Drive Standard" }; + config.opt("filament_colour")->values = { "#FF0000" }; + config.opt("filament_type")->values = { "PLA" }; + config.opt("filament_vendor")->values = { "Generic" }; + config.opt("filament_ids")->values = { "GFL99" }; + Preset::normalize(config); + bundle.load_config_model("test.3mf", std::move(config), Semver(), &pub); + + REQUIRE(bundle.filament_presets.size() == 3); + // No shrink: all three project entries survive, with slot 0 synced to the published + // colour at its unshifted index and slots 1-2 untouched. + CHECK(bundle.project_config.opt("filament_colour")->values == std::vector{ "#ABCDEF", "#222222", "#333333" }); + CHECK(bundle.project_config.opt("filament_multi_colour")->values == std::vector{ "#ABCDEF", "#222222", "#333333" }); + CHECK(bundle.project_config.opt("filament_map")->values.size() == 3); + // The published colour still reached slot 0's preset in place. + CHECK(bundle.filaments.find_preset("My PLA", false, true)->config.opt("filament_colour")->values == std::vector{ "#ABCDEF" }); + } } // A published slot is seeded from an unused library preset and the values are written onto it @@ -2108,14 +2143,16 @@ TEST_CASE("Published 3MF rejects out-of-range vector variants and variant-suffix PublishedConfig pub; pub.published = true; - pub.published_keys = { "wiping_volumes_extruders#5", "wiping_volumes_extruders#1", "layer_height#0" }; + pub.published_keys = { "wiping_volumes_extruders#5", "wiping_volumes_extruders#1", "wiping_volumes_extruders#abc", "layer_height#0" }; bundle.load_config_model("test.3mf", std::move(config), Semver(), &pub); // In-range variant applied element-wise; the out-of-range one did not resize the vector. CHECK(bundle.prints.get_edited_preset().config.opt("wiping_volumes_extruders")->values == std::vector{ 10., 150. }); CHECK(bundle.prints.get_edited_preset().config.opt("wiping_volumes_extruders")->values.size() == 2); - // Out-of-range variant and variant-suffixed scalar are reported as skipped. + // Out-of-range variant, malformed variant and variant-suffixed scalar are reported as + // skipped; the malformed one must not fall back to element 0. CHECK(contains_key(pub.skipped_keys, "wiping_volumes_extruders#5")); + CHECK(contains_key(pub.skipped_keys, "wiping_volumes_extruders#abc")); CHECK(contains_key(pub.skipped_keys, "layer_height#0")); CHECK_FALSE(contains_key(pub.skipped_keys, "wiping_volumes_extruders#1")); // The scalar was never applied.