From dfb963c70df515213c452094c20c83720bc017ee Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Tue, 18 May 2021 12:53:28 +1000 Subject: Fix T88026: Repeated switch to rendered viewport crashes Resolve ownership ambiguity with shared physics pointers. Previously, LIB_ID_CREATE_NO_MAIN allowed pointer sharing with the source ID so physics caches can be shared between original and evaluated data: (Object.soft.shared & Object.rigidbody_object.shared). This only worked properly for LIB_TAG_COPIED_ON_WRITE ID's, as LIB_TAG_NO_MAIN can be used in situations where the original ID's lifetime limited by it's original data. This commit adds `LIB_ID_COPY_SET_COPIED_ON_WRITE` so ID's only share memory with original data for ID's evaluated in the depsgraph. For all other uses, a full copy of physics data is made. Ref D11228#287094 --- source/blender/blenkernel/BKE_lib_id.h | 4 ++++ source/blender/blenkernel/intern/lib_id.c | 7 +++++++ source/blender/blenkernel/intern/object.c | 4 ++-- source/blender/blenkernel/intern/rigidbody.c | 4 +++- source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc | 6 ++++-- source/blender/makesdna/DNA_ID.h | 9 ++++++++- 6 files changed, 28 insertions(+), 6 deletions(-) diff --git a/source/blender/blenkernel/BKE_lib_id.h b/source/blender/blenkernel/BKE_lib_id.h index adac92105ee..6b706f3bcd0 100644 --- a/source/blender/blenkernel/BKE_lib_id.h +++ b/source/blender/blenkernel/BKE_lib_id.h @@ -104,6 +104,10 @@ enum { * specific code in some copy cases (mostly for node trees). */ LIB_ID_CREATE_LOCAL = 1 << 9, + /** Create for the depsgraph, when set #LIB_TAG_COPIED_ON_WRITE must be set. + * Internally this is used to share some pointers instead of duplicating them. */ + LIB_ID_COPY_SET_COPIED_ON_WRITE = 1 << 10, + /* *** Specific options to some ID types or usages. *** */ /* *** May be ignored by unrelated ID copying functions. *** */ /** Object only, needed by make_local code. */ diff --git a/source/blender/blenkernel/intern/lib_id.c b/source/blender/blenkernel/intern/lib_id.c index 795a5ad9468..7429fe050dc 100644 --- a/source/blender/blenkernel/intern/lib_id.c +++ b/source/blender/blenkernel/intern/lib_id.c @@ -1245,6 +1245,13 @@ void BKE_libblock_copy_ex(Main *bmain, const ID *id, ID **r_newid, const int ori } BLI_assert(new_id != NULL); + if ((flag & LIB_ID_COPY_SET_COPIED_ON_WRITE) != 0) { + new_id->tag |= LIB_TAG_COPIED_ON_WRITE; + } + else { + new_id->tag &= ~LIB_TAG_COPIED_ON_WRITE; + } + const size_t id_len = BKE_libblock_get_alloc_info(GS(new_id->name), NULL); const size_t id_offset = sizeof(ID); if ((int)id_len - (int)id_offset > 0) { /* signed to allow neg result */ /* XXX ????? */ diff --git a/source/blender/blenkernel/intern/object.c b/source/blender/blenkernel/intern/object.c index ef553e60eb8..825f660fa3a 100644 --- a/source/blender/blenkernel/intern/object.c +++ b/source/blender/blenkernel/intern/object.c @@ -2292,7 +2292,7 @@ Object *BKE_object_add_for_data( void BKE_object_copy_softbody(Object *ob_dst, const Object *ob_src, const int flag) { SoftBody *sb = ob_src->soft; - bool tagged_no_main = ob_dst->id.tag & LIB_TAG_NO_MAIN; + const bool is_orig = (flag & LIB_ID_COPY_SET_COPIED_ON_WRITE) == 0; ob_dst->softflag = ob_src->softflag; if (sb == NULL) { @@ -2333,7 +2333,7 @@ void BKE_object_copy_softbody(Object *ob_dst, const Object *ob_src, const int fl sbn->scratch = NULL; - if (!tagged_no_main) { + if (is_orig) { sbn->shared = MEM_dupallocN(sb->shared); sbn->shared->pointcache = BKE_ptcache_copy_list( &sbn->shared->ptcaches, &sb->shared->ptcaches, flag); diff --git a/source/blender/blenkernel/intern/rigidbody.c b/source/blender/blenkernel/intern/rigidbody.c index 19078446009..2539b990210 100644 --- a/source/blender/blenkernel/intern/rigidbody.c +++ b/source/blender/blenkernel/intern/rigidbody.c @@ -260,10 +260,12 @@ static RigidBodyOb *rigidbody_copy_object(const Object *ob, const int flag) RigidBodyOb *rboN = NULL; if (ob->rigidbody_object) { + const bool is_orig = (flag & LIB_ID_COPY_SET_COPIED_ON_WRITE) == 0; + /* just duplicate the whole struct first (to catch all the settings) */ rboN = MEM_dupallocN(ob->rigidbody_object); - if ((flag & LIB_ID_CREATE_NO_MAIN) == 0) { + if (is_orig) { /* This is a regular copy, and not a CoW copy for depsgraph evaluation */ rboN->shared = MEM_callocN(sizeof(*rboN->shared), "RigidBodyOb_Shared"); } diff --git a/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc b/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc index e1959c8bf5e..43bcb23a38a 100644 --- a/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc +++ b/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc @@ -304,7 +304,8 @@ bool id_copy_inplace_no_main(const ID *id, ID *newid) bool result = (BKE_id_copy_ex(nullptr, (ID *)id_for_copy, &newid, - LIB_ID_COPY_LOCALIZE | LIB_ID_CREATE_NO_ALLOCATE) != nullptr); + (LIB_ID_COPY_LOCALIZE | LIB_ID_CREATE_NO_ALLOCATE | + LIB_ID_COPY_SET_COPIED_ON_WRITE)) != nullptr); #ifdef NESTED_ID_NASTY_WORKAROUND if (result) { @@ -333,7 +334,8 @@ bool scene_copy_inplace_no_main(const Scene *scene, Scene *new_scene) bool result = (BKE_id_copy_ex(nullptr, id_for_copy, (ID **)&new_scene, - LIB_ID_COPY_LOCALIZE | LIB_ID_CREATE_NO_ALLOCATE) != nullptr); + (LIB_ID_COPY_LOCALIZE | LIB_ID_CREATE_NO_ALLOCATE | + LIB_ID_COPY_SET_COPIED_ON_WRITE)) != nullptr); #ifdef NESTED_ID_NASTY_WORKAROUND if (result) { diff --git a/source/blender/makesdna/DNA_ID.h b/source/blender/makesdna/DNA_ID.h index fa60bba54d5..8bf9afafa1b 100644 --- a/source/blender/makesdna/DNA_ID.h +++ b/source/blender/makesdna/DNA_ID.h @@ -555,8 +555,15 @@ enum { /* RESET_AFTER_USE tag existing data before linking so we know what is new. */ LIB_TAG_PRE_EXISTING = 1 << 11, - /* The data-block is a copy-on-write/localized version. */ + /** + * The data-block is a copy-on-write/localized version. + * + * \warning This should not be cleared on existing data. + * If support for this is needed, see T88026 as this flag controls memory ownership + * of physics *shared* pointers. + */ LIB_TAG_COPIED_ON_WRITE = 1 << 12, + LIB_TAG_COPIED_ON_WRITE_EVAL_RESULT = 1 << 13, LIB_TAG_LOCALIZED = 1 << 14, -- cgit v1.2.3