From a0f4a5dffe942da214aaf19f0ed20d4df0287c07 Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Thu, 1 Jun 2023 16:37:04 +1000 Subject: [PATCH 1/4] Fix #108495: Pasting a material crashes --- source/blender/blenkernel/BKE_material.h | 14 -- source/blender/blenkernel/intern/material.cc | 186 ------------------ .../blender/editors/render/render_shading.cc | 168 +++++++++++++++- .../windowmanager/intern/wm_init_exit.cc | 2 - 4 files changed, 163 insertions(+), 207 deletions(-) diff --git a/source/blender/blenkernel/BKE_material.h b/source/blender/blenkernel/BKE_material.h index e22171c8b36..c9ae45806b7 100644 --- a/source/blender/blenkernel/BKE_material.h +++ b/source/blender/blenkernel/BKE_material.h @@ -173,20 +173,6 @@ void ramp_blend(int type, float r_col[3], float fac, const float col[3]); /** \} */ -/* -------------------------------------------------------------------- */ -/** \name Copy/Paste - * \{ */ - -void BKE_material_copybuf_clear(void); -void BKE_material_copybuf_free(void); -void BKE_material_copybuf_copy(struct Main *bmain, struct Material *ma); -/** - * \return true when the material was modified. - */ -bool BKE_material_copybuf_paste(struct Main *bmain, struct Material *ma); - -/** \} */ - /* -------------------------------------------------------------------- */ /** \name Default Materials * \{ */ diff --git a/source/blender/blenkernel/intern/material.cc b/source/blender/blenkernel/intern/material.cc index d1e1dd2a07c..114ae7c5db4 100644 --- a/source/blender/blenkernel/intern/material.cc +++ b/source/blender/blenkernel/intern/material.cc @@ -75,8 +75,6 @@ static CLG_LogRef LOG = {"bke.material"}; -static void material_clear_data(ID *id); - static void material_init_data(ID *id) { Material *material = (Material *)id; @@ -132,26 +130,6 @@ static void material_copy_data(Main *bmain, ID *id_dst, const ID *id_src, const /* TODO: Duplicate Engine Settings and set runtime to nullptr. */ } -/** - * Ensure pointers to allocated memory is cleared - * (the kind of data that would have to be copied). - * - * \note Keep in sync with #material_free_data. - * \note Doesn't handle animation data (`ma->adt`). - */ -static void material_clear_data(ID *id) -{ - Material *material = (Material *)id; - - BLI_listbase_clear(&material->gpumaterial); - material->texpaintslot = nullptr; - material->gp_style = nullptr; - material->nodetree = nullptr; - material->preview = nullptr; - - id->icon_id = 0; -} - static void material_free_data(ID *id) { Material *material = (Material *)id; @@ -1920,170 +1898,6 @@ void ramp_blend(int type, float r_col[3], const float fac, const float col[3]) } } -/* -------------------------------------------------------------------- */ -/** \name Material Copy/Paste - * - * As materials may reference other data, the clipboard only stores a subset of all possible data. - * The material and it's node-tree are stored and nothing else. - * Notably the following variables are *not* part of the clipboard. - * - * - The #ID (name, fake-user, custom-properties .. etc). - * - Animation data (#Material::adt, #bNodeTree::adt). - * - Texture paint slots (#Material::texpaintslot) - * as this is cache and references other ID's. - * - Grease pencil style (#Material::gp_style) - * could be supported but ID's and pointers would need to be handled carefully. - * - * When pasting, some data in the destination material is left as-is: - * - The #ID. - * - Animation data, with the exception that pasting a material without a node-tree - * will clear the existing materials node-tree & its animation. - * Note that applying existing animation to the pasted material might not make sense - * and may reference data-paths that don't resolve (depending on the kind of material). - * The user might need to clear the animation in this case. - * - * \{ */ - -static Material matcopybuf; -static short matcopied = 0; - -void BKE_material_copybuf_clear(void) -{ - if (matcopied) { - BKE_material_copybuf_free(); - } - matcopybuf = blender::dna::shallow_zero_initialize(); - matcopied = 0; -} - -/** - * Some members should *never* be set, ensure this is always the case. - * Call at the beginning & end of functions that manipulate the clipboard. - */ -static void material_copybuf_assert_is_valid() -{ - BLI_assert(!matcopybuf.id.icon_id); - BLI_assert(!matcopybuf.id.py_instance); - BLI_assert(!matcopybuf.adt); - BLI_assert(!matcopybuf.preview); - if (matcopybuf.nodetree) { - BLI_assert(!matcopybuf.nodetree->id.py_instance); - BLI_assert(!matcopybuf.nodetree->adt); - BLI_assert(!matcopybuf.nodetree->owner_id); - } -} - -void BKE_material_copybuf_free(void) -{ - material_copybuf_assert_is_valid(); - - material_free_data(&matcopybuf.id); - matcopied = 0; -} - -void BKE_material_copybuf_copy(Main *bmain, Material *ma) -{ - material_copybuf_assert_is_valid(); - - if (matcopied) { - BKE_material_copybuf_free(); - } - - matcopybuf = blender::dna::shallow_copy(*ma); - - /* Not essential, but we never want to use any ID values from the source, - * this ensures that never happens. */ - memset(&matcopybuf.id, 0, sizeof(ID)); - - /* Ensure dangling pointers are never copied back into a material. */ - material_clear_data(&matcopybuf.id); - - /* Unhandled by materials generic data functions. */ - matcopybuf.adt = nullptr; - - if (ma->nodetree != nullptr) { - /* Never copy animation data. */ - struct { - AnimData *adt; - } backup; - backup.adt = ma->nodetree->adt; - ma->nodetree->adt = nullptr; - - matcopybuf.nodetree = blender::bke::ntreeCopyTree_ex(ma->nodetree, bmain, false); - matcopybuf.nodetree->owner_id = nullptr; - - ma->nodetree->adt = backup.adt; - } - - /* TODO: Duplicate Engine Settings and set runtime to nullptr. */ - matcopied = 1; - - material_copybuf_assert_is_valid(); -} - -bool BKE_material_copybuf_paste(Main *bmain, Material *ma) -{ - material_copybuf_assert_is_valid(); - - if (matcopied == 0) { - return false; - } - - /* `matcopybuf` never has animation data, no need to check. */ - const bool has_animdata = (ma->adt != nullptr || (ma->nodetree && ma->nodetree->adt)); - const bool has_node_tree = (ma->nodetree || matcopybuf.nodetree); - - AnimData *backup_nodetree_adt = nullptr; - if (ma->nodetree && matcopybuf.nodetree) { - /* Keep data to apply back to the new node-tree. */ - std::swap(backup_nodetree_adt, ma->nodetree->adt); - } - - /* Handles freeing nodes and and other run-time data (previews) for e.g. */ - material_free_data(&ma->id); - - /* Copy from `matcopybuf` preserving some members. - * NOTE: animation data isn't stored in the clipboard, any existing animation will be left as-is. - * Any undesired animation will have to be manually cleared by the user. */ - { - struct { - ID id; - AnimData *adt; - } backup; - backup.id = ma->id; - backup.adt = ma->adt; - - *ma = blender::dna::shallow_copy(matcopybuf); - - ma->id = backup.id; - ma->adt = backup.adt; - } - - if (matcopybuf.nodetree != nullptr) { - BLI_assert(matcopybuf.nodetree->adt == nullptr); - ma->nodetree = blender::bke::ntreeCopyTree_ex(matcopybuf.nodetree, bmain, false); - ma->nodetree->owner_id = &ma->id; - - /* Restore animation pointer (if set). */ - BLI_assert(ma->nodetree->adt == nullptr); - ma->nodetree->adt = backup_nodetree_adt; - } - - if (has_node_tree || has_animdata) { - /* Important to run this when the embedded tree if freed, - * otherwise the depsgraph holds a reference to the (now freed) `ma->nodetree`. - * Also run this when a new node-tree is set to ensure it's accounted for. - * This also applies to animation data which is likely to be stored in the depsgraph. */ - DEG_relations_tag_update(bmain); - } - - material_copybuf_assert_is_valid(); - - return true; -} - -/** \} */ - void BKE_material_eval(struct Depsgraph *depsgraph, Material *material) { DEG_debug_print_eval(depsgraph, __func__, material->id.name, material); diff --git a/source/blender/editors/render/render_shading.cc b/source/blender/editors/render/render_shading.cc index 66eaaabb127..40b9bc80bd0 100644 --- a/source/blender/editors/render/render_shading.cc +++ b/source/blender/editors/render/render_shading.cc @@ -10,6 +10,7 @@ #include "MEM_guardedalloc.h" +#include "DNA_ID.h" #include "DNA_curve_types.h" #include "DNA_light_types.h" #include "DNA_lightprobe_types.h" @@ -23,12 +24,15 @@ #include "BLI_listbase.h" #include "BLI_math_vector.h" +#include "BLI_path_util.h" #include "BLI_utildefines.h" #include "BLT_translation.h" #include "BKE_anim_data.h" #include "BKE_animsys.h" +#include "BKE_appdir.h" +#include "BKE_blender_copybuffer.h" #include "BKE_brush.h" #include "BKE_context.h" #include "BKE_curve.h" @@ -37,6 +41,8 @@ #include "BKE_image.h" #include "BKE_layer.h" #include "BKE_lib_id.h" +#include "BKE_lib_query.h" +#include "BKE_lib_remap.h" #include "BKE_lightprobe.h" #include "BKE_linestyle.h" #include "BKE_main.h" @@ -2730,8 +2736,7 @@ void TEXTURE_OT_slot_move(wmOperatorType *ot) /** \name Material Copy Operator * \{ */ -/* material copy/paste */ -static int copy_material_exec(bContext *C, wmOperator * /*op*/) +static int copy_material_exec(bContext *C, wmOperator *op) { Material *ma = static_cast( CTX_data_pointer_get_type(C, "material", &RNA_Material).data); @@ -2740,7 +2745,23 @@ static int copy_material_exec(bContext *C, wmOperator * /*op*/) return OPERATOR_CANCELLED; } - BKE_material_copybuf_copy(CTX_data_main(C), ma); + char filepath[FILE_MAX]; + Main *bmain = CTX_data_main(C); + + BKE_copybuffer_copy_begin(bmain); + + /* TODO(@ideasman42): this could potentially expand into many ID data types. + * We may want to limit this to avoid expanding entire scenes & object data, + * although this is more of a worst case scenario. */ + BKE_copybuffer_copy_tag_ID(&ma->id); + + BLI_path_join(filepath, sizeof(filepath), BKE_tempdir_base(), "copybuffer_material.blend"); + BKE_copybuffer_copy_end(bmain, filepath, op->reports); + + return OPERATOR_FINISHED; + + /* We are all done! */ + BKE_report(op->reports, RPT_INFO, "Copied material to internal clipboard"); return OPERATOR_FINISHED; } @@ -2768,6 +2789,7 @@ void MATERIAL_OT_copy(wmOperatorType *ot) static int paste_material_exec(bContext *C, wmOperator *op) { + Main *bmain = CTX_data_main(C); Material *ma = static_cast( CTX_data_pointer_get_type(C, "material", &RNA_Material).data); @@ -2776,11 +2798,147 @@ static int paste_material_exec(bContext *C, wmOperator *op) return OPERATOR_CANCELLED; } - if (!BKE_material_copybuf_paste(CTX_data_main(C), ma)) { - BKE_report(op->reports, RPT_WARNING, "No material in the internal clipboard to paste"); + /* Read copy buffer .blend file. */ + char filepath[FILE_MAX]; + Main *temp_bmain = BKE_main_new(); + + STRNCPY(temp_bmain->filepath, BKE_main_blendfile_path_from_global()); + BLI_path_join(filepath, sizeof(filepath), BKE_tempdir_base(), "copybuffer_material.blend"); + + /* NOTE(@ideasman42) The node tree might reference different kinds of ID types. + * It's not clear-cut which ID types should be included, although it's unlikely + * users would want an entire scene & it's objects to be included. + * Filter a subset of ID types with some reasons for including them. */ + const uint64_t ntree_filter = ( + /* Material is necessary for reading the clipboard. */ + FILTER_ID_MA | + /* Node-groups. */ + FILTER_ID_NT | + /* Image textures. */ + FILTER_ID_IM | + /* Internal text (scripts). */ + FILTER_ID_TXT | + /* Texture coordinates may reference objects. + * Note that object data is *not* included. */ + FILTER_ID_OB); + + if (!BKE_copybuffer_read(temp_bmain, filepath, op->reports, ntree_filter)) { + BKE_report(op->reports, RPT_ERROR, "Internal clipboard is empty"); + BKE_main_free(temp_bmain); return OPERATOR_CANCELLED; } + /* Make sure data from this file is usable for material paste. */ + if (!BLI_listbase_is_single(&temp_bmain->materials)) { + BKE_report(op->reports, RPT_ERROR, "Internal clipboard is not from a material"); + BKE_main_free(temp_bmain); + return OPERATOR_CANCELLED; + } + + Material *ma_from = static_cast(temp_bmain->materials.first); + + /* Important to run this when the embedded tree if freed, + * otherwise the depsgraph holds a reference to the (now freed) `ma->nodetree`. + * Also run this when a new node-tree is set to ensure it's accounted for. + * This also applies to animation data which is likely to be stored in the depsgraph. */ + bool update_relations = false; + if (ma->nodetree || ma_from->nodetree) { + update_relations = true; + } + + /* Keep animation by moving local animation to the paste node-tree. */ + if (ma->nodetree && ma_from->nodetree) { + BLI_assert(ma_from->nodetree->adt == nullptr); + std::swap(ma->adt, ma_from->adt); + } + + /* Needed to update #SpaceNode::nodetree else a stale pointer is used. */ + if (ma->nodetree) { + bNodeTree *nodetree = ma->nodetree; + BKE_libblock_remap(bmain, ma->nodetree, ma_from->nodetree, ID_REMAP_FORCE_UI_POINTERS); + + /* Free & clear data here, so user counts are handled, otherwise it's + * freed as part of #BKE_main_free which doesn't handle user-counts. */ + /* Walk over all the embedded nodes ID's (non-recursively). */ + BKE_library_foreach_ID_link( + bmain, + &nodetree->id, + [](LibraryIDLinkCallbackData *cb_data) -> int { + if (cb_data->cb_flag & IDWALK_CB_USER) { + id_us_min(*cb_data->id_pointer); + *cb_data->id_pointer = nullptr; + } + return IDWALK_RET_NOP; + }, + nullptr, + IDWALK_NOP); + + ntreeFreeEmbeddedTree(nodetree); + MEM_freeN(nodetree); + ma->nodetree = nullptr; + } + + /* Swap data-block content. */ + { + /* It's important to keep: `id` & `adt` so the current material name, + * custom properties, etc are maintained and animation data isn't clobbered. */ + Material ma_temp; + MEMCPY_STRUCT_AFTER(&ma_temp, ma, adt); + MEMCPY_STRUCT_AFTER(ma, ma_from, adt); + MEMCPY_STRUCT_AFTER(ma_from, &ma_temp, adt); + + /* Keep these values by swapping them back, because the values in the copy buffer + * wont link to other ID's usefully. Other members such as `gpumaterial` + * wont have useful values in the clipboard file either. */ + std::swap(ma->gpumaterial, ma_from->gpumaterial); + std::swap(ma->texpaintslot, ma_from->texpaintslot); + std::swap(ma->gp_style, ma_from->gp_style); + } + + /* The node-tree from the clipboard is now assigned to the local material, + * however the ID's it references are still part of `temp_bmain`. + * These data-blocks references must be cleared or replaces with references to `bmain`. + * TODO(@ideasman42): support merging indirectly referenced data-blocks besides the material, + * this would be useful for pasting materials with node-groups between files. */ + if (ma->nodetree) { + ma->nodetree->owner_id = &ma->id; + + /* Map remote ID's to local ones. */ + BKE_library_foreach_ID_link( + bmain, + &ma->nodetree->id, + [](LibraryIDLinkCallbackData *cb_data) -> int { + Main *bmain = static_cast
(cb_data->user_data); + ID **id_p = cb_data->id_pointer; + if (*id_p) { + if (cb_data->cb_flag & IDWALK_CB_USER) { + id_us_min(*id_p); + } + ListBase *lb = which_libbase(bmain, GS((*id_p)->name)); + ID *id_local = static_cast( + BLI_findstring(lb, (*id_p)->name + 2, offsetof(ID, name) + 2)); + *id_p = id_local; + if (id_local) { + id_us_plus(id_local); + id_lib_extern(id_local); + } + } + return IDWALK_RET_NOP; + }, + bmain, + IDWALK_NOP); + + LISTBASE_FOREACH (bNode *, node, &ma->nodetree->nodes) { + if (node->id == nullptr) { + continue; + } + } + } + BKE_main_free(temp_bmain); + + if (update_relations) { + DEG_relations_tag_update(bmain); + } DEG_id_tag_update(&ma->id, ID_RECALC_COPY_ON_WRITE); WM_event_add_notifier(C, NC_MATERIAL | ND_SHADING_LINKS, ma); diff --git a/source/blender/windowmanager/intern/wm_init_exit.cc b/source/blender/windowmanager/intern/wm_init_exit.cc index ec751ea2682..5b541c48533 100644 --- a/source/blender/windowmanager/intern/wm_init_exit.cc +++ b/source/blender/windowmanager/intern/wm_init_exit.cc @@ -349,7 +349,6 @@ void WM_init(bContext *C, int argc, const char **argv) } } - BKE_material_copybuf_clear(); ED_render_clear_mtex_copybuf(); wm_history_file_read(); @@ -608,7 +607,6 @@ void WM_exit_ex(bContext *C, const bool do_python, const bool do_user_exit_actio DRW_subdiv_free(); } - BKE_material_copybuf_free(); ANIM_fcurves_copybuf_free(); ANIM_drivers_copybuf_free(); ANIM_driver_vars_copybuf_free(); -- 2.30.2 From 51ad2fe920e42b93f1d14947bb2f824af340be01 Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Thu, 1 Jun 2023 20:48:25 +1000 Subject: [PATCH 2/4] Update based on review --- .../blender/editors/render/render_shading.cc | 147 ++++++++++-------- 1 file changed, 79 insertions(+), 68 deletions(-) diff --git a/source/blender/editors/render/render_shading.cc b/source/blender/editors/render/render_shading.cc index 40b9bc80bd0..514ad6e56aa 100644 --- a/source/blender/editors/render/render_shading.cc +++ b/source/blender/editors/render/render_shading.cc @@ -2787,6 +2787,41 @@ void MATERIAL_OT_copy(wmOperatorType *ot) /** \name Material Paste Operator * \{ */ +/** + * Clear ID's as freeing the data-block doesn't handle reference counting. + */ +static int paste_material_nodetree_ids_decref(LibraryIDLinkCallbackData *cb_data) +{ + if (cb_data->cb_flag & IDWALK_CB_USER) { + id_us_min(*cb_data->id_pointer); + *cb_data->id_pointer = nullptr; + } + return IDWALK_RET_NOP; +} + +/** + * Re-map ID's from the clipboard to ID's in `main`, by name. + */ +static int paste_material_nodetree_ids_relink_or_clear(LibraryIDLinkCallbackData *cb_data) +{ + Main *bmain = static_cast
(cb_data->user_data); + ID **id_p = cb_data->id_pointer; + if (*id_p) { + if (cb_data->cb_flag & IDWALK_CB_USER) { + id_us_min(*id_p); + } + ListBase *lb = which_libbase(bmain, GS((*id_p)->name)); + ID *id_local = static_cast( + BLI_findstring(lb, (*id_p)->name + 2, offsetof(ID, name) + 2)); + *id_p = id_local; + if (id_local) { + id_us_plus(id_local); + id_lib_extern(id_local); + } + } + return IDWALK_RET_NOP; +} + static int paste_material_exec(bContext *C, wmOperator *op) { Main *bmain = CTX_data_main(C); @@ -2837,19 +2872,10 @@ static int paste_material_exec(bContext *C, wmOperator *op) Material *ma_from = static_cast(temp_bmain->materials.first); - /* Important to run this when the embedded tree if freed, - * otherwise the depsgraph holds a reference to the (now freed) `ma->nodetree`. - * Also run this when a new node-tree is set to ensure it's accounted for. - * This also applies to animation data which is likely to be stored in the depsgraph. */ - bool update_relations = false; - if (ma->nodetree || ma_from->nodetree) { - update_relations = true; - } - /* Keep animation by moving local animation to the paste node-tree. */ if (ma->nodetree && ma_from->nodetree) { BLI_assert(ma_from->nodetree->adt == nullptr); - std::swap(ma->adt, ma_from->adt); + std::swap(ma->nodetree->adt, ma_from->nodetree->adt); } /* Needed to update #SpaceNode::nodetree else a stale pointer is used. */ @@ -2861,39 +2887,48 @@ static int paste_material_exec(bContext *C, wmOperator *op) * freed as part of #BKE_main_free which doesn't handle user-counts. */ /* Walk over all the embedded nodes ID's (non-recursively). */ BKE_library_foreach_ID_link( - bmain, - &nodetree->id, - [](LibraryIDLinkCallbackData *cb_data) -> int { - if (cb_data->cb_flag & IDWALK_CB_USER) { - id_us_min(*cb_data->id_pointer); - *cb_data->id_pointer = nullptr; - } - return IDWALK_RET_NOP; - }, - nullptr, - IDWALK_NOP); + bmain, &nodetree->id, paste_material_nodetree_ids_decref, nullptr, IDWALK_NOP); ntreeFreeEmbeddedTree(nodetree); MEM_freeN(nodetree); ma->nodetree = nullptr; } - /* Swap data-block content. */ - { - /* It's important to keep: `id` & `adt` so the current material name, - * custom properties, etc are maintained and animation data isn't clobbered. */ - Material ma_temp; - MEMCPY_STRUCT_AFTER(&ma_temp, ma, adt); - MEMCPY_STRUCT_AFTER(ma, ma_from, adt); - MEMCPY_STRUCT_AFTER(ma_from, &ma_temp, adt); +/* Swap data-block content, while swapping isn't always needed, + * it means memory is properly freed in the case of allocations.. */ +#define SWAP_MEMBER(member) std::swap(ma->member, ma_from->member) - /* Keep these values by swapping them back, because the values in the copy buffer - * wont link to other ID's usefully. Other members such as `gpumaterial` - * wont have useful values in the clipboard file either. */ - std::swap(ma->gpumaterial, ma_from->gpumaterial); - std::swap(ma->texpaintslot, ma_from->texpaintslot); - std::swap(ma->gp_style, ma_from->gp_style); - } + /* Intentionally skip: + * - Texture painting slots. + * - Preview render. + * - Grease pencil styles (we could although they reference many ID's themselves). + */ + SWAP_MEMBER(flag); + SWAP_MEMBER(r); + SWAP_MEMBER(g); + SWAP_MEMBER(b); + SWAP_MEMBER(a); + SWAP_MEMBER(specr); + SWAP_MEMBER(specg); + SWAP_MEMBER(specb); + SWAP_MEMBER(spec); + SWAP_MEMBER(roughness); + SWAP_MEMBER(metallic); + SWAP_MEMBER(use_nodes); + SWAP_MEMBER(index); + SWAP_MEMBER(nodetree); + SWAP_MEMBER(line_col); + SWAP_MEMBER(line_priority); + SWAP_MEMBER(vcol_alpha); + SWAP_MEMBER(vcol_alpha); + + SWAP_MEMBER(alpha_threshold); + SWAP_MEMBER(refract_depth); + SWAP_MEMBER(blend_method); + SWAP_MEMBER(blend_shadow); + SWAP_MEMBER(blend_flag); + +#undef SWAP_MEMBER /* The node-tree from the clipboard is now assigned to the local material, * however the ID's it references are still part of `temp_bmain`. @@ -2902,43 +2937,19 @@ static int paste_material_exec(bContext *C, wmOperator *op) * this would be useful for pasting materials with node-groups between files. */ if (ma->nodetree) { ma->nodetree->owner_id = &ma->id; - /* Map remote ID's to local ones. */ BKE_library_foreach_ID_link( - bmain, - &ma->nodetree->id, - [](LibraryIDLinkCallbackData *cb_data) -> int { - Main *bmain = static_cast
(cb_data->user_data); - ID **id_p = cb_data->id_pointer; - if (*id_p) { - if (cb_data->cb_flag & IDWALK_CB_USER) { - id_us_min(*id_p); - } - ListBase *lb = which_libbase(bmain, GS((*id_p)->name)); - ID *id_local = static_cast( - BLI_findstring(lb, (*id_p)->name + 2, offsetof(ID, name) + 2)); - *id_p = id_local; - if (id_local) { - id_us_plus(id_local); - id_lib_extern(id_local); - } - } - return IDWALK_RET_NOP; - }, - bmain, - IDWALK_NOP); - - LISTBASE_FOREACH (bNode *, node, &ma->nodetree->nodes) { - if (node->id == nullptr) { - continue; - } - } + bmain, &ma->nodetree->id, paste_material_nodetree_ids_relink_or_clear, bmain, IDWALK_NOP); } BKE_main_free(temp_bmain); - if (update_relations) { - DEG_relations_tag_update(bmain); - } + /* Important to run this when the embedded tree if freed, + * otherwise the depsgraph holds a reference to the (now freed) `ma->nodetree`. + * Also run this when a new node-tree is set to ensure it's accounted for. + * This also applies to animation data which is likely to be stored in the depsgraph. + * Always call instead of checking when it *might* be needed. */ + DEG_relations_tag_update(bmain); + DEG_id_tag_update(&ma->id, ID_RECALC_COPY_ON_WRITE); WM_event_add_notifier(C, NC_MATERIAL | ND_SHADING_LINKS, ma); -- 2.30.2 From 82f1cb69721a8273ff1e33f33a5f2aa6df68b6c8 Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Thu, 1 Jun 2023 21:55:04 +1000 Subject: [PATCH 3/4] Support multiple materials being included and fix user counts increasing for non user id links --- .../blender/editors/render/render_shading.cc | 56 ++++++++++++++++--- 1 file changed, 49 insertions(+), 7 deletions(-) diff --git a/source/blender/editors/render/render_shading.cc b/source/blender/editors/render/render_shading.cc index 514ad6e56aa..4902aef6777 100644 --- a/source/blender/editors/render/render_shading.cc +++ b/source/blender/editors/render/render_shading.cc @@ -38,6 +38,7 @@ #include "BKE_curve.h" #include "BKE_editmesh.h" #include "BKE_global.h" +#include "BKE_idprop.h" #include "BKE_image.h" #include "BKE_layer.h" #include "BKE_lib_id.h" @@ -2736,6 +2737,12 @@ void TEXTURE_OT_slot_move(wmOperatorType *ot) /** \name Material Copy Operator * \{ */ +/** + * Property names that marks this material as active, + * needed to check which material is active when multiple are written. + */ +static const char *material_clipboard_prop_id = "__active_material_clipboard__"; + static int copy_material_exec(bContext *C, wmOperator *op) { Material *ma = static_cast( @@ -2748,6 +2755,21 @@ static int copy_material_exec(bContext *C, wmOperator *op) char filepath[FILE_MAX]; Main *bmain = CTX_data_main(C); + /* Mark is the material to use (others may be expanded). */ + const bool had_prop_group = (ma->id.properties != nullptr); + IDProperty *properties = nullptr; + if (!had_prop_group) { + properties = IDP_GetProperties(&ma->id, true); + } + IDProperty *prop_mark = IDP_GetPropertyFromGroup(properties, material_clipboard_prop_id); + const bool had_prop_item = prop_mark != nullptr; + if (!had_prop_item) { + IDPropertyTemplate val = {}; + prop_mark = IDP_New(IDP_INT, &val, __func__); + STRNCPY(prop_mark->name, material_clipboard_prop_id); + IDP_ReplaceInGroup_ex(properties, prop_mark, NULL); + } + BKE_copybuffer_copy_begin(bmain); /* TODO(@ideasman42): this could potentially expand into many ID data types. @@ -2758,7 +2780,14 @@ static int copy_material_exec(bContext *C, wmOperator *op) BLI_path_join(filepath, sizeof(filepath), BKE_tempdir_base(), "copybuffer_material.blend"); BKE_copybuffer_copy_end(bmain, filepath, op->reports); - return OPERATOR_FINISHED; + /* Clear mark (as needed). */ + if (!had_prop_group) { + IDP_FreeProperty(properties); + ma->id.properties = nullptr; + } + else if (!had_prop_item) { + IDP_FreeFromGroup(properties, prop_mark); + } /* We are all done! */ BKE_report(op->reports, RPT_INFO, "Copied material to internal clipboard"); @@ -2814,10 +2843,10 @@ static int paste_material_nodetree_ids_relink_or_clear(LibraryIDLinkCallbackData ID *id_local = static_cast( BLI_findstring(lb, (*id_p)->name + 2, offsetof(ID, name) + 2)); *id_p = id_local; - if (id_local) { + if (cb_data->cb_flag & IDWALK_CB_USER) { id_us_plus(id_local); - id_lib_extern(id_local); } + id_lib_extern(id_local); } return IDWALK_RET_NOP; } @@ -2863,15 +2892,28 @@ static int paste_material_exec(bContext *C, wmOperator *op) return OPERATOR_CANCELLED; } + /* There may be multiple materials, + * check for a property that marks this as the active material. */ + Material *ma_from = nullptr; + LISTBASE_FOREACH (Material *, ma, &temp_bmain->materials) { + if (ma->id.properties == nullptr) { + continue; + } + IDProperty *prop_mark = IDP_GetPropertyFromGroup(ma->id.properties, + material_clipboard_prop_id); + if (prop_mark) { + ma_from = ma; + break; + } + } + /* Make sure data from this file is usable for material paste. */ - if (!BLI_listbase_is_single(&temp_bmain->materials)) { + if (ma_from == nullptr) { BKE_report(op->reports, RPT_ERROR, "Internal clipboard is not from a material"); BKE_main_free(temp_bmain); return OPERATOR_CANCELLED; } - Material *ma_from = static_cast(temp_bmain->materials.first); - /* Keep animation by moving local animation to the paste node-tree. */ if (ma->nodetree && ma_from->nodetree) { BLI_assert(ma_from->nodetree->adt == nullptr); @@ -2936,7 +2978,7 @@ static int paste_material_exec(bContext *C, wmOperator *op) * TODO(@ideasman42): support merging indirectly referenced data-blocks besides the material, * this would be useful for pasting materials with node-groups between files. */ if (ma->nodetree) { - ma->nodetree->owner_id = &ma->id; + ma->nodetree->owner_id = nullptr; /* Map remote ID's to local ones. */ BKE_library_foreach_ID_link( bmain, &ma->nodetree->id, paste_material_nodetree_ids_relink_or_clear, bmain, IDWALK_NOP); -- 2.30.2 From f3e3f1cb12fb6f789070955519c7707340e99d16 Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Thu, 1 Jun 2023 23:53:24 +1000 Subject: [PATCH 4/4] Replace IDProperty tagging with a flag --- source/blender/blenkernel/intern/blendfile.cc | 20 ++++++-- .../blender/editors/render/render_shading.cc | 49 +++---------------- source/blender/makesdna/DNA_ID.h | 9 ++++ 3 files changed, 33 insertions(+), 45 deletions(-) diff --git a/source/blender/blenkernel/intern/blendfile.cc b/source/blender/blenkernel/intern/blendfile.cc index 53b2baa8c2c..fe096836e84 100644 --- a/source/blender/blenkernel/intern/blendfile.cc +++ b/source/blender/blenkernel/intern/blendfile.cc @@ -970,18 +970,32 @@ void BKE_blendfile_workspace_config_data_free(WorkspaceConfigFileData *workspace /** \name Blend File Write (Partial) * \{ */ -void BKE_blendfile_write_partial_begin(Main *bmain_src) +static void blendfile_write_partial_clear_flags(Main *bmain_src) { - BKE_main_id_tag_all(bmain_src, LIB_TAG_NEED_EXPAND | LIB_TAG_DOIT, false); + ListBase *lbarray[INDEX_ID_MAX]; + int a = set_listbasepointers(bmain_src, lbarray); + while (a--) { + LISTBASE_FOREACH (ID *, id, lbarray[a]) { + id->tag &= ~(LIB_TAG_NEED_EXPAND | LIB_TAG_DOIT); + id->flag &= ~(LIB_CLIPBOARD_MARK); + } + } +} + +void BKE_blendfile_write_partial_begin(Main *bmain) +{ + blendfile_write_partial_clear_flags(bmain); } void BKE_blendfile_write_partial_tag_ID(ID *id, bool set) { if (set) { id->tag |= LIB_TAG_NEED_EXPAND | LIB_TAG_DOIT; + id->flag |= LIB_CLIPBOARD_MARK; } else { id->tag &= ~(LIB_TAG_NEED_EXPAND | LIB_TAG_DOIT); + id->flag &= ~LIB_CLIPBOARD_MARK; } } @@ -1080,7 +1094,7 @@ bool BKE_blendfile_write_partial(Main *bmain_src, void BKE_blendfile_write_partial_end(Main *bmain_src) { - BKE_main_id_tag_all(bmain_src, LIB_TAG_NEED_EXPAND | LIB_TAG_DOIT, false); + blendfile_write_partial_clear_flags(bmain_src); } /** \} */ diff --git a/source/blender/editors/render/render_shading.cc b/source/blender/editors/render/render_shading.cc index 4902aef6777..15e3e3c3761 100644 --- a/source/blender/editors/render/render_shading.cc +++ b/source/blender/editors/render/render_shading.cc @@ -38,7 +38,6 @@ #include "BKE_curve.h" #include "BKE_editmesh.h" #include "BKE_global.h" -#include "BKE_idprop.h" #include "BKE_image.h" #include "BKE_layer.h" #include "BKE_lib_id.h" @@ -2737,12 +2736,6 @@ void TEXTURE_OT_slot_move(wmOperatorType *ot) /** \name Material Copy Operator * \{ */ -/** - * Property names that marks this material as active, - * needed to check which material is active when multiple are written. - */ -static const char *material_clipboard_prop_id = "__active_material_clipboard__"; - static int copy_material_exec(bContext *C, wmOperator *op) { Material *ma = static_cast( @@ -2756,39 +2749,13 @@ static int copy_material_exec(bContext *C, wmOperator *op) Main *bmain = CTX_data_main(C); /* Mark is the material to use (others may be expanded). */ - const bool had_prop_group = (ma->id.properties != nullptr); - IDProperty *properties = nullptr; - if (!had_prop_group) { - properties = IDP_GetProperties(&ma->id, true); - } - IDProperty *prop_mark = IDP_GetPropertyFromGroup(properties, material_clipboard_prop_id); - const bool had_prop_item = prop_mark != nullptr; - if (!had_prop_item) { - IDPropertyTemplate val = {}; - prop_mark = IDP_New(IDP_INT, &val, __func__); - STRNCPY(prop_mark->name, material_clipboard_prop_id); - IDP_ReplaceInGroup_ex(properties, prop_mark, NULL); - } - BKE_copybuffer_copy_begin(bmain); - /* TODO(@ideasman42): this could potentially expand into many ID data types. - * We may want to limit this to avoid expanding entire scenes & object data, - * although this is more of a worst case scenario. */ BKE_copybuffer_copy_tag_ID(&ma->id); BLI_path_join(filepath, sizeof(filepath), BKE_tempdir_base(), "copybuffer_material.blend"); BKE_copybuffer_copy_end(bmain, filepath, op->reports); - /* Clear mark (as needed). */ - if (!had_prop_group) { - IDP_FreeProperty(properties); - ma->id.properties = nullptr; - } - else if (!had_prop_item) { - IDP_FreeFromGroup(properties, prop_mark); - } - /* We are all done! */ BKE_report(op->reports, RPT_INFO, "Copied material to internal clipboard"); @@ -2823,8 +2790,8 @@ static int paste_material_nodetree_ids_decref(LibraryIDLinkCallbackData *cb_data { if (cb_data->cb_flag & IDWALK_CB_USER) { id_us_min(*cb_data->id_pointer); - *cb_data->id_pointer = nullptr; } + *cb_data->id_pointer = nullptr; return IDWALK_RET_NOP; } @@ -2846,6 +2813,9 @@ static int paste_material_nodetree_ids_relink_or_clear(LibraryIDLinkCallbackData if (cb_data->cb_flag & IDWALK_CB_USER) { id_us_plus(id_local); } + else if (cb_data->cb_flag & IDWALK_CB_USER_ONE) { + id_us_ensure_real(id_local); + } id_lib_extern(id_local); } return IDWALK_RET_NOP; @@ -2895,14 +2865,9 @@ static int paste_material_exec(bContext *C, wmOperator *op) /* There may be multiple materials, * check for a property that marks this as the active material. */ Material *ma_from = nullptr; - LISTBASE_FOREACH (Material *, ma, &temp_bmain->materials) { - if (ma->id.properties == nullptr) { - continue; - } - IDProperty *prop_mark = IDP_GetPropertyFromGroup(ma->id.properties, - material_clipboard_prop_id); - if (prop_mark) { - ma_from = ma; + LISTBASE_FOREACH (Material *, ma_iter, &temp_bmain->materials) { + if (ma_iter->id.flag & LIB_CLIPBOARD_MARK) { + ma_from = ma_iter; break; } } diff --git a/source/blender/makesdna/DNA_ID.h b/source/blender/makesdna/DNA_ID.h index 6f19955d8fc..a1c73b3bb7a 100644 --- a/source/blender/makesdna/DNA_ID.h +++ b/source/blender/makesdna/DNA_ID.h @@ -709,6 +709,15 @@ enum { * was kept around (because e.g. detected as user-edited). */ LIB_LIB_OVERRIDE_RESYNC_LEFTOVER = 1 << 13, + /** + * This `id` was explicitly copied as part of a clipboard copy operation. + * When reading the clipboard back, this can be used to check which ID's are + * intended to be part of the clipboard, compared with ID's that were indirectly referenced. + * + * While the flag is typically cleared, a saved file may have this set for some data-blocks, + * so it must be treated as dirty. + */ + LIB_CLIPBOARD_MARK = 1 << 14, }; /** -- 2.30.2