Render: Skip writing output if no Composite node exists #117129

Merged
Omar Emara merged 5 commits from OmarEmaraDev/blender:skip-no-composite-write into main 2024-01-24 12:00:59 +01:00
Member

This patch changes the render pipeline such that it doesn't write the
render result if the render pipeline is a compositor one whose node tree
does not have an output Composite node.

This is done to avoid double outputs when users use the File Output node
for saving the results instead. Additionally, it fixes crashes and empty
outputs when the node tree does not reference any render layers and has
no Composite output.

The render pipeline early fails if neither a File Output or a Composite
output node exists, so an output is guaranteed to always exist.

Fixes #115983.

This patch changes the render pipeline such that it doesn't write the render result if the render pipeline is a compositor one whose node tree does not have an output Composite node. This is done to avoid double outputs when users use the File Output node for saving the results instead. Additionally, it fixes crashes and empty outputs when the node tree does not reference any render layers and has no Composite output. The render pipeline early fails if neither a File Output or a Composite output node exists, so an output is guaranteed to always exist. Fixes #115983.
Omar Emara added 1 commit 2024-01-15 12:55:24 +01:00
This patch changes the render pipeline such that it doesn't write the
render result if the render pipeline is a compositor one whose node tree
does not have an output Composite node.

This is done to avoid double outputs when users use the File Output node
for saving the results instead. Additionally, it fixes crashes and empty
outputs when the node tree does not reference any render layers and has
no Composite output.

The render pipeline early fails if neither a File Output or a Composite
output node exists, so an output is guaranteed to always exist.

Fixes #115983 and #57699.
Brecht Van Lommel was assigned by Omar Emara 2024-01-15 12:55:42 +01:00
Sergey Sharybin was assigned by Omar Emara 2024-01-15 12:55:42 +01:00
Brecht Van Lommel was unassigned by Omar Emara 2024-01-15 12:55:58 +01:00
Sergey Sharybin was unassigned by Omar Emara 2024-01-15 12:55:58 +01:00
Omar Emara requested review from Brecht Van Lommel 2024-01-15 12:56:10 +01:00
Omar Emara requested review from Sergey Sharybin 2024-01-15 12:56:10 +01:00

For me this raises design questions that came up in #57699.

The compositor is one step in the render pipeline. The output can go to a file, but it can also go to the image editor or feed into the sequencer. I'm not sure what the right way to think about this conceptually is.

Maybe it's, there's no Composite output, so therefore there is no render result image to pass on to the next step in the pipeline. If that next step is file output, there is nothing to write. If it's the image editor, there is nothing to display. If it's the sequencer, the strip will not generate an image but some other strip still might.

Have you checked what happens when you have a scene strip without a compositor node, but another strip that does add an image? Does it still write that, or does this change break it?

For me this raises design questions that came up in #57699. The compositor is one step in the render pipeline. The output can go to a file, but it can also go to the image editor or feed into the sequencer. I'm not sure what the right way to think about this conceptually is. Maybe it's, there's no Composite output, so therefore there is no render result image to pass on to the next step in the pipeline. If that next step is file output, there is nothing to write. If it's the image editor, there is nothing to display. If it's the sequencer, the strip will not generate an image but some other strip still might. Have you checked what happens when you have a scene strip without a compositor node, but another strip that does add an image? Does it still write that, or does this change break it?
Author
Member

@brecht I am not sure what this means:

but it can also go to the image editor.

Because if there is no Composite node, the compositor will not write anything to the IMA_TYPE_R_RESULT viewer image. Or is there some special way that the pipeline can do that itself from the written output?

I am looking into the sequencer bit now.

@brecht I am not sure what this means: > but it can also go to the image editor. Because if there is no Composite node, the compositor will not write anything to the `IMA_TYPE_R_RESULT` viewer image. Or is there some special way that the pipeline can do that itself from the written output? I am looking into the sequencer bit now.
Author
Member

Okay, so Scene strips suffer from the same crash issue when no composite output exist. But this is not related to this change and is orthogonal to it. I will submit a separate fix for it.

As for image viewer of render result, it will just show an empty image in that case, but it is also not related to this change, so if we want to change this behavior somehow, we can just do that in a separate branch.

