DiskBlur

[danieldresser-ie]

It's been a bit of a scramble to turn this from research code to something properly tested and reasonably cleanly layed out.

I'm now feeling fairly good about the reliability and overall organization of this code. There are definitely still some of the lower level parts that could use more comments ( like applyScanlinesHalfKernel and the compute() for scanlinesLUTPlug() ), but I know I've already spent too long cleaning this up. Hopefully the overall state is somewhere where it can be reviewed somewhat productively.

[johnhaddon]

Thanks Daniel! Seems to be working nicely in my limited testing, and at a pleasing speed.

I've taken a top-down approach to review, mostly commenting on the user-visible stuff and the overall code structure. I see you have a bunch of TODO items for documenting specific functions/sections, but I think what would be more valuable would be some higher-level introductory description about the overall strategy.

Like I say though, seems to be working well, so I don't think we should spend too much time reorganising. If my suggestions about object-orienting things like the scanline LUT don't chime with you, then let's just add some high-level commenting and move on.

Cheers...
John

[murraystevenson]

I've run into a crash in DiskBlur::computeChannelData() if you have set the planeDividers plug and then try to blur an image with no alpha channel. Disabling the Shuffle in the script below should trigger it.

import Gaffer
import GafferImage
import IECore
import imath

Gaffer.Metadata.registerValue( parent, "serialiser:milestoneVersion", 1, persistent=False )
Gaffer.Metadata.registerValue( parent, "serialiser:majorVersion", 6, persistent=False )
Gaffer.Metadata.registerValue( parent, "serialiser:minorVersion", 0, persistent=False )
Gaffer.Metadata.registerValue( parent, "serialiser:patchVersion", 0, persistent=False )

__children = {}

__children["DiskBlur"] = GafferImage.DiskBlur( "DiskBlur" )
parent.addChild( __children["DiskBlur"] )
__children["DiskBlur"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["Macaw"] = GafferImage.ImageReader( "Macaw" )
parent.addChild( __children["Macaw"] )
__children["Macaw"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["Shuffle"] = GafferImage.Shuffle( "Shuffle" )
parent.addChild( __children["Shuffle"] )
__children["Shuffle"]["shuffles"].addChild( Gaffer.ShufflePlug( "shuffle0", flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["Shuffle"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["DiskBlur"]["in"].setInput( __children["Shuffle"]["out"] )
__children["DiskBlur"]["radius"].setValue( 25.0 )
__children["DiskBlur"]["planeDividers"].setValue( IECore.FloatVectorData( [ -32, -16, -8, -4, -2, -1, 1, 2, 4, 8, 16, 32 ] ) )
__children["DiskBlur"]["__uiPosition"].setValue( imath.V2f( 2.35000038, -5.0500021 ) )
__children["Macaw"]["fileName"].setValue( '${GAFFER_ROOT}/resources/images/macaw.exr' )
__children["Macaw"]["__uiPosition"].setValue( imath.V2f( 2.35000038, 10.4500017 ) )
__children["Shuffle"]["in"].setInput( __children["Macaw"]["out"] )
__children["Shuffle"]["shuffles"]["shuffle0"]["source"].setValue( '__white' )
__children["Shuffle"]["shuffles"]["shuffle0"]["destination"].setValue( 'A' )
__children["Shuffle"]["__uiPosition"].setValue( imath.V2f( 2.35000038, 3.11406064 ) )


del __children

[danieldresser-ie]

I've run into a crash in DiskBlur::computeChannelDat

Addressed in 7b997ae

[danieldresser-ie]

Finished the documentation / cleanup pass on the low level details.

[johnhaddon]

Thanks for the update Daniel - I've gone through it all and closed the relevant conversations, and added comments to a few others.

[danieldresser-ie]

Addressed all remaining comments, plus two commits optimizing computeScanlinesLUT: 4bb69b0, 1600519, and a final commit adding a Changelog entry.

To the best of my knowledge, this should be good to merge, aside from not having been squashed yet - it will be a trivial squash, everything just goes into one commit.

[johnhaddon]

Thanks Daniel - I've replied to a couple of things inline, but I think we're close enough to merge if needs be. Seems worth getting into today's alpha release either way...

[johnhaddon]

That's vexing - any theory as to why? hashChannelData() isn't even called that often, so I'm struggling to see why this would be so at all.

[johnhaddon]

Maybe it makes sense to just have these be pass-through connections from the input

I perhaps wasn't clear enough, but by "these" I was referring to hash/computeSampleOffsets() too.

[danieldresser-ie]

Addressed final round of minor feedback, and squashed so it's ready to merge.