Compositor: Make split viewer a regular split node #114245

Habib Gahbiche · 2023-10-29T10:37:06+01:00

Habib Gahbiche commented

2023-10-29 10:37:06 +01:00

Behavior:

Changes:

Renamed Split Viewer Node to Split Node
Split Node is now under Utilities (similar to Switch node)
Versioning: split viewer from 4.0 and before is replaced with the new split node connected to a new viewer node.

Todo after submitting the patch:

Translation
Add regression tests for CPU and GPU compositors

Behavior: <img src="/attachments/bc2d9e9c-743a-4354-888a-299038518158" width="700" /> Changes: - Renamed Split Viewer Node to Split Node - Split Node is now under `Utilities` (similar to Switch node) - Versioning: split viewer from 4.0 and before is replaced with the new split node connected to a new viewer node. Todo after submitting the patch: - [x] Translation - [x] Add regression tests for CPU and GPU compositors

Screenshot 2023-11-11 at 17.51.46.jpg

245 KiB

Habib Gahbiche added 3 commits 2023-10-29 10:37:24 +01:00

428d1b51fc Compositor: make SplitViewer a regular split node

a2b45d7d62 Merge remote-tracking branch 'origin/main' into com-split-node

22ab96d42f Renamed missed comments

Habib Gahbiche changed title from ~~Compositor: Make split viewer a regular split node~~ to WIP: Compositor: Make split viewer a regular split node

2023-10-29 10:50:45 +01:00

Omar Emara commented

2023-10-30 08:39:58 +01:00

I am aware this is still a WIP, but the domain of the node should no longer be dependent on the render region size. For the realtime compositor, to correct that, the compute_domain method should just be removed.

I am aware this is still a WIP, but the domain of the node should no longer be dependent on the render region size. For the realtime compositor, to correct that, the `compute_domain` method should just be removed.

Habib Gahbiche commented

2023-10-31 21:04:58 +01:00

@OmarEmaraDev I see thanks.

I am not sure how to unify behaviors here. For me, the tiled/full frame behavior seems more reasonable because factor = 50% always cuts both inputs in half, whereas for GPU compositor seems to compute the 50% threshold based on the output size, not the input image sizes.

I'm curious... Does computing the domain based on the input size and the output size align well with the GPU compositor Domain design?

@OmarEmaraDev I see thanks. I am not sure how to unify behaviors here. For me, the tiled/full frame behavior seems more reasonable because `factor = 50%` always cuts both inputs in half, whereas for GPU compositor seems to compute the 50% threshold based on the output size, not the input image sizes. I'm curious... Does computing the domain based on the input size _and_ the output size align well with the GPU compositor `Domain` design?

Habib Gahbiche referenced this pull request

2023-10-31 21:09:14 +01:00

Compositor: canvas and transforms #108944

Omar Emara commented

2023-10-31 21:45:07 +01:00

Does computing the domain based on the input size and the output size align well with the GPU compositor Domain design?

What do you mean by the output size here? The render size?

> Does computing the domain based on the input size and the output size align well with the GPU compositor Domain design? What do you mean by the output size here? The render size?

Habib Gahbiche commented

2023-10-31 21:52:02 +01:00

Yes, I mean render or viewer size (where viewer size can be different from final render size - at least for full frame compositor)

Omar Emara commented

2023-11-01 09:48:22 +01:00

The viewer size is determined by the domain of the input to the viewer node, so making the domain of a node depend on the viewer size doesn't really make sense. You can, however, depend on the render size as well as the inputs. This is how the Ellipse and Box Mask nodess work for instance. See their compute_domain method.
My opinion is that the node is no longer a viewer node, it is just a normal node and should not get special treatment or operate in the domain of the render size.
Realistically, the node is typically used when both inputs have a similar size, so we should not care much about special cases where the images have significantly different domains as you demonstrated in your examples.

