Cover the substitutions of a preset held back for its parent

A preset whose parent is not in the collection yet is read once per pass it
waits, and each read appends to the caller's substitutions list. The load drops
what it read before deferring, so only the pass that keeps the preset reports
its substitutions; without that, one preset is listed once per pass and the
count depends on how deep it sits in the hierarchy.

src/libslic3r/Preset.cpp

@@ -1624,6 +1624,7 @@ void PresetCollection::reset(bool delete_files)
     unlock();
     m_map_alias_to_profile_name.clear();
     m_map_system_profile_renamed.clear();
+    m_unresolved_parents.clear();
 }
 
 void PresetCollection::add_default_preset(const std::vector<std::string> &keys, const Slic3r::StaticPrintConfig &defaults, const std::string &preset_name)
@@ -1676,19 +1677,37 @@ void PresetCollection::load_presets(
     }
 
     std::string errors_cummulative;
-    // Store the loaded presets into a new vector, otherwise the binary search for already existing presets would be broken.
-    // (see the "Preset already present, not loading" message).
-    std::deque<Preset> presets_loaded;
 
     //BBS: get the extruder related info for this preset collection
     std::string extruder_id_name, extruder_variant_name;
     std::set<std::string> *key_set1 = nullptr, *key_set2 = nullptr;
     Preset::get_extruder_names_and_keysets(m_type, extruder_id_name, extruder_variant_name, &key_set1, &key_set2);
 
-    //BBS: change to json format
+    std::vector<boost::filesystem::path> pending;
     for (auto &dir_entry : boost::filesystem::directory_iterator(dir))
