Page MenuHome

Compositor node preview enhancement
ClosedPublic

Authored by Jeroen Bakker (jbakker) on Dec 2 2013, 9:13 PM.

Details

Summary

This patch is a completed task for the compositor feedback from bf-compositor. There the node preview were discussed and some changes have been proposed:

  1. Hide all node previews except for the input nodes.
  2. Make it possible to change the colors of all node-classes.

The node preview part was just a few lines, but kept me thinking. Do we need to have a specific init method in the bNodeTreeType for these specifics (Perhaps lukas can review this)
The node-classes colors were already implemented but not available for all node classes as it re-used attributes that were used by the syntax highlighting of the text-space. The next thing we did:

  • Spliced the in/out color to 2 separate options
  • Spliced the Operator color to each component
  • Added the missing node classes (also for material trees and texture trees)
  • Made sure the newly created attributes are readable from SDNA
  • Made sure the naming convention was used inside the RNA. This will change some python bindings, but these bindings (UserPreferences) are hidden (or I don't know how to trigger them)
  • Created conversions of old themes when loading them.

Diff Detail

Event Timeline

Hi Jeroen,

I tried to apply this patch but not successful. Here's the log http://www.pasteall.org/47768. And if i keep compiling it, I’ve got this on my compile log http://www.pasteall.org/47769.

thanks

Hey!
Yeah, I'm having problems as well:

patch: **** malformed patch at line 12: #define BLENDER_MINVERSION 262

Cheers,

Seb

I don't like adding such type-based code in a generic function ...

Would suggest to implement the method i outlined on bf-compositor, using a common init function for compositor nodes (and calling it from specialized subclass init functions). There is a second variant of init callbacks "initfunc_api" which also takes a context argument. This would allow the nodes to check user preferences or tree settings for the preview option.

It may seem like a lot of unnecessary work and over the top for such a simple feature, but it is much cleaner design-wise and can be extended in the future. It makes this behavior into a feature of the compositor nodes rather than making assumptions in blenkernel. Unfortunately such polymorphism is cumbersome in C, which is why i'd like to replace the node UI with python in the long run.

Also IMO the node->nclass property does not fit into the concept of extendable node systems very well. At the very least i'd like to replace it with a string property rather than a hardcoded enum, so new classes can be added dynamically. Even better would be a custom bool property flag for compositor nodes "is_input_node". This way a node can be tagged as an input node without having to dump all of them in the same node class.

Theme colors present a similar problem: Just adding all of the classes as theme colors adds a lot of boilerplate code which is only extendable in C. I guess that's ok for now since we have this system in place, but we should think about a more flexible solution based on customizable "color palettes" or so. An addon could then specify a new theme color for a group of node types.

Brecht Van Lommel (brecht) requested changes to this revision.Dec 6 2013, 2:48 PM

The theme changes look ok to me given the current system. I also agree about moving that preview check into the init function.

I'm marking this as "Request Changes", I suggest to use this when reviewing since it makes the Blocking Others / Action Required / Waiting on Others status work nicely.

Jeroen Bakker (jbakker) updated this revision to Unknown Object (????).Dec 8 2013, 10:19 PM

Made changes to the way how preview nodes are hidden. I added a new method pointer to the bNodeTreeType. This way the implementation is not visible from node.c
I do not like to update all init methods of almost every compositor node as this 'rule' will be forgotten or are not aware by developer who implement new nodes.
It also saves a lot of lines of code.

Leaving this for Lukas to approve, not sure what he had in mind exactly for this init function.

Alright, i think this is a good compromise. Eventually when nodes get a true polymorphic implementation this would become redundant and could be merged into a base class init method. For C nodes should be ok.

Feature has been committed