Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FilterPlug #1815

Merged
merged 6 commits into from Aug 16, 2016
Merged

FilterPlug #1815

merged 6 commits into from Aug 16, 2016

Conversation

@johnhaddon
Copy link
Member

johnhaddon commented Aug 15, 2016

To date, we've been using a plain old IntPlug to provide the output of a Filter node and the input to a FilteredSceneProcessor. But this alone was insufficient to provide the guarantee that only Filter outputs could be connected to a FilteredSceneProcessor input. We used FilteredSceneProcessor::acceptsInput() to add constraints in an attempt to enforce this, but these have their problems as witnessed by #1814 and the SceneProcessorTest.testDeriveAndUseExternalFilters test case added in this PR.

The FilterPlug is intended to provide a stronger mechanism for making the guarantees we need. By using a dedicated plug type, we ensure that the plug semantics are preserved when undergoing promotion, and nodes can rely on the plug type rather than a custom acceptsInput() method which will become more flawed as other means of duplicating/promoting plugs appear in the future.

This is a pattern that has repeated itself every single time we've used a less strongly typed plug along with a custom Node::acceptsInput() implementation, rather than use a custom plug type from the outset. We've already made an analogous transition from Plug to TaskPlug in GafferDispatch, and shortly will be adding a ShaderPlug for use as the input to a ShaderAssignment. The rationale for the original implementations was to reduce the proliferation of many specialist plug types, but it seems clear at this point that the general rule should be that a custom plug type should be preferred to Node::acceptsInput overloads.

johnhaddon added 6 commits Aug 12, 2016
To date, we've been using a plain old IntPlug to provide the output of a Filter node and the input to a FilteredSceneProcessor. But this alone was insufficient to provide the guarantee that only Filter outputs could be connected to a FilteredSceneProcessor input. We used FilteredSceneProcessor::acceptsInput() to add constraints in an attempt to enforce this, but these have their problems as witnessed by #1814 and `SceneProcessorTest.testDeriveAndUseExternalFilters`.

The FilterPlug is intended to provide a stronger mechanism for making the guarantees we need. By using a dedicated plug type, we ensure that the plug semantics are preserved when undergoing promotion, and nodes can rely on the plug type rather than a custom `acceptsInput()` method which will become more flawed as other means of duplicating/promoting plugs appear in the future.

This is a pattern that has repeated itself every single time we've used a less strongly typed plug along with a custom `Node::acceptsInput()` implementation, rather than use a custom plug type from the outset. We've already made an analogous transition from Plug to TaskPlug in GafferDispatch, and shortly will be adding a ShaderPlug for use as the input to a ShaderAssignment. The rationale for the original implementations was to reduce the proliferation of many specialist plug types, but it seems clear at this point that the general rule should be that a custom plug type should be preferred to `Node::acceptsInput` overloads.
This allows us to remove the problematic `Node::acceptsInput()` overload logic. A couple of test cases needed tweaking to accommodate the slightly modified logic, but the failure cases were not representative of current practice.

Fixes #1814.
@andrewkaufman andrewkaufman merged commit c92aa24 into GafferHQ:master Aug 16, 2016
2 checks passed
2 checks passed
codacy/pr Good work! A perfect pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@johnhaddon johnhaddon deleted the johnhaddon:filterInputs branch Aug 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.