blender
/
blender
136
406
Fork 590
Code Issues 6.3k Pull Requests 762 Projects 19 Wiki Activity

Fix #108495: Pasting a material crashes #108496

Closed
Campbell Barton wants to merge 4 commits from ideasman42/blender:pr-material-clipboard-as-blend into blender-v3.6-release
pull from: ideasman42/blender:pr-material-clipboard-as-blend
merge into: blender:blender-v3.6-release

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 4 Commits 4 Files Changed 6 +49 -7
1 changed files with 49 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 82f1cb6972 - Show all commits

56
source/blender/editors/render/render_shading.cc
View File

@ -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<Material *>(
@ -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<ID *>(
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) {
ideasman42 marked this conversation as resolved Outdated
Bastien Montagne commented 2023-06-01 15:02:48 +02:00
Outdated
Review
Copy Link

Should also check for IDWALK_CB_USER_ONE and call id_us_ensure_real in such case.

Although probably not effectively needed here, would rather keep ID management code as consistent as possible.

Should also check for `IDWALK_CB_USER_ONE` and call `id_us_ensure_real` in such case. Although probably not effectively needed here, would rather keep ID management code as consistent as possible.
id_us_plus(id_local);
id_lib_extern(id_local);
}
id_lib_extern(id_local);
}
ideasman42 marked this conversation as resolved Outdated
Bastien Montagne commented 2023-06-01 10:35:03 +02:00
Outdated
Review
Copy Link

This block does not make sense to me, why only handle animdata if the material has an embedded nodetree?

This block does not make sense to me, why only handle animdata if the material has an embedded nodetree?
Campbell Barton commented 2023-06-01 12:04:46 +02:00
Outdated
Review
Copy Link

This is correct but I can see why it seems strange.

  • The nodetree in the clipboard never has animation data (ensured by the code which copies).
  • The nodetree being overwritten might have animation data.
  • If the nodetree in the clipboard is NULL, then we loose this materials animation data (as the node-tree is cleared).
  • If the local node-tree has animation data, then it can be transfered to the new node tree.

We could create a node-tree for the purpose of keeping the animation data around, I don't have such a strong opinion on this though.

Either way, I'll update comment.

This is correct but I can see why it seems strange. - The nodetree in the clipboard never has animation data (ensured by the code which copies). - The nodetree being overwritten might have animation data. - If the nodetree in the clipboard is NULL, then we loose this materials animation data (as the node-tree is cleared). - If the local node-tree has animation data, then it can be transfered to the new node tree. We could create a node-tree for the purpose of keeping the animation data around, I don't have such a strong opinion on this though. Either way, I'll update comment.
Bastien Montagne commented 2023-06-01 12:20:03 +02:00
Outdated
Review
Copy Link

But here you are swapping the animation data of the materials, not of their nodetrees?

But here you are swapping the animation data of the materials, not of their nodetrees?
Campbell Barton commented 2023-06-01 12:43:35 +02:00
Outdated
Review
Copy Link

Ugh, your right, I'll push an update soon.

Ugh, your right, I'll push an update soon.
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) {
ideasman42 marked this conversation as resolved Outdated
Bastien Montagne commented 2023-06-01 14:55:14 +02:00
Outdated
Review
Copy Link

Do not use ma as name here, it shadows ma declared at beginning of the function... and makes the code fairly confusing to read. ma_iter?

Do not use `ma` as name here, it shadows `ma` declared at beginning of the function... and makes the code fairly confusing to read. `ma_iter`?
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. */
ideasman42 marked this conversation as resolved Outdated
Bastien Montagne commented 2023-06-01 10:40:14 +02:00
Outdated
Review
Copy Link

Please use proper callback for non-trivial logic, this is way too long to belong to such lamda syntax.

Please use proper callback for non-trivial logic, this is way too long to belong to such lamda syntax.
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<Material *>(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);