Page MenuHome

Improve naming coherency for FileOutput Node inputs.
Needs ReviewPublic

Authored by Laurent Noel (c2ba) on Dec 13 2019, 9:06 PM.

Details

Summary

This patch addresses T72331 and T72335.

Currently, when we switch to OpenEXR Multilayer file format on a File Output Node, the naming of inputs is not kept. This is due to file slots and layer slots not being kept in synchronization when renaming an input. The same is true for input sockets names.

This patch provides modifications to keep the same name between file slot, layer slot and input socket (identifier and name for the input socket) at a given input index.

Diff Detail

Branch
uas_file_output_node_fixes (branched from master)
Build Status
Buildable 6004
Build 6004: arc lint + arc unit

Event Timeline

Laurent Noel (c2ba) set the repository for this revision to rB Blender.

Hi Laurant!

thanks for the patch, I will need some investigation of the current state of code.
Questions that I have are (not towards this patch, but overall when thinking of the issues we encounter):

  • Why don't we merge the sockdata->path and sockdata->layer to a single structure? The original developer was CPP oriented, but perhaps a more C-approach would help.
  • Should we then remove the whole path and layer and just use sock->name? I do beleive we should not store the same value multiple times as that leads to more maintenance.
  • Do we support relative paths that does not fit as layer name. eg do the naming convention match.
  • We should add versioning code for existing files. But we should wait until we have a more clear sight of what needs to be done. Too many conversions can reduce data quality.
  • OpenEXR specification is not clear about the support of utf8. Think they leave it to the pipeline/applications to figure it out. But I see that the current implementation already did utf8 string copy.

I hope to have some time to go over these topics the upcoming weeks and come back to you on the code review ...

Hi Jeroen :)

  • Why don't we merge the sockdata->path and sockdata->layer to a single structure? The original developer was CPP oriented, but perhaps a more C-approach would help.
  • Should we then remove the whole path and layer and just use sock->name? I do beleive we should not store the same value multiple times as that leads to more maintenance.

Agree, I was wondering why it was needed to have two list of names (even three if we consider names of the sockets). I can work on that.

  • Do we support relative paths that does not fit as layer name. eg do the naming convention match.

I will check that today.

Also @Yannick Castaing (0_BoUBoU_0) our CG sup has other issues with the file output node in its current state, he is prototyping a new one with node groups that I may implement in C and propose later here.

  • Do we support relative paths that does not fit as layer name. eg do the naming convention match.

We did a few tests:

  • relative paths are supported: the output file is written in a subfolder matching the relative path
  • the same relative path can be used as a layer name in a Multilayer EXR image (meaning "/" is supported in the name of such layer)
  • We can also use '/' in the name of an input socket

Consequently, I don't see any reason why we should keep file_slot.path, layer.name and input.name => the 3 can be merged in just input.name. Also it would remove the need to store a NodeImageMultiFileSocket in bNodeSocket::storage, wich is deprecated as I saw in a comment, and only used by Image and OutputFile nodes.