blender
/
blender
128
396
Fork 568
Code Issues 6.2k Pull Requests 751 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 +207 -210
Campbell Barton commented 2023-06-01 09:26:26 +02:00
Owner
Copy Link

References to data-blocks in a material were stored in-memory and could crash if the data-blocks referenced by the material no longer existed when pasting.

Resolve by using a blend-file for material copy/paste, matching how the clipboard works in the 3D view-port.

Currently there is no support for including indirectly linked data-blocks when pasting the material.
Instead, data-blocks are restored by name, by inspecting the current file.

This also fixes a crash where the SpaceNode::nodetree could point to freed memory when pasting a material.

This PR implements basic copy/paste using a blend file in the temporary directory, while using name look-ups isn't ideal, functionality isn't lost for copy/pasting between materials in the same file. Pasting into files without those data-blocks causes them to be cleared instead of crashing.

References to data-blocks in a material were stored in-memory and could crash if the data-blocks referenced by the material no longer existed when pasting. Resolve by using a blend-file for material copy/paste, matching how the clipboard works in the 3D view-port. Currently there is no support for including indirectly linked data-blocks when pasting the material. Instead, data-blocks are restored by name, by inspecting the current file. This also fixes a crash where the `SpaceNode::nodetree` could point to freed memory when pasting a material. --- This PR implements basic copy/paste using a blend file in the temporary directory, while using name look-ups isn't ideal, functionality isn't lost for copy/pasting between materials in the same file. Pasting into files without those data-blocks causes them to be cleared instead of crashing.
Campbell Barton force-pushed pr-material-clipboard-as-blend from 184446ba8c to c5dd7815ef 2023-06-01 09:28:42 +02:00 Compare
Campbell Barton requested review from Bastien Montagne 2023-06-01 09:28:47 +02:00
Campbell Barton commented 2023-06-01 09:47:32 +02:00
Author
Owner
Copy Link

@mont29 would you be able to double check BKE_library_foreach_ID_link is being used correctly?