Okay, so Scene strips suffer from the same crash issue when no composite output exist. But this is not related to this change and is orthogonal to it. I will submit a separate fix for it. As for image viewer of render result, it will just show an empty image in that case, but it is also not related to this change, so if we want to change this behavior somehow, we can just do that in a separate branch.
Sergey Sharybin requested changes 2024-01-15 14:33:47 +01:00
Sergey Sharybin left a comment
Owner

When there is no Composite output node the Composite render pass is not created, but the render result still contains all other passes, and they can be written. This change will break setup which were relying on the view layer passes (like combined) to be saved via render pipeline, and extra passes saved via File Output nodes.

Such setups might be used on the farms to have some easily viewable render output going via render pipeline, with a more complete output to a multi-layer EXR with all the other passes, possibly re-combined, denoised, cryptomatt-ed and so on.

I don't think such change is something we should be taking lightly.
If we are to go this direction it should be limited to cases where there is no Render Layer node and the render output of the compositor is empty.

Sequencer I don't think will be a problem here, as sequencer disallows having self scene as its strip, and the flags are stored on scene-specific Render.

Even for the described current behavior the code needs to be tweaked to do proper checks for the output node.

When there is no Composite output node the Composite render pass is not created, but the render result still contains all other passes, and they can be written. This change will break setup which were relying on the view layer passes (like combined) to be saved via render pipeline, and extra passes saved via File Output nodes. Such setups might be used on the farms to have some easily viewable render output going via render pipeline, with a more complete output to a multi-layer EXR with all the other passes, possibly re-combined, denoised, cryptomatt-ed and so on. I don't think such change is something we should be taking lightly. If we are to go this direction it should be limited to cases where there is no Render Layer node and the render output of the compositor is empty. Sequencer I don't think will be a problem here, as sequencer disallows having self scene as its strip, and the flags are stored on scene-specific `Render`. Even for the described current behavior the code needs to be tweaked to do proper checks for the output node.
@ -1157,2 +1157,4 @@
}
/** Returns true if the scene has a composite output. */
static bool compositor_has_output(Scene *scene)

The naming is quite close to the existing node_tree_has_compositor_output, think its better to:

  • Rename node_tree_has_compositor_output to node_tree_has_any_compositor_output
  • Rename compositor_has_output to node_tree_has_composite_output
