Refractor: Compositor: Render layer node to dynamic declaration #114629

Open
Amine Bensalem wants to merge 2 commits from HamilcarR/blender:nodes-108728-cmp_node_r_layers into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
5 changed files with 66 additions and 197 deletions

View File

@ -58,9 +58,18 @@ void RenderLayersNode::test_render_link(NodeConverter &converter,
}
for (NodeOutput *output : get_output_sockets()) {
NodeImageLayer *storage = (NodeImageLayer *)output->get_bnode_socket()->storage;
const char *socket_name = output->get_bnode_socket()->name;
const char *selected_render_pass;
if (STREQ(socket_name, "Image") ||
HamilcarR marked this conversation as resolved Outdated

Not sure what is the point of doing this?

Not sure what is the point of doing this?
  1. NodeImageLayer is no longer needed for socket storages as far as I understand , so I just removed it .
  2. The socket identifier part is to check if that socket can be used for the "RE_PASSNAME_COMBINED" (sorry , forgot this define existed) ... the earlier version was doing roughly the same thing , using a NodeImageLayer as socket storage.
    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" ...
1) NodeImageLayer is no longer needed for socket storages as far as I understand , so I just removed it . 2) The socket identifier part is to check if that socket can be used for the "RE_PASSNAME_COMBINED" (sorry , forgot this define existed) ... the earlier version was doing roughly the same thing , using a NodeImageLayer as socket storage. 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 :

const char *socket_name = output->get_bnode_socket()->name ; 

const char *pass_name = socket_name ; 

if(STREQ(socket_name , "Alpha") || STREQ(socket_name , "Image")){
   
     pass_name = RE_PASSNAME_COMBINED ; 
}
RenderPass *rpass = (RenderPass *)BLI_findstring(
  &rl->passes, pass_name, offsetof(RenderPass, name));
