From 591936c7ed69d82405099f7cf539d1f09dfa6c01 Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Fri, 26 May 2023 16:44:08 +0200 Subject: [PATCH 1/4] Fix (unreported) wrong logic in readfile lookup for existing library ID. In readfile code, when looking up for an already existing Library ID based on the filepath, the logic handling said file path was wrong. NOTE: This probably does not have any effect in practice, but better be safe than sorry. Found while investigating issues when opening `lib/tests/libraries_and_linking/libraries/main_scene.blend`. --- source/blender/blenloader/intern/readfile.cc | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/source/blender/blenloader/intern/readfile.cc b/source/blender/blenloader/intern/readfile.cc index 02bcf57e553..9682b09f62d 100644 --- a/source/blender/blenloader/intern/readfile.cc +++ b/source/blender/blenloader/intern/readfile.cc @@ -2668,6 +2668,16 @@ static void direct_link_library(FileData *fd, Library *lib, Main *main) { Main *newmain; + /* Make sure we have full path in lib->filepath_abs */ + /* Note that since existing libs are searched by their absolute path, this has to be generated + * before the lookup below. Otherwise, in case the stored absolute filepath is not 'correct' (may + * be empty, or have been stored in a different 'relative path context'), the comparison below + * will always fail, leading to creating duplicates IDs of a same library. */ + /* TODO: May be worth checking whether conparison below could use `lib->filepath` instead? */ + STRNCPY(lib->filepath_abs, lib->filepath); + BLI_path_abs(lib->filepath_abs, fd->relabase); + BLI_path_normalize(lib->filepath_abs); + /* check if the library was already read */ for (newmain = static_cast
(fd->mainlist->first); newmain; newmain = newmain->next) { if (newmain->curlib) { @@ -2698,11 +2708,6 @@ static void direct_link_library(FileData *fd, Library *lib, Main *main) } } - /* Make sure we have full path in lib->filepath_abs */ - STRNCPY(lib->filepath_abs, lib->filepath); - BLI_path_abs(lib->filepath_abs, fd->relabase); - BLI_path_normalize(lib->filepath_abs); - // printf("direct_link_library: filepath %s\n", lib->filepath); // printf("direct_link_library: filepath_abs %s\n", lib->filepath_abs); From e260bc64daa11fa65569aceb92c1133879ebe4ea Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Fri, 26 May 2023 17:38:30 +0200 Subject: [PATCH 2/4] 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; } From ff126ede17229c5eaa140d80c6918824d02e4f42 Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Fri, 26 May 2023 17:51:52 +0200 Subject: [PATCH 3/4] 'Fix' (unreported) issue that liboverride code can corrupt Main namemap. Does not happen very often, but that weak handling of copying linked data as linked data currently can lead to an invalid namemap in Main. This is a known issue, fixing it requires addressing #107847. In the mean time, work around it by re-validating and fixing the namemap after the problematic liboverride calls. NOTE: only identified issue currently is the proxy conversion of linked proxies. The other cases *should* be fine. Found while investigating issues when opening the `lib/tests/libraries_and_linking/libraries/main_scene.blend` file. --- source/blender/blenkernel/intern/blendfile.cc | 3 +++ source/blender/blenkernel/intern/blendfile_link_append.c | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/source/blender/blenkernel/intern/blendfile.cc b/source/blender/blenkernel/intern/blendfile.cc index 7876d94b505..53b2baa8c2c 100644 --- a/source/blender/blenkernel/intern/blendfile.cc +++ b/source/blender/blenkernel/intern/blendfile.cc @@ -442,6 +442,9 @@ static void setup_app_data(bContext *C, * linked libraries. */ if (mode != LOAD_UNDO && !blendfile_or_libraries_versions_atleast(bmain, 302, 1)) { BKE_lib_override_library_main_proxy_convert(bmain, reports); + /* Currently liboverride code can generate invalid namemap. This is a known issue, requires + * #107847 to be properly fixed. */ + BKE_main_namemap_validate_and_fix(bmain); } if (mode != LOAD_UNDO && !blendfile_or_libraries_versions_atleast(bmain, 302, 3)) { diff --git a/source/blender/blenkernel/intern/blendfile_link_append.c b/source/blender/blenkernel/intern/blendfile_link_append.c index 668b8a47bff..9f03ff47003 100644 --- a/source/blender/blenkernel/intern/blendfile_link_append.c +++ b/source/blender/blenkernel/intern/blendfile_link_append.c @@ -1011,6 +1011,10 @@ static void blendfile_link_append_proxies_convert(Main *bmain, ReportList *repor BlendFileReadReport bf_reports = {.reports = reports}; BKE_lib_override_library_main_proxy_convert(bmain, &bf_reports); + /* Currently liboverride code can generate invalid namemap. This is a known issue, requires + * #107847 to be properly fixed. */ + BKE_main_namemap_validate_and_fix(bmain); + if (bf_reports.count.proxies_to_lib_overrides_success != 0 || bf_reports.count.proxies_to_lib_overrides_failures != 0) { From 4c5998ee0bc2d2f763f977893656fc51ca7002bc Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Fri, 26 May 2023 18:37:22 +0200 Subject: [PATCH 4/4] Unittest: Add very basic 'old file loading' test. Use existing `lib/tests/libraries_and_linking/library_test_scene.blend` essentially as 'file loads without error' test. Also does very basic proxy -> liboverrides conversion check. This test could be extended a lot, but just opening this file already allowed to identify three bugs in current 3.6/main code, and an issue in an upcoming refactor of the readfile code... --- tests/python/CMakeLists.txt | 1 + .../python/bl_blendfile_library_overrides.py | 34 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/tests/python/CMakeLists.txt b/tests/python/CMakeLists.txt index 109cea6214f..e3799e14612 100644 --- a/tests/python/CMakeLists.txt +++ b/tests/python/CMakeLists.txt @@ -168,6 +168,7 @@ add_blender_test( blendfile_library_overrides --python ${CMAKE_CURRENT_LIST_DIR}/bl_blendfile_library_overrides.py -- --output-dir ${TEST_OUT_DIR}/blendfile_io/ + --test-dir "${TEST_SRC_DIR}/libraries_and_linking" ) # ------------------------------------------------------------------------------ diff --git a/tests/python/bl_blendfile_library_overrides.py b/tests/python/bl_blendfile_library_overrides.py index 6890bb1e660..840e12f6dba 100644 --- a/tests/python/bl_blendfile_library_overrides.py +++ b/tests/python/bl_blendfile_library_overrides.py @@ -296,10 +296,37 @@ class TestLibraryOverridesResync(TestHelper, unittest.TestCase): assert obj_armature.constraints[0].target == obj_ctrl2 +class TestLibraryOverridesFromProxies(TestHelper, unittest.TestCase): + # Very basic test, could be improved/extended. + # NOTE: Tests way more than only liboverride proxy conversion actually, since this is a fairly old .blend file. + + MAIN_BLEND_FILE = "library_test_scene.blend" + + def __init__(self, args): + self.args = args + + self.test_dir = pathlib.Path(self.args.test_dir) + self.assertTrue(self.test_dir.exists(), + 'Test dir {0} should exist'.format(self.test_dir)) + + bpy.ops.wm.read_homefile(use_empty=True, use_factory_startup=True) + + def test_open_linked_proxy_file(self): + bpy.ops.wm.open_mainfile(filepath=str(self.test_dir / self.MAIN_BLEND_FILE)) + + # Check stability of 'same name' fixing for IDs. + direct_linked_A = bpy.data.libraries["lib.002"] + assert direct_linked_A.filepath == "//libraries/direct_linked_A.blend" + + assert bpy.data.objects['HairCubeArmatureGroup_proxy'].library == direct_linked_A + assert bpy.data.objects['HairCubeArmatureGroup_proxy'].override_library != None + + TESTS = ( TestLibraryOverrides, TestLibraryTemplate, TestLibraryOverridesResync, + TestLibraryOverridesFromProxies, ) @@ -316,6 +343,13 @@ def argparse_create(): help="Where to output temp saved blendfiles", required=False, ) + parser.add_argument( + "--test-dir", + dest="test_dir", + default=".", + help="Where are the test blendfiles", + required=False, + ) return parser