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

Merged
Hans Goudey merged 12 commits from Weikang-Qiu/blender:rewrite-node-switch-view into main 2023-08-23 16:35:42 +02:00
Contributor

See #108728.

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
Weikang-Qiu added 1 commit 2023-07-13 04:47:20 +02:00
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

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

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
@ -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;

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.

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.
Author
Contributor

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?

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.
Author
Contributor

I see. Thanks for your explanation. Fixed it.

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
Author
Contributor

WIP removed.

WIP removed.
Weikang-Qiu added 1 commit 2023-07-14 13:49:26 +02:00
Iliya Katushenock reviewed 2023-07-14 13:57:40 +02:00
@ -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});

Unused?

Unused?
Author
Contributor

damn my editor is not well configured. fixed.

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
Iliya Katushenock reviewed 2023-07-14 14:05:38 +02:00
@ -53,3 +27,1 @@
return;
}
Scene *scene = (Scene *)node.id;

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

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

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

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
Member

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, ..

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, ..
@ -54,3 +27,2 @@
}
Scene *scene = reinterpret_cast<Scene *>(node.id);
if (scene == nullptr) {

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).
Author
Contributor

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

I see. will take some time to think about it.
Weikang-Qiu marked this conversation as resolved
@ -57,4 +29,1 @@
blender::bke::nodeRemoveAllSockets(ntree, node);
/* make sure there is always one socket */
cmp_node_switch_view_sanitycheck(ntree, node);
return;

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

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

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?

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

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

"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?

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.

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.
Author
Contributor

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

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

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

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

Ok so let's just delete it.

Ok so let's just delete it.
Author
Contributor

Ok so let's just delete it.

Ok so let's just delete it.
Weikang-Qiu marked this conversation as resolved
@ -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);

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.
Author
Contributor

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

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

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

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

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

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
Weikang-Qiu added 1 commit 2023-07-16 11:33:20 +02:00
modify comments of init_switch_view
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
7b99c0529b
Author
Contributor

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

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.
Iliya Katushenock requested review from Hans Goudey 2023-07-16 11:40:06 +02:00
Member

@blender-bot build

@blender-bot build
Hans Goudey reviewed 2023-08-22 01:18:01 +02:00
@ -28,3 +23,1 @@
static bNodeSocket *ntreeCompositSwitchViewAddSocket(bNodeTree *ntree,
bNode *node,
const char *name)
static void node_declare_dynamic(const bNodeTree &ntree,
Member
/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
Member

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.

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.

In theory, we should be ready to this, since declarations should apply to all nodes?

In theory, we should be ready to this, since declarations should apply to all nodes?
Weikang-Qiu added 1 commit 2023-08-22 01:57:32 +02:00
Hans Goudey added 1 commit 2023-08-23 15:51:59 +02:00
Merge branch 'main' into rewrite-node-switch-view
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
6501f8481c
Member

@blender-bot build

@blender-bot build
Member

@blender-bot build

@blender-bot build
Hans Goudey approved these changes 2023-08-23 16:35:01 +02:00
Hans Goudey merged commit 5dd9e57878 into main 2023-08-23 16:35:42 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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 Milestone
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#110042
No description provided.