SetFilter #851
Merged
SetFilter #851
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
At a later date this will give us a single place to control the Context in which filter queries are made.
This includes a useful right click menu for populating the set plug with the names of sets available in the connected scenes.
Now we have an optimised Context implementation, the performance difference appears negligible, and using the standard Context for everything is preferable over having special cases. I did consider modifying the Context class to allow RunTimeTyped storage rather than just Data storage, so that the scene plug could be stored without encoding as uint64_t, but decided against it. Because RunTimeTyped (or GraphComponent, or Plug) doesn't have a copy() method, it would make implementing the ownership semantics the Context requires impossible, or would require documenting special cases for non-copyable types. It seems preferable to have one nasty isolated cast for an isolated use case than to confuse an API which is central to everything.
Since we already have access to the context, it doesn't make sense to pull it out of thread local storage again.
Also improved documentation in FilteredSceneProcessor to reduce the chance of doing it again.
UnionFilter is now compatible with SetFilter inputs. Fixes GafferHQ#92.
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 implements the SetFilter as required by #92.
The tricksy part about the SetFilter compared to the other existing filters is that it requires an input scene to compute the result. As discussed in #92, people expect to be able to plug the same filter into multiple nodes, and automatically have the appropriate input scene for each stream considered in each case. And they're not willing to manage input connections manually which seems fair enough. This leads us to a use of the Context I hadn't anticipated originally - when the FilteredSceneProcessor is pulling on the filter input, it places the input ScenePlug in the context too, so the filters can have access to it. This does make me slightly uneasy, as placing a plug in there is quite different to placing pure data, but it does work, and David and I felt it was preferable to any of the other solutions we'd considered. The only real consequence I can imagine at present is that it would now be harder to implement some sort of remote computation server, because a Context can't be serialised and sent afar in the same way as it could when it was pure data. But I don't think that was something that was going to happen soon anyway.
The path I took in implementing this is slightly winding, in that at one point there was basically a secondary filter context for passing the input scene. I did this just to get things rolling while I was tearing up the Context for optimisation and wasn't sure how it was going to turn out. In the end it turned out to be a fairly simple change and a good speedup so I felt it was best to just use the standard context for passing the input scene after all. If you feel it makes for a confusing history and a tough review, I could rebase it into fewer commits, but my guess was it's slightly easier to follow in the smaller chunks it's in at present...