Page MenuHome

Structural cleanup for the compositor
AbandonedPublic

Authored by Thomas Dinges (dingto) on Feb 11 2014, 1:11 PM.

Details

Summary

Many parts of the compositor are unnecessarily complicated. This patch
aims at reducing the complexity of writing nodes and making the code
more transparent.

A number of further improvements can be made beyond this patch, but
since it touches all of the node files i consider it preferable to make
a checkpoint and get this approved and verified first.

Layered structure

Currently the compositor mixes nodes (internal copies from bNode) and
actual operations (elementary executable components) in the same graph,
which leads to lots of implicit type assumptions and confusing
distinctions.

Separate these graphs from each other:

  • NodeGraph: internal representation of the bNodeTree DNA data. Most nodes have a 1:1 class equivalent, except for a few nodes (groups, switch, muted nodes) which are replaced by simple proxies.
  • Operations graph (currently still "ExecutionSystem"):

Actual implementations, generated from the NodeGraph in a second step.
Nodes define a conversion method to transform themselves into a set of
operations based on data copied from DNA nodes.

Dedicated classes: What happens Where

Many functions are in awkward places and pass through multiple different
classes before the final implementation (e.g. node->addLink wrapping
execution system function).

In particular the construction of the NodeGraph and ExecutionSystem
(operation graph) should be handled by compiler classes. These can
store temporary state, like socket maps, and are created only as local
variables for converting DNA nodes to node graph and node graph to
operations graph respectively. That way the graph structures themselves
are largely free of the intricacies of how these structures are
converted and easier to understand.

Diff Detail

Branch
compo_structural_cleanup

Event Timeline

Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Feb 12 2014, 2:54 PM

Separating Node and Operation graphs.

New socket and connection classes replace the shared classes,
InputSocket, OutputSocket, SocketConnection are only used for NodeGraph
now (to be renamed later).

This ensures we don't get confused at any point about what type of
socket or connection a function expects. A lot of bloated type checking
code (e.g. isOperation) becomes unnecessary. Many previously shared
properties are now clearly assigned to just one of these types (e.g.
bNode/bNodeTree pointers only exist in NodeGraph, while
determineResolution is only used in operations).

Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Feb 13 2014, 3:39 PM

Renamed graph classes to follow a common naming pattern throughout node
and graph structures:

NodeGraph / OpGraph: base container class
Node / Operation: main components
NodeInput, NodeOutput / OpInput, OpOutput: sockets in node/operation
NodeLink, OpLink: connection between sockets

All classes now have proper names, with the exception of ExecutionSystem
(OpGraph), which still needs some reorganization for the
ExecutionGroups.

Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Feb 14 2014, 4:48 PM

Changed the compiler and context arguments of the convertToOperations
method from pointers to references.

Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Feb 14 2014, 9:07 PM

Moved all construction logic for operations from the ExecutionSystem
into the NodeCompiler class.

The idea is that the compiler is responsible for creating all the
operations, connections and groups and for performing any optimizations
(such as removing unused operations, resolving proxies, later constant
folding). The ExecutionSystem should see the operations graph as
immutable and not have to modify the structure itself.

The functionality in the compiler may be split up a bit further later
for clarity, e.g. store actual data in a dedicated OpGraph container.

Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Feb 15 2014, 11:42 AM

Removed explicit OpLink instance from the ExecutionSystem and operation
input/output classes.

For evaluating the compositor operations a single input pointer to the
connected output is perfectly sufficient. Actual connection list is only
used internally in the compiler now.

Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Feb 15 2014, 1:28 PM

NodeLink made into an internal class of the NodeGraph and connections
list removed from NodeOutput.

Just like with operations there really is no need to access node links
directly from outputs. The node graph in its construction method can
access the internal link vector, for nodes the connection status of
output sockets is irrelevant.

Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Feb 16 2014, 9:55 AM
  • Fix for Blur operations: use a copy of the NodeBlurData instead of keeping a pointer
  • Fixed the DebugInfo class used for graphviz dumps
Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Feb 16 2014, 2:29 PM
  • Divided the NodeCompiler class into a basic abstract interface and an implementation class NodeCompilerImpl. The reason for this is to make the convertToOperations methods in nodes as simple as possible by keeping only the main public functions in the NodeCompiler interface and hide all the details of storage and optimization functions in the implementation class.
  • Sanity for the ExecutionGroup::addOperation function. This things was pure madness, considering what it actually does: collect input operations into groups, using a few exceptions based on operation complexity settings.
Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Feb 16 2014, 5:22 PM

Fix for order of determineResolutions, read/write buffering and grouping
processes.