The naming is quite close to the existing `node_tree_has_compositor_output`, think its better to: - Rename `node_tree_has_compositor_output` to `node_tree_has_any_compositor_output` - Rename `compositor_has_output ` to `node_tree_has_composite_output`
OmarEmaraDev marked this conversation as resolved
@ -1159,0 +1174,4 @@
for (const bNode *node : node_tree->all_nodes()) {
if (node->type == CMP_NODE_COMPOSITE && node->flag & NODE_DO_OUTPUT &&
!(node->flag & NODE_MUTED))

I don't think the composite node can be muted.

There also needs to be recursion into node groups, as the composite output might be in a node group.

I don't think the composite node can be muted. There also needs to be recursion into node groups, as the composite output might be in a node group.
Author
Member

They can be muted through RNA, just not through the mute operator. For instance, see report #109676.

Also, I just realized that this is a difference between CPU and GPU compositor, because the realtime compositor only considers output nodes in the root context, and I think this is the correct behavior. We do not have a mechanism for tagging active composite outputs across node groups, and allowing File Outputs inside node groups is rather dangerous considering node groups are shared.

Viewer nodes are an exception to this rule, in that we consider the active context, and not the root context, since it is useful for interactive compositing and is safe to do.

They can be muted through RNA, just not through the mute operator. For instance, see report #109676. Also, I just realized that this is a difference between CPU and GPU compositor, because the realtime compositor only considers output nodes in the root context, and I think this is the correct behavior. We do not have a mechanism for tagging active composite outputs across node groups, and allowing File Outputs inside node groups is rather dangerous considering node groups are shared. Viewer nodes are an exception to this rule, in that we consider the active context, and not the root context, since it is useful for interactive compositing and is safe to do.

Heh, I'm not sure what you mean with that question.

I just meant to say that the next step after compositing can be file output, image display or feeding into the sequencer. Not that there is anything particular to be solved regarding the image editor, if the compositor does not write to the IMA_TYPE_R_RESULT viewer image when there is no composite node that is fine.

Heh, I'm not sure what you mean with that question. I just meant to say that the next step after compositing can be file output, image display or feeding into the sequencer. Not that there is anything particular to be solved regarding the image editor, if the compositor does not write to the `IMA_TYPE_R_RESULT` viewer image when there is no composite node that is fine.
Author
Member

If we are to go this direction it should be limited to cases where there is no Render Layer node and the render output of the compositor is empty.

I suppose I can do that if there is an agreement.

> If we are to go this direction it should be limited to cases where there is no Render Layer node and the render output of the compositor is empty. I suppose I can do that if there is an agreement.
Omar Emara added 2 commits 2024-01-15 15:38:39 +01:00

If we are to go this direction it should be limited to cases where there is no Render Layer node and the render output of the compositor is empty.

I think another way of saying this is, don't write out a file if the render result is effectively empty. Maybe some buffer got allocated but nothing actually filled it in, so there's nothing to write.

That seems like a sensible thing to do regardless, and if it solves the crash then great.

That does not resolve #57699, which would be nice if we could get solve additionally, in a separate PR if needed. I think it may be reasonable for non-multilayer file formats to skip writing files when there is no composite node. With the idea that in such a case the "composite" layer in the render result is effectively empty or missing, so there is nothing to write. With versioning we could insert a composite node if we wanted to avoid breaking compatibility, at least as far as .blend files are concerned, not scripts.

My only concern is if that logic would be too difficult to understand for users. But I think in practice this only happens when you already created a file output node, because if you have no compositing output nodes Blender will refuse to even start rendering. So it may be pretty natural.

> If we are to go this direction it should be limited to cases where there is no Render Layer node and the render output of the compositor is empty. I think another way of saying this is, don't write out a file if the render result is effectively empty. Maybe some buffer got allocated but nothing actually filled it in, so there's nothing to write. That seems like a sensible thing to do regardless, and if it solves the crash then great. That does not resolve #57699, which would be nice if we could get solve additionally, in a separate PR if needed. I think it may be reasonable for non-multilayer file formats to skip writing files when there is no composite node. With the idea that in such a case the "composite" layer in the render result is effectively empty or missing, so there is nothing to write. With versioning we could insert a composite node if we wanted to avoid breaking compatibility, at least as far as .blend files are concerned, not scripts. My only concern is if that logic would be too difficult to understand for users. But I think in practice this only happens when you already created a file output node, because if you have no compositing output nodes Blender will refuse to even start rendering. So it may be pretty natural.

I think another way of saying this is, don't write out a file if the render result is effectively empty. Maybe some buffer got allocated but nothing actually filled it in, so there's nothing to write.

On a functional level indeed that is a more clear saying. I was more looking from technical implementation, as it is not that easy to tell if something got written to the pass or not, and there are comments like

/* A render-layer should always have a "Combined" pass. */
 render_layer_add_pass(rr, rl, 4, "Combined", view, "RGBA", false);

Perhaps we can remove all all non-allocated passes after render engine is done rendering, and check if render result has any passes before writing. Not sure if it breaks assumptions somewhere else about the "render-layer should always have combined pass" . Needs a deeper check.

> I think another way of saying this is, don't write out a file if the render result is effectively empty. Maybe some buffer got allocated but nothing actually filled it in, so there's nothing to write. On a functional level indeed that is a more clear saying. I was more looking from technical implementation, as it is not that easy to tell if something got written to the pass or not, and there are comments like ``` /* A render-layer should always have a "Combined" pass. */ render_layer_add_pass(rr, rl, 4, "Combined", view, "RGBA", false); ``` Perhaps we can remove all all non-allocated passes after render engine is done rendering, and check if render result has any passes before writing. Not sure if it breaks assumptions somewhere else about the "render-layer should always have combined pass" . Needs a deeper check.
Author
Member

The suggested behavior is currently implemented, should I do something else?

The suggested behavior is currently implemented, should I do something else?

I think the suggestion from @Sergey was to still save the result from any render layer nodes, which the implementation in this PR doesn't do?

I think the suggestion from @Sergey was to still save the result from any render layer nodes, which the implementation in this PR doesn't do?
Author
Member

@brecht I moved the check in the else block of compositor_needs_render(), so it will only skip writing if no render layer node exists. But maybe I missed something, I will retest after I finish the thing at hand and confirm that.

@brecht I moved the check in the else block of `compositor_needs_render()`, so it will only skip writing if no render layer node exists. But maybe I missed something, I will retest after I finish the thing at hand and confirm that.
Brecht Van Lommel approved these changes 2024-01-22 16:33:19 +01:00
Brecht Van Lommel left a comment
Owner

Ah, I missed that. Looks good to me then.

Ah, I missed that. Looks good to me then.

This patch still breaks behavior when the compositor output is in a node group. This isn't something that should be done as a bi-product of another fix, and only mentioned in an inlined comment.

If the render pipeline is to only consider output from the "root" tree it needs to be done consistently, and breaking changes are to be mentioned explicitly.

On a code side it is confusing why node_tree_has_any_compositor_output checks for outputs in the node groups, but node_tree_has_compositor_output does not.

This patch still breaks behavior when the compositor output is in a node group. This isn't something that should be done as a bi-product of another fix, and only mentioned in an inlined comment. If the render pipeline is to only consider output from the "root" tree it needs to be done consistently, and breaking changes are to be mentioned explicitly. On a code side it is confusing why `node_tree_has_any_compositor_output` checks for outputs in the node groups, but `node_tree_has_compositor_output ` does not.
Omar Emara added 2 commits 2024-01-23 14:11:46 +01:00
Omar Emara requested review from Sergey Sharybin 2024-01-23 14:13:14 +01:00
Sergey Sharybin approved these changes 2024-01-24 10:05:18 +01:00
Omar Emara merged commit c60321f6dc into main 2024-01-24 12:00:59 +01:00
Omar Emara deleted branch skip-no-composite-write 2024-01-24 12:01:05 +01:00
First-time contributor

Hi there,

Has this fix been released already? I am currently using Blender 4.1 on Windows and not seeing this behavior unless I am doing something wrong. As I understand, if I have this setup (with no Composite node):

image

I should only expect Blender to write to the File Output node path (C:/test/test.####.exr) and not the Output path as well (/tmp) with this fix?

Thanks!

Jared

Hi there, Has this fix been released already? I am currently using Blender 4.1 on Windows and not seeing this behavior unless I am doing something wrong. As I understand, if I have this setup (with no Composite node): ![image](/attachments/dba2eb7a-bda0-4e2c-a271-185135d1c5ee) I should only expect Blender to write to the File Output node path (C:/test/test.####.exr) and not the Output path as well (/tmp\) with this fix? Thanks! Jared
426 KiB
Author
Member

@Jared-Foulds The final change that landed is not exactly what the PR describes. This change makes it such that the output is only saved if a Composite or a Render Layer node exist, and since you have a Render Layer node, it will be saved. See the above discussion for more information.

@Jared-Foulds The final change that landed is not exactly what the PR describes. This change makes it such that the output is only saved if a Composite **or** a Render Layer node exist, and since you have a Render Layer node, it will be saved. See the above discussion for more information.
First-time contributor

@Jared-Foulds The final change that landed is not exactly what the PR describes. This change makes it such that the output is only saved if a Composite or a Render Layer node exist, and since you have a Render Layer node, it will be saved. See the above discussion for more information.

Ah ok thanks! So after reading the discussion, I don't think this issue #57699 is solved then because you would still need a Render Layer node to make a custom multilayer EXR with a File Output node right? Or am I missing something?

> @Jared-Foulds The final change that landed is not exactly what the PR describes. This change makes it such that the output is only saved if a Composite **or** a Render Layer node exist, and since you have a Render Layer node, it will be saved. See the above discussion for more information. Ah ok thanks! So after reading the discussion, I don't think this issue https://projects.blender.org/blender/blender/issues/57699 is solved then because you would still need a Render Layer node to make a custom multilayer EXR with a File Output node right? Or am I missing something?
Author
Member

@Jared-Foulds Indeed, that report was accidentally closed, I reconfirmed it. Thanks for note.

@Jared-Foulds Indeed, that report was accidentally closed, I reconfirmed it. Thanks for note.
First-time contributor

Awesome, thanks! That was the actual issue I was interested in anyways haha.

And thanks in general for all the great work you guys are doing here with Blender!!

Awesome, thanks! That was the actual issue I was interested in anyways haha. And thanks in general for all the great work you guys are doing here with Blender!!
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 project
No Assignees
4 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#117129
No description provided.