Since freeing embedded node-tree doesn't free it's user counts, I used BKE_library_foreach_ID_link to clear all ID references and decrement their user counts (otherwise pasting doesn't decrement user counts and often increases them - on data-block reuse).

What I'm not so sure of is that checking IDWALK_CB_USER alone is correct, it's possible other flags should be excluded - although as IDWALK_RECURSE isn't set, I would assume only ID's within the embedded node-tree are iterated over.

@mont29 would you be able to double check `BKE_library_foreach_ID_link` is being used correctly? Since freeing embedded node-tree doesn't free it's user counts, I used `BKE_library_foreach_ID_link` to clear all ID references and decrement their user counts (otherwise pasting doesn't decrement user counts and often increases them - on data-block reuse). What I'm not so sure of is that checking `IDWALK_CB_USER` alone is correct, it's possible other flags should be excluded - although as `IDWALK_RECURSE` isn't set, I would assume only ID's within the embedded node-tree are iterated over.
Campbell Barton force-pushed pr-material-clipboard-as-blend from c5dd7815ef to a0f4a5dffe 2023-06-01 09:52:48 +02:00 Compare
Bastien Montagne reviewed 2023-06-01 11:07:46 +02:00
Bastien Montagne left a comment
Owner
Copy Link

To answer to the question, yes, checking IDWALK_CB_USER should be enough to handle refcounted ID usages.

For the rest of the review, I can't say am very happy with how complicated and specific the logic of the pasting is, I have the feeling this could be way simpler/cleaner... But quickly reading existing code, it's already doing fairly dark things, so maybe it's OK. I also do not have enough time to fully understand all that pasting logic (and there is no real documentation of expected behavior when pasting the material afaict), so feel free to ignore the comments below if they do not make sense.

However, I have a strong feeling that current code is still far from being bullet-proofed regarding ID relationships management, so would really not recommend the change for 3.6. The existing code has been broken as it is since "forever", so people who use it know about it and how to deal with its limitations, while new code has a fair risk of introducing new behaviors and/or bugs imho.

To answer to the question, yes, checking `IDWALK_CB_USER` should be enough to handle refcounted ID usages. For the rest of the review, I can't say am very happy with how complicated and specific the logic of the pasting is, I have the feeling this could be way simpler/cleaner... But quickly reading existing code, it's already doing fairly dark things, so maybe it's OK. I also do not have enough time to fully understand all that pasting logic (and there is no real documentation of expected behavior when pasting the material afaict), so feel free to ignore the comments below if they do not make sense. However, I have a strong feeling that current code is still far from being bullet-proofed regarding ID relationships management, so would really not recommend the change for 3.6. The existing code has been broken as it is since "forever", so people who use it know about it and how to deal with its limitations, while new code has a fair risk of introducing new behaviors and/or bugs imho.
source/blender/editors/render/render_shading.cc Outdated
@ -2781,0 +2829,4 @@
}
/* Make sure data from this file is usable for material paste. */
if (!BLI_listbase_is_single(&temp_bmain->materials)) {
Bastien Montagne commented 2023-06-01 10:29:32 +02:00
Owner
Copy Link

Not sure about this check, a material could reference another one, in which case both will be written in the buffer .blend, and read back into temp_main?

Not sure about this check, a material could reference another one, in which case both will be written in the buffer .blend, and read back into `temp_main`?
Campbell Barton commented 2023-06-01 12:48:02 +02:00
Author
Owner
Copy Link

I checked on ways to link to materials from the shader and didn't see any - although someone could reference a material from an ID property, so we should support multiple materials.

I checked on ways to link to materials from the shader and didn't see any - although someone could reference a material from an ID property, so we should support multiple materials.
Campbell Barton commented 2023-06-01 12:48:02 +02:00
Author
Owner
Copy Link

I checked on ways to link to materials from the shader and didn't see any - although someone could reference a material from an ID property, so we should support multiple materials.

I checked on ways to link to materials from the shader and didn't see any - although someone could reference a material from an ID property, so we should support multiple materials.
ideasman42 marked this conversation as resolved
source/blender/editors/render/render_shading.cc Outdated
@ -2784,0 +2847,4 @@
}
/* Keep animation by moving local animation to the paste node-tree. */
if (ma->nodetree && ma_from->nodetree) {
Bastien Montagne commented 2023-06-01 10:35:03 +02:00
Owner
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
Author
Owner
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
Owner
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
Author
Owner
Copy Link

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

Ugh, your right, I'll push an update soon.
ideasman42 marked this conversation as resolved
source/blender/editors/render/render_shading.cc Outdated
@ -2784,0 +2882,4 @@
{
/* 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;
Bastien Montagne commented 2023-06-01 10:38:47 +02:00
Owner
Copy Link

Would rather see this logic replaced by BKE_lib_id_swap, with 'back-swapping' of data that needs to be kept. Or if not possible, have a comment why own custom logic is needed here.

Would rather see this logic replaced by `BKE_lib_id_swap`, with 'back-swapping' of data that needs to be kept. Or if not possible, have a comment why own custom logic is needed here.
Campbell Barton commented 2023-06-01 12:06:53 +02:00
Author
Owner
Copy Link

Agree this swapping is more involved than I'd like, probably simpler to swap all members we want to copy. Since swapping then keeping some gets a bit involved, while it reduces the chance we forget some members, most features are added as nodes these days anyway.

Agree this swapping is more involved than I'd like, probably simpler to swap all members we want to copy. Since swapping then keeping some gets a bit involved, while it reduces the chance we forget some members, most features are added as nodes these days anyway.
ideasman42 marked this conversation as resolved
source/blender/editors/render/render_shading.cc Outdated
@ -2784,0 +2907,4 @@
BKE_library_foreach_ID_link(
bmain,
&ma->nodetree->id,
[](LibraryIDLinkCallbackData *cb_data) -> int {
Bastien Montagne commented 2023-06-01 10:40:14 +02:00
Owner
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.
ideasman42 marked this conversation as resolved
source/blender/editors/render/render_shading.cc Outdated
@ -2784,0 +2928,4 @@
bmain,
IDWALK_NOP);
LISTBASE_FOREACH (bNode *, node, &ma->nodetree->nodes) {
Bastien Montagne commented 2023-06-01 10:41:30 +02:00
Owner
Copy Link

This looks useless?

This looks useless?
ideasman42 marked this conversation as resolved
source/blender/editors/render/render_shading.cc Outdated
@ -2784,0 +2936,4 @@
}
BKE_main_free(temp_bmain);
if (update_relations) {
Bastien Montagne commented 2023-06-01 10:51:58 +02:00
Owner
Copy Link

Would always tag deg for relations update tbh. IMHO it's very hard to be sure that they are not needed in such case.

Would always tag deg for relations update tbh. IMHO it's very hard to be sure that they are not needed in such case.
ideasman42 marked this conversation as resolved
Campbell Barton force-pushed pr-material-clipboard-as-blend from 0c6a1b4969 to 51ad2fe920 2023-06-01 12:55:59 +02:00 Compare
Campbell Barton added 1 commit 2023-06-01 13:55:57 +02:00
Campbell Barton commented 2023-06-01 13:57:08 +02:00
Author
Owner
Copy Link
  • Resolved multiple materials (tag the material, not fool proof but should be OK in practice).
  • Fixed user-count being increased on the target material.
- Resolved multiple materials (tag the material, not fool proof but should be OK in practice). - Fixed user-count being increased on the target material.
Bastien Montagne reviewed 2023-06-01 15:14:20 +02:00
Bastien Montagne left a comment
Owner
Copy Link

Generally looks 'as good as it can be' now, besides fairly minor notes below.

My main suggestion would be to use an ID flag instead of special IDProp to mark the 'copypasted ID (LIB_COPYPASTE`) ? Think it would be simpler, and easy to re-use in other ID types too as needed.

For the record, really unhappy with how low-level in ID management area this code has to go (weird extremely specific ID refcounting, remapping, un-assignment, etc.). In ideal/the longer term imho whole copy/paste system has to be refactored, to get a generic logic that can be used by any ID type, and specific handling for each type in callbacks in the IDTypeInfo structs... But that is way beyond the scope of this patch of course.

Generally looks 'as good as it can be' now, besides fairly minor notes below. My main suggestion would be to use an ID flag instead of special IDProp to mark the 'copypasted` ID (`LIB_COPYPASTE`) ? Think it would be simpler, and easy to re-use in other ID types too as needed. --------------------- For the record, really unhappy with how low-level in ID management area this code has to go (weird extremely specific ID refcounting, remapping, un-assignment, etc.). In ideal/the longer term imho whole copy/paste system has to be refactored, to get a generic logic that can be used by any ID type, and specific handling for each type in callbacks in the IDTypeInfo structs... But that is way beyond the scope of this patch of course.
source/blender/editors/render/render_shading.cc Outdated
@ -2769,0 +2823,4 @@
{
if (cb_data->cb_flag & IDWALK_CB_USER) {
id_us_min(*cb_data->id_pointer);
*cb_data->id_pointer = nullptr;
Bastien Montagne commented 2023-06-01 14:39:54 +02:00
Owner
Copy Link

Find it weird that this is only cleared in refcounting case? Any reason not to always clear that pointer?

Find it weird that this is only cleared in refcounting case? Any reason not to always clear that pointer?
Campbell Barton commented 2023-06-01 16:00:15 +02:00
Author
Owner
Copy Link

Good catch, should always be cleared.

Good catch, should always be cleared.
ideasman42 marked this conversation as resolved
source/blender/editors/render/render_shading.cc Outdated
@ -2769,0 +2843,4 @@
ID *id_local = static_cast<ID *>(
BLI_findstring(lb, (*id_p)->name + 2, offsetof(ID, name) + 2));
*id_p = id_local;
if (cb_data->cb_flag & IDWALK_CB_USER) {
Bastien Montagne commented 2023-06-01 15:02:48 +02:00
Owner
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.
ideasman42 marked this conversation as resolved
source/blender/editors/render/render_shading.cc Outdated
@ -2781,0 +2895,4 @@
/* 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) {
Bastien Montagne commented 2023-06-01 14:55:14 +02:00
Owner
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`?
ideasman42 marked this conversation as resolved
Campbell Barton force-pushed pr-material-clipboard-as-blend from 1c6f3d2916 to 2476a39a61 2023-06-01 15:56:48 +02:00 Compare
Campbell Barton force-pushed pr-material-clipboard-as-blend from 2476a39a61 to b230b1135f 2023-06-01 15:59:55 +02:00 Compare
Campbell Barton commented 2023-06-01 16:01:52 +02:00
Author
Owner
Copy Link
  • Use flag instead of property pointer (added blendfile_write_partial_clear_flags to handle tagging & flagging in one pass).
  • Clear pointer (even without users).
  • IDWALK_CB_USER_ONE handling.
  • Other minor changes based on feedback.
- Use flag instead of property pointer (added `blendfile_write_partial_clear_flags` to handle tagging & flagging in one pass). - Clear pointer (even without users). - IDWALK_CB_USER_ONE handling. - Other minor changes based on feedback.
Campbell Barton force-pushed pr-material-clipboard-as-blend from b230b1135f to f3e3f1cb12 2023-06-01 16:12:54 +02:00 Compare
Bastien Montagne approved these changes 2023-06-01 16:25:28 +02:00
Campbell Barton commented 2023-06-02 07:01:15 +02:00
Author
Owner
Copy Link

Committed 5177e2f20b. Closing.

Committed 5177e2f20b37eb5c97f0ed989febd7f783ae4f1f. Closing.
Campbell Barton closed this pull request 2023-06-02 07:01:16 +02:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Bastien Montagne
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset Browser

  
Interest
Asset Browser Project Overview

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
Blender Asset Bundle

  
Interest
BlendFile

  
Interest
Collada

  
Interest
Compatibility
This issue affects/is about backward or forward compatibility

  
Interest
Compositing

  
Interest
Core

  
Interest
Cycles

  
Interest
Dependency Graph

  
Interest
Development Management

  
Interest
EEVEE

  
Interest
EEVEE & Viewport

  
Interest
Freestyle

  
Interest
Geometry Nodes

  
Interest
Grease Pencil

  
Interest
ID Management

  
Interest
Images & Movies

  
Interest
Import Export

  
Interest
Line Art

  
Interest
Masking

  
Interest
Metal

  
Interest
Modeling

  
Interest
Modifiers

  
Interest
Motion Tracking

  
Interest
Nodes & Physics

  
Interest
OpenGL

  
Interest
Overlay

  
Interest
Overrides

  
Interest
Performance

  
Interest
Physics

  
Interest
Pipeline, Assets & IO

  
Interest
Platforms, Builds & Tests

  
Interest
Python API

  
Interest
Render & Cycles

  
Interest
Render Pipeline

  
Interest
Sculpt, Paint & Texture

  
Interest
Text Editor

  
Interest
Translations

  
Interest
Triaging

  
Interest
Undo

  
Interest
USD

  
Interest
User Interface

  
Interest
UV Editing

  
Interest
VFX & Video

  
Interest
Video Sequencer

  
Interest
Virtual Reality

  
Interest
Vulkan

  
Interest
Wayland
Issues relating to Wayland windowing support.

  
Interest
Workbench

  
Interest: X11

Issues relating to Xorg/X11 support.

  
Legacy
Blender 2.8 Project

  
Legacy
Milestone 1: Basic, Local Asset Browser

  
Legacy
OpenGL Error

  
Meta
Good First Issue

  
Meta
Papercut

  
Meta
Retrospective

  
Meta
Security
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
Module
Animation & Rigging

  
Module
Core

  
Module
Development Management

  
Module
EEVEE & Viewport

  
Module
Grease Pencil

  
Module
Modeling

  
Module
Nodes & Physics

  
Module
Pipeline, Assets & IO

  
Module
Platforms, Builds & Tests

  
Module
Python API

  
Module
Render & Cycles

  
Module
Sculpt, Paint & Texture

  
Module
Triaging

  
Module
User Interface

  
Module
VFX & Video

  
Platform
FreeBSD

  
Platform
Linux

  
Platform
macOS

  
Platform
Windows

  
Priority
High

  
Priority
Low

  
Priority
Normal

  
Priority
Unbreak Now!

  
Status
Archived

  
Status
Confirmed

  
Status
Duplicate

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Known Issue

  
Type
Patch

  
Type
Report

  
Type
To Do

No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#108496
No description provided.
Delete Branch "ideasman42/blender:pr-material-clipboard-as-blend"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?