These have to be performed in the correct order to ensure that
additional operations are included in groups and that resolutions for
all the operations are calculated correctly.

Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Feb 16 2014, 7:08 PM

Bring back node groups, by adding a method to resolve proxy operations.

Proxies can be added by any node. For groups they are used to provide
a connection of the group instance with outside nodes. After all nodes
have been converted to operations, the proxy operations are resolved,
i.e. all downstream connections are replaced by going back to the first
non-proxy output.

A bunch of cleanup in addition to this: All optimization procedures that
happen after the main node conversion now have their own little
functions to avoid ballooning up the main convertToOperations method.

Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Feb 17 2014, 9:07 AM

Pruning unreachable operations: This removes up any operations that will
never be executed to clean up the operations graph.

Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Feb 19 2014, 11:00 AM

Merge branch 'master' into compo_structural_cleanup

Fixed bug in complex operation buffering, this was not properly checking
connections for input sockets and connecting them to arbitrary write
buffer operations.

Naming of the Node:

End of january we had a discussion in the institute that you really wanted that the compositor would have an own language with a compiler. This is a good idea but a step too far for now.
In this path you reserve the next Classes: Compiler, Operation that are better suited for this future project.

Also compilers normally works autonomous, your implementation of the compiler is strictly integrated using delegates to the source. The design pattern that is closest to your implementation are Factory, or builder. In the anti-patterns it is better suited for DTO/DTS.

I would rather see that the compiler (or what name that it will have) to be a static implementation. As now for no usage the compiler is passed to every node. You could also seperate the public interface and the friend interface better when using two static classes.

I do see the benefits for this splice, but still need to look if it fits the proposal we are working on. In this proposal there will be a forth data structure (Tiles+dependencies) that will be generated.

  • Missing documentation or outdated documentation (overall design in the COM_compositor.h etc)
  • We should stick to a single name policy (Link or Connection, not both)
  • Do not use short hand names as it is not clear (use Operation iso Op)
  • We think that NodeOperation is a better name than Operation (a bit too generic) see above for the explanation
  • The name Graph is wrong as it really is not a graph (nodes are connected with one or multiple sockets, this doesn't fit in the idea of a graph too well), that is also why we called it a system.

No matter in which direction we will go, the current compositor code is very difficult to understand, and imho Lukas patch is a big step into the right direction.

For the record, we do not disagree with the implementation,
but we should really concern about the naming and future extensibility.

btw. has anyone looked at the time it takes to build the operation tree with this design? I haven't seen any records on that. I always used 0.2 seconds for a complex tree on a 2Ghz machine as a reference.

Some quick replies on @Jeroen Bakker (jbakker) comment.

So, first things first. Don't see anything bad about laying bits needed for custom language (it could be based om OSL or even be own language or so, doesn't really matter at this point) on the early stages. IMO, that's gonna to save some time doing re-design once including all the bits we'll need in the future rather than including features one-by-one. Maybe too much vague explanation, but hope you got the point :)

One thing here tho, we do not necessarily will put all the bits now, we just need to be sure further extending of the compo wouldn't lead to need of its re-designing.

I'll leave all the design patterns up to you guys ;) Mainly because design pattern is just a tool, there's never single pattern for the particular task. I'll totally trust Lukas here he's much more to C++ than i am.

Agree on the missing documentation, it always was (and will be? :( ) an issue in blender.. However, wouldn't be so much polar about this to Lukas. Where's the documentation about details of OpenCL implementation (bunch of methods with undocumented arguments and so). Also, spending time on putting documentation to the patch which has probability of being rejected.. IMO it's not a huge deal to copy-paste stuff from the commit message (which is rather explicit here) to the code once we consider code-wise the patch is good.

Also agree on Link vs. Connection. But one thing here -- compo violated name from the very beginning. "Connection" was introduced by the new design, we used to (and still using in DNA and editors) use "Link". Go figure ;)

Short names agree in general (i love verbose names and always use them in Libmv). But not so much fussed about using shorten versions which are common in the computer science (Op, Ctx and so on).

NodeOperation vs. Operation eeeh. That's tricky. Operation sounds more generic, however NodeOperation is somehow misleading as well by some sort of implicit mention in the name that it's something to do with the node, which doesn't really exist. It's more like ImageprocessOperation or so. Could be some better name.

Graph is even more tricky question. Speaking of generality of the names, System is really vague and too general ;) And didn't really get why graph is wrong here? It's a heterogeneous multi-graph ;)

Just own thoughts about naming :)

Please all look at http://git.blender.org/gitweb/gitweb.cgi/blender.git/blob/HEAD:/source/blender/compositor/COM_compositor.h before commenting on documentation!

