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

Switch refactor #2812

Merged
merged 17 commits into from Oct 13, 2018

Conversation

Projects
None yet
2 participants
@johnhaddon
Copy link
Member

commented Sep 26, 2018

Previously, our Switch class was a funky templated thing that was used both as a "mix-in" to create things like SceneSwitch and ImageSwitch, and also more directly to make SwitchComputeNode, which could be used generically with any plug types you added. Aside from the complexity, this had another downside : the SceneSwitch and ImageSwitch contained important optimisations that the generic SwitchComputeNode didn't have. And of course, we discovered SwitchComputeNodes being used directly with scenes and images in the wild.

This refactors everything to completely remove the specialised switch types, turning Switch into a regular non-templated node with all the optimisations of the specialised nodes. We've ended up with less code, simpler code, and a single Switch node that users can use for any purpose without performance concerns.

Backwards compatibility for old files is provided by the usual compatibility configs. There are a few wrinkles here I've documented in the commit messages themselves, but hopefully nothing of real concern. I've kept c113e2f distinct from 87ecd12 both for clarity and because we could backport it to previous release branches in order to simplify the process of updating custom filter code for use in the new version.

If this passes muster, I'll go on to do the same with our funky ContextProcessor mixin classes like TimeWarp etc. This should be more straightforward than the Switch refactor, but will have similar benefits.

johnhaddon added some commits Sep 24, 2018

Config : Remove filterCompatibility.py
This belongs to the same era as the InputGenerator compatibility files removed in #2805.

> Note : IE developers should be aware that some internal code is still referring to
> plugs named "match", and this will need updating to "out".
Gaffer : Remove SwitchDependencyNode
It is not used anywhere, and removing it allows us to remove some complexity. This is a first step towards detemplating the Switch class and removing all the specialised Switch nodes.
Switch : Replace SwitchTraits with ContextAlgo::GlobalScope
This means that a regular SwitchComputeNode now has the same context optimisations as the specialised SceneSwitch and ImageSwitch nodes.
GafferScene : Remove SceneSwitch
Breaking Changes :

- GafferScene : Removed SceneSwitch node : use a regular Switch instead. Backwards compatibility
  for old files is provided by converting SceneSwitches during loading.
GafferScene : Remove ShaderSwitch
Breaking Changes :

- GafferScene : Removed ShaderSwitch node : use a regular Switch instead. Backwards compatibility
  for old files is provided by converting ShaderSwitches during loading.
GafferScene : Remove FilterSwitch
Note the special handling required in `FilterPlug::sceneAffectsMatch()` to keep the query working when a FilterPlug receives an input from a context-varying Switch. This is similar to the custom handling required in Shader/ShaderPlug : the generic Switch node works perfectly as long as you stay within the confines of the ComputeNode framework, but if you step outside of it you have to accept some additional responsibility. The upshot of this is that it's now essential to call `FilterPlug::sceneAffectsMatch()` rather than `Filter::sceneAffectsMatch()`, so `Filter::sceneAffectsMatch()` is now private.

Breaking Changes :

- GafferScene : Removed FilterSwitch node : use a regular Switch instead. Backwards compatibility
  for old files is provided by converting FilterSwitches during loading.
- Filter : Made `sceneAffectsMatch()` protected. Use the new `FilterPlug::sceneAffectsMatch()` method
  instead.
GafferImage : Remove ImageSwitch
Breaking Changes :

- GafferImage : Removed ImageSwitch node : use a regular Switch instead. Backwards compatibility
  for old files is provided by converting ImageSwitches during loading.
FilterPlug : Add `sceneAffectsMatch()` method
This is more convenient than the `Filter::sceneAffectsMatch()` method it replaces, and better fits our general design whereby plugs provide the public interface to the internal capabilities of nodes.
Switch : Remove templating, and rename SwitchComputeNode to Switch
A backwards compatibility config file provides compatibility for Python scripts, and typedefs provide source compatibility for C++ files.

Fixes #2123.
Image/OSL/UI/Scene : Don't rely on SwitchComputeNode typedefs
Refer to the new Switch node directly instead.
Python files : Remove reliance on switch compatibility configs
There is some subtlety here for the ShaderSwitch case. When ShaderSwitches were first introduced, they always had "in" and "out" plugs added during construction, but in 0.31.0.0 this ceased to be the case, so that we could support multiple parameter types (previously we only supported RSL coshaders). When we did that, we added `shaderSwitchCompatibility.py` to auto-add the old "in"/"out" plugs on first attempted access, and we still have that same compatibility code in our new `switchCompatibility.py` file. The difference is that the new ShaderSwitch stub isn't a true C++ class as before, so the compatibility `__getitem__` is lost through slicing when retrieving the node from C++. The bottom line is this :

```
s = GafferScene.ShaderSwitch()
s["in"] # Works

script = Gaffer.ScriptNode()
script["s"] = GafferScene.ShaderSwitch()
script["s"]["in"] # Fails
```

The compatibility we have now is sufficient for loading old files, but will fail for any custom code which has the latter form above. The existence of such code seems unlikely since to support current shader parameters (rather than RSL coshaders) a call to `setup()` is required. But I felt it was important to document this in case...
SwitchUI : Improve input nodule spacing
This now matches the spacing for the old ImageSwitch/SceneSwitch nodes.
@andrewkaufman
Copy link
Member

