From a165a324b8d1f1501191ec7a6c33221a1393371b Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Wed, 8 Oct 2025 17:16:23 +0200 Subject: [PATCH] Fix ViewLayer resync wrongly assuming success in some cases. `BKE_view_layer_synced_ensure` would report success and clear the 'out of sync' flag of the viewlayer, even if the call to `BKE_layer_collection_sync` could not actually perform the resync (e.g. because resync is forbidden by one or more calls to `BKE_layer_collection_resync_forbid`). While logically fairly bad, this issue did not seem to have any practical consequences. So would rather not backport this to the 5.0 beta branch. This fix also reveals at least one place where the usage of `BKE_view_layer_synced_ensure` and related is muddy and not ideal: the Scene's `foreach_id' code. This is patched as best as possible in this commit, but is something that will have to be properly fixed at some point most likely. Also add some documentation to this API - although the whole thing needs a real reafctor at some point still, name-wise and organization-wise. Pull Request: https://projects.blender.org/blender/blender/pulls/147635 --- source/blender/blenkernel/BKE_layer.hh | 87 ++++++++++++++++--- source/blender/blenkernel/intern/layer.cc | 85 ++++++++++++------ .../blender/blenkernel/intern/layer_utils.cc | 7 +- source/blender/blenkernel/intern/scene.cc | 19 +++- 4 files changed, 158 insertions(+), 40 deletions(-) diff --git a/source/blender/blenkernel/BKE_layer.hh b/source/blender/blenkernel/BKE_layer.hh index d94ba0eec72..f07ba6e3fea 100644 --- a/source/blender/blenkernel/BKE_layer.hh +++ b/source/blender/blenkernel/BKE_layer.hh @@ -150,21 +150,56 @@ void BKE_layer_collection_resync_allow(); */ void BKE_layer_collection_doversion_2_80(const Scene *scene, ViewLayer *view_layer); -void BKE_main_collection_sync(const Main *bmain); -void BKE_scene_collection_sync(const Scene *scene); +/** + * Tag all viewlayers of all the scenes of the given `main` as being #VIEW_LAYER_OUT_OF_SYNC. + * + * Also directly update all local viewlayers (used in 3DView in local mode). + * + * \return `true` if all viewlayers were successfully tagged (or resynced for the local ones), + * `false` otherwise. See also #BKE_layer_collection_sync. + */ +bool BKE_main_collection_sync(const Main *bmain); +/** Same as for #BKE_main_collection_sync, but for a single scene only. */ +bool BKE_scene_collection_sync(const Scene *scene); +/** + * Similar to #BKE_main_collection_sync, but does additional cache cleanups and depsgraph tagging, + * required after remapping objects/collections ID pointers. + * + * \return `true` if all viewlayers were successfully tagged (or resynced for the local ones), + * `false` otherwise. See also #BKE_layer_collection_sync. + */ +bool BKE_main_collection_sync_remap(const Main *bmain); /** * Update view layer collection tree from collections used in the scene. * This is used when collections are removed or added, both while editing * and on file loaded in case linked data changed or went missing. + * + * \warning Calling this function directly should almost never be necessary, and should be avoided + * at all costs. It is utterly unsafe in multi-threaded context, among other risks. The typical + * process is to tag view layers for updates with #BKE_view_layer_need_resync_tag (or the more + * general #BKE_scene_collection_sync/#BKE_main_collection_sync), and only enure the layers are + * up-to-date when actually needed, using #BKE_view_layer_synced_ensure and realted API. + * + * \return `true` if the viewlayer was successfully resynced (or already in sync), `false` if a + * resync was needed but could not be performed (e.g. because resync is locked by one or more calls + * to #BKE_layer_collection_resync_forbid). */ -void BKE_layer_collection_sync(const Scene *scene, ViewLayer *view_layer); -void BKE_layer_collection_local_sync(const Scene *scene, ViewLayer *view_layer, const View3D *v3d); +bool BKE_layer_collection_sync(const Scene *scene, ViewLayer *view_layer); /** - * Sync the local collection for all the 3D Viewports. + * Sync the local visibility of collections & objects, for the given 3D Viewport in 'local' view + * mode. + * + * \return `true` if the local viewport info were successfully resynced, `false` otherwise. See + * also #BKE_layer_collection_sync. */ -void BKE_layer_collection_local_sync_all(const Main *bmain); - -void BKE_main_collection_sync_remap(const Main *bmain); +bool BKE_layer_collection_local_sync(const Scene *scene, ViewLayer *view_layer, const View3D *v3d); +/** + * Sync the local visibility of collections & objects, for all 3D Viewports in 'local' view mode. + * + * \return `true` if all local info were successfully resynced, `false` otherwise. See also + * #BKE_layer_collection_sync. + */ +bool BKE_layer_collection_local_sync_all(const Main *bmain); /** * Return the first matching #LayerCollection in the #ViewLayer for the Collection. @@ -546,15 +581,43 @@ Object *BKE_view_layer_active_object_get(const ViewLayer *view_layer); Object *BKE_view_layer_edit_object_get(const ViewLayer *view_layer); ListBase *BKE_view_layer_object_bases_get(ViewLayer *view_layer); +/** + * Same as the above, but does not assert that the viewlayer is synced. + * + * \warning Use with _extreme_ care, as it means the data returned by this call may not be valid. + */ +ListBase *BKE_view_layer_object_bases_unsynced_get(ViewLayer *view_layer); + Base *BKE_view_layer_active_base_get(ViewLayer *view_layer); LayerCollection *BKE_view_layer_active_collection_get(ViewLayer *view_layer); +/** + * Tag the given view-layer as being #VIEW_LAYER_OUT_OF_SYNC with the hierarchy of collections and + * objects it represents. + * + * This allows to defer the actual resync process to when up-to-date data is required (see + * #BKE_view_layer_synced_ensure and related API). + */ void BKE_view_layer_need_resync_tag(ViewLayer *view_layer); -void BKE_view_layer_synced_ensure(const Scene *scene, ViewLayer *view_layer); - -void BKE_scene_view_layers_synced_ensure(const Scene *scene); -void BKE_main_view_layers_synced_ensure(const Main *bmain); +/** + * Ensure that the given `scene`'s `view_layer` is fully in sync with the hierarchy of collections + * and objects it represents. + * + * \return `true` if the viewlayer was successfully resynced, `false` otherwise. See also + * #BKE_layer_collection_sync. + */ +bool BKE_view_layer_synced_ensure(const Scene *scene, ViewLayer *view_layer); +/** + * \return `true` if all viewlayers were successfully resynced, `false` otherwise. See also + * #BKE_layer_collection_sync. + */ +bool BKE_scene_view_layers_synced_ensure(const Scene *scene); +/** + * \return `true` if all viewlayers were successfully resynced, `false` otherwise. See also + * #BKE_layer_collection_sync. + */ +bool BKE_main_view_layers_synced_ensure(const Main *bmain); ViewLayerAOV *BKE_view_layer_add_aov(ViewLayer *view_layer); void BKE_view_layer_remove_aov(ViewLayer *view_layer, ViewLayerAOV *aov); diff --git a/source/blender/blenkernel/intern/layer.cc b/source/blender/blenkernel/intern/layer.cc index bae74a09d06..f117a60f73f 100644 --- a/source/blender/blenkernel/intern/layer.cc +++ b/source/blender/blenkernel/intern/layer.cc @@ -988,34 +988,52 @@ void BKE_view_layer_need_resync_tag(ViewLayer *view_layer) view_layer->flag |= VIEW_LAYER_OUT_OF_SYNC; } -void BKE_view_layer_synced_ensure(const Scene *scene, ViewLayer *view_layer) +bool BKE_view_layer_synced_ensure(const Scene *scene, ViewLayer *view_layer) { BLI_assert(scene); BLI_assert(view_layer); + bool is_all_resynced = true; if (view_layer->flag & VIEW_LAYER_OUT_OF_SYNC) { - BKE_layer_collection_sync(scene, view_layer); - view_layer->flag &= ~VIEW_LAYER_OUT_OF_SYNC; + if (BKE_layer_collection_sync(scene, view_layer)) { + view_layer->flag &= ~VIEW_LAYER_OUT_OF_SYNC; + } + else { + is_all_resynced = false; + } } + + return is_all_resynced; } -void BKE_scene_view_layers_synced_ensure(const Scene *scene) +bool BKE_scene_view_layers_synced_ensure(const Scene *scene) { + bool is_all_resynced = true; LISTBASE_FOREACH (ViewLayer *, view_layer, &scene->view_layers) { - BKE_view_layer_synced_ensure(scene, view_layer); + if (!BKE_view_layer_synced_ensure(scene, view_layer)) { + is_all_resynced = false; + } } + return is_all_resynced; } -void BKE_main_view_layers_synced_ensure(const Main *bmain) +bool BKE_main_view_layers_synced_ensure(const Main *bmain) { + bool is_all_resynced = true; for (const Scene *scene = static_cast(bmain->scenes.first); scene; scene = static_cast(scene->id.next)) { - BKE_scene_view_layers_synced_ensure(scene); + if (!BKE_scene_view_layers_synced_ensure(scene)) { + is_all_resynced = false; + } } /* NOTE: This is not (yet?) covered by the dirty tag and deferred re-sync system. */ - BKE_layer_collection_local_sync_all(bmain); + if (!BKE_layer_collection_local_sync_all(bmain)) { + is_all_resynced = false; + } + + return is_all_resynced; } static void layer_collection_objects_sync(ViewLayer *view_layer, @@ -1306,15 +1324,15 @@ void BKE_layer_collection_doversion_2_80(const Scene *scene, ViewLayer *view_lay } } -void BKE_layer_collection_sync(const Scene *scene, ViewLayer *view_layer) +bool BKE_layer_collection_sync(const Scene *scene, ViewLayer *view_layer) { if (no_resync > 0) { - return; + return false; } if (!scene->master_collection) { /* Happens for old files that don't have versioning applied yet. */ - return; + return false; } if (BLI_listbase_is_empty(&view_layer->layer_collections)) { @@ -1415,23 +1433,27 @@ void BKE_layer_collection_sync(const Scene *scene, ViewLayer *view_layer) view_layer->active_collection = static_cast( view_layer->layer_collections.first); } + + return true; } -void BKE_scene_collection_sync(const Scene *scene) +bool BKE_scene_collection_sync(const Scene *scene) { if (no_resync > 0) { - return; + return false; } LISTBASE_FOREACH (ViewLayer *, view_layer, &scene->view_layers) { BKE_view_layer_need_resync_tag(view_layer); } + + return true; } -void BKE_main_collection_sync(const Main *bmain) +bool BKE_main_collection_sync(const Main *bmain) { if (no_resync > 0) { - return; + return false; } /* TODO: if a single collection changed, figure out which @@ -1439,19 +1461,25 @@ void BKE_main_collection_sync(const Main *bmain) /* TODO: optimize for file load so only linked collections get checked? */ + bool is_all_resynced = true; for (const Scene *scene = static_cast(bmain->scenes.first); scene; scene = static_cast(scene->id.next)) { - BKE_scene_collection_sync(scene); + if (!BKE_scene_collection_sync(scene)) { + is_all_resynced = false; + } } - BKE_layer_collection_local_sync_all(bmain); + if (!BKE_layer_collection_local_sync_all(bmain)) { + is_all_resynced = false; + } + return is_all_resynced; } -void BKE_main_collection_sync_remap(const Main *bmain) +bool BKE_main_collection_sync_remap(const Main *bmain) { if (no_resync > 0) { - return; + return false; } /* On remapping of object or collection pointers free caches. */ @@ -1486,7 +1514,7 @@ void BKE_main_collection_sync_remap(const Main *bmain) DEG_id_tag_update_ex((Main *)bmain, &collection->id, ID_RECALC_SYNC_TO_EVAL); } - BKE_main_collection_sync(bmain); + return BKE_main_collection_sync(bmain); } /** \} */ @@ -1777,10 +1805,10 @@ static void layer_collection_local_sync(const Scene *scene, } } -void BKE_layer_collection_local_sync(const Scene *scene, ViewLayer *view_layer, const View3D *v3d) +bool BKE_layer_collection_local_sync(const Scene *scene, ViewLayer *view_layer, const View3D *v3d) { if (no_resync > 0) { - return; + return false; } const ushort local_collections_uid = v3d->local_collections_uid; @@ -1794,14 +1822,17 @@ void BKE_layer_collection_local_sync(const Scene *scene, ViewLayer *view_layer, LISTBASE_FOREACH (LayerCollection *, layer_collection, &view_layer->layer_collections) { layer_collection_local_sync(scene, view_layer, layer_collection, local_collections_uid, true); } + + return true; } -void BKE_layer_collection_local_sync_all(const Main *bmain) +bool BKE_layer_collection_local_sync_all(const Main *bmain) { if (no_resync > 0) { - return; + return false; } + bool is_all_resynced = true; LISTBASE_FOREACH (Scene *, scene, &bmain->scenes) { LISTBASE_FOREACH (ViewLayer *, view_layer, &scene->view_layers) { LISTBASE_FOREACH (bScreen *, screen, &bmain->screens) { @@ -1811,12 +1842,16 @@ void BKE_layer_collection_local_sync_all(const Main *bmain) } View3D *v3d = static_cast(area->spacedata.first); if (v3d->flag & V3D_LOCAL_COLLECTIONS) { - BKE_layer_collection_local_sync(scene, view_layer, v3d); + if (!BKE_layer_collection_local_sync(scene, view_layer, v3d)) { + is_all_resynced = false; + } } } } } } + + return is_all_resynced; } void BKE_layer_collection_isolate_local( diff --git a/source/blender/blenkernel/intern/layer_utils.cc b/source/blender/blenkernel/intern/layer_utils.cc index 645810f66e8..26795d339fd 100644 --- a/source/blender/blenkernel/intern/layer_utils.cc +++ b/source/blender/blenkernel/intern/layer_utils.cc @@ -186,11 +186,16 @@ Vector BKE_view_layer_array_from_objects_in_mode_unique_data(const Sce return BKE_view_layer_array_from_objects_in_mode_params(scene, view_layer, v3d, ¶ms); } +ListBase *BKE_view_layer_object_bases_unsynced_get(ViewLayer *view_layer) +{ + return &view_layer->object_bases; +} + ListBase *BKE_view_layer_object_bases_get(ViewLayer *view_layer) { BLI_assert_msg((view_layer->flag & VIEW_LAYER_OUT_OF_SYNC) == 0, "Object Bases out of sync, invoke BKE_view_layer_synced_ensure."); - return &view_layer->object_bases; + return BKE_view_layer_object_bases_unsynced_get(view_layer); } Base *BKE_view_layer_active_base_get(ViewLayer *view_layer) diff --git a/source/blender/blenkernel/intern/scene.cc b/source/blender/blenkernel/intern/scene.cc index 04d9d708168..b435256f513 100644 --- a/source/blender/blenkernel/intern/scene.cc +++ b/source/blender/blenkernel/intern/scene.cc @@ -875,8 +875,23 @@ static void scene_foreach_id(ID *id, LibraryForeachIDData *data) BKE_lib_query_idpropertiesForeachIDLink_callback(prop, data); })); - BKE_view_layer_synced_ensure(scene, view_layer); - LISTBASE_FOREACH (Base *, base, BKE_view_layer_object_bases_get(view_layer)) { + /* FIXME: Although ideally this should always have access to synced data, this is not always + * the case (FOREACH_ID can be called in context where re-syncing is blocked, while effectively + * modifying the viewlayer or collections data, see e.g. #id_delete code which remaps all + * deleted ID usages to null). + * + * There is no obvious solution to this problem, so for now working around with some 'band-aid' + * special code and asserts. + * + * In the future, there may be need for a new `IDWALK_CB` flag to mark existing pointer values + * as unsafe to access in such cases. */ + const bool is_synced = BKE_view_layer_synced_ensure(scene, view_layer); + if (!is_synced) { + BLI_assert_msg((flag & IDWALK_RECURSE) == 0, + "foreach_id should never recurse in case it cannot ensure that all " + "viewlayers are in synced with their collections"); + } + LISTBASE_FOREACH (Base *, base, BKE_view_layer_object_bases_unsynced_get(view_layer)) { BKE_LIB_FOREACHID_PROCESS_IDSUPER( data, base->object,