diff --git a/include/GafferScene/SceneElementProcessor.h b/include/GafferScene/SceneElementProcessor.h index 5b63f794c32..43eadb453dc 100644 --- a/include/GafferScene/SceneElementProcessor.h +++ b/include/GafferScene/SceneElementProcessor.h @@ -45,6 +45,10 @@ namespace GafferScene /// The SceneElementProcessor class provides a base class for modifying elements of an input /// scene while leaving the scene hierarchy unchanged. +/// \todo This "all in one" base class for modifying bounds/transforms/attributes/objects +/// is feeling a bit unwieldy, and it seems that typical derived classes only ever modify +/// one thing anyway. Perhaps we'd be better off with individual TransformProcessor, +/// AttributeProcessor, ObjectProcessor and Deformer base classes. class SceneElementProcessor : public FilteredSceneProcessor { @@ -101,6 +105,8 @@ class SceneElementProcessor : public FilteredSceneProcessor virtual void hashProcessedAttributes( const ScenePath &path, const Gaffer::Context *context, IECore::MurmurHash &h ) const; virtual IECore::ConstCompoundObjectPtr computeProcessedAttributes( const ScenePath &path, const Gaffer::Context *context, IECore::ConstCompoundObjectPtr inputAttributes ) const; + /// Note that if you implement processesObject() in such a way as to deform the object, you /must/ also + /// implement processesBound() appropriately. virtual bool processesObject() const; virtual void hashProcessedObject( const ScenePath &path, const Gaffer::Context *context, IECore::MurmurHash &h ) const; virtual IECore::ConstObjectPtr computeProcessedObject( const ScenePath &path, const Gaffer::Context *context, IECore::ConstObjectPtr inputObject ) const; @@ -111,7 +117,7 @@ class SceneElementProcessor : public FilteredSceneProcessor enum BoundMethod { PassThrough = 0, - Direct = 1, + Processed = 1, Union = 2 }; diff --git a/python/GafferSceneTest/TransformTest.py b/python/GafferSceneTest/TransformTest.py index bb26255b077..5ecfde02e7a 100644 --- a/python/GafferSceneTest/TransformTest.py +++ b/python/GafferSceneTest/TransformTest.py @@ -372,5 +372,20 @@ def testResetWorldWithMatchingAncestors( self ) : IECore.M44f.createTranslated( IECore.V3f( 1, 2, 3 ) ) ) + def testObjectBoundIncludedWhenDescendantsMatch( self ) : + + s = GafferScene.Cube() + + f = GafferScene.PathFilter() + f["paths"].setValue( IECore.StringVectorData( [ "/..." ] ) ) # the dread ellipsis! + + t = GafferScene.Transform() + t["in"].setInput( s["out"] ) + t["filter"].setInput( f["out"] ) + t["transform"]["translate"].setValue( IECore.V3f( 1 ) ) + + self.assertSceneValid( t["out"] ) + self.assertEqual( t["out"].bound( "/" ), IECore.Box3f( IECore.V3f( 0.5 ), IECore.V3f( 1.5 ) ) ) + if __name__ == "__main__": unittest.main() diff --git a/src/GafferScene/SceneElementProcessor.cpp b/src/GafferScene/SceneElementProcessor.cpp index 0d30e7386b3..56167157064 100644 --- a/src/GafferScene/SceneElementProcessor.cpp +++ b/src/GafferScene/SceneElementProcessor.cpp @@ -39,6 +39,7 @@ #include "GafferScene/SceneElementProcessor.h" #include "GafferScene/Filter.h" +#include "GafferScene/SceneAlgo.h" using namespace IECore; using namespace Gaffer; @@ -91,14 +92,26 @@ void SceneElementProcessor::hashBound( const ScenePath &path, const Gaffer::Cont { switch( boundMethod( context ) ) { - case Direct : + case Processed : FilteredSceneProcessor::hashBound( path, context, parent, h ); inPlug()->boundPlug()->hash( h ); hashProcessedBound( path, context, h ); break; case Union : - h = hashOfTransformedChildBounds( path, outPlug() ); + { + ConstInternedStringVectorDataPtr childNames = inPlug()->childNamesPlug()->getValue(); + if( childNames->readable().size() ) + { + FilteredSceneProcessor::hashBound( path, context, parent, h ); + h.append( hashOfTransformedChildBounds( path, outPlug(), childNames.get() ) ); + inPlug()->objectPlug()->hash( h ); + } + else + { + h = inPlug()->boundPlug()->hash(); + } break; + } case PassThrough : h = inPlug()->boundPlug()->hash(); break; @@ -109,10 +122,33 @@ Imath::Box3f SceneElementProcessor::computeBound( const ScenePath &path, const G { switch( boundMethod( context ) ) { - case Direct : + case Processed : return computeProcessedBound( path, context, inPlug()->boundPlug()->getValue() ); case Union : - return unionOfTransformedChildBounds( path, outPlug() ); + { + // We want to return the union of all the transformed child bounds and the + // bound for the object at this location. But we want to avoid computing the + // object itself at all costs for obvious reasons - a bound should be a thing + // you compute cheaply before deciding if you want the object or not. + Imath::Box3f result; + ConstInternedStringVectorDataPtr childNames = inPlug()->childNamesPlug()->getValue(); + if( childNames->readable().size() ) + { + result = unionOfTransformedChildBounds( path, outPlug(), childNames.get() ); + // We do have to resort to computing the object here, but its exceedingly + // rare to have an object at a location which also has children, so typically + // we should be receiving a NullObject cheaply. + result.extendBy( bound( inPlug()->objectPlug()->getValue().get() ) ); + } + else + { + // Because there are no children, we know that the input bound is the + // bound of the input object on its own, and can just pass that through + // directly. + result = inPlug()->boundPlug()->getValue(); + } + return result; + } default : return inPlug()->boundPlug()->getValue(); } @@ -273,7 +309,6 @@ IECore::ConstObjectPtr SceneElementProcessor::computeProcessedObject( const Scen return inputObject; } -/// \todo This needs updating to return a bitmask now that filters return a bitmask. SceneElementProcessor::BoundMethod SceneElementProcessor::boundMethod( const Gaffer::Context *context ) const { const bool pBound = processesBound(); @@ -282,19 +317,12 @@ SceneElementProcessor::BoundMethod SceneElementProcessor::boundMethod( const Gaf if( pBound || pTransform ) { const Filter::Result f = filterValue( context ); - if( f & Filter::ExactMatch ) + if( pBound && (f & Filter::ExactMatch) ) { - if( pBound ) - { - return Direct; - } - else - { - // changing only the transform at a matched location has no effect - // on the bound of that location - fall through to default case. - } + return Processed; } - else if( f & Filter::DescendantMatch ) + + if( f & Filter::DescendantMatch ) { return Union; }