Compositor: Refactor File Output node #113982
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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
EEVEE & Viewport
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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#113982
Loading…
Reference in New Issue
No description provided.
Delete Branch "OmarEmaraDev/blender:file-output-node-refactor"
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?
This patches refactors the compositor File Output mechanism and
implements the file output node for the Realtime Compositor. The
refactor was done for the following reasons:
resource where the result of each compositor execution for each
view was accumulated and stored in the global resource, until the
last view is executed, when the EXR is finally saved. Aside from
relying on global resources, this can cause effective memory leaks
since the compositor can be interrupted before the EXR is written and
closed.
have multiple compositor implementations.
existing implementation, like the inability to save 4D vectors, lossy
compression of data passes, and inability to save single values
passes.
The refactor first introduced a new structure called the Compositor
Render Context. This context stores compositor information related to
the render pipeline and is persistent across all compositor executions
of all views. Its extended lifetime relative to a single compositor
execution lends itself well to store data that is accumulated across
views. The context currently has a map of File Output objects. Those
objects wrap a Render Result structure and can be used to construct
multi-view images which can then be saved after all views are executed
using the existing BKE_image_render_write function.
Minor adjustments were made to the BKE and RE modules to allow saving
using the BKE_image_render_write function. Namely, the function now
allows the use of a source image format for saving as well as the
ability to not save the render result as a render by introducing two new
default arguments. Further, for multi-layer EXR saving, the existent of
a single unnamed render layer will omit the layer name from the EXR
channel full name, and only the pass, view, and channel ID will remain.
Finally, the Render Result to Image Buffer conversion now take he number
of channels into account, instead of always assuming color channels.
The patch implements the File Output node in the Realtime Compositor
using the aforementioned mechanisms, replaces the implementation of the
CPU compositor using the same Realtime Compositor implementation, and
setup the necessary logic in the render pipeline code.
Depends on #114339.
WIP: Compositor: Refactor File Output nodeto Compositor: Refactor File Output nodeJust a quick pass of code review. On a design description level it sounds reasonable. On the code side it nice to see that there is no duplicated tricky logic. You even managed to delete more lines than add!
One of the big things I am confused about is that it seems vector are stored as 4 elements now? Is my understanding correct? If yes, why is it so?
@ -81,2 +81,3 @@
const bool stamp,
const char *filepath_basis);
const char *filepath_basis,
const struct ImageFormatData *source_format = nullptr,
Would be nice the explain what
source_format
is for in the comment above.@ -785,6 +785,7 @@ bool BKE_image_render_write_exr(ReportList *reports,
/* Other render layers. */
int nr = (rr->have_combined) ? 1 : 0;
const int layers_count = BLI_listbase_count(&rr->layers);
Small optimization:
const bool has_multiple_layers = BLI_listbase_count_at_most(&rr->layers, 2) > 1;
@ -0,0 +45,4 @@
{
int size = get_channels_count(datatype);
return static_cast<float *>(
MEM_callocN(width * height * size * sizeof(float), "File Output Buffer."));
MEM_malloc_array(size_t(width) * height * size, sizeof(float), "File Output Buffer").
This will reduce the possible overflow issues. Can also move
size
to the second argument.@ -0,0 +160,4 @@
}
}
/* --------------------
Not sure if that is from an existing code. We have a different way in the guideline defining such code sections: https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments (scroll down a bit to the Comment Sections)
Just a quick pass of code review. On a design description level it sounds reasonable. On the code side it nice to see that there is no duplicated tricky logic. You even managed to delete more lines than add!
One of the big things I am confused about is that it seems vector are stored as 4 elements now? Is my understanding correct? If yes, why is it so?
@Sergey They are stored as 4 elements but not necessarily saved as such. The user can choose the RGB color format to save them as three element vectors or the user can choose RGBA color format to save them as four element vectors.
But this is not something that is possible to control on a per-pass level when the File Output is using Multilayer EXR?
I've prepared a simple file. Before this change the vector socket is stored as
Image_001.{X,Y,Z}
it is stored asImage_001.{r,g,b,a}
. It is nice to better supportVector
pass output, but having extra component forNormal
pass sounds a bit wasteful. The change also "breaks" channel naming in the EXR (not immediately sure if it is really bad, but it feels so from the interop perspective).Not sure we should be doing such changes as part of a refactor, and adding support of the node to the GPU compositor. It feels like more proper would be to have 2,3, and 4- element vectors as socket types, and derive channel naming from them. So that Normal is stored as {x,y,z}, Vector is stored as {r,g,b,a}, and UV is stored as {x,y}.
P.S. The alpha handling for Vector is different in tiled and full-frame and GPU: tiled initializes alpha to 1, the latter two initialize it to 0.
I see. I missed that case.
@Sergey Okay, so I will restore three-components XYZ vectors and split the patch into a refactor and a GPU implementation.
I tested various different types of passes, file formats, stereo, all seems to be working well.
And almost the same lines of code as before, nice.
@ -922,2 +933,3 @@
const bool stamp,
const char *filepath_basis)
const char *filepath_basis,
const ImageFormatData *source_format,
I would name this just
format
. It is the file format that will be written, not of the source image or something like that.I guess this was taken from
BKE_image_format_init_for_write
, but in that context it means that the source data initializedimage_format
from.@ -0,0 +15,4 @@
#include "BLI_string.h"
#include "BLI_string_utf8.h"
#include "BLI_string_utils.hh"
#include "BLI_task.hh"
This include seems unnecessary, no multithreading so far.
Oops, I meant to delete this comment.
@blender-bot build
@blender-bot build
I wonder if (speaking about refactoring) it wouldn't make more sense to refactor also the output panel giving the chance to add multiple formats and not just one
@lsscpp This is already possible, just uncheck "Use Node Format" for the inputs and control each of their formats independently.
I meant in the property editor, which I know is not related to nodes
I see. Not sure if that would be a desirable feature. In any case, it is not really part of the compositor module so I am not going to personally look into it in the near future. Also, RCS and DevTalk are better suited for such requests/discussions.