WIP: Refractor: Compositor: Render layer node to dynamic declaration #114629
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
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
4 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#114629
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "HamilcarR/blender:nodes-108728-cmp_node_r_layers"
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?
Implemented dynamic declarations for
CMP_NODE_R_LAYERS
.Also
CMP_NODE_R_LAYERS
andCMP_NODE_IMAGE
no longer share functions for socket creation and update .See: #108728
Thanks for working on that. Just to simplify next reviews and be ready to send this into the main, please always keep the code in correct format (See: https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Clang_Format).
This will be simple by always call
make format
in your root blender folder before commit changes.Sorry that i didn't noticed that. This file is mixing of two node with different functionality!
It's good to split this nodes after all changes for me. Since dynamic declaration will let us to delete a lot of legacy code related with editing and updating of sockets, there is will not so many shared code i hope.
For now, i clearly see that right now any storage is doesn't make any sense (due to the fact what that is done to reuse
Image
node code of sockets update what is actually use this socket storage structure) for this node.I'll have to ask @OmarEmaraDev to be sure (i am not the best one in the compositor computing).
I also wrote few comments about general things and format.
@ -688,0 +695,4 @@
eNodeSocketDatatype type)
{
NodeDeclarationBuilder *builder = static_cast<NodeDeclarationBuilder*>(userdata);
if(std::string(name) != std::string(RE_PASSNAME_COMBINED)){
StringRefNull
instead ofstd::string
@ -688,0 +698,4 @@
if(std::string(name) != std::string(RE_PASSNAME_COMBINED)){
switch(type){
case SOCK_FLOAT:
builder->add_output<decl::Float>(name);
builder->add_output(type, name);
Now this might works too.
@ -688,0 +717,4 @@
NodeDeclarationBuilder builder(r_declaration);
builder.add_output<decl::Color>("Image");
builder.add_output<decl::Float>("Alpha");
if(scene){
Better to always use early return pattern. See:
@ -788,3 +831,3 @@
row = uiLayoutRow(col, true);
uiItemR(row, ptr, "layer", UI_ITEM_R_SPLIT_EMPTY_NAME, "", ICON_NONE);
Unrelated change.
@mod_moder Hi , yes it's not ready for a merge yet , I just needed to check if this was going the right direction .
indeed , there's also the CMP_NODE_IMAGE that used to share functions with the render layer node ,but I think I can manage just splitting them apart each in their own respective files.
Although , last question , I tried playing around with node.storage ... is this structure's content writen on disk ? Maybe on a temp file , or a blend file after saving the session ?
DNA structures is C structs that is stored as binary image in undo copy of blend file and in real blend file on the disk, right.
If that is not pointer to other structure, everything should be happedn automaticly. You can find manuall processing of that structures in
ntreeBlendWrite
andntreeBlendReadData
functions.But now i pretty sure that new storage is not necessary.
Everything that should happen here is call of render engine to build sockets for layers. Previously shared code can be keep in places if other one node still use this. I just have to edit #108728 to ensure other one node to refactoring.
f2c7e34462
to87d5b54584
WIP: Refractor render layer node to dynamic declarationto Refractor render layer node to dynamic declarationRefractor render layer node to dynamic declarationto Compositor #108728 : Refractor render layer node to dynamic declaration@blender-bot build
Only blender organization members with write access can start builds. See documentation for details.
@ -61,1 +61,3 @@
NodeImageLayer *storage = (NodeImageLayer *)output->get_bnode_socket()->storage;
const char *socket_identifier = output->get_bnode_socket()->identifier;
if (strstr(socket_identifier, "Combined")) {
socket_identifier = "Combined";
Not sure what is the point of doing this?
There's probably a better way of doing this now that you point it out.
I gave identifiers to image and alpha sockets that are "Combined_i" and "Combined_a" ...
In this case , what do you think would be the best way of doing things ?
Maybe :
@ -70,2 +73,3 @@
if (STREQ(rpass->name, RE_PASSNAME_COMBINED) &&
STREQ(output->get_bnode_socket()->name, "Alpha")) {
STREQ(output->get_bnode_socket()->name, "Alpha"))
{
Unrelated format change?
this was clang-format ... I don't know why it corrected this line , and if I should let it do it's thing ... it's weird , because I use the exact same .clang-format as the official repo.
Can you try just
make format
instead?make format
raises an exception in the python wrapper :NotADirectoryError: [Errno 20] Not a directory: 'clang-format-8.0'
I'm currently trying to fix this
I have no idea why the script doesn't detect the clang-format binary in lib/ , I had to hard code the path into the script.
I'll fix that later .
I ran make format , added new commit for it.
Branch can be merged on my side
@ -913,3 +812,3 @@
cmp_node_type_base(&ntype, CMP_NODE_R_LAYERS, "Render Layers", NODE_CLASS_INPUT);
blender::bke::node_type_socket_templates(&ntype, nullptr, cmp_node_rlayers_out);
ntype.declare_dynamic = file_ns::node_rlayer_declare_dynamic;
Can you merge this with main? There was some refactor of this callback #113742 .
yep , it merges for me.
Have you merge this with latest main? Can you push it in to pr?
Hi @mod_moder , I just pushed the merged updated main.
Edit : you are right , there's a conflict because of the callback refractor , I'm gonna put a WIP back on the PR in the meantime.
Now it should be working . I updated the callback and renamed some stuff , should I re-merge main on the branch ?
Compositor #108728 : Refractor render layer node to dynamic declarationto Refractor: Compositor: Render layer node to dynamic declarationRefractor: Compositor: Render layer node to dynamic declarationto WIP: Refractor: Compositor: Render layer node to dynamic declarationWIP: Refractor: Compositor: Render layer node to dynamic declarationto WIP: Refractor: Compositor: Render layer node to dynamic declarationWIP: Refractor: Compositor: Render layer node to dynamic declarationto Refractor: Compositor: Render layer node to dynamic declaration@mod_moder Hi , I think this is finally ready . I tested it with most nodes it seems to be working as intended .
Do you want to test it on your side as well ?
Maybe should I add some unit testing ? if so , do you have advises on potential test cases ?
I'll just have to check this one this week, and we can move forward with this.
ok , let me know if you have questions
Thanks for await.
Just first first crash (that ocure only with this patch):
Use Nodes
button.Stack trace file is attached.
Hi , it seems there was a problem with the socket identifiers . Crash should be fixed now , according to my tests.
Also :
"Render passes not supported in the viewport compositor" , this isn't unexpected , right ?
Yeah, realtime compositor limitation.
fea3696a99
to5313e82181
My course work still require me to not relax and come back to blender, but i finally find the time to look in to this!
Sorry for necromancy, i just tested this as it seems this works good. Just few comments about general thing in a code add. I'll have to ask someone to check this in vfx side, but generally this is ready. Will check this again, before approve, after this will be approved in other side.
@ -701,7 +576,7 @@ void register_node_type_cmp_image()
void node_cmp_rlayers_outputs(bNodeTree *ntree, bNode *node)
I am wondering if that is possible to delete this function at all and replace all it's calls as declaration update directly.
Hey @mod_moder sorry for the (big) delay , had a bit on my plate lately...
I brought most modifications you requested , except this one ... this one is a tough cookie to crack :
I can't manage to find a proper and clean way to update the sockets declarations when checking render passes after deleting this function .
what happens is that after enabling a render pass , it isn't displayed on the node , but if I trigger other events like creating a link to a node , or launching a render , the node gets updated on the UI , and it displays the enabled render passes.
You say there's a way to do a declaration update ? should I deal with the
bNodeSockets
themselves in the declare function or would that be a hack ?I means, you can replace
node_cmp_rlayers_outputs
byupdate_node_declaration_and_sockets
in other files.Although it will probably be easier to do this as a separate cleanup.
I put the call to
update_node_declaration_and_sockets
inntreeCompositUpdateRLayers
What about declaration of
node_cmp_rlayers_outputs
?ah yes , good catch ...
I'll remove
node_cmp_rlayers_register_pass
as well since it's not needed anymore ?If there is no any other usage, then yeah
it's done
@ -719,0 +603,4 @@
if (!STREQ(name, RE_PASSNAME_COMBINED)) {
switch (type) {
case SOCK_FLOAT:
builder->add_output<decl::Float>(name);
After
f3a1dd1eb5
, it's possible to pass type of value as another runtime argument in toadd_output
method directly.@ -953,8 +851,6 @@ void register_node_type_cmp_rlayers()
ntype.flag |= NODE_PREVIEW;
node_type_storage(
&ntype, nullptr, file_ns::node_composit_free_rlayers, file_ns::node_composit_copy_rlayers);
Is both static functions is still necessary?
Stoop, why this function is still needed at all, there is no custom storage for now. Actually, this might cause memory leak right now?
Hello , no worries !
I'm currently having issues with my computer , the disk has it's FS corrupted for some reason , and I lost everything, I see you made some additional notes , I'll bring fixes for them this week.
Refractor: Compositor: Render layer node to dynamic declarationto WIP: Refractor: Compositor: Render layer node to dynamic declaration3a8196fc23
to2c97fc1669
2c97fc1669
to5313e82181
WIP: Refractor: Compositor: Render layer node to dynamic declarationto Refractor: Compositor: Render layer node to dynamic declaration@ -338,3 +218,2 @@
/* XXX make this into a generic socket verification function for dynamic socket replacement
* (multi-layer, groups, static templates). */
static void cmp_node_image_verify_outputs(bNodeTree *ntree, bNode *node, bool rlayer)
* (multi-layer, groups, static templates) */
Unrelated change of comment (
.
).sorry I didn't see the update , this one slipped through indeed
Description of PR now can be changed (see: https://developer.blender.org/docs/handbook/guidelines/commit_messages/).
@ -342,4 +222,4 @@
bNodeSocket *sock, *sock_next;
LinkNodePair available_sockets = {nullptr, nullptr};
/* XXX make callback */
The comment can be removed now as resolved?
@ -558,0 +444,4 @@
static void node_rlayer_declare(NodeDeclarationBuilder &builder)
{
builder.add_output<decl::Color>("Image");
The logic here seems to be relying on the fact that render engine always has Combined pass, and adds Image socket here, and Alpha later on when
cmp_node_create_sockets
is called with pass nameRE_PASSNAME_COMBINED
.It is not really always the case that render engine has to have Combined pass. It is un-conditional for the current default render engines, as this pass is always required internally, but some external render engine can respect the enabled state of the Combined pass in the View Layer settings.
So to match the old code the patch needs to inly add Image and Alpha if there is Combined pass in the render engine.
Thank you for the feedback .
By this , do you mean checking the presence of the Combined pass as functionality , or just checking it's on/off state ?
Edit : So , I tested for a bit , by only adding image and alpha sockets when
RE_PASSNAME_COMBINED
is present , this causes thevoid ED_node_composit_default(const bContext *C, Scene *sce)
function to try retrieving sockets that arenull
because they haven't yet been created.I didn't see this have any other effect than not creating the default links between the render layer node and composite node ... yet.
If you have any advice on how to test this more thoroughly , I'd be interested.
Unfortunately, the default compositor setup is now invalid: the link between render layers node and the output is missing. This seems to be caused by the fact that in the old system the render layers node had init callback, which was verifying output sockets. Now there is no such callback, so sockets are not verified until
BKE_ntree_update_main_tree
is called.@HamilcarR Can you check if names and identifiers of sockets is the same in blender with and without this changes? (this can be done via python) If no, we probably need to add versioning to replace old identifiers of sockets in files from older versions by new convention. Node declaration update are treated different identifiers as different sockets and does not keep the links.
By pure curiosity , does the default link between the r layers node and the composite node sockets only for UI and user experience ( meaning you open the compositor , you have basic nodes ready) , or is there non obvious mechanisms and features , relying on the fact those two nodes are automatically linked by default ?
@HamilcarR On the behavior level it is more of an UX thing: if such a link was not added automatically it could lead to a confusing situation then enabling compositing nodes makes your render result black.
On the actual code side it might go deeper. I think currently when adding nodes from Python it is possible to add Render Layer node and add links to all passes immediately after. With feels like this change might affect of this behavior. Didn't have time to verify it myself yet, but is definitely something to keep in mind and not introduce breaking changes.
Refractor: Compositor: Render layer node to dynamic declarationto WIP: Refractor: Compositor: Render layer node to dynamic declarationCheckout
From your project repository, check out a new branch and test the changes.