dillongoostudios/goo-engine
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Core: IDRemapper: Refactor handling of 'never null' ID usages.

Browse Source 
Existing code would allow tagging on request IDs which had a 'never
null' usage potentially cleared by the remapping operation (e.g. if an
Object obdata would have been set to `nullptr`).

While this worked for the current extremely restricted usecase (ID
deletion), this was not the best design, as it forced the ID remapping
user code to be very careful about its own usages of the `LIB_TAG_DOIT`
tag.

This commit replaces internal tagging by adding such IDs to a Set in
`IDRemapper` class, which user code can then use to find which IDs
(would have) had a 'never null' ID pointer cleared.

There are two additional changes induced by this commit:
* `BKE_libblock_unlink` `do_flag_never_null` parameter is removed.
  As it is not used in current codebase, simpler to remove than update
  the code to support it.
* `ID_REMAP_FLAG_NEVER_NULL_USAGE` option is renamed to
  `ID_REMAP_STORE_NEVER_NULL_USAGE`.
  In addition, its behavior is slightly modified:
  * Before, the owner ID would systematically be tagged if it had such
    'never null' ID usages, regardless of whether said ID usages (would)
    have actually been remapped to `nullptr`.
  * Now, the owner ID is only added to the `never_null_users` set if its
    'never null' usages (would) have been cleared.
