From e260bc64daa11fa65569aceb92c1133879ebe4ea Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Fri, 26 May 2023 17:38:30 +0200 Subject: [PATCH] Fix (unreported) broken 'fixing' code in ID name uniqueness handling. Logic in `main_namemap_validate_and_fix` could end up re-generating a thousand of time the names of IDs because of an invalid assumption about processed IDs being re-processable (in case they get renamed). Also do not `CLOG_ERROR` when checking and fixing errors, if this code is called to fix errors, it means errors are expected. Use `CLOG_INFO` instead, or `CLOG_WARN` when the info is really important (like when IDs had to be renamed). And finally, simplify code clearing invalid namemaps, there is now a function to handle this task, `BKE_main_namemap_clear`. Issues & improvements found while working on readfile errors when opening `lib/tests/libraries_and_linking/libraries/main_scene.blend`. --- .../blender/blenkernel/intern/main_namemap.cc | 84 +++++++++++++------ 1 file changed, 58 insertions(+), 26 deletions(-) diff --git a/source/blender/blenkernel/intern/main_namemap.cc b/source/blender/blenkernel/intern/main_namemap.cc index 3aa0e522dfa..c6c13e267c0 100644 --- a/source/blender/blenkernel/intern/main_namemap.cc +++ b/source/blender/blenkernel/intern/main_namemap.cc @@ -403,34 +403,51 @@ struct Uniqueness_Key { static bool main_namemap_validate_and_fix(Main *bmain, const bool do_fix) { Set id_names_libs; + Set id_validated; bool is_valid = true; ListBase *lb_iter; FOREACH_MAIN_LISTBASE_BEGIN (bmain, lb_iter) { LISTBASE_FOREACH_MUTABLE (ID *, id_iter, lb_iter) { + if (id_validated.contains(id_iter)) { + /* Do not re-check an already validated ID. */ + continue; + } + Uniqueness_Key key; STRNCPY(key.name, id_iter->name); key.lib = id_iter->lib; if (!id_names_libs.add(key)) { is_valid = false; - CLOG_ERROR(&LOG, - "ID name '%s' (from library '%s') is found more than once", - id_iter->name, - id_iter->lib != nullptr ? id_iter->lib->filepath : ""); if (do_fix) { - /* NOTE: this may imply moving this ID in its listbase, however re-checking it later is - * not really an issue. */ + CLOG_WARN(&LOG, + "ID name '%s' (from library '%s') is found more than once", + id_iter->name, + id_iter->lib != nullptr ? id_iter->lib->filepath : ""); + /* NOTE: this may imply moving this ID in its listbase. The logic below will add the ID + * to the validated set if it can now be added to `id_names_libs`, and will prevent + * further checking (which would fail again, since the new ID name/lib key has already + * been added to `id_names_libs`). */ BKE_id_new_name_validate( bmain, which_libbase(bmain, GS(id_iter->name)), id_iter, nullptr, true); STRNCPY(key.name, id_iter->name); if (!id_names_libs.add(key)) { + /* This is a serious error, very likely a bug, keep it as CLOG_ERROR even when doing + * fixes. */ CLOG_ERROR(&LOG, "\tID has been renamed to '%s', but it still seems to be already in use", id_iter->name); } else { CLOG_WARN(&LOG, "\tID has been renamed to '%s'", id_iter->name); + id_validated.add(id_iter); } } + else { + CLOG_ERROR(&LOG, + "ID name '%s' (from library '%s') is found more than once", + id_iter->name, + id_iter->lib != nullptr ? id_iter->lib->filepath : ""); + } } UniqueName_Map *name_map = get_namemap_for(bmain, id_iter, false); @@ -445,11 +462,23 @@ static bool main_namemap_validate_and_fix(Main *bmain, const bool do_fix) STRNCPY(key_namemap.name, id_iter->name + 2); if (!type_map->full_names.contains(key_namemap)) { is_valid = false; - CLOG_ERROR(&LOG, - "ID name '%s' (from library '%s') exists in current Main, but is not listed in " - "the namemap", - id_iter->name, - id_iter->lib != nullptr ? id_iter->lib->filepath : ""); + if (do_fix) { + CLOG_INFO( + &LOG, + 3, + "ID name '%s' (from library '%s') exists in current Main, but is not listed in " + "the namemap", + id_iter->name, + id_iter->lib != nullptr ? id_iter->lib->filepath : ""); + } + else { + CLOG_ERROR( + &LOG, + "ID name '%s' (from library '%s') exists in current Main, but is not listed in " + "the namemap", + id_iter->name, + id_iter->lib != nullptr ? id_iter->lib->filepath : ""); + } } } } @@ -472,11 +501,23 @@ static bool main_namemap_validate_and_fix(Main *bmain, const bool do_fix) key.lib = lib; if (!id_names_libs.contains(key)) { is_valid = false; - CLOG_ERROR(&LOG, - "ID name '%s' (from library '%s') is listed in the namemap, but does not " - "exists in current Main", - key.name, - lib != nullptr ? lib->filepath : ""); + if (do_fix) { + CLOG_INFO( + &LOG, + 3, + "ID name '%s' (from library '%s') is listed in the namemap, but does not " + "exists in current Main", + key.name, + lib != nullptr ? lib->filepath : ""); + } + else { + CLOG_ERROR( + &LOG, + "ID name '%s' (from library '%s') is listed in the namemap, but does not " + "exists in current Main", + key.name, + lib != nullptr ? lib->filepath : ""); + } } } } @@ -491,16 +532,7 @@ static bool main_namemap_validate_and_fix(Main *bmain, const bool do_fix) } /* Clear all existing namemaps. */ - lib = nullptr; - UniqueName_Map **name_map_p = &bmain->name_map; - do { - BLI_assert(name_map_p != nullptr); - if (*name_map_p != nullptr) { - BKE_main_namemap_destroy(name_map_p); - } - lib = static_cast((lib == nullptr) ? bmain->libraries.first : lib->id.next); - name_map_p = (lib != nullptr) ? &lib->runtime.name_map : nullptr; - } while (lib != nullptr); + BKE_main_namemap_clear(bmain); return is_valid; }