Attributes : Change type of extraAttributes to CompoundObjectPlug

[johnhaddon]

This allows it to express the full range of possible attributes, including shader networks. The initial motivation is for @alex-savenko-at-cinesite to be able to use it to convert large shading node graphs into simple blobs on a CustomAttributes node at lookdev publish time. This provides a halfway house between fully dynamic and fully baked lookdev.

I'm a little uncomfortable with proposing this so close to the release of 0.59 (tomorrow, if all goes well). But I have included a unit test to cover the three backwards compatibility cases I could think of - setValue( compoundData ), setInput( atomicCompoundDataPlug ) and expressions outputting CompoundData. Would welcome opinions from @andrewkaufman, @ivanimanishi and @alex-savenko-at-cinesite as to whether we should include this in 0.59 or save it for 0.60.

[themissingcow]

LGTM

[alex-savenko-at-cinesite]

The transition to 0.59 will require a lot of testing anyway, so we would not mind an extra feature to test. I am slightly concerned about forward-compatibility (saving a look or a template in 0.59 and loading it in 0.58), but it is unlikely that artists will use extraAttributes plug in their scripts.
So my vote is for 0.59 or 0.59.X.

[johnhaddon]

@themissingcow, thanks for the review. I've added one further commit - 4df9a97 - to deal with the problem we discussed earlier, whereby although we could serialise ShaderNetwork blobs, we couldn't serialise them if they were stored inside a CompoundObject.

@alex-savenko-at-cinesite, thanks for the vote for 0.59 and for the reasoning behind it. I just want to clarify that 0.59.X isn't an option, as we can't make an ABI breaking change like this in a minor version.

[andrewkaufman]

Am I correct in thinking this would be the first serialization break in 0.59? Is there any way to provide forward compatibility in 0.58 (so a gfr or grf created in 0.59 can be loaded in 0.58)?

My concern is that we're expecting 0.59 to have several small-ish bugs (related to dependency updates), and when that happens it usually means people switching back and forth several times, and an extended period where we have to lockdown studio level publishing of Boxes...

[themissingcow]

My concern is that we're expecting 0.59 to have several small-ish bugs (related to dependency updates)

I'd expect some from my programming too 🙃. Joking aside there are a lot of spreadsheet changes. It's had reasonable testing here, but you know what its like with a UI as complex as that one...

[ivanimanishi]

Am I correct in thinking this would be the first serialization break in 0.59? Is there any way to provide forward compatibility in 0.58 (so a gfr or grf created in 0.59 can be loaded in 0.58)?

I think that it would be possible to add, in a 0.58 installation, a config that does the opposite of what the config in this PR is doing:

import types

import IECore
import Gaffer
import GafferScene


def __extraAttributesSetValue( self, value ) :

	if isinstance( value, IECore.CompoundObject ) :
		value = IECore.CompoundData( {
			k : v for k, v in value.items()
		} )

	Gaffer.AtomicCompoundDataPlug.setValue( self, value )

def __attributesGetItem( originalGetItem ) :

	def getItem( self, key ) :

		result = originalGetItem( self, key )
		if key == "extraAttributes" :
			result.setValue = types.MethodType( __extraAttributesSetValue, result )
		return result

	return getItem

GafferScene.Attributes.__getitem__ = __attributesGetItem( GafferScene.Attributes.__getitem__ )

I ran a quick test here and it seems to do the job (in 0.58):

root['CustomAttributes']['extraAttributes'].setValue(IECore.CompoundObject({"a": IECore.IntData(1)}))
print(root['CustomAttributes']['extraAttributes'].getValue())
>> IECore.CompoundData({'a':IECore.IntData( 1 )})

[johnhaddon]

Am I correct in thinking this would be the first serialization break in 0.59?

Nearly. There are changes to TransformPlug/Transform2DPlug to fix serialisation of default values for references, and they won't be loadable in 0.58, because it lacks the extended constructor needed. As far as I know, the Box/Reference workflow overhaul in 0.59 still generates .grf files compatible with 0.58 otherwise, but there are substantial changes there.

Is there any way to provide forward compatibility in 0.58 (so a gfr or grf created in 0.59 can be loaded in 0.58)?

I'm hesitant to say it can be made to work perfectly for all situations, but I just checked and there's actually a fair amount of compatibility already. If you load python/GafferSceneTest/extraAttributes-0.58.5.2.gfr in Gaffer 0.59, and resave it immediately, 2 of the 3 cases tested continue to work in Gaffer 0.58. The one that fails is the raw setValue() call because in 0.58 it will be trying to do AtomicCompoundDataPlug.setValue( compoundObject ). That could be fixed easily with a shim like startup/GafferScene/attributesCompatibility.py. The other two cases work because the expression and the connection continue to output CompoundData rather than CompoundObject.

My concern is that we're expecting 0.59 to have several small-ish bugs (related to dependency updates), and when that happens it usually means people switching back and forth several times

I think that's a very reasonable concern, but I'm not sure 0.60 will provide any more certainty in that regard. And 0.59 has at least benefited from a decent beta period (mostly tested externally to Cinesite/IE admittedly).

[andrewkaufman]

If you load python/GafferSceneTest/extraAttributes-0.58.5.2.gfr in Gaffer 0.59, and resave it immediately, 2 of the 3 cases tested continue to work in Gaffer 0.58. The one that fails is the raw setValue() call because in 0.58 it will be trying to do AtomicCompoundDataPlug.setValue( compoundObject ). That could be fixed easily with a shim like startup/GafferScene/attributesCompatibility.py. The other two cases work because the expression and the connection continue to output CompoundData rather than CompoundObject.

What if you do the opposite, author the expression in Gaffer 0.59 (using CompoundObject) and load that into 0.58? Seems that case probably isn't handled now, but would be with a shim like Ivan proposed, correct?

[johnhaddon]

What if you do the opposite, author the expression in Gaffer 0.59 (using CompoundObject) and load that into 0.58? Seems that case probably isn't handled now, but would be with a shim like Ivan proposed, correct?

It needed a bit more than just the shim, but I've dealt with this case in #4049.

[johnhaddon]

Rebased to fix merge conflict.