- The viewer size is determined by the domain of the input to the viewer node, so making the domain of a node depend on the viewer size doesn't really make sense. You can, however, depend on the render size as well as the inputs. This is how the Ellipse and Box Mask nodess work for instance. See their `compute_domain` method. - My opinion is that the node is no longer a viewer node, it is just a normal node and should not get special treatment or operate in the domain of the render size. - Realistically, the node is typically used when both inputs have a similar size, so we should not care much about special cases where the images have significantly different domains as you demonstrated in your examples.

Sergey Sharybin added this to the Compositing project 2023-11-01 10:27:28 +01:00

Habib Gahbiche commented

2023-11-01 10:45:40 +01:00

Thanks for clarifying :)

My opinion is that the node is no longer a viewer node, it is just a normal node and should not get special treatment or operate in the domain of the render size.

I agree here, I was just wondering about the design in general.

Realistically, the node is typically used when both inputs have a similar size, so we should not care much about special cases where the images have significantly different domains as you demonstrated in your examples.

That might be the case, but I think it makes sense to unify behaviors when possible, especially between viewport and final render.

Thanks for clarifying :) > My opinion is that the node is no longer a viewer node, it is just a normal node and should not get special treatment or operate in the domain of the render size. I agree here, I was just wondering about the design in general. > Realistically, the node is typically used when both inputs have a similar size, so we should not care much about special cases where the images have significantly different domains as you demonstrated in your examples. That might be the case, but I think it makes sense to unify behaviors when possible, especially between viewport and final render.

Habib Gahbiche added 4 commits 2023-11-01 11:07:02 +01:00

dd57489759 Rename variables

5ef7561580 Compute domain automatically

2a2129eb3e Merge remote-tracking branch 'origin/main' into com-split-node

b99b67b440 Larger image decides canvas size

Omar Emara commented

2023-11-01 12:05:50 +01:00

That might be the case, but I think it makes sense to unify behaviors when possible, especially between viewport and final render.

Definitely. I was talking about the 50% difference you mentioned in your previous comment.

> That might be the case, but I think it makes sense to unify behaviors when possible, especially between viewport and final render. Definitely. I was talking about the 50% difference you mentioned in your previous comment.

Sergey Sharybin commented

2023-11-08 14:00:14 +01:00

To me it does not seem that the missing translation is an intrinsic part of this PR.

There is already plenty of calls Image *ima = BKE_image_ensure_viewer(bmain, IMA_TYPE_COMPOSITE, "Viewer Node"); which does not interact with the translation mechanism. I think those should become BKE_image_ensure_viewer(bmain, IMA_TYPE_COMPOSITE, IFACE_("Viewer Node")), (and similar thing for the render result). But that's a bigger change, and best to not mix it with the split node implementation.

To me it does not seem that the missing translation is an intrinsic part of this PR. There is already plenty of calls `Image *ima = BKE_image_ensure_viewer(bmain, IMA_TYPE_COMPOSITE, "Viewer Node");` which does not interact with the translation mechanism. I think those should become `BKE_image_ensure_viewer(bmain, IMA_TYPE_COMPOSITE, IFACE_("Viewer Node"))`, (and similar thing for the render result). But that's a bigger change, and best to not mix it with the split node implementation.

Habib Gahbiche added 3 commits 2023-11-09 23:40:38 +01:00

6cf3cb6d82 Realtime compositor: align behavior with fullframe compositor

- Larger image decides domain size
- Viewport and viewer node behave the same way

d9e51043a5 Merge remote-tracking branch 'origin/main' into com-split-node

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

ea06326932 Remove unnecessary variables

Habib Gahbiche commented

2023-11-09 23:44:47 +01:00

@blender-bot build

Habib Gahbiche added 1 commit 2023-11-11 17:58:31 +01:00

29400b157c Align behavior of GPU and fullframe compositors

Habib Gahbiche added 1 commit 2023-11-12 10:09:36 +01:00

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

