Fix #120103: crash pasting geonodes referencing an ID into new file #120576

Closed

Philipp Oeser wants to merge 2 commits from lichtwerk/blender:120103 into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

Philipp Oeser commented

2024-04-12 15:54:16 +02:00

Member

The was already functionality for checking bNode.id when pasting
(reassigning IDs -- name-based -- when not in a new file), but we can
also have IDs referenced in sockets (e.g. SOCKET_OBJECT) and these were
not handled leading to crashes since the sockets were just duplicated.

Now also do what is done with bNode.id with IDs referenced in sockets.

NOTE: unsure if we should increment usercount here (but apparently
bNode.id also does not do it)
NOTE: there is a bit of duplicated code (but since assigning directly to
socket.default_value didnt update properly had to go through typed
socket values)
NOTE: introduces NodeClipboardItemIDInfo which could also be used for
the handling of bNode.id

The was already functionality for checking bNode.id when pasting (reassigning IDs -- name-based -- when not in a new file), but we can also have IDs referenced in sockets (e.g. SOCKET_OBJECT) and these were not handled leading to crashes since the sockets were just duplicated. Now also do what is done with bNode.id with IDs referenced in sockets. NOTE: unsure if we should increment usercount here (but apparently bNode.id also does not do it) NOTE: there is a bit of duplicated code (but since assigning directly to socket.default_value didnt update properly had to go through typed socket values) NOTE: introduces NodeClipboardItemIDInfo which could also be used for the handling of bNode.id

Philipp Oeser added 1 commit 2024-04-12 15:54:27 +02:00

c649457573 Fix #120103 : crash pasting geonodes referencing an ID into new file

The was already functionality for checking bNode.id when pasting
(reassigning IDs -- name-based -- when not in a new file), but we can
also have IDs referenced in sockets (e.g. SOCKET_OBJECT) and these were
not handled leading to crashes since the sockets were just duplicated.

Now also do what is done with bNode.id with IDs referenced in sockets.

NOTE: unsure if we should increment usercount here (but apparently
bNode.id also does not do it)
NOTE: there is a bit of duplicated code (but since assigning directly to
socket.default_value didnt update properly had to go through typed
socket values)
NOTE: introduces NodeClipboardItemIDInfo which could also be used for
the handling of bNode.id

Philipp Oeser added the

label 2024-04-12 15:54:40 +02:00

Philipp Oeser added this to the Nodes & Physics project 2024-04-12 15:54:46 +02:00

Philipp Oeser added the

label 2024-04-12 15:54:53 +02:00

Philipp Oeser requested review from Hans Goudey 2024-04-12 15:55:01 +02:00

Philipp Oeser requested review from Bastien Montagne 2024-04-12 15:55:14 +02:00

Philipp Oeser referenced this pull request

2024-04-12 15:56:07 +02:00

Pasting Geo Nodes referencing an ID (e.g. Set Material or Object Info) into new file crashes Blender #120103

Hans Goudey requested changes 2024-04-12 16:49:42 +02:00

Hans Goudey left a comment

Member

Overall this fix makes sense, but I'd suggest pushing the changes a bit further to reduce code duplication and boilerplate. Some ideas:

Add a local function that gives the ID ** pointer in a socket
Move the existing library information to use the new struct
Add utility functions to create the ID info struct and to remap an ID pointer later on

Overall this fix makes sense, but I'd suggest pushing the changes a bit further to reduce code duplication and boilerplate. Some ideas: - Add a local function that gives the `ID **` pointer in a socket - Move the existing library information to use the new struct - Add utility functions to create the ID info struct and to remap an ID pointer later on

Philipp Oeser added 1 commit 2024-04-12 17:34:02 +02:00

f13c2f0d17 Move the existing library information to use the new struct

Bastien Montagne requested changes 2024-04-12 17:47:43 +02:00

Bastien Montagne left a comment

Owner

Yet another case of 'custom handling' of ID pointers... ;(

I think this fix would be a good opportunity to fix it properly - which means using the generic 'foreach_id' system.

From a quick look, node_foreach_id should be refactored to split out in a new function the handling of a single node, and that new function should be made public, for the node paste code to use it and handle all potential ID pointers (looks like current PR would at the very least still miss IDProperties ?).

I would also rather store, in the clipboard data, a mapping of all ID pointers found (something like orig_id_pointer -> (id_name, id_library, optional_new_id_pointer)). That way, handling of ID pointers can be fully generic, and lookup based on ID name/library path can be done only once. Further more, the generic foreach_id code will gives info, like whether the currently processed pointer should affect usercounts or not, etc.

Note that I don't think we have other use-cases of using the foreach_id system on a sub-part of an ID, this may require some work to get it working properly...

Yet another case of 'custom handling' of ID pointers... ;( I think this fix would be a good opportunity to fix it properly - which means using the generic 'foreach_id' system. From a quick look, `node_foreach_id` should be refactored to split out in a new function the handling of a single node, and that new function should be made public, for the node paste code to use it and handle _all_ potential ID pointers (looks like current PR would at the very least still miss IDProperties ?). I would also rather store, in the clipboard data, a mapping of all ID pointers found (something like `orig_id_pointer -> (id_name, id_library, optional_new_id_pointer)`). That way, handling of ID pointers can be fully generic, and lookup based on ID name/library path can be done only once. Further more, the generic foreach_id code will gives info, like whether the currently processed pointer should affect usercounts or not, etc. Note that I don't think we have other use-cases of using the foreach_id system on a sub-part of an ID, this may require some work to get it working properly...

Philipp Oeser commented

2024-04-12 17:54:45 +02:00

Author

Member

Sounds like a bigger project :) but yeah, will give it a go next week...

Sounds like a bigger project :) but yeah, will give it a go next week...

Philipp Oeser commented

2024-04-26 13:34:15 +02:00

Author

Member

Will close in favor of !121122

Will close in favor of !121122

Philipp Oeser closed this pull request

2024-04-26 13:34:18 +02:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

3 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#120576

No description provided.