IMO this patch changes the structure and the overview documentation has not been updated. If we don't update the documentation in this file we should remove the documentation at all. Making it harder for people to understand the code itself. They need to reverse engineer the working via code? result => getting only experienced people in the compositor, taking longer time for people to get used to the system.

This is not the way even for big Open source project how they should work as you all agree that event this file cannot be updated!


One big issue with Blender (and has always been) is getting new developers in. Blender2.5 did a good job in that, and if we also want this for this patch to happen we should really look at our target audience. For who should the code be clear. Experienced Blender devs or Newby's. The part that this patch is addressing in implemeting new nodes. normally new nodes (in typically studios) are implemented by not core blender developers. So if we want to address this we should focus on our target audience.

When it will be possible to create node-plugins in blender the API reflects only a part of the full API and that should be clear. IMO the moving of the code to a seperate class is OK. Naming it a compiler is not OK. Just look for a reference to the Nuke API that is a well-named API. And in stead of a Compiler class IMO it would be better to use a static class or a bunch of methods in a seperate namespace.


Well when introducing a compiler I would expect that there will be an Operand. By that time "Op" can mean two things.
IMO class names and methods should be full names for clarification.


I agree that Connection is not a blender's name and Link is correct. I only say when changing Link to Connection what this patch introduces is the other way around. Saying that Blender's internal name (Link) is not correct and Connection is correct!

IMO when changing naming convention you should do it for the whole module. As this patch only addresses some parts of the module, we will get a hybrid format Not saying to new developers what is the correct one.


But if you all don't see the big picture of the impact of the naming-convention of this patch on future development, do not expect Blender's compositor to evolve. We are spending a lot of time and community money on doing things that is not in the right direction.


Having said this, I still agree with the implementation, only the naming is not well thought about and implemented and there is no vision or design behind this patch. My concerns are stated here. If the community thinks that I am incorrect, like a lot of developers is pointing out, please be aware that this could be a bigger issue that they can oversee.

Well, was a bit of misunderstanding about some things, but don't want to spend more time describing what i meant.

So if the issue is just in naming, then few things:

  • Let's making sure the patch doesn't increase entropy of link vs. connection naming (as in: patch uses link all over the new code)
  • Get rid of connection name as a separate patch (current master already mixes the names here)
  • If "Compiler" is not considered good enough -- tell what would fit into the updated design. We can not judge on how good or bad name would be from the bigger design changes POV when we don't know exactly what's gonna to be changed..
  • Op vs. Operation -- lets choose full name. Think neither me nor Lukas really fussed using full name here.
  • NodeOperation vs. Operation would also depend on how updated design will look like. It might be neither of this names fitting here really.

Would it solve the issues with the patch? Currently i'm not even sure what are exact stoppers for the patch, or whether it's just to be re-considered from scratch or what.

Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Mar 12 2014, 10:01 AM

Merge branch 'master' into compo_structural_cleanup

Updated new Corner Pin node to use the compiler

Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Mar 12 2014, 10:17 AM

Cleanup: Changed all uses of "connection" to "link" for shorter names and consistency with the rest of Blender code.

Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Mar 12 2014, 11:09 AM

Cleanup: Renamed class names with shortended "Op" prefix to full "Operation" names.

Note: Link classes inside NodeGraph and NodeCompiler are now simply named "Link", since these are internal helper structs for these graph constructors only, no need to use an additional prefix.

Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Mar 12 2014, 12:53 PM

Renamed Operation back to NodeOperation.

Discussion about this name change has come to no definite conclusion,
decision was to retain the current naming for now.

Hi Lukas, good work.

Concerning the compiler I would suggest to start with the psuedo you shared with me last week. It used the names ...Converter and ...Builder (at least I remember these names). We can than merge the code to head.

As discussed we don't believe that these names are final, but we can collect some feedback when working with the system. We can discuss the implementation (static in stead of instance) in a later stage.

Jeroen

Lukas Toenne (lukastoenne) updated this revision to Unknown Object (????).Mar 19 2014, 8:40 AM

Renaming the compiler class(es) according to requests by AtMind.

  • NodeCompiler -> NodeConverter: The 'compiler' name was contested as being out of place for a graph construction helper.
  • NodeCompilerImpl -> NodeOperationBuilder: This contains the innards of the actual construction methods, storage containers for operations created by nodes and optimization functions for finalizing the result

Use NodeConverter as an interface wrapper around the NodeOperationBuilder instead of an abstract base class.

Inline documentation for the NodeConverter methods. Documentation may be added to other classes, but this is the most important since it is used very frequently by all the nodes and needs to be well understood by node developers.

Thomas Dinges (dingto) commandeered this revision.Apr 27 2014, 8:48 PM

This was commited, so can be closed. ;)