c406392ab5 Support versioning

Habib Gahbiche commented

2023-11-12 10:12:21 +01:00

@blender-bot build

Habib Gahbiche referenced this pull request

2023-11-12 11:34:00 +01:00

Fullframe compositor: unify behavior of size inference with realtime-compositor #114755

Habib Gahbiche changed title from ~~WIP: Compositor: Make split viewer a regular split node~~ to Compositor: Make split viewer a regular split node

2023-11-12 11:34:40 +01:00

Habib Gahbiche requested review from Sergey Sharybin 2023-11-12 11:34:57 +01:00

Habib Gahbiche requested review from Omar Emara 2023-11-12 11:34:57 +01:00

Habib Gahbiche reviewed 2023-11-12 11:38:26 +01:00

source/blender/blenloader/intern/versioning_400.cc Outdated

						
				@ -419,0 +471,4 @@

				    FOREACH_NODETREE_BEGIN (bmain, ntree, id) {

				      if (ntree->type == NTREE_COMPOSIT) {

				        versioning_replace_splitviewer(ntree);

Habib Gahbiche commented

2023-11-12 11:38:26 +01:00

Not sure this belongs here inside do_versions_after_linking_400() or inside blo_do_versions_400() or in both. From testing it doesn't seem to make any difference.

Not sure this belongs here inside `do_versions_after_linking_400()` or inside `blo_do_versions_400()` or in both. From testing it doesn't seem to make any difference.

Omar Emara commented

2023-11-15 10:23:58 +01:00

Put it in blo_do_versions_400, so that we can do the stuff I mentioned above.

Put it in `blo_do_versions_400`, so that we can do the stuff I mentioned above.

zazizizou marked this conversation as resolved

Habib Gahbiche reviewed 2023-11-12 11:39:20 +01:00

source/blender/nodes/composite/nodes/node_composite_split.cc

						
				@ -0,0 +1,144 @@

				/* SPDX-FileCopyrightText: 2006 Blender Authors

Habib Gahbiche commented

2023-11-12 11:39:20 +01:00

It looks like a new file has been added but it has just been renamed. Do we update the copyright date in this case?

Omar Emara commented

2023-11-15 10:08:44 +01:00

I would just update that.

Omar Emara commented

2023-11-13 11:41:34 +01:00

If input images have different sizes (by area), then output size is equal to the larger image.

This seems to be wrong. The GPU compositor uses the domain of the first linked input, the Full Frame Compositor was also designed like this from the start, so this is how it should be done.

> If input images have different sizes (by area), then output size is equal to the larger image. This seems to be wrong. The GPU compositor uses the domain of the first linked input, the Full Frame Compositor was also designed like this from the start, so this is how it should be done.

Habib Gahbiche commented

2023-11-13 20:36:38 +01:00

The GPU compositor uses the domain of the first linked input, the Full Frame Compositor was also designed like this from the start,.

This doesn't make sense to me for though, at least for this node. But looking more into the other nodes (especially mix/overlay node) I can see this behavior is different. I will update it in the next patch.

> The GPU compositor uses the domain of the first linked input, the Full Frame Compositor was also designed like this from the start,. This doesn't make sense to me for though, at least for this node. But looking more into the other nodes (especially mix/overlay node) I can see this behavior is different. I will update it in the next patch.

Habib Gahbiche added 2 commits 2023-11-13 21:01:48 +01:00

c061eaef9f Revert to original behavior: make first image decide the canvas size

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

5147126fa6 Merge remote-tracking branch 'origin/main' into com-split-node

Habib Gahbiche commented

2023-11-13 21:02:42 +01:00

@blender-bot build

Omar Emara requested changes 2023-11-15 10:32:28 +01:00

source/blender/blenloader/intern/versioning_400.cc Outdated

						
				@ -327,0 +336,4 @@

				      continue;

				    }

				    bNodeType *node_type_info = new bNodeType();

				    node->typeinfo = node_type_info;

Omar Emara commented

2023-11-15 10:26:25 +01:00

Change the type of the node directly. Most of other code can be removed then.

STRNCPY(node->idname, "CompositorNodeSplit");
node->type = CMP_NODE_SPLIT;
MEM_freeN(node->storage);

Change the type of the node directly. Most of other code can be removed then. ``` STRNCPY(node->idname, "CompositorNodeSplit"); node->type = CMP_NODE_SPLIT; MEM_freeN(node->storage); ```

Habib Gahbiche commented

2023-11-16 12:33:50 +01:00

I'm not sure how to achieve that. node doesn't have an output socket so linking to viewer node doesn't work. So I took version_geometry_nodes_replace_transfer_attribute_node() from versioning_300.cc as a reference.

I'm not sure how to achieve that. `node` doesn't have an output socket so linking to viewer node doesn't work. So I took `version_geometry_nodes_replace_transfer_attribute_node()` from `versioning_300.cc` as a reference.

Omar Emara commented

2023-11-16 12:45:48 +01:00

Can you try adding the socket using nodeAddStaticSocket?

Can you try adding the socket using `nodeAddStaticSocket`?

Habib Gahbiche commented

2023-11-16 13:04:33 +01:00

Change type, manually free storage and add output socket.... isn't deleting and constructing a new node cleaner at this point? :)

Omar Emara commented

2023-11-16 13:07:33 +01:00

You would still need to manually delete the existing node and its storage. But the main advantage is that you will not have to relink the inputs, transfer the data, or mess with typeinfo; which is most of the versioning code you have now.

Habib Gahbiche commented

2023-11-16 13:57:35 +01:00

Ok, I don't mind either way

zazizizou marked this conversation as resolved

source/blender/compositor/nodes/COM_SplitNode.cc Outdated

						
				@ -0,0 +20,4 @@

				  SplitOperation *split_operation = new SplitOperation();

				  split_operation->set_split_percentage(node->custom1);

				  split_operation->set_xsplit(!node->custom2);

Omar Emara commented

2023-11-15 10:16:32 +01:00

Prefer explicit comparison with the enum.

zazizizou marked this conversation as resolved

source/blender/nodes/NOD_static_types.h Outdated

						
				@ -169,3 +169,3 @@

				DefNode(CompositorNode, CMP_NODE_CHANNEL_MATTE,  def_cmp_channel_matte,  "CHANNEL_MATTE",  ChannelMatte,     "Channel Key",       ""              )

				DefNode(CompositorNode, CMP_NODE_FLIP,           def_cmp_flip,           "FLIP",           Flip,             "Flip",              ""              )

				DefNode(CompositorNode, CMP_NODE_SPLITVIEWER,    def_cmp_splitviewer,    "SPLITVIEWER",    SplitViewer,      "Split Viewer",      ""              )

				DefNode(CompositorNode, CMP_NODE_SPLIT,          def_cmp_split,          "SPLITNODE",      Split,            "Split Node",        ""              )

Omar Emara commented

2023-11-15 10:11:21 +01:00

Omit "Node" from the names.

zazizizou marked this conversation as resolved

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

						
				@ -101,7 +101,7 @@ static void local_merge(Main *bmain, bNodeTree *localtree, bNodeTree *ntree)

				  LISTBASE_FOREACH (bNode *, lnode, &localtree->nodes) {

				    if (bNode *orig_node = nodeFindNodebyName(ntree, lnode->name)) {

				      if (ELEM(lnode->type, CMP_NODE_VIEWER, CMP_NODE_SPLITVIEWER)) {

Omar Emara commented

2023-11-15 10:10:56 +01:00

Replace ELEM by a simple comparison, same for the rest of the patch.

Replace `ELEM` by a simple comparison, same for the rest of the patch.

zazizizou marked this conversation as resolved

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

						
				@ -0,0 +33,4 @@

				static void node_composit_init_split(bNodeTree * /*ntree*/, bNode *node)

				{

				  ImageUser *iuser = MEM_cnew<ImageUser>(__func__);

Omar Emara commented

2023-11-15 10:08:07 +01:00

Remove storage.

zazizizou marked this conversation as resolved

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

						
				@ -0,0 +113,4 @@

				  static bNodeType ntype;

				  cmp_node_type_base(&ntype, CMP_NODE_SPLIT, "Split Node", NODE_CLASS_LAYOUT);

Omar Emara commented

2023-11-15 10:09:25 +01:00

Omit "Node" from the name. NODE_CLASS_LAYOUT seems wrong here, maybe just NODE_CLASS_CONVERTER or NODE_CLASS_OP_FILTER.

Omit "Node" from the name. `NODE_CLASS_LAYOUT` seems wrong here, maybe just `NODE_CLASS_CONVERTER` or `NODE_CLASS_OP_FILTER`.

Habib Gahbiche commented

2023-11-16 12:37:22 +01:00

CONVERTER and FILTER don't sound right either... Switch node also uses NODE_CLASS_LAYOUT. Do you suggest I change that as well (in a separate patch) ?

`CONVERTER` and `FILTER` don't sound right either... Switch node also uses `NODE_CLASS_LAYOUT`. Do you suggest I change that as well (in a separate patch) ?

Omar Emara commented

2023-11-16 12:40:40 +01:00

Switch node could pass as a layout node to me, it doesn't really process anything.
Classes don't make sense for many nodes, so I wouldn't worry too much about it.

Switch node could pass as a layout node to me, it doesn't really process anything. Classes don't make sense for many nodes, so I wouldn't worry too much about it.

zazizizou marked this conversation as resolved

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

						
				@ -0,0 +118,4 @@

				  ntype.draw_buttons = file_ns::node_composit_buts_split;

				  ntype.flag |= NODE_PREVIEW;

				  ntype.initfunc = file_ns::node_composit_init_split;

				  node_type_storage(&ntype, "ImageUser", node_free_standard_storage, node_copy_standard_storage);

Omar Emara commented

2023-11-15 10:07:38 +01:00

The node storage can be removed.

zazizizou marked this conversation as resolved

Habib Gahbiche added 1 commit 2023-11-16 14:01:41 +01:00

05ba55801d Address review comments: remove node storage and remove node from name

Omar Emara requested changes 2023-11-16 15:09:09 +01:00

source/blender/blenkernel/BKE_node.h Outdated

						
				@ -1019,2 +1019,3 @@

				#define CMP_NODE_FLIP 239

				#define CMP_NODE_SPLITVIEWER 240

				#define CMP_NODE_SPLITVIEWER__DEPRECATED \

				  240 /* Split viewer node is now a regular split node: CMP_NODE_SPLIT */

Omar Emara commented

2023-11-16 15:06:56 +01:00

Put the comment above the define. I know other places do it the current way, but that shouldn't be.

zazizizou marked this conversation as resolved

source/blender/blenkernel/intern/node.cc Outdated

						
				@ -3451,7 +3450,7 @@ void ntreeSetOutput(bNodeTree *ntree)

				      if (ELEM(node->type, CMP_NODE_OUTPUT_FILE, GEO_NODE_VIEWER)) {

				        continue;

				      }

				      const bool node_is_output = ELEM(node->type, CMP_NODE_VIEWER, CMP_NODE_SPLITVIEWER);

Omar Emara commented

2023-11-16 15:05:30 +01:00

There are still uses of ELEM here and in many other places.

There are still uses of `ELEM` here and in many other places.

Habib Gahbiche commented

2023-11-17 11:57:38 +01:00

ELEM is used in other places for single element comparison. I'm leaving out node_gizmo.cc, to better conform with the coding style rule "When making changes, conform to the style and conventions of the surrounding code."

`ELEM` is used in other places for single element comparison. I'm leaving out `node_gizmo.cc`, to better conform with the coding style rule "When making changes, conform to the style and conventions of the surrounding code."

Omar Emara commented

2023-11-17 12:12:49 +01:00

That file also uses simple comparisons for single values, so I don't think this is a convention really. But don't have a strong opinion, you can leave it as is.

Habib Gahbiche commented

2023-11-17 12:26:01 +01:00

👍

Habib Gahbiche added 3 commits 2023-11-17 12:26:23 +01:00

eeb8d74a88 Coding style

62baa54b70 Merge remote-tracking branch 'origin/main' into com-split-node

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

87e0925f4c Fix build errors

Habib Gahbiche commented

2023-11-17 12:26:36 +01:00

@blender-bot build

Omar Emara commented

2023-11-17 12:28:59 +01:00

It seems there are some bad formatting, can you clang format?

Habib Gahbiche added 1 commit 2023-11-17 12:40:54 +01:00

26a0397b0d Make Format

Omar Emara approved these changes 2023-11-21 17:23:01 +01:00

Omar Emara left a comment

Looks good, except for those two comments.

source/blender/blenloader/intern/versioning_400.cc

						
				@ -328,0 +337,4 @@

				    STRNCPY(node->idname, "CompositorNodeSplit");

				    node->type = CMP_NODE_SPLIT;

				    MEM_freeN(node->storage);

Omar Emara commented

2023-11-21 17:03:38 +01:00

node->storage = nullptr;

``` node->storage = nullptr; ```

OmarEmaraDev marked this conversation as resolved

source/blender/blenloader/intern/versioning_400.cc

						
				@ -328,0 +342,4 @@

				    bNode *viewer_node = nodeAddStaticNode(nullptr, ntree, CMP_NODE_VIEWER);

				    /* Nodes are created stacked on top of each other, so separate them a bit. */

				    viewer_node->locx = node->locx + node->width + viewer_node->width / 4.0f;

				    viewer_node->locy = node->locy;

Omar Emara commented

2023-11-21 17:20:16 +01:00

viewer_node->flag &= ~NODE_PREVIEW;

``` viewer_node->flag &= ~NODE_PREVIEW; ```

OmarEmaraDev marked this conversation as resolved

Habib Gahbiche added 1 commit 2023-12-03 22:34:51 +01:00

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

e21fd61ad1 Merge remote-tracking branch 'origin/main' into com-split-node

Habib Gahbiche commented

2023-12-03 22:35:28 +01:00

@blender-bot build

Habib Gahbiche merged commit 153f14be2b into main

2023-12-03 23:20:55 +01:00

Habib Gahbiche referenced this issue from a commit

2023-12-03 23:20:56 +01:00

Compositor: Make split viewer a regular split node

Habib Gahbiche deleted branch com-split-node

2023-12-03 23:20:56 +01:00

Omar Emara commented

2023-12-04 10:42:14 +01:00

@zazizizou My two comments above weren't handled.

Omar Emara commented

2023-12-04 12:26:38 +01:00

I fixed the comments in main.

Brian Savery (AMD) referenced this issue from a commit

2023-12-28 20:07:42 +01:00

Compositor: Make split viewer a regular split node

Daniel Salazar commented

2024-01-16 07:00:25 +01:00

The word Node in the name is redundant since all of them are nodes. It should be just Split.

Edit: Just built latest and it's corrected already :)

The word _Node_ in the name is redundant since all of them are nodes. It should be just _Split_. Edit: Just built latest and it's corrected already :)

Sign in to join this conversation.

No reviewers

No Label

				`@ -0,0 +1,144 @@`
				`/* SPDX-FileCopyrightText: 2006 Blender Authors`

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Compositor: Make split viewer a regular split node #114245