Cleanup: Refactor Switch View node to use dynamic declarations #110042

Weikang-Qiu · 2023-07-13T04:43:29+02:00

Weikang-Qiu commented

2023-07-13 04:43:29 +02:00

Use new api (declare_dynamic) for swich view node, avoid modifying sockets directly.

See [#108728](https://projects.blender.org/blender/blender/issues/108728). Use new api (`declare_dynamic`) for swich view node, avoid modifying sockets directly.

Weikang-Qiu added 3 commits 2023-07-13 04:43:41 +02:00

a8a3535f53 refractor of switch view node

de947d2f18 clean

a1ad809b77 add hidden label

Weikang-Qiu added 1 commit 2023-07-13 04:47:20 +02:00

015e0612ac clean head file

Iliya Katushenock added the

Interest

Compositing

label 2023-07-13 19:49:13 +02:00

Iliya Katushenock added this to the Nodes & Physics project 2023-07-13 19:49:21 +02:00

Iliya Katushenock requested review from Iliya Katushenock 2023-07-13 19:49:36 +02:00

Iliya Katushenock commented

2023-07-13 19:50:18 +02:00

I see that still wip, so tag me if you`re be ready / need help

Iliya Katushenock reviewed 2023-07-13 19:52:28 +02:00

source/blender/nodes/NOD_node_declaration.hh Outdated

						
				@ -153,6 +153,7 @@ class SocketDeclaration {

				  /** Defined by whether the socket is part of the node's input or

				   * output socket declaration list. Included here for convenience. */

				  eNodeSocketInOut in_out;

				  bool is_hidden = false;

Iliya Katushenock commented

2023-07-13 19:52:28 +02:00

Our task is to remove such sockets, not to hide them. The end goal is just declarations. If the socket is hidden, then the user did it. Not a declaration.

Weikang-Qiu commented

2023-07-14 02:43:15 +02:00

Hidden is for the situation that the user unchecks the view.
https://drive.google.com/file/d/1Esov4A0zbV-G-uFNHRLqW5PUTakhlyi6/view?usp=sharing

In the original implementation, they hide the node
https://projects.blender.org/blender/blender/src/commit/474b492b3893b8923563584d44dc3e5354767bfd/source/blender/nodes/composite/nodes/node_composite_switchview.cc#L75-L80

Could you explain more why if you still think we should delete it under such situation?

Hidden is for the situation that the user unchecks the view. [https://drive.google.com/file/d/1Esov4A0zbV-G-uFNHRLqW5PUTakhlyi6/view?usp=sharing](https://drive.google.com/file/d/1Esov4A0zbV-G-uFNHRLqW5PUTakhlyi6/view?usp=sharing) In the original implementation, they hide the node [https://projects.blender.org/blender/blender/src/commit/474b492b3893b8923563584d44dc3e5354767bfd/source/blender/nodes/composite/nodes/node_composite_switchview.cc#L75-L80](https://projects.blender.org/blender/blender/src/commit/474b492b3893b8923563584d44dc3e5354767bfd/source/blender/nodes/composite/nodes/node_composite_switchview.cc#L75-L80) Could you explain more why if you still think we should delete it under such situation?

Iliya Katushenock commented

2023-07-14 11:16:15 +02:00

Collections are now used as a way to organize light linking. If this were integrated into the compositor, would it be tedious to implement socket hiding for all four kinds of collection hiding?

The main task #108728 and #104126 is that if the socket is not visible, then it should not be. This:

Simplifies python access (see: #103395).
Removes invisible links (That still connected other sockets).
Allows you to process this through declarations and the possibilities that this gives.

For this reason, socket deleting is the norm.
If we don't want to lose the values of temporarily hidden layers, then we just need a mechanism to do so (as in Nodes: Temporary socket value save).

On the other hand, we have:

Node groups where sockets are removed.
Simulation nodes where the socket are removed.
Sockets in this node also change to an infinite number.

And on the another one hand, if the socket should be hidden, then what functionality does this give? What can it be used for?
This is just a legacy concept that previously used static arrays of sockets, and their creation took place in the manual mode of the opretaors.

_Collections are now used as a way to organize light linking. If this were integrated into the compositor, would it be tedious to implement socket hiding for all four kinds of collection hiding?_ The main task #108728 and #104126 is that if the socket is not visible, then it should not be. This: - Simplifies python access (see: #103395). - Removes invisible links (That still connected other sockets). - Allows you to process this through declarations and the possibilities that this gives. For this reason, socket deleting is the norm. If we don't want to lose the values of temporarily hidden layers, then we just need a mechanism to do so (as in [Nodes: Temporary socket value save](https://archive.blender.org/developer/D17136)). On the other hand, we have: - Node groups where sockets are removed. - Simulation nodes where the socket are removed. Sockets in this node also change to an infinite number. And on the another one hand, if the socket should be hidden, then what functionality does this give? What can it be used for? This is just a legacy concept that previously used static arrays of sockets, and their creation took place in the manual mode of the opretaors.

Weikang-Qiu commented

2023-07-14 13:50:15 +02:00

I see. Thanks for your explanation. Fixed it.

mod_moder marked this conversation as resolved

Weikang-Qiu changed title from ~~WIP: Refractor of Switch View Node~~ to Refractor of Switch View Node

2023-07-14 03:19:03 +02:00

Weikang-Qiu commented

2023-07-14 03:19:16 +02:00

WIP removed.

Weikang-Qiu added 1 commit 2023-07-14 13:49:26 +02:00

6a616a56ce delete sockets instead of hiding

Iliya Katushenock reviewed 2023-07-14 13:57:40 +02:00

source/blender/nodes/composite/nodes/node_composite_switchview.cc

						
				@ -42,3 +25,1 @@

				  bNodeSocket *sock = ntreeCompositSwitchViewAddSocket(ntree, node, "No View");

				  sock->flag |= SOCK_HIDDEN;

				  b.add_output<decl::Color>(N_("Image")).default_value({0.0f, 0.0f, 0.0f, 1.0f});

Iliya Katushenock commented

2023-07-14 13:57:40 +02:00

Unused?

Weikang-Qiu commented

2023-07-14 14:03:35 +02:00

damn my editor is not well configured. fixed.

Weikang-Qiu marked this conversation as resolved

Weikang-Qiu added 1 commit 2023-07-14 14:03:39 +02:00

2ceb5c7e25 remove unused

Iliya Katushenock reviewed 2023-07-14 14:05:38 +02:00

source/blender/nodes/composite/nodes/node_composite_switchview.cc Outdated

						
				@ -53,3 +27,1 @@

				    return;

				  }

				  Scene *scene = (Scene *)node.id;

Iliya Katushenock commented

2023-07-14 14:05:38 +02:00

Do not use C-style cast in C++ code. To cast void ptr to any other ptr type, use static_cast. See: https://wiki.blender.org/wiki/Style_Guide/C_Cpp

Iliya Katushenock commented

2023-07-14 14:08:31 +02:00

Also, if you think you have solved the problem, you can click Resolve conversation.

Weikang-Qiu marked this conversation as resolved

Weikang-Qiu force-pushed rewrite-node-switch-view from bb2eaf5883 to e1761d27d4

2023-07-14 14:16:06 +02:00

Compare

Weikang-Qiu force-pushed rewrite-node-switch-view from e1761d27d4 to baf25795ba

2023-07-14 14:16:48 +02:00

Compare

Iliya Katushenock requested changes 2023-07-14 14:23:22 +02:00

Iliya Katushenock left a comment

Thanks for work, a few comments. Haven't tested behavior yet. It makes sense to check how it works now. We can be sure that old files are opened correctly, as updated when editing, ..

source/blender/nodes/composite/nodes/node_composite_switchview.cc

						
				@ -54,3 +27,2 @@

				  }

				  Scene *scene = reinterpret_cast<Scene *>(node.id);

				  if (scene == nullptr) {

Iliya Katushenock commented

2023-07-14 14:22:01 +02:00

I'm not sure about this, but should we remove the output socket in this case?
I think it is required to comply with the current behavior in this regard (in pr description should be added info about current/new behavior too).

I'm not sure about this, but should we remove the output socket in this case? I think it is required to comply with the current behavior in this regard (in pr description should be added info about current/new behavior too).

Weikang-Qiu commented

2023-07-14 14:38:53 +02:00

I see. will take some time to think about it.

Weikang-Qiu marked this conversation as resolved

source/blender/nodes/composite/nodes/node_composite_switchview.cc

						
				@ -57,4 +29,1 @@

				    blender::bke::nodeRemoveAllSockets(ntree, node);

				    /* make sure there is always one socket */

				    cmp_node_switch_view_sanitycheck(ntree, node);

				    return;

Iliya Katushenock commented

2023-07-14 14:21:52 +02:00

Also, "No View" make sense to keep too in early retern.

Also, `"No View"` make sense to keep too in early retern.

Weikang-Qiu commented

2023-07-16 05:53:15 +02:00

I cannot figure out what "No View" is used for. I haven't seen any bug when there's no input sockets. Do you have any idea?

I cannot figure out what `"No View"` is used for. I haven't seen any bug when there's no input sockets. Do you have any idea?

Iliya Katushenock commented

2023-07-16 10:05:38 +02:00

It's more like just saving the behavior. If someone opens an old file, that socket should still be there.

Weikang-Qiu commented

2023-07-16 10:31:17 +02:00

"No View" is hidden in the old implementation... it will never be displayed?

`"No View"` is hidden in the old implementation... it will never be displayed?

Iliya Katushenock commented

2023-07-16 10:32:29 +02:00

Hm, then I'm not exactly sure what its purpose is, it seems strange if it's hidden all the time... yes, in that case it should just be deleted.

Weikang-Qiu commented

2023-07-16 10:46:13 +02:00

It is written by Dalai Felinto 8 years ago. Is it appropriate to ask him in the chat?

Iliya Katushenock commented

2023-07-16 10:51:49 +02:00

May, although there is little certainty that at least something else was remembered or it is relevant ...

Weikang-Qiu commented

2023-07-16 11:31:21 +02:00

Ok so let's just delete it.

Weikang-Qiu commented

2023-07-16 11:31:23 +02:00

Ok so let's just delete it.

Weikang-Qiu marked this conversation as resolved

source/blender/nodes/composite/nodes/node_composite_switchview.cc Outdated

						
				@ -108,3 +45,2 @@

				  Scene *scene = CTX_data_scene(C);

				  bNodeTree *ntree = (bNodeTree *)ptr->owner_id;

				  bNode *node = (bNode *)ptr->data;

				  bNode *node = reinterpret_cast<bNode *>(ptr->data);

Iliya Katushenock commented

2023-07-14 14:22:05 +02:00

I mean, new code should be kept clean.
Changes to the old should be moved to a separate cleanup.

I mean, new code should be kept clean. Changes to the old should be moved to a separate cleanup.

Weikang-Qiu commented

2023-07-14 14:31:06 +02:00

do you mean we should move the cleanup to a separate commit?

Iliya Katushenock commented

2023-07-14 14:33:33 +02:00

Yep (if you wanna do this). For now, changes in unrelated functions should not be included in pr.

Weikang-Qiu commented

2023-07-14 14:37:30 +02:00

I see. But actually this part is not purely old.The function was modified.

mod_moder marked this conversation as resolved

Weikang-Qiu force-pushed rewrite-node-switch-view from baf25795ba to d7650009e1

2023-07-14 14:27:02 +02:00

Compare

Weikang-Qiu added 2 commits 2023-07-16 11:30:58 +02:00

d3797e887b keep old C style code unchanged

ea56c6a6fa adjust logic when scene is nullptr

Weikang-Qiu added 1 commit 2023-07-16 11:33:20 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

7b99c0529b modify comments of init_switch_view

Weikang-Qiu commented

2023-07-16 11:36:39 +02:00

Thanks for work, a few comments. Haven't tested behavior yet. It makes sense to check how it works now. We can be sure that old files are opened correctly, as updated when editing, ..

I've manually tested some files in #72171 and haven't seen any error. Is there any further test we should make?

> Thanks for work, a few comments. Haven't tested behavior yet. It makes sense to check how it works now. We can be sure that old files are opened correctly, as updated when editing, .. I've manually tested some files in #72171 and haven't seen any error. Is there any further test we should make?

Iliya Katushenock approved these changes 2023-07-16 11:39:48 +02:00

Iliya Katushenock commented

2023-07-16 11:39:59 +02:00

It looks like it's ready. Will have to play around with this later to be sure. But for now, I'll mark it as done.
I know that Hans won't be here this week, so it'll probably have to wait for now.

It looks like it's ready. Will have to play around with this later to be sure. But for now, I'll mark it as done. I know that Hans won't be here this week, so it'll probably have to wait for now.

👍 1

Iliya Katushenock requested review from Hans Goudey 2023-07-16 11:40:06 +02:00

Hans Goudey commented

2023-08-22 01:04:40 +02:00

@blender-bot build

Hans Goudey reviewed 2023-08-22 01:18:01 +02:00

source/blender/nodes/composite/nodes/node_composite_switchview.cc Outdated

						
				@ -28,3 +23,1 @@

				static bNodeSocket *ntreeCompositSwitchViewAddSocket(bNodeTree *ntree,

				                                                     bNode *node,

				                                                     const char *name)

				static void node_declare_dynamic(const bNodeTree &ntree,

Hans Goudey commented

2023-08-22 01:18:01 +02:00

/home/hans/Blender-Git/blender/source/blender/nodes/composite/nodes/node_composite_switchview.cc:23:51: warning: unused parameter ‘ntree’ [-Wunused-parameter]
   23 | static void node_declare_dynamic(const bNodeTree &ntree,
      |                                  ~~~~~~~~~~~~~~~~~^~~~~

The warning can be silenced like this const bNodeTree & /*ntree*/,

``` /home/hans/Blender-Git/blender/source/blender/nodes/composite/nodes/node_composite_switchview.cc:23:51: warning: unused parameter ‘ntree’ [-Wunused-parameter] 23 | static void node_declare_dynamic(const bNodeTree &ntree, | ~~~~~~~~~~~~~~~~~^~~~~ ``` The warning can be silenced like this `const bNodeTree & /*ntree*/,`

Weikang-Qiu marked this conversation as resolved

Hans Goudey changed title from ~~Refractor of Switch View Node~~ to Cleanup: Refactor Switch View node to use dynamic declarations

2023-08-22 01:24:45 +02:00

Hans Goudey commented

2023-08-22 01:27:02 +02:00

This sort of does change behavior, as it makes the node update more often to the new views described in the stereoscopy panel in the property editor (for example it updates when connecting to another node, or when loading a file). I don't really think that's a bad thing, but having never used this node it's hard to say for sure. I wonder if we can get someone experienced with the compositor to weigh in.