left a comment

The code seems fine, but I have yet to successfully test it on a production scene (due to varied reliance on the removed compatibility configs throughout the ie codebase). Hopefully I'll be able to confirm tomorrow.

Show resolved Hide resolved include/GafferScene/Filter.h
Show resolved Hide resolved src/GafferScene/FilterProcessor.cpp Outdated

@johnhaddon johnhaddon force-pushed the johnhaddon:switchRefactorRebase3 branch from 32aa2b6 to d455746 Sep 27, 2018

ArrayPlug : Remove FilterPlug compatibility hack
In c113e2f we updated `FilterProcessor::sceneAffectsMatch()` to assume that all input plugs were FilterPlugs and not the IntPlugs used prior to version 0.28.0.0. This commit follows suit, turning any attempt to load an obsolete setup into a loading error.

Breaking Changes :

- UnionFilter : Removed compatibility for nodes created prior to version 0.28.0.0.
@andrewkaufman

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

I've tested on a few production scenes with no issues.

We do have one odd production failure using a SwitchComputeNode which seems to still fail with this branch, but I don't think that should hold up merging particularly.

@andrewkaufman

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

So I have a one line patch on top of this branch that fixes the production failure, but I'm not sure its the right thing to do.

diff --git a/src/Gaffer/Switch.cpp b/src/Gaffer/Switch.cpp
index ccf693a..276ef74 100644
--- a/src/Gaffer/Switch.cpp
+++ b/src/Gaffer/Switch.cpp
@@ -283,7 +283,7 @@ void Switch::plugSet( Plug *plug )
 
 void Switch::plugInputChanged( Plug *plug )
 {
-       if( plug == indexPlug() || plug == enabledPlug() )
+       if( plug == indexPlug() || plug == enabledPlug() || plug == outPlug() )
        {
                updateInternalConnection();
        }

The problem seems to be that the serialized file contains a switch with an index driven indirectly by an expression. The serialization includes a setInput call on the out plug (presumably because of the indirect connection) and the script-load never triggers updateInternalConnection(). My patch here forces updateInternalConnection() to be called when that setInput gets deserialized.

But should the setInput have ever existed in the first place? Is that a legacy thing that no longer happens? That would at least explain why I haven't been able to write a test that reproduces the failure...

And as an aside, why doesn't setup() get serialized? My first idea was to add updateInternalConnection() to setup() assuming it'd be deserialized after everything else (like it is for BoxIOs). But upon investigation setup() doesn't serialized for switches or dots, only for BoxIOs (and only when they are copy/pasted on their own?).

@johnhaddon

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2018

Thanks for digging in to that one. That switch is definitely in an odd state, because if an expression is driving the index, there should be no internal connection to the out plug. That's dealt with in updateInternalConnection(), where we disconnect if variesWithContext( indexPlug() ). The patch might work to get us out of that state, but the true bug must be elsewhere - wherever we got into that state in the first place. Is it possible that it wasn't the switch that made that state, but instead it was done by some custom code meddling with the switch? If you manually remove the bad connection from the bad setup, and serialise and reload, does it remain removed, or come back somehow? I'm reluctant to merge the patch because it's basically illogical - "if the input to the out plug has changed, then update the input to the out plug".

The serialisation of setup() on the other hand would be a great improvement. Just the other day I witnessed someone doing a "manual setup" using code they'd cut+pasted out of a serialisation, when they should have been calling setup() instead. That was for a BoxIO actually, because although that does have a call to setup() in the serialisation, it still relies on addChild() to make the plugs. Ideally all our "setupable" nodes would serialise just the setup() call and not use addChild() at all. I suggest we make a separate ticket for that, since it'll take a bit of working out, and it's also best done after the ContextProcessor overhaul (the followup to this PR), since that will involve adding a setup() method to ContextProcessors too.

@andrewkaufman

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

I'm reluctant to merge the patch because it's basically illogical

Yeah, makes sense. I didn't think it was a mergable suggestion, just the only way I found to fix the problem scene currently. We only have one problem scene and the person has already worked around it by using an expression instead of a switch.

Is it possible that it wasn't the switch that made that state, but instead it was done by some custom code meddling with the switch?

It seems unlikely to me. Its just a Switch sitting inside a Box. It is switching StringPlugs which are connected to custom nodes that might do such meddling, but I don't think that'd affect the serialization of the Switch itself. Perhaps one thing is that the Switch.index is connected to a BoolPlug promoted on a Reference. The BoolPlug is driven from an OSLExpression internal to the Reference. Can you think of any reason that could confuse variesWithContext()?

If you manually remove the bad connection from the bad setup, and serialise and reload, does it remain removed, or come back somehow?

Yes, that does fix the problem. Ok, lets forget about it until it comes up again.

@andrewkaufman andrewkaufman merged commit efaa97b into GafferHQ:master Oct 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@johnhaddon johnhaddon deleted the johnhaddon:switchRefactorRebase3 branch Oct 24, 2018

This was referenced Oct 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.