Geometry Nodes: Link drag search value moving improvement #107470

Merged

Hans Goudey merged 7 commits from mod_moder/blender:tmp_add_other_value_option_to_dnd_moving into main

2023-05-02 18:50:42 +02:00

Iliya Katushenock commented

2023-04-29 20:12:51 +02:00

Member

This PR based on #107461.
Now, when grag and drop going be used for create new input value node, it aslo
be moved socket value in to node storage. This means, by design, that result of
evaluation on node tree will don't be change.

Right now a few nodes (in shader in many cases) implemented without node
storage, but by storing value in socket itself. Float value input node unsupported
in this way too.

For data blocks, the original value is set to null, thus avoiding the existence of unused pointers.

This PR based on https://projects.blender.org/blender/blender/pulls/107461. Now, when grag and drop going be used for create new input value node, it aslo be moved socket value in to node storage. This means, by design, that result of evaluation on node tree will don't be change. Right now a few nodes (in shader in many cases) implemented without node storage, but by storing value in socket itself. Float value input node unsupported in this way too. For data blocks, the original value is set to null, thus avoiding the existence of unused pointers. <image src="https://projects.blender.org/attachments/40bd8c66-1ce5-4cd1-a454-fe250f7fb4bb"> <image src="https://projects.blender.org/attachments/a1f08b61-8e3b-4802-866d-246e785cff83"> <image src="https://projects.blender.org/attachments/c600fb88-8096-4941-8e88-a7cee38f2596"> <image src="https://projects.blender.org/attachments/d3643bc5-7a0d-4a89-9787-19363e13705e">

2023-04-29-21-18-20.gif

794 KiB

2023-04-29-21-18-01.gif

1.2 MiB

2023-04-29-21-20-11.gif

1.8 MiB

2023-04-29-21-19-12.gif

1.3 MiB

👍 1 ❤️ 2 🚀 2

Iliya Katushenock added the

label 2023-04-29 20:12:51 +02:00

Iliya Katushenock added 3 commits 2023-04-29 20:12:56 +02:00

486d200783 init

54b17286f3 Merge branch 'main' into tmp_cleanup_split_value_move_function

19a7a167e5 init

Iliya Katushenock requested review from Hans Goudey 2023-04-29 20:13:07 +02:00

Iliya Katushenock added this to the Nodes & Physics project 2023-04-29 20:13:11 +02:00

Nika Kutsniashvili commented

2023-04-30 13:16:49 +02:00

Contributor

Will this work for all node editors?

Will this work for all node editors?

Iliya Katushenock commented

2023-04-30 13:33:02 +02:00

Author

Member

Will this work for all node editors?

At the moment, some of the nodes are implemented, it seems to me strange logic. They store their values in sockets, not node storage data blocks. It's not very consistent, which is the reason I went around them. This means that all shader nodes are, at the very least, unsupported. Hopefully this can change in the near future.

> Will this work for all node editors? At the moment, some of the nodes are implemented, it seems to me strange logic. They store their values in sockets, not node storage data blocks. It's not very consistent, which is the reason I went around them. This means that all shader nodes are, at the very least, unsupported. Hopefully this can change in the near future.

Nika Kutsniashvili commented

2023-04-30 14:10:28 +02:00

Contributor

Shader Node Editor hasn't gotten any updates that Geometry Nodes Editor got. It is seemingly abandoned at the moment and developers are hesitating to work on it over difficulty of the task of the rewrite. I hope this changes soon. Shader UI is really lacking and incosistencies between node editors weakens the UX of entire Blender.

Shader Node Editor hasn't gotten any updates that Geometry Nodes Editor got. It is seemingly abandoned at the moment and developers are hesitating to work on it over difficulty of the task of the rewrite. I hope this changes soon. Shader UI is really lacking and incosistencies between node editors weakens the UX of entire Blender.

Hans Goudey reviewed 2023-05-01 00:01:23 +02:00