In this case , what do you think would be the best way of doing things ? Maybe : ```Cpp const char *socket_name = output->get_bnode_socket()->name ; const char *pass_name = socket_name ; if(STREQ(socket_name , "Alpha") || STREQ(socket_name , "Image")){ pass_name = RE_PASSNAME_COMBINED ; } RenderPass *rpass = (RenderPass *)BLI_findstring( &rl->passes, pass_name, offsetof(RenderPass, name)); ```
STREQ(socket_name, "Alpha"))
{
selected_render_pass = RE_PASSNAME_COMBINED;
}
else {
selected_render_pass = socket_name;
}
RenderPass *rpass = (RenderPass *)BLI_findstring(
&rl->passes, storage->pass_name, offsetof(RenderPass, name));
&rl->passes, selected_render_pass, offsetof(RenderPass, name));
if (rpass == nullptr) {
missing_socket_link(converter, output);
continue;
HamilcarR marked this conversation as resolved Outdated

Unrelated format change?

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.

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?

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

`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

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

View File

@ -637,11 +637,14 @@ void ED_node_composit_default(const bContext *C, Scene *sce)
in->locy = 200.0f;
blender::bke::node_set_active(sce->nodetree, in);
/* Sockets aren't created before a call to update bmain, which leads to both Render Layers and Composite nodes to have their link missing.*/
BKE_ntree_update_main(CTX_data_main(C), nullptr);
Review

it is a bit strange to update an entire bmain. it also kind of suggests scripts which used to create node setups in the similar way to the ED_node_composit_default are no longer working?

it is a bit strange to update an entire `bmain`. it also kind of suggests scripts which used to create node setups in the similar way to the `ED_node_composit_default` are no longer working?

I guess here can be two nullptr arguments, not sure. In case node tree is edited we have to update everything that is depends on the node tree, so this is correct. Like in versioning, we do not call depsgraph update, think at the point of creating default version of the node tree this also might be such as there is no need to actually flush depsgraph to re-evaluate this update tag.

I guess here can be two `nullptr` arguments, not sure. In case node tree is edited we have to update everything that is depends on the node tree, so this is correct. Like in versioning, we do not call depsgraph update, think at the point of creating default version of the node tree this also might be such as there is no need to actually flush depsgraph to re-evaluate this update tag.
/* Links from color to color. */
bNodeSocket *fromsock = (bNodeSocket *)in->outputs.first;
bNodeSocket *tosock = (bNodeSocket *)out->inputs.first;
blender::bke::node_add_link(sce->nodetree, in, fromsock, out, tosock);
if (fromsock && tosock) {
Review

Why extra nullptr checks? Did it cause issues? Is it some safety?
Better to avoid non-stricktly required changes for this PR.

Why extra `nullptr` checks? Did it cause issues? Is it some safety? Better to avoid non-stricktly required changes for this PR.
blender::bke::node_add_link(sce->nodetree, in, fromsock, out, tosock);
}
BKE_ntree_update_main_tree(CTX_data_main(C), sce->nodetree, nullptr);
}

View File

@ -26,13 +26,6 @@ struct ViewLayer;
extern blender::bke::bNodeTreeType *ntreeType_Composite;
void node_cmp_rlayers_outputs(bNodeTree *ntree, bNode *node);
void node_cmp_rlayers_register_pass(bNodeTree *ntree,
bNode *node,
Scene *scene,
ViewLayer *view_layer,
const char *name,
eNodeSocketDatatype type);
const char *node_cmp_rlayers_sock_to_pass(int sock_index);
void register_node_type_cmp_custom_group(blender::bke::bNodeType *ntype);

View File

@ -200,7 +200,7 @@ void ntreeCompositUpdateRLayers(bNodeTree *ntree)
for (bNode *node : ntree->all_nodes()) {
if (node->type == CMP_NODE_R_LAYERS) {
node_cmp_rlayers_outputs(ntree, node);
blender::nodes::update_node_declaration_and_sockets(*ntree, *node);
}
else if (node->type == CMP_NODE_CRYPTOMATTE &&
node->custom1 == CMP_NODE_CRYPTOMATTE_SOURCE_RENDER)

View File

@ -217,140 +217,14 @@ static void cmp_node_image_create_outputs(bNodeTree *ntree,
}
}

Unrelated change of comment (.).

Unrelated change of comment (`.`).

sorry I didn't see the update , this one slipped through indeed

sorry I didn't see the update , this one slipped through indeed
struct RLayerUpdateData {
LinkNodePair *available_sockets;
int prev_index;
};
void node_cmp_rlayers_register_pass(bNodeTree *ntree,
bNode *node,
Scene *scene,
ViewLayer *view_layer,
const char *name,
eNodeSocketDatatype type)
{
RLayerUpdateData *data = (RLayerUpdateData *)node->storage;
if (scene == nullptr || view_layer == nullptr || data == nullptr || node->id != (ID *)scene) {
return;
}
ViewLayer *node_view_layer = (ViewLayer *)BLI_findlink(&scene->view_layers, node->custom1);
if (node_view_layer != view_layer) {
return;
}
/* Special handling for the Combined pass to ensure compatibility. */
if (STREQ(name, RE_PASSNAME_COMBINED)) {
cmp_node_image_add_pass_output(
ntree, node, "Image", name, -1, type, true, data->available_sockets, &data->prev_index);
cmp_node_image_add_pass_output(ntree,
node,
"Alpha",
name,
-1,
SOCK_FLOAT,
true,
data->available_sockets,
&data->prev_index);
}
else {
cmp_node_image_add_pass_output(
ntree, node, name, name, -1, type, true, data->available_sockets, &data->prev_index);
}
}
struct CreateOutputUserData {
bNodeTree &ntree;
bNode &node;
};
static void cmp_node_rlayer_create_outputs_cb(void *userdata,
Scene *scene,
ViewLayer *view_layer,
const char *name,
int /*channels*/,
const char * /*chanid*/,
eNodeSocketDatatype type)
{
CreateOutputUserData &data = *(CreateOutputUserData *)userdata;
node_cmp_rlayers_register_pass(&data.ntree, &data.node, scene, view_layer, name, type);
}
static void cmp_node_rlayer_create_outputs(bNodeTree *ntree,
bNode *node,
LinkNodePair *available_sockets)
{
Scene *scene = (Scene *)node->id;
if (scene) {
RenderEngineType *engine_type = RE_engines_find(scene->r.engine);
if (engine_type && engine_type->update_render_passes) {
ViewLayer *view_layer = (ViewLayer *)BLI_findlink(&scene->view_layers, node->custom1);
if (view_layer) {
RLayerUpdateData *data = (RLayerUpdateData *)MEM_mallocN(sizeof(RLayerUpdateData),
"render layer update data");
data->available_sockets = available_sockets;
data->prev_index = -1;
node->storage = data;
CreateOutputUserData userdata = {*ntree, *node};
RenderEngine *engine = RE_engine_create(engine_type);
RE_engine_update_render_passes(
engine, scene, view_layer, cmp_node_rlayer_create_outputs_cb, &userdata);
RE_engine_free(engine);
if ((scene->r.mode & R_EDGE_FRS) &&
(view_layer->freestyle_config.flags & FREESTYLE_AS_RENDER_PASS))
{
node_cmp_rlayers_register_pass(
ntree, node, scene, view_layer, RE_PASSNAME_FREESTYLE, SOCK_RGBA);
}
MEM_freeN(data);
node->storage = nullptr;
return;
}
}
}
int prev_index = -1;
cmp_node_image_add_pass_output(ntree,
node,
"Image",
RE_PASSNAME_COMBINED,
RRES_OUT_IMAGE,
SOCK_RGBA,
true,
available_sockets,
&prev_index);
cmp_node_image_add_pass_output(ntree,
node,
"Alpha",
RE_PASSNAME_COMBINED,
RRES_OUT_ALPHA,
SOCK_FLOAT,
true,
available_sockets,
&prev_index);
}
/* 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) */
static void cmp_node_image_verify_outputs(bNodeTree *ntree, bNode *node)
{
bNodeSocket *sock, *sock_next;
LinkNodePair available_sockets = {nullptr, nullptr};

The comment can be removed now as resolved?

The comment can be removed now as resolved?
/* XXX make callback */
if (rlayer) {
cmp_node_rlayer_create_outputs(ntree, node, &available_sockets);
}
else {
cmp_node_image_create_outputs(ntree, node, &available_sockets);
}
cmp_node_image_create_outputs(ntree, node, &available_sockets);
/* Get rid of sockets whose passes are not available in the image.
* If sockets that are not available would be deleted, the connections to them would be lost
@ -376,7 +250,7 @@ static void cmp_node_image_verify_outputs(bNodeTree *ntree, bNode *node, bool rl
break;
}
}
if (!link && (!rlayer || sock_index >= NUM_LEGACY_SOCKETS)) {
if (!link && sock_index >= NUM_LEGACY_SOCKETS) {
MEM_freeN(sock->storage);
blender::bke::node_remove_socket(ntree, node, sock);
}
@ -395,7 +269,7 @@ static void cmp_node_image_update(bNodeTree *ntree, bNode *node)
{
/* avoid unnecessary updates, only changes to the image/image user data are of interest */
if (node->runtime->update & NODE_UPDATE_ID) {
cmp_node_image_verify_outputs(ntree, node, false);
cmp_node_image_verify_outputs(ntree, node);
}
cmp_node_update_default(ntree, node);
@ -410,7 +284,7 @@ static void node_composit_init_image(bNodeTree *ntree, bNode *node)
iuser->flag |= IMA_ANIM_ALWAYS;
/* setup initial outputs */
cmp_node_image_verify_outputs(ntree, node, false);
cmp_node_image_verify_outputs(ntree, node);
}
static void node_composit_free_image(bNode *node)
@ -521,11 +395,6 @@ void register_node_type_cmp_image()
/* **************** RENDER RESULT ******************** */
void node_cmp_rlayers_outputs(bNodeTree *ntree, bNode *node)
{
cmp_node_image_verify_outputs(ntree, node, true);
}
const char *node_cmp_rlayers_sock_to_pass(int sock_index)
{
if (sock_index >= NUM_LEGACY_SOCKETS) {
@ -538,23 +407,54 @@ const char *node_cmp_rlayers_sock_to_pass(int sock_index)
namespace blender::nodes::node_composite_render_layer_cc {
static void cmp_node_create_sockets(void *userdata,
Scene * /*scene*/,
ViewLayer * /*view_layer*/,
const char *name,
int /*channels*/,
const char * /*channel_id*/,
eNodeSocketDatatype type)
{
NodeDeclarationBuilder *builder = static_cast<NodeDeclarationBuilder *>(userdata);
if (!STREQ(name, RE_PASSNAME_COMBINED)) {
builder->add_output(type, name);
}
else {
builder->add_output<decl::Color>("Image");
builder->add_output<decl::Float>("Alpha");
}
}
static void node_rlayer_declare(NodeDeclarationBuilder &builder)
{
const bNode *node = builder.node_or_null();
if (node == nullptr || node->id == nullptr) {
return;
}
Scene *scene = reinterpret_cast<Scene *>(node->id);
RenderEngineType *engine_type = RE_engines_find(scene->r.engine);
RenderEngine *engine = RE_engine_create(engine_type);
ViewLayer *view_layer = (ViewLayer *)BLI_findlink(&scene->view_layers, node->custom1);
if (!view_layer) {
return;
}
RE_engine_update_render_passes(
engine, scene, view_layer, cmp_node_create_sockets, (void *)&builder);
RE_engine_free(engine);
if ((scene->r.mode & R_EDGE_FRS) &&
(view_layer->freestyle_config.flags & FREESTYLE_AS_RENDER_PASS))
{

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 name RE_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.

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 name `RE_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.

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 the void ED_node_composit_default(const bContext *C, Scene *sce) function to try retrieving sockets that are null 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.

> 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 the `void ED_node_composit_default(const bContext *C, Scene *sce)` function to try retrieving sockets that are `null` 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.
builder.add_output<decl::Color>(RE_PASSNAME_FREESTYLE, RE_PASSNAME_FREESTYLE);
}
}
static void node_composit_init_rlayers(const bContext *C, PointerRNA *ptr)
{
Scene *scene = CTX_data_scene(C);
bNode *node = (bNode *)ptr->data;
int sock_index = 0;
node->id = &scene->id;
id_us_plus(node->id);
for (bNodeSocket *sock = (bNodeSocket *)node->outputs.first; sock;
sock = sock->next, sock_index++)
{
NodeImageLayer *sockdata = MEM_cnew<NodeImageLayer>(__func__);
sock->storage = sockdata;
STRNCPY(sockdata->pass_name, node_cmp_rlayers_sock_to_pass(sock_index));
}
}
static bool node_composit_poll_rlayers(const blender::bke::bNodeType * /*ntype*/,
@ -586,38 +486,6 @@ static bool node_composit_poll_rlayers(const blender::bke::bNodeType * /*ntype*/
return true;
}
static void node_composit_free_rlayers(bNode *node)
{
/* free extra socket info */
LISTBASE_FOREACH (bNodeSocket *, sock, &node->outputs) {
if (sock->storage) {
MEM_freeN(sock->storage);
}
}
}
static void node_composit_copy_rlayers(bNodeTree * /*dst_ntree*/,
bNode *dest_node,
const bNode *src_node)
{
/* copy extra socket info */
const bNodeSocket *src_output_sock = (bNodeSocket *)src_node->outputs.first;
bNodeSocket *dest_output_sock = (bNodeSocket *)dest_node->outputs.first;
while (dest_output_sock != nullptr) {
dest_output_sock->storage = MEM_dupallocN(src_output_sock->storage);
src_output_sock = src_output_sock->next;
dest_output_sock = dest_output_sock->next;
}
}
static void cmp_node_rlayers_update(bNodeTree *ntree, bNode *node)
{
cmp_node_image_verify_outputs(ntree, node, true);
cmp_node_update_default(ntree, node);
}
static void node_composit_buts_viewlayers(uiLayout *layout, bContext *C, PointerRNA *ptr)
{
bNode *node = (bNode *)ptr->data;
@ -779,7 +647,7 @@ void register_node_type_cmp_rlayers()
static blender::bke::bNodeType ntype;
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 = file_ns::node_rlayer_declare;
ntype.draw_buttons = file_ns::node_composit_buts_viewlayers;
ntype.initfunc_api = file_ns::node_composit_init_rlayers;
ntype.poll = file_ns::node_composit_poll_rlayers;
@ -787,10 +655,6 @@ void register_node_type_cmp_rlayers()
ntype.realtime_compositor_unsupported_message = N_(
"Render passes not supported in the Viewport compositor");
ntype.flag |= NODE_PREVIEW;
blender::bke::node_type_storage(
&ntype, nullptr, file_ns::node_composit_free_rlayers, file_ns::node_composit_copy_rlayers);
ntype.updatefunc = file_ns::cmp_node_rlayers_update;
ntype.initfunc = node_cmp_rlayers_outputs;
blender::bke::node_type_size_preset(&ntype, blender::bke::eNodeSizePreset::Large);
blender::bke::node_register_type(&ntype);