This commit is contained in:
Bastien Montagne
2024-02-14 12:31:18 +01:00
committed by Bastien Montagne
parent 5c87dfd269
commit a3bf395a10
4 changed files with 62 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files
source/blender/blenkernel/BKE_lib_remap.hh
+23 -8
View File
@@ -22,6 +22,7 @@
#include "BLI_compiler_attrs.h"
#include "BLI_map.hh"
#include "BLI_set.hh"
#include "BLI_span.hh"
#include "BLI_utildefines.h"
@@ -48,10 +49,11 @@ enum {
*/
ID_REMAP_SKIP_NEVER_NULL_USAGE = 1 << 1,
/**
* This tells the callback func to flag with #LIB_DOIT all IDs
* using target one with a 'never NULL' pointer (like e.g. #Object.data).
* Store in the #IDRemapper all IDs using target one with a 'never NULL' pointer (like e.g.
* #Object.data), when such ID usage has (or should have) been remapped to `nullptr`. See also
* #ID_REMAP_FORCE_NEVER_NULL_USAGE and #ID_REMAP_SKIP_NEVER_NULL_USAGE.
*/
ID_REMAP_FLAG_NEVER_NULL_USAGE = 1 << 2,
ID_REMAP_STORE_NEVER_NULL_USAGE = 1 << 2,
/**
* This tells the callback func to force setting IDs
* using target one with a 'never NULL' pointer to NULL.
@@ -163,12 +165,8 @@ void BKE_libblock_remap(Main *bmain, void *old_idv, void *new_idv, int remap_fla
/**
* Unlink given \a id from given \a bmain
* (does not touch to indirect, i.e. library, usages of the ID).
*
* \param do_flag_never_null: If true, all IDs using \a idv in a 'non-NULL' way are flagged by
* #LIB_TAG_DOIT flag (quite obviously, 'non-NULL' usages can never be unlinked by this function).
*/
void BKE_libblock_unlink(Main *bmain, void *idv, bool do_flag_never_null, bool do_skip_indirect)
ATTR_NONNULL();
void BKE_libblock_unlink(Main *bmain, void *idv, bool do_skip_indirect) ATTR_NONNULL();
/**
* Similar to libblock_remap, but only affects IDs used by given \a idv ID.
@@ -263,10 +261,17 @@ class IDRemapper {
blender::Map<ID *, ID *> mappings_;
IDTypeFilter source_types_ = 0;
/**
* Store all IDs using another ID with the 'NEVER_NULL' flag, which have (or
* should have been) remapped to `nullptr`.
*/
blender::Set<ID *> never_null_users_;
public:
void clear(void)
{
mappings_.clear();
never_null_users_.clear();
source_types_ = 0;
}
@@ -302,6 +307,16 @@ class IDRemapper {
IDRemapperApplyOptions options,
ID *id_self = nullptr) const;
void never_null_users_add(ID *id)
{
never_null_users_.add(id);
}
const blender::Set<ID *> &never_null_users(void) const
{
return never_null_users_;
}
/** Iterate over all remapping pairs in the remapper, and call the callback function on them. */
void iter(IDRemapperIterFunction func, void *user_data) const
{
source/blender/blenkernel/intern/lib_id_delete.cc
+8 -3
View File
@@ -20,6 +20,7 @@
#include "BLI_linklist.h"
#include "BLI_listbase.h"
#include "BLI_set.hh"
#include "BLI_vector.hh"
#include "BKE_anim_data.h"
@@ -214,7 +215,7 @@ void BKE_id_free_us(Main *bmain, void *idv) /* test users */
}
if (id->us == 0) {
BKE_libblock_unlink(bmain, id, false, false);
BKE_libblock_unlink(bmain, id, false);
BKE_id_free(bmain, id);
}
@@ -234,7 +235,7 @@ static size_t id_delete(Main *bmain,
const int free_flag = LIB_ID_FREE_NO_UI_USER |
(do_tagged_deletion ? LIB_ID_FREE_NO_MAIN | LIB_ID_FREE_NO_USER_REFCOUNT :
0);
const int remapping_flags = (ID_REMAP_FLAG_NEVER_NULL_USAGE | ID_REMAP_FORCE_NEVER_NULL_USAGE |
const int remapping_flags = (ID_REMAP_STORE_NEVER_NULL_USAGE | ID_REMAP_FORCE_NEVER_NULL_USAGE |
ID_REMAP_FORCE_INTERNAL_RUNTIME_POINTERS | extra_remapping_flags);
ListBase tagged_deleted_ids = {nullptr};
@@ -293,13 +294,17 @@ static size_t id_delete(Main *bmain,
}
}
/* Will tag 'never nullptr' users of this ID too.
/* Will store in remapper all 'never nullptr' users of this ID too.
*
* NOTE: #BKE_libblock_unlink() cannot be used here, since it would ignore indirect
* links, this can lead to nasty crashing here in second, actual deleting loop.
* Also, this will also flag users of deleted data that cannot be unlinked
* (object using deleted obdata, etc.), so that they also get deleted. */
BKE_libblock_remap_multiple_locked(bmain, id_remapper, remapping_flags);
/* Tag cleared 'never-null' user IDs for deletion too. */
for (ID *never_null_user_id : id_remapper.never_null_users()) {
never_null_user_id->tag |= tag;
}
id_remapper.clear();
}
source/blender/blenkernel/intern/lib_remap.cc
+7 -8
View File
@@ -256,8 +256,11 @@ static int foreach_libblock_remap_callback(LibraryIDLinkCallbackData *cb_data)
skip_reference);
#endif
if ((id_remap_data->flag & ID_REMAP_FLAG_NEVER_NULL_USAGE) && (cb_flag & IDWALK_CB_NEVER_NULL)) {
id_owner->tag |= LIB_TAG_DOIT;
if ((id_remap_data->flag & ID_REMAP_STORE_NEVER_NULL_USAGE) &&
(cb_flag & IDWALK_CB_NEVER_NULL) &&
(expected_mapping_result == ID_REMAP_RESULT_SOURCE_UNASSIGNED))
{
id_remapper.never_null_users_add(id_owner);
}
/* Special hack in case it's Object->data and we are in edit mode, and new_id is not nullptr
@@ -715,13 +718,9 @@ void BKE_libblock_remap_multiple(Main *bmain, IDRemapper &mappings, const int re
BKE_main_unlock(bmain);
}
void BKE_libblock_unlink(Main *bmain,
void *idv,
const bool do_flag_never_null,
const bool do_skip_indirect)
void BKE_libblock_unlink(Main *bmain, void *idv, const bool do_skip_indirect)
{
const int remap_flags = (do_skip_indirect ? ID_REMAP_SKIP_INDIRECT_USAGE : 0) |
(do_flag_never_null ? ID_REMAP_FLAG_NEVER_NULL_USAGE : 0);
const int remap_flags = (do_skip_indirect ? ID_REMAP_SKIP_INDIRECT_USAGE : 0);
BKE_main_lock(bmain);
source/blender/blenkernel/intern/lib_remap_test.cc
+24 -14
View File
@@ -32,6 +32,8 @@
#include "MEM_guardedalloc.h"
using namespace blender::bke::id;
namespace blender::bke::tests {
class TestData {
@@ -298,7 +300,7 @@ TEST(lib_remap, never_null_usage_flag_not_requested_on_delete)
EXPECT_EQ(context.test_data.object->id.tag & LIB_TAG_DOIT, 0);
}
TEST(lib_remap, never_null_usage_flag_requested_on_delete)
TEST(lib_remap, never_null_usage_storage_requested_on_delete)
{
Context<MeshObjectTestData> context;
@@ -306,14 +308,19 @@ TEST(lib_remap, never_null_usage_flag_requested_on_delete)
EXPECT_EQ(context.test_data.object->data, context.test_data.mesh);
EXPECT_EQ(context.test_data.object->id.tag & LIB_TAG_DOIT, 0);
/* Never null usage is requested so the flag should be set. */
BKE_libblock_remap(context.test_data.bmain,
context.test_data.mesh,
nullptr,
ID_REMAP_SKIP_NEVER_NULL_USAGE | ID_REMAP_FLAG_NEVER_NULL_USAGE);
/* Never null usage is requested so the owner ID (the Object) should be added to the set. */
IDRemapper remapper;
remapper.add(&context.test_data.mesh->id, nullptr);
BKE_libblock_remap_multiple_locked(
context.test_data.bmain,
remapper,
(ID_REMAP_SKIP_NEVER_NULL_USAGE | ID_REMAP_STORE_NEVER_NULL_USAGE));
/* Never null usages unassignement is not enforced (no #ID_REMAP_FORCE_NEVER_NULL_USAGE), so the
* obdta should still use the original mesh. */
EXPECT_EQ(context.test_data.object->data, context.test_data.mesh);
EXPECT_NE(context.test_data.object->data, nullptr);
EXPECT_EQ(context.test_data.object->id.tag & LIB_TAG_DOIT, LIB_TAG_DOIT);
EXPECT_TRUE(remapper.never_null_users().contains(&context.test_data.object->id));
}
TEST(lib_remap, never_null_usage_flag_not_requested_on_remap)
@@ -332,7 +339,7 @@ TEST(lib_remap, never_null_usage_flag_not_requested_on_remap)
EXPECT_EQ(context.test_data.object->id.tag & LIB_TAG_DOIT, 0);
}
TEST(lib_remap, never_null_usage_flag_requested_on_remap)
TEST(lib_remap, never_null_usage_storage_requested_on_remap)
{
Context<MeshObjectTestData> context;
Mesh *other_mesh = BKE_mesh_add(context.test_data.bmain, nullptr);
@@ -341,13 +348,16 @@ TEST(lib_remap, never_null_usage_flag_requested_on_remap)
EXPECT_EQ(context.test_data.object->data, context.test_data.mesh);
EXPECT_EQ(context.test_data.object->id.tag & LIB_TAG_DOIT, 0);
/* Never null usage is requested so the flag should be set. */
BKE_libblock_remap(context.test_data.bmain,
context.test_data.mesh,
other_mesh,
ID_REMAP_SKIP_NEVER_NULL_USAGE | ID_REMAP_FLAG_NEVER_NULL_USAGE);
/* Never null usage is requested, but the obdata is remapped to another Mesh, not to `nullptr`,
* so the `never_null_users` set should remain empty. */
IDRemapper remapper;
remapper.add(&context.test_data.mesh->id, &other_mesh->id);
BKE_libblock_remap_multiple_locked(
context.test_data.bmain,
remapper,
(ID_REMAP_SKIP_NEVER_NULL_USAGE | ID_REMAP_STORE_NEVER_NULL_USAGE));
EXPECT_EQ(context.test_data.object->data, other_mesh);
EXPECT_EQ(context.test_data.object->id.tag & LIB_TAG_DOIT, LIB_TAG_DOIT);
EXPECT_TRUE(remapper.never_null_users().is_empty());
}
/** \} */