source/blender/blenkernel/intern/node.cc Outdated

					
				@ -2488,3 +2543,1 @@

				  for (ID **dst_value : dst_values) {

				    *dst_value = *src_socket_value;

				    id_us_plus(*dst_value);

				  void **src_value = static_cast<void **>(socket_value_storage(src));

Hans Goudey commented

2023-04-30 23:59:13 +02:00

Member

Why cast to void ** here? Seems like void * would be fine.

Why cast to `void **` here? Seems like `void *` would be fine.

mod_moder marked this conversation as resolved

source/blender/blenkernel/intern/node.cc Outdated

					
				@ -2495,0 +2555,4 @@

				   * C++ equivalent:

				   *   src_value = {};

				   */

				  BUFFER_FOR_CPP_TYPE_VALUE(src_type, src_zero_value);

Hans Goudey commented

2023-05-01 00:01:20 +02:00

Member

What about this:

src_type.destruct(src_value);
src_type.value_initialize(src_value);

What about this: ```cpp src_type.destruct(src_value); src_type.value_initialize(src_value); ```

mod_moder marked this conversation as resolved

Iliya Katushenock added 2 commits 2023-05-01 08:20:00 +02:00

5283ee0ed5 Merge branch 'main' into tmp_add_other_value_option_to_dnd_moving

99b5d8082e progress

Hans Goudey requested changes 2023-05-01 14:59:48 +02:00

source/blender/blenkernel/intern/node.cc Outdated

					
				@ -2429,0 +2440,4 @@

				  switch (node.type) {

				    case FN_NODE_INPUT_BOOL: {

				      return &reinterpret_cast<NodeInputBool *>(node.storage)->boolean;

				    }

Hans Goudey commented

2023-05-01 14:57:58 +02:00

Member

IMO the brackets don't really help here, they just make the function take more space. They aren't necessary since you aren't declaring any variables in the cases. Same with below.

IMO the brackets don't really help here, they just make the function take more space. They aren't necessary since you aren't declaring any variables in the cases. Same with below.

mod_moder marked this conversation as resolved

source/blender/blenkernel/intern/node.cc Outdated

					
				@ -2492,3 +2548,2 @@

				  id_us_min(*src_socket_value);

				  *src_socket_value = nullptr;

				  convert.convert_to_uninitialized(src_type, dst_type, src_value, dst_value);

Hans Goudey commented

2023-05-01 14:59:35 +02:00

Member

I think convert_to_initialized might be correct here actually, since the destination is already a valid socket value, it will already be initialized. Not that it makes a difference right now...

I think `convert_to_initialized` might be correct here actually, since the destination is already a valid socket value, it will already be initialized. Not that it makes a difference right now...

Iliya Katushenock commented

2023-05-01 15:10:55 +02:00

Author

Member

Right now we no have a convert_to_initialized and i not sure if that make some difference now too. (This case really simple)

Right now we no have a `convert_to_initialized` and i not sure if that make some difference now too. (This case really simple)

Hans Goudey commented

2023-05-01 15:53:31 +02:00

Member

Oh, my bad, I searched and fount convert_to_initialized_n but missed the _n. Indeed it doesn't matter here currently.

Oh, my bad, I searched and fount `convert_to_initialized_n` but missed the `_n`. Indeed it doesn't matter here currently.

mod_moder marked this conversation as resolved

Iliya Katushenock added 1 commit 2023-05-01 15:42:48 +02:00

f7c3574216 progress

Iliya Katushenock requested review from Hans Goudey 2023-05-01 15:43:00 +02:00

Hans Goudey approved these changes 2023-05-01 15:54:08 +02:00

Hans Goudey left a comment

Member

I'll still test this before committing it, but the code looks good.

I'll still test this before committing it, but the code looks good.

Iliya Katushenock commented

2023-05-01 15:58:55 +02:00

Author

Member

Yes, thank for reviews and testing Hans.

Yes, thank for reviews and testing Hans.

Hans Goudey requested changes 2023-05-01 22:23:26 +02:00

Hans Goudey left a comment

Member

This is really nice! A great usability improvement! A few things:

A noticed a problem with the "value" node coming from an integer socket (see video below)
Also in the video, it resets the size of the grid node to zero. Maybe it should reset to the default value from the node socket declaration instead.

This is really nice! A great usability improvement! A few things: - A noticed a problem with the "value" node coming from an integer socket (see video below) - Also in the video, it resets the size of the grid node to zero. Maybe it should reset to the default value from the node socket declaration instead.

Screencast from 2023-05-01 16-21-09.webm

267 KiB

source/blender/blenkernel/intern/node.cc Outdated

					
				@ -2429,0 +2455,4 @@

				  return nullptr;

				}

				void *socket_value_storage(bNodeSocket &socket)

Hans Goudey commented

2023-05-01 22:19:03 +02:00

Member

/home/hans/blender-git/blender/source/blender/blenkernel/intern/node.cc:2434:7: warning: no previous declaration for ‘void* blender::bke::node_static_value_storage_for(bNode&, const bNodeSocket&)’ [-Wmissing-declarations]
 2434 | void *node_static_value_storage_for(bNode &node, const bNodeSocket &socket)
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/hans/blender-git/blender/source/blender/blenkernel/intern/node.cc:2458:7: warning: no previous declaration for ‘void* blender::bke::socket_value_storage(bNodeSocket&)’ [-Wmissing-declarations]
 2458 | void *socket_value_storage(bNodeSocket &socket)
      |       ^~~~~~~~~~~~~~~~~~~~

``` /home/hans/blender-git/blender/source/blender/blenkernel/intern/node.cc:2434:7: warning: no previous declaration for ‘void* blender::bke::node_static_value_storage_for(bNode&, const bNodeSocket&)’ [-Wmissing-declarations] 2434 | void *node_static_value_storage_for(bNode &node, const bNodeSocket &socket) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/hans/blender-git/blender/source/blender/blenkernel/intern/node.cc:2458:7: warning: no previous declaration for ‘void* blender::bke::socket_value_storage(bNodeSocket&)’ [-Wmissing-declarations] 2458 | void *socket_value_storage(bNodeSocket &socket) | ^~~~~~~~~~~~~~~~~~~~ ```

mod_moder marked this conversation as resolved

source/blender/blenkernel/intern/node.cc Outdated

					
				@ -2429,0 +2479,4 @@

				    case SOCK_MATERIAL:

				      return &socket.default_value_typed<bNodeSocketValueMaterial>()->value;

				    case SOCK_STRING:

				      break;

Hans Goudey commented

2023-05-01 22:19:34 +02:00

Member

Seems simpler to return null here as a way to not support strings, rather than checking again below:

    case SOCK_STRING:
      /* Strings are not supported here. */
      return nullptr;

Seems simpler to return null here as a way to not support strings, rather than checking again below: ``` case SOCK_STRING: /* Strings are not supported here. */ return nullptr; ```

mod_moder marked this conversation as resolved

Hans Goudey changed title from ~~Geometry Nodes: Drag and drop value moving improvement~~ to Geometry Nodes: Link drag search value moving improvement

2023-05-01 22:23:51 +02:00

Iliya Katushenock commented

2023-05-01 22:30:01 +02:00

Author

Member

A noticed a problem with the "value" node coming from an integer socket (see video below)

Yeah, i mention this little above. Right now a few nodes (in shader in many cases) implemented without node storage, but by storing value in socket itself.
I planning to offer comming soon some plans about refactoring this places for reduce legacy design moments.

> A noticed a problem with the "value" node coming from an integer socket (see video below) Yeah, i mention this little above. Right now a few nodes (in shader in many cases) implemented without node storage, but by storing value in socket itself. I planning to offer comming soon some plans about refactoring this places for reduce legacy design moments.

Hans Goudey commented

2023-05-01 22:32:35 +02:00

Member

Ah I see, makes sense now, thanks.

Ah I see, makes sense now, thanks.

Iliya Katushenock added 1 commit 2023-05-01 23:01:10 +02:00

7fd4323a4c progress

Iliya Katushenock requested review from Hans Goudey 2023-05-01 23:03:36 +02:00

Hans Goudey approved these changes 2023-05-02 18:32:05 +02:00

Hans Goudey referenced this issue from a commit

2023-05-02 18:50:42 +02:00

Nodes: Improve link drag search value moving

Hans Goudey merged commit 71e4f48180 into main

2023-05-02 18:50:42 +02:00

Iliya Katushenock deleted branch tmp_add_other_value_option_to_dnd_moving

2023-05-02 19:00:23 +02:00

Howard Trickey referenced this issue from a commit

2023-05-29 02:51:42 +02:00

Merge in orign/main:

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#107470

No description provided.