+        pending.emplace_back(dir_entry.path());
+    // The iteration order of directory_iterator is unspecified; sort so that the number of passes
+    // below, and the presets that survive them, do not depend on the filesystem.
+    std::sort(pending.begin(), pending.end());
+
+    size_t loaded_count = 0;
+    // A preset may inherit another preset from this same directory, but the presets loaded here
+    // become visible to find_preset2() only once they are merged into m_presets at the end of a
+    // pass. A preset whose parent has not been merged yet is therefore deferred and retried on a
+    // further pass instead of being dropped; each pass resolves one more level of the hierarchy.
+    // A pass that resolves nothing means the remaining parents genuinely do not exist (or form a
+    // cycle), and only then are they reported as errors.
+    while (!pending.empty()) {
+        // Store the loaded presets into a new vector, otherwise the binary search for already existing presets would be broken.
+        // (see the "Preset already present, not loading" message).
+        std::deque<Preset> presets_loaded;
+        // Preset file and the parent name it could not resolve yet.
+        std::vector<std::pair<boost::filesystem::path, std::string>> deferred;
+
+        //BBS: change to json format
+        for (const auto &preset_path : pending)
         {
-        std::string file_name = dir_entry.path().filename().string();
+            std::string file_name = preset_path.filename().string();
             //if (Slic3r::is_ini_file(dir_entry)) {
             if (Slic3r::is_json_file(file_name)) {
                 // Remove the .ini suffix.
@@ -1703,7 +1722,10 @@ void PresetCollection::load_presets(
                 try {
                     Preset preset(m_type, canonical_name, false);
                     preset.bundle_id = resolved_origin.bundle_id;
-                preset.file = dir_entry.path().string();
+                    preset.file = preset_path.string();
+                    // Substitutions reported below are rolled back if this preset ends up deferred,
+                    // so that a retried preset does not report them twice.
+                    const size_t substitutions_before = substitutions.size();
                     // Load the preset file, apply preset values on top of defaults.
                     try {
                         fs::path idx_path(preset.file);
@@ -1754,8 +1776,6 @@ void PresetCollection::load_presets(
                             // Orca: try to find if the parent preset has been renamed
                             inherit_preset = this->find_preset2(inherits_value);
                             Preset::normalize_inherits(config, inherit_preset);
-                    } else {
-                        ;
                         }
                         const Preset& default_preset = this->default_preset_for(config);
                         if (inherit_preset) {
@@ -1767,8 +1787,11 @@ void PresetCollection::load_presets(
                         else {
                             auto inherits_config2 = dynamic_cast<ConfigOptionString *>(inherits_config);
                             if ((inherits_config2 && !inherits_config2->value.empty())) {
-                            BOOST_LOG_TRIVIAL(error) << boost::format("can not find parent %1% for config %2%!")%inherits_config2->value %preset.file;
+                                // The parent may be another preset of this same pass, not merged into
-                            ++m_errors;
+                                // m_presets yet. Retry once it is; only a pass that resolves nothing
+                                // reports the parent as missing.
+                                substitutions.resize(substitutions_before);
+                                deferred.emplace_back(preset_path, inherits_config2->value);
                                 continue;
                             }
                             // We support custom root preset now
@@ -1846,8 +1869,24 @@ void PresetCollection::load_presets(
         if (presets_loaded.size() > 0)
             m_presets.insert(m_presets.end(), std::make_move_iterator(presets_loaded.begin()), std::make_move_iterator(presets_loaded.end()));
         sort_presets();
+        loaded_count += presets_loaded.size();
+
+        if (deferred.empty())
+            break;
+        if (presets_loaded.empty()) {
+            for (const auto &entry : deferred) {
+                BOOST_LOG_TRIVIAL(error) << boost::format("can not find parent %1% for config %2%!")%entry.second %entry.first.string();
+                m_unresolved_parents[entry.first.string()] = entry.second;
+                ++m_errors;
+            }
+            break;
+        }
+        pending.clear();
+        for (auto &entry : deferred)
+            pending.emplace_back(std::move(entry.first));
+    }
     //BBS: add config related logs
-    BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << boost::format(": loaded %1% presets from %2%, type %3%")%presets_loaded.size() %dir %Preset::get_type_string(m_type);
+    BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << boost::format(": loaded %1% presets from %2%, type %3%")%loaded_count %dir %Preset::get_type_string(m_type);
     //this->select_preset(first_visible_idx());
     if (! errors_cummulative.empty())
         throw Slic3r::RuntimeError(errors_cummulative);
@@ -3308,6 +3347,16 @@ Preset* PresetCollection::find_preset(const std::string &name, bool first_visibl
     return first_visible_if_not_found ? &this->first_visible() : nullptr;
 }
 
+std::string PresetCollection::unresolved_parent(const boost::filesystem::path &file) const
+{
+    for (const auto &[dropped_file, parent] : m_unresolved_parents) {
+        boost::system::error_code ec;
+        if (boost::filesystem::equivalent(file, boost::filesystem::path(dropped_file), ec) && !ec)
+            return parent;
+    }
+    return {};
+}
+
 Preset* PresetCollection::find_preset2(const std::string& name, bool auto_match/* = true */)
 {
     auto preset = find_preset(name, false, true);

src/libslic3r/Preset.hpp

@@ -2,6 +2,7 @@
 #define slic3r_Preset_hpp_
 
 #include <deque>
+#include <map>
 #include <set>
 #include <string>
 #include <unordered_map>
@@ -728,6 +729,9 @@ public:
     {
         return const_cast<PresetCollection*>(this)->find_preset2(name, auto_match);
     }
+    // Name of the parent that kept the preset file from loading, or empty if the file loaded
+    // (or was never seen). Lets a caller that fails to find a preset explain why it is missing.
+    std::string unresolved_parent(const boost::filesystem::path &file) const;
     
     size_t first_visible_idx() const;
     // Return the index of the first visible, compatible, system base preset
@@ -965,6 +969,8 @@ private:
 
     // Orca: used for validation only
     int m_errors = 0;
+    // Preset files dropped by load_presets() because their parent does not exist, keyed by file path.
+    std::map<std::string, std::string> m_unresolved_parents;
 };
 
 // Printer supports the FFF and SLA technologies, with different set of configuration values,

src/libslic3r/PresetBundle.cpp

@@ -506,6 +506,11 @@ bool PresetBundle::resolve_preset_config(DynamicPrintConfig &config, Preset::Typ
     }
     if (error == "Preset identity is ambiguous")
         return false;
+    // The file was seen but dropped at load time; say so rather than reporting it as unknown.
+    if (const std::string parent = collection->unresolved_parent(source_path); !parent.empty()) {
+        error = "Preset was not loaded because its parent preset \"" + parent + "\" was not found";
+        return false;
+    }
     if (!allow_source_manifest) {
         error = "Preset was not found in the loaded bundle";
         return false;

tests/libslic3r/test_preset_bundle_loading.cpp

@@ -39,6 +39,20 @@ void write_preset_with_inherits(const DynamicPrintConfig &default_config, const
     config.save_to_json(file.string(), name, "User", "1.0.0");
 }
 
+// Write a user preset json holding only "inherits" plus the given overrides, the way a GUI-saved
+// user preset stores its diff against its parent. Anything else is inherited at load time.
+void write_sparse_preset(const fs::path &file, const std::string &name, const std::string &inherits,
+                         const std::vector<std::pair<std::string, std::string>> &overrides)
+{
+    DynamicPrintConfig config;
+    config.option<ConfigOptionString>(BBL_JSON_KEY_INHERITS, true)->value = inherits;
+    for (const auto &override_pair : overrides)
+        config.set_deserialize_strict(override_pair.first, override_pair.second);
+
+    fs::create_directories(file.parent_path());
+    config.save_to_json(file.string(), name, "User", "1.0.0");
+}
+
 // Add an in-memory preset (no file) with the given inherits value (empty => root preset).
 Preset &add_inmemory_preset(PresetCollection &coll, const std::string &name, const std::string &inherits = {})
 {
@@ -365,6 +379,100 @@ std::vector<std::string> &compatible_list(PresetCollection &coll, const std::str
 
 } // namespace
 
+TEST_CASE("A user preset inheriting a user preset from the same directory is loaded", "[Preset][Inherits][Regression]")
+{
+    ScopedTemporaryDir temp_dir;
+    PresetBundle       bundle;
+
+    // The parent sorts after the child, so the child is necessarily reached before its parent is
+    // in the collection - the case a single load pass cannot resolve.
+    const fs::path preset_dir = temp_dir.path() / PRESET_PRINT_NAME;
+    write_sparse_preset(preset_dir / "AA Child.json", "AA Child", "ZZ Root", {{"layer_height", "0.15"}});
+    write_sparse_preset(preset_dir / "ZZ Root.json", "ZZ Root", "", {{"layer_height", "0.3"}, {"top_shell_layers", "7"}});
+
+    PresetsConfigSubstitutions substitutions;
+    bundle.prints.load_presets(temp_dir.path().string(), PRESET_PRINT_NAME, substitutions,
+                               ForwardCompatibilitySubstitutionRule::Disable);
+
+    REQUIRE(bundle.prints.find_preset("ZZ Root") != nullptr);
+    const Preset *child = bundle.prints.find_preset("AA Child");
+    REQUIRE(child != nullptr);
+    CHECK_FALSE(bundle.has_errors());
+    REQUIRE(bundle.prints.get_preset_parent(*child) != nullptr);
+    CHECK(bundle.prints.get_preset_parent(*child)->name == "ZZ Root");
+    // The child's own override wins, and what it does not override comes from the parent rather
+    // than from the collection defaults.
+    CHECK_THAT(child->config.opt_float("layer_height"), Catch::Matchers::WithinAbs(0.15, 1e-9));
+    CHECK(child->config.opt_int("top_shell_layers") == 7);
+}
+
+TEST_CASE("A preset whose parent exists nowhere is reported, not loaded", "[Preset][Inherits][Regression]")
+{
+    ScopedTemporaryDir temp_dir;
+    PresetBundle       bundle;
+
+    const fs::path orphan_file = temp_dir.path() / PRESET_PRINT_NAME / "Orphan.json";
+    write_sparse_preset(orphan_file, "Orphan", "No Such Parent", {{"layer_height", "0.15"}});
+
+    PresetsConfigSubstitutions substitutions;
+    bundle.prints.load_presets(temp_dir.path().string(), PRESET_PRINT_NAME, substitutions,
+                               ForwardCompatibilitySubstitutionRule::Disable);
+
+    CHECK(bundle.prints.find_preset("Orphan") == nullptr);
+    CHECK(bundle.has_errors());
+    CHECK(bundle.prints.unresolved_parent(orphan_file) == "No Such Parent");
+
+    // Resolving the dropped file names the missing parent instead of reporting the file as unknown.
+    DynamicPrintConfig config;
+    std::string        error;
+    CHECK_FALSE(bundle.resolve_preset_config(config, Preset::TYPE_PRINT, orphan_file.string(),
+                                             ForwardCompatibilitySubstitutionRule::Disable, error, false));
+    CHECK(error == "Preset was not loaded because its parent preset \"No Such Parent\" was not found");
+}
+
+TEST_CASE("Presets inheriting each other in a cycle are reported, not loaded", "[Preset][Inherits][Regression]")
+{
+    ScopedTemporaryDir temp_dir;
+    PresetBundle       bundle;
+
+    const fs::path preset_dir = temp_dir.path() / PRESET_PRINT_NAME;
+    write_sparse_preset(preset_dir / "Ping.json", "Ping", "Pong", {{"layer_height", "0.15"}});
+    write_sparse_preset(preset_dir / "Pong.json", "Pong", "Ping", {{"layer_height", "0.3"}});
+
+    PresetsConfigSubstitutions substitutions;
+    bundle.prints.load_presets(temp_dir.path().string(), PRESET_PRINT_NAME, substitutions,
+                               ForwardCompatibilitySubstitutionRule::Disable);
+
+    CHECK(bundle.prints.find_preset("Ping") == nullptr);
+    CHECK(bundle.prints.find_preset("Pong") == nullptr);
+    CHECK(bundle.has_errors());
+}
+
+TEST_CASE("A preset held back for its parent reports its substitutions once", "[Preset][Inherits][Regression]")
+{
+    ScopedTemporaryDir temp_dir;
+    PresetBundle       bundle;
+
+    // The child sorts before its parent, so it is held back for a pass and its file is read
+    // twice. The bogus boolean makes every read produce a substitution.
+    const fs::path preset_dir = temp_dir.path() / PRESET_PRINT_NAME;
+    fs::create_directories(preset_dir);
+    std::ofstream((preset_dir / "AA Child.json").string())
+        << R"({"type":"process","name":"AA Child","from":"User","version":"1.0.0",)"
+        << R"("inherits":"ZZ Root","spiral_mode":"sometimes"})";
+    write_sparse_preset(preset_dir / "ZZ Root.json", "ZZ Root", "", {{"layer_height", "0.3"}});
+
+    PresetsConfigSubstitutions substitutions;
+    bundle.prints.load_presets(temp_dir.path().string(), PRESET_PRINT_NAME, substitutions,
+                               ForwardCompatibilitySubstitutionRule::Enable);
+
+    REQUIRE(bundle.prints.find_preset("AA Child") != nullptr);
+    // A read that ends in the preset being held back must not leave its substitutions behind,
+    // otherwise the same preset is listed once per pass it waited.
+    CHECK(std::count_if(substitutions.begin(), substitutions.end(),
+                        [](const PresetConfigSubstitutions &s) { return s.preset_name == "AA Child"; }) == 1);
+}
+
 TEST_CASE("Renamed printer/process names are normalized into compatible lists on load", "[Preset][Rename]")
 {
     PresetBundle bundle;