ExecutableNode improvements #838
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The base class didn't provide any useful functionality as far as abstraction was concerned - it was just a place where we put utility functions the derived classes needed for talking to renderers. Since that functionality could be useful elsewhere, it makes more sense to provide it in a namespace available to all. There is similar functionality buried in SceneProcedural that we should move to RendererAlgo.h at a later date. For now we're just moving the code from the Render class, because we need to move the Render class out of the way to do implement some needed refactoring of the Executable code.
I originally suggested to Lucio that he develop the Executable nodes using multiple inheritance of an Executable interface class, so that nodes could be both say a SceneNode and also Executable. That turned out to be a bad idea because the class hierarchy was rather confusing and it put the onus on developers to remember to call various methods to add plugs, test inputs were accepted etc. This led to problems like the crash bug in GafferHQ#681, and was just generally a bit more awkward than necessary. This commit simplifies things so there is now just a single ExecutableNode base class which fits in nicely with the DependencyNode and ComputeNode base classes. The implementations for the various derived classes are now a fair bit simpler, and the bindings are performed with a new ExecutableNodeClass which matches nicely with the NodeClass and DependencyNodeClass we already have.
|
This wouldn't merge after the sets merge, but I've rebased it now so it should be good to go. |
andrewkaufman
added a commit
that referenced
this pull request
May 15, 2014
ExecutableNode improvements
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a bit of a tidy up of the Executable code so it's in slightly better shape for someone to take on #109. The meat of it is backing out of an awkward multiple inheritance setup where any Node subclass could inherit an additional Executable interface class to become Executable - I forget why I pushed for it at the time but it's since become clear that it wasn't the right way to go, hence I'm tidying up my mess. There is now a single ExecutableNode base class which sits nicely with the DependencyNode and ComputeNode classes, and things are simpler all round.
My final commit outlines some work that I believe is pretty critical in the ExecutableNode::Task class - Lucio and I talked about it briefly via mail but rather than implement it I've left it as a todo item for now. We should discuss further and implement early on when doing #109.