Cleanup: Refactor Switch View node to use dynamic declarations #110042
No reviewers
Labels
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 project
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#110042
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "Weikang-Qiu/blender:rewrite-node-switch-view"
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?
See #108728.
Use new api (
declare_dynamic
) for swich view node, avoid modifying sockets directly.I see that still wip, so tag me if you`re be ready / need help
@ -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.
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?
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:
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:
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.
I see. Thanks for your explanation. Fixed it.
WIP: Refractor of Switch View Nodeto Refractor of Switch View NodeWIP removed.
@ -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?
damn my editor is not well configured. fixed.
@ -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
Also, if you think you have solved the problem, you can click Resolve conversation.
bb2eaf5883
toe1761d27d4
e1761d27d4
tobaf25795ba
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 see. will take some time to think about it.
@ -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.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.
"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.
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 ...
Ok so let's just delete it.
Ok so let's just delete it.
@ -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.
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.
I see. But actually this part is not purely old.The function was modified.
baf25795ba
tod7650009e1
I've manually tested some files in #72171 and haven't seen any error. Is there any further test we should make?
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.
@blender-bot build
@ -28,3 +23,1 @@
static bNodeSocket *ntreeCompositSwitchViewAddSocket(bNodeTree *ntree,
bNode *node,
const char *name)
static void node_declare_dynamic(const bNodeTree &ntree,
The warning can be silenced like this
const bNodeTree & /*ntree*/,
Refractor of Switch View Nodeto Cleanup: Refactor Switch View node to use dynamic declarationsThis 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?
@blender-bot build
@blender-bot build