Add resync tagging to BKE_main_collection_sync_remap #101074

New Issue

Monique Dewanchand · 2022-09-14T21:26:49+02:00

2022-09-14 21:26:49 +02:00

Add a different tag to collection cache resyncing.
This should be a different tag then the view layer caches.

BKE_main_collection_sync_remap frees existing hashes. It also reconstructs the hashes to ensure that the remapping that was done didn't create duplicates. This can happen in the situation where you replace an object with a different object that already exists in the same view layer.
Doing de-duplication in this can be triggered many times and result to a process what isn't expected.

Deduplication should not be the responsibility of a function that only clears and tags.
Creation function should only create, not remove inconsistency

A possible (still dirty) solution would be to not recreate during remap, but do the deduplication during first usage.
Temporarly the de-duplication spends most time in resizing the hash_map. By giving a size hint resizing could be eliminated fo rmost cases.

Best solution would be to move the responsibility to the actual functionality that could make these incorrect states.

Experiment

With this experiment we postpone the de-duplication check; all access to the object_bases_hash is guarded with a NULL check and is already recreated when needed. So not necessary to recreate during remap. De-duplication is performed during view layer syncing. It is unclear to me in what situation postponing the de-duplication would lead to different results/wouldn't work.

Not added as a patch as this doesn't solve the responsibility issue, it just moves it to another time.
PS this of course improves performance and many code paths can be cleaned up.

diff --git a/source/blender/blenkernel/intern/layer.c b/source/blender/blenkernel/intern/layer.c
index 5b54f16661c..1a9457cae79 100644
--- a/source/blender/blenkernel/intern/layer.c
+++ b/source/blender/blenkernel/intern/layer.c
@@ -384,7 +384,7 @@ Base *BKE_view_layer_base_find(ViewLayer *view_layer, Object *ob)
   BLI_assert_msg((view_layer->flag & VIEW_LAYER_OUT_OF_SYNC) == 0,
                  "View layer out of sync, invoke BKE_view_layer_synced_ensure.");
   if (!view_layer->object_bases_hash) {
-    view_layer_bases_hash_create(view_layer, false);
+    view_layer_bases_hash_create(view_layer, true);
   }
 
   return BLI_ghash_lookup(view_layer->object_bases_hash, ob);
@@ -1308,7 +1308,7 @@ void BKE_layer_collection_sync(const Scene *scene, ViewLayer *view_layer)
 
   /* Create object to base hash if it does not exist yet. */
   if (!view_layer->object_bases_hash) {
-    view_layer_bases_hash_create(view_layer, false);
+    view_layer_bases_hash_create(view_layer, true);
   }
 
   /* Clear visible and selectable flags to be reset. */
@@ -1426,7 +1426,7 @@ void BKE_main_collection_sync_remap(const Main *bmain)
       /* Directly re-create the mapping here, so that we can also deal with duplicates in
        * `view_layer->object_bases` list of bases properly. This is the only place where such
        * duplicates should be fixed, and not considered as a critical error. */
-      view_layer_bases_hash_create(view_layer, true);
+      //view_layer_bases_hash_create(view_layer, true);
     }
 
     BKE_collection_object_cache_free(scene->master_collection);

Add a different tag to collection cache resyncing. This should be a different tag then the view layer caches. BKE_main_collection_sync_remap frees existing hashes. It also reconstructs the hashes to ensure that the remapping that was done didn't create duplicates. This can happen in the situation where you replace an object with a different object that already exists in the same view layer. Doing de-duplication in this can be triggered many times and result to a process what isn't expected. 1. Deduplication should not be the responsibility of a function that only clears and tags. 2. Creation function should only create, not remove inconsistency A possible (still dirty) solution would be to not recreate during remap, but do the deduplication during first usage. Temporarly the de-duplication spends most time in resizing the hash_map. By giving a size hint resizing could be eliminated fo rmost cases. Best solution would be to move the responsibility to the actual functionality that could make these incorrect states. ## Experiment With this experiment we postpone the de-duplication check; all access to the object_bases_hash is guarded with a NULL check and is already recreated when needed. So not necessary to recreate during remap. De-duplication is performed during view layer syncing. It is unclear to me in what situation postponing the de-duplication would lead to different results/wouldn't work. Not added as a patch as this doesn't solve the responsibility issue, it just moves it to another time. PS this of course improves performance and many code paths can be cleaned up. ``` diff --git a/source/blender/blenkernel/intern/layer.c b/source/blender/blenkernel/intern/layer.c index 5b54f16661c..1a9457cae79 100644 --- a/source/blender/blenkernel/intern/layer.c +++ b/source/blender/blenkernel/intern/layer.c @@ -384,7 +384,7 @@ Base *BKE_view_layer_base_find(ViewLayer *view_layer, Object *ob) BLI_assert_msg((view_layer->flag & VIEW_LAYER_OUT_OF_SYNC) == 0, "View layer out of sync, invoke BKE_view_layer_synced_ensure."); if (!view_layer->object_bases_hash) { - view_layer_bases_hash_create(view_layer, false); + view_layer_bases_hash_create(view_layer, true); } return BLI_ghash_lookup(view_layer->object_bases_hash, ob); @@ -1308,7 +1308,7 @@ void BKE_layer_collection_sync(const Scene *scene, ViewLayer *view_layer) /* Create object to base hash if it does not exist yet. */ if (!view_layer->object_bases_hash) { - view_layer_bases_hash_create(view_layer, false); + view_layer_bases_hash_create(view_layer, true); } /* Clear visible and selectable flags to be reset. */ @@ -1426,7 +1426,7 @@ void BKE_main_collection_sync_remap(const Main *bmain) /* Directly re-create the mapping here, so that we can also deal with duplicates in * `view_layer->object_bases` list of bases properly. This is the only place where such * duplicates should be fixed, and not considered as a critical error. */ - view_layer_bases_hash_create(view_layer, true); + //view_layer_bases_hash_create(view_layer, true); } BKE_collection_object_cache_free(scene->master_collection); ```

Monique Dewanchand self-assigned this 2022-09-14 21:26:49 +02:00

Monique Dewanchand commented

2022-09-14 21:26:49 +02:00

Changed status from 'Needs Triage' to: 'Confirmed'

Monique Dewanchand commented

2022-09-14 21:26:49 +02:00

Added subscriber: @monique

Monique Dewanchand removed their assignment 2022-09-18 20:18:15 +02:00

Bastien Montagne commented

2022-11-18 12:24:00 +01:00

Added subscriber: @mont29

Bastien Montagne commented

2022-11-18 12:24:00 +01:00

I do not want to systematically silently fix duplicates in base hashes. These should only ever happen after remapping. All other cases are critical failures somewhere in code.

Doing deferred base hash rebuild would be fine, but then I'd say you need some extra tagging for the 'dirty viewlayer' system, that tells code whether it should accept and fix duplicates in bases, or not (when calling BKE_view_layer_synced_ensure).

I do not want to systematically silently fix duplicates in base hashes. These should only ever happen after remapping. All other cases are critical failures somewhere in code. Doing deferred base hash rebuild would be fine, but then I'd say you need some extra tagging for the 'dirty viewlayer' system, that tells code whether it should accept and fix duplicates in bases, or not (when calling `BKE_view_layer_synced_ensure`).

Philipp Oeser removed the

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Add resync tagging to BKE_main_collection_sync_remap #101074

Experiment