From 21675ae8e458b466d69028e2078487409549e78a Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 24 Jan 2025 12:33:24 +0000 Subject: [PATCH 1/6] SceneWriterTest : Remove unused includes --- python/GafferSceneTest/SceneWriterTest.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/python/GafferSceneTest/SceneWriterTest.py b/python/GafferSceneTest/SceneWriterTest.py index 43a5be14ed6..9f76735a247 100644 --- a/python/GafferSceneTest/SceneWriterTest.py +++ b/python/GafferSceneTest/SceneWriterTest.py @@ -35,7 +35,6 @@ ########################################################################## import unittest -import threading import inspect import imath @@ -43,7 +42,6 @@ import IECoreScene import Gaffer -import GafferTest import GafferDispatch import GafferScene import GafferSceneTest From 52227aae3e559459cd48dff688546830821e95e9 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 24 Jan 2025 15:05:00 +0000 Subject: [PATCH 2/6] SceneWriterTest : Add subtests for different file formats There was a heavy bias towards testing with `.scc` files, but those are not particularly relevant to the majority of Gaffer users. --- python/GafferSceneTest/SceneWriterTest.py | 239 +++++++++++----------- 1 file changed, 121 insertions(+), 118 deletions(-) diff --git a/python/GafferSceneTest/SceneWriterTest.py b/python/GafferSceneTest/SceneWriterTest.py index 9f76735a247..15e65f10e14 100644 --- a/python/GafferSceneTest/SceneWriterTest.py +++ b/python/GafferSceneTest/SceneWriterTest.py @@ -48,6 +48,11 @@ class SceneWriterTest( GafferSceneTest.SceneTestCase ) : + __extensions = [ + f".{x}" for x in IECoreScene.SceneInterface.supportedExtensions( IECore.IndexedIO.Write ) + if x not in [ "usdz" ] ## \todo Fix registration in IECoreUSD - `.usdz` isn't supported for writing + ] + def testWrite( self ) : s = GafferScene.Sphere() @@ -63,15 +68,16 @@ def testWrite( self ) : writer = GafferScene.SceneWriter() script["writer"] = writer writer["in"].setInput( g["out"] ) - writer["fileName"].setValue( self.temporaryDirectory() / "test.scc" ) - - writer.execute() - sc = IECoreScene.SceneCache( str( self.temporaryDirectory() / "test.scc" ), IECore.IndexedIO.OpenMode.Read ) + for extension in self.__extensions : + with self.subTest( extension = extension ) : - t = sc.child( "group" ) + writer["fileName"].setValue( self.temporaryDirectory() / ("test" + extension) ) + writer.execute() - self.assertEqual( t.readTransformAsMatrix( 0 ), imath.M44d().translate( imath.V3d( 5, 0, 2 ) ) ) + sc = IECoreScene.SceneInterface.create( str( writer["fileName"].getValue() ), IECore.IndexedIO.OpenMode.Read ) + t = sc.child( "group" ) + self.assertEqual( t.readTransformAsMatrix( 0 ), imath.M44d().translate( imath.V3d( 5, 0, 2 ) ) ) def testWriteAnimation( self ) : @@ -85,64 +91,71 @@ def testWriteAnimation( self ) : script["zExpression"].setExpression( 'parent["group"]["transform"]["translate"]["z"] = context.getFrame() * 2' ) script["writer"] = GafferScene.SceneWriter() script["writer"]["in"].setInput( script["group"]["out"] ) - script["writer"]["fileName"].setValue( self.temporaryDirectory() / "test.scc" ) - - with Gaffer.Context() : - script["writer"].executeSequence( [ 1, 1.5, 2 ] ) - - sc = IECoreScene.SceneCache( str( self.temporaryDirectory() / "test.scc" ), IECore.IndexedIO.OpenMode.Read ) - t = sc.child( "group" ) - - self.assertEqual( t.readTransformAsMatrix( 0 ), imath.M44d().translate( imath.V3d( 1, 0, 2 ) ) ) - self.assertEqual( t.readTransformAsMatrix( 1 / 24.0 ), imath.M44d().translate( imath.V3d( 1, 0, 2 ) ) ) - self.assertEqual( t.readTransformAsMatrix( 1.5 / 24.0 ), imath.M44d().translate( imath.V3d( 1.5, 0, 3 ) ) ) - self.assertEqual( t.readTransformAsMatrix( 2 / 24.0 ), imath.M44d().translate( imath.V3d( 2, 0, 4 ) ) ) - - def testSceneCacheRoundtrip( self ) : - - scene = IECoreScene.SceneCache( str( self.temporaryDirectory() / "fromPython.scc" ), IECore.IndexedIO.OpenMode.Write ) - sc = scene.createChild( "a" ) - sc.writeObject( IECoreScene.MeshPrimitive.createBox(imath.Box3f(imath.V3f(0),imath.V3f(1))), 0 ) - matrix = imath.M44d().translate( imath.V3d( 1, 0, 0 ) ).rotate( imath.V3d( 0, 0, IECore.degreesToRadians( -30 ) ) ) - sc.writeTransform( IECore.M44dData( matrix ), 0 ) - sc = sc.createChild( "b" ) - sc.writeObject( IECoreScene.MeshPrimitive.createBox(imath.Box3f(imath.V3f(0),imath.V3f(1))), 0 ) - sc.writeTransform( IECore.M44dData( matrix ), 0 ) - sc = sc.createChild( "c" ) - sc.writeObject( IECoreScene.MeshPrimitive.createBox(imath.Box3f(imath.V3f(0),imath.V3f(1))), 0 ) - sc.writeTransform( IECore.M44dData( matrix ), 0 ) - - del scene, sc - - def testCacheFile( f ) : - sc = IECoreScene.SceneCache( str( f ), IECore.IndexedIO.OpenMode.Read ) - a = sc.child( "a" ) - self.assertTrue( a.hasObject() ) - self.assertIsInstance( a.readObject( 0 ), IECoreScene.MeshPrimitive ) - self.assertTrue( a.readTransformAsMatrix( 0 ).equalWithAbsError( matrix, 1e-6 ) ) - b = a.child( "b" ) - self.assertTrue( b.hasObject() ) - self.assertIsInstance( b.readObject( 0 ), IECoreScene.MeshPrimitive ) - self.assertTrue( b.readTransformAsMatrix( 0 ).equalWithAbsError( matrix, 1e-6 ) ) - c = b.child( "c" ) - self.assertTrue( c.hasObject() ) - self.assertIsInstance( c.readObject( 0 ), IECoreScene.MeshPrimitive ) - self.assertTrue( c.readTransformAsMatrix( 0 ).equalWithAbsError( matrix, 1e-6 ) ) - - testCacheFile( self.temporaryDirectory() / "fromPython.scc" ) - reader = GafferScene.SceneReader() - reader["fileName"].setValue( self.temporaryDirectory() / "fromPython.scc" ) + for extension in self.__extensions : + with self.subTest( extension = extension ) : - script = Gaffer.ScriptNode() - writer = GafferScene.SceneWriter() - script["writer"] = writer - writer["in"].setInput( reader["out"] ) - writer["fileName"].setValue( self.temporaryDirectory() / "test.scc" ) - writer.execute() + script["writer"]["fileName"].setValue( self.temporaryDirectory() / ("test" + extension) ) + script["writer"].executeSequence( [ 1, 1.5, 2 ] ) + + + sc = IECoreScene.SceneInterface.create( script["writer"]["fileName"].getValue(), IECore.IndexedIO.OpenMode.Read ) + t = sc.child( "group" ) + + self.assertTrue( t.readTransformAsMatrix( 0 ).equalWithAbsError( imath.M44d().translate( imath.V3d( 1, 0, 2 ) ), 1e-6 ) ) + self.assertTrue( t.readTransformAsMatrix( 1 / 24.0 ).equalWithAbsError( imath.M44d().translate( imath.V3d( 1, 0, 2 ) ), 1e-6 ) ) + self.assertTrue( t.readTransformAsMatrix( 1.5 / 24.0 ).equalWithAbsError( imath.M44d().translate( imath.V3d( 1.5, 0, 3 ) ), 1e-6 ) ) + self.assertTrue( t.readTransformAsMatrix( 2 / 24.0 ).equalWithAbsError( imath.M44d().translate( imath.V3d( 2, 0, 4 ) ), 1e-6 ) ) + + def testSceneReaderRoundtrip( self ) : + + for extension in self.__extensions : + with self.subTest( extension = extension ) : + + filePath = self.temporaryDirectory() / ("fromPython" + extension) + + scene = IECoreScene.SceneInterface.create( str( filePath ), IECore.IndexedIO.OpenMode.Write ) + sc = scene.createChild( "a" ) + sc.writeObject( IECoreScene.MeshPrimitive.createBox(imath.Box3f(imath.V3f(0),imath.V3f(1))), 0 ) + matrix = imath.M44d().translate( imath.V3d( 1, 0, 0 ) ).rotate( imath.V3d( 0, 0, IECore.degreesToRadians( -30 ) ) ) + sc.writeTransform( IECore.M44dData( matrix ), 0 ) + sc = sc.createChild( "b" ) + sc.writeObject( IECoreScene.MeshPrimitive.createBox(imath.Box3f(imath.V3f(0),imath.V3f(1))), 0 ) + sc.writeTransform( IECore.M44dData( matrix ), 0 ) + sc = sc.createChild( "c" ) + sc.writeObject( IECoreScene.MeshPrimitive.createBox(imath.Box3f(imath.V3f(0),imath.V3f(1))), 0 ) + sc.writeTransform( IECore.M44dData( matrix ), 0 ) + + del scene, sc + + def testCacheFile( f ) : + sc = IECoreScene.SceneInterface.create( str( filePath ), IECore.IndexedIO.OpenMode.Read ) + a = sc.child( "a" ) + self.assertTrue( a.hasObject() ) + self.assertIsInstance( a.readObject( 0 ), IECoreScene.MeshPrimitive ) + self.assertTrue( a.readTransformAsMatrix( 0 ).equalWithAbsError( matrix, 1e-6 ) ) + b = a.child( "b" ) + self.assertTrue( b.hasObject() ) + self.assertIsInstance( b.readObject( 0 ), IECoreScene.MeshPrimitive ) + self.assertTrue( b.readTransformAsMatrix( 0 ).equalWithAbsError( matrix, 1e-6 ) ) + c = b.child( "c" ) + self.assertTrue( c.hasObject() ) + self.assertIsInstance( c.readObject( 0 ), IECoreScene.MeshPrimitive ) + self.assertTrue( c.readTransformAsMatrix( 0 ).equalWithAbsError( matrix, 1e-6 ) ) + + testCacheFile( filePath ) + + reader = GafferScene.SceneReader() + reader["fileName"].setValue( filePath ) - testCacheFile( self.temporaryDirectory() / "test.scc" ) + script = Gaffer.ScriptNode() + writer = GafferScene.SceneWriter() + script["writer"] = writer + writer["in"].setInput( reader["out"] ) + writer["fileName"].setValue( self.temporaryDirectory() / "written.scc" ) + writer.execute() + testCacheFile( writer["fileName"].getValue() ) def testCanWriteSets( self ): @@ -159,14 +172,12 @@ def testCanWriteSets( self ): sphereGroup["in"][0].setInput( s["out"] ) sphereGroup["name"].setValue( 'sphereGroup' ) - sn = GafferScene.Set( "Set" ) script.addChild( sn ) sn["paths"].setValue( IECore.StringVectorData( [ '/sphereGroup' ] ) ) sn["name"].setValue( 'foo' ) sn["in"].setInput( sphereGroup["out"] ) - sn2 = GafferScene.Set( "Set" ) script.addChild( sn2 ) sn2["mode"].setValue( 1 ) # add to the existing set @@ -181,30 +192,28 @@ def testCanWriteSets( self ): g["in"][0].setInput( sn2["out"] ) g["in"][1].setInput( c["out"] ) - writer = GafferScene.SceneWriter() - script.addChild( writer ) - - script["writer"] = writer - writer["in"].setInput( g["out"] ) - writer["fileName"].setValue( self.temporaryDirectory() / "setTest.scc" ) - - writer.execute() - - sc = IECoreScene.SceneCache( str( self.temporaryDirectory() / "setTest.scc" ), IECore.IndexedIO.OpenMode.Read ) + script["writer"] = GafferScene.SceneWriter() + script["writer"]["in"].setInput( g["out"] ) - scGroup = sc.child("group") - scSphereGroup = scGroup.child("sphereGroup") - scSphere = scSphereGroup.child("sphere") + for extension in set( self.__extensions ) - { ".abc" } : # Sets not implemented for Alembic + with self.subTest( extension = extension ) : - self.assertEqual( scGroup.readTags(), [] ) + script["writer"]["fileName"].setValue( self.temporaryDirectory() / ("setTest" + extension) ) + script["writer"].execute() - self.assertEqual( scSphereGroup.readTags(), [ IECore.InternedString("foo") ] ) + sc = IECoreScene.SceneInterface.create( str( script["writer"]["fileName"].getValue() ), IECore.IndexedIO.OpenMode.Read ) + scGroup = sc.child( "group" ) + scSphereGroup = scGroup.child( "sphereGroup" ) + scSphere = scSphereGroup.child( "sphere" ) - self.assertEqual( set (scSphere.readTags() ), set([IECore.InternedString("foo"), IECore.InternedString("ObjectType:MeshPrimitive")])) + extraSets = [ IECore.InternedString( "ObjectType:MeshPrimitive" ) ] if extension in ( ".scc", ".lscc" ) else [] - scCube = scGroup.child("cube") - self.assertEqual( scCube.readTags() , [ IECore.InternedString("ObjectType:MeshPrimitive") ] ) + self.assertEqual( scGroup.readTags(), [] ) + self.assertEqual( scSphereGroup.readTags(), [ IECore.InternedString( "foo" ) ] ) + self.assertEqual( set( scSphere.readTags() ), set( [ IECore.InternedString( "foo" ) ] + extraSets ) ) + scCube = scGroup.child( "cube" ) + self.assertEqual( scCube.readTags(), extraSets ) def testHash( self ) : @@ -275,20 +284,6 @@ def testPassThroughSerialisation( self ) : ss = s.serialise() self.assertFalse( "out" in ss ) - def testAlembic( self ) : - - p = GafferScene.Plane() - - w = GafferScene.SceneWriter() - w["in"].setInput( p["out"] ) - w["fileName"].setValue( self.temporaryDirectory() / "test.abc" ) - w["task"].execute() - - r = GafferScene.SceneReader() - r["fileName"].setInput( w["fileName"] ) - - self.assertScenesEqual( p["out"], r["out"] ) - def testFileNameWithArtificalFrameDependency( self ) : script = Gaffer.ScriptNode() @@ -404,12 +399,17 @@ def testChildNamesOrder( self ) : writer = GafferScene.SceneWriter() writer["in"].setInput( duplicate["out"] ) - writer["fileName"].setValue( self.temporaryDirectory() / "test.abc" ) - writer["task"].execute() reader = GafferScene.SceneReader() reader["fileName"].setInput( writer["fileName"] ) - self.assertScenesEqual( reader["out"], writer["in"] ) + + for extension in set( self.__extensions ) - { ".scc", ".lscc" } : # SceneCache doesn't preserve order + with self.subTest( extension = extension ) : + + writer["fileName"].setValue( self.temporaryDirectory() / ( "test" + extension ) ) + writer["task"].execute() + + self.assertScenesEqual( reader["out"], writer["in"], checks = { "childNames" } ) def testAnimatedSets( self ) : @@ -428,15 +428,9 @@ def testAnimatedSets( self ) : script["fileWriter"] = GafferScene.SceneWriter() script["fileWriter"]["in"].setInput( script["sphere"]["out"] ) - script["fileWriter"]["fileName"].setValue( self.temporaryDirectory() / "testSingle.usd" ) - script["fileWriter"]["task"].executeSequence( range( 1, 10 ) ) script["sequenceWriter"] = GafferScene.SceneWriter() script["sequenceWriter"]["in"].setInput( script["sphere"]["out"] ) - script["sequenceWriter"]["fileName"].setValue( self.temporaryDirectory() / "testSingle.usd" ) - script["sequenceWriter"]["task"].executeSequence( range( 1, 10 ) ) - script["sequenceWriter"]["fileName"].setValue( self.temporaryDirectory() / "sequence.#.usd" ) - script["sequenceWriter"]["task"].executeSequence( range( 1, 10 ) ) fileReader = GafferScene.SceneReader() fileReader["fileName"].setInput( script["fileWriter"]["fileName"] ) @@ -444,21 +438,30 @@ def testAnimatedSets( self ) : sequenceReader = GafferScene.SceneReader() sequenceReader["fileName"].setInput( script["sequenceWriter"]["fileName"] ) - with Gaffer.Context() as c : - for f in range( 1, 10 ) : - c.setFrame( f ) - # Sets were only written once for the single file, so we - # expect them to be taken from frame 1. - self.assertIn( "A", fileReader["out"].setNames() ) - self.assertNotIn( "B", fileReader["out"].setNames() ) - # For the file-per-frame sequence, we expect exactly the - # right sets. - if f % 2 : - self.assertIn( "A", sequenceReader["out"].setNames() ) - self.assertNotIn( "B", sequenceReader["out"].setNames() ) - else : - self.assertNotIn( "A", sequenceReader["out"].setNames() ) - self.assertIn( "B", sequenceReader["out"].setNames() ) + for extension in set( self.__extensions ) - { ".abc" } : # Sets not supported for Alembix + with self.subTest( extension = extension ) : + + script["fileWriter"]["fileName"].setValue( self.temporaryDirectory() / ( "testSingle" + extension ) ) + script["fileWriter"]["task"].executeSequence( range( 1, 10 ) ) + + script["sequenceWriter"]["fileName"].setValue( self.temporaryDirectory() / ( "sequence.#" + extension ) ) + script["sequenceWriter"]["task"].executeSequence( range( 1, 10 ) ) + + with Gaffer.Context() as c : + for f in range( 1, 10 ) : + c.setFrame( f ) + # Sets were only written once for the single file, so we + # expect them to be taken from frame 1. + self.assertIn( "A", fileReader["out"].setNames() ) + self.assertNotIn( "B", fileReader["out"].setNames() ) + # For the file-per-frame sequence, we expect exactly the + # right sets. + if f % 2 : + self.assertIn( "A", sequenceReader["out"].setNames() ) + self.assertNotIn( "B", sequenceReader["out"].setNames() ) + else : + self.assertNotIn( "A", sequenceReader["out"].setNames() ) + self.assertIn( "B", sequenceReader["out"].setNames() ) def testWriteInvalidUSDChildName( self ) : From 6762ad579f3703ed8151a027b7a1ce46c9616634 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 24 Jan 2025 15:43:36 +0000 Subject: [PATCH 3/6] SceneWriterTest : Add test coverage for `gaffer:globals` I do not know why this feature exists. It was introduced in 269cd4cea0857935c4e6ce3c244da8fc7125767c, which purported to be about something completely different. I believe it can only possibly work with the `.scc` format. It isn't matched by similiar functionality in the SceneReader, which doesn't make any attempt to load globals. So I can only assume it was needed internally at Image Engine at some point in time - perhaps for Caribou? I have no idea if it is still needed or not. For now, adding a unit test so I don't break the feature during refactoring. But I suspect it should be removed at some point in the future. --- python/GafferSceneTest/SceneWriterTest.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/python/GafferSceneTest/SceneWriterTest.py b/python/GafferSceneTest/SceneWriterTest.py index 15e65f10e14..ce7970083cc 100644 --- a/python/GafferSceneTest/SceneWriterTest.py +++ b/python/GafferSceneTest/SceneWriterTest.py @@ -499,5 +499,22 @@ def testWriteAnimationWithInvalidUSDChildName( self ) : context.setFrame( frame ) self.assertPathsEqual( reader["out"], "/_1", sphere["out"], "/1" ) + def testWriteGlobals( self ) : + + # Tests a feature that only works with `.scc`, isn't used by SceneReader, + # and has no documented purpose. Perhaps used at Image Engine? + + options = GafferScene.StandardOptions() + options["options"]["renderCamera"]["enabled"].setValue( True ) + options["options"]["renderCamera"]["value"].setValue( "/camera" ) + + writer = GafferScene.SceneWriter() + writer["in"].setInput( options["out"] ) + writer["fileName"].setValue( self.temporaryDirectory() / "test.scc" ) + writer["task"].execute() + + scene = IECoreScene.SceneCache( writer["fileName"].getValue(), IECore.IndexedIO.Read ) + self.assertEqual( scene.readAttribute( "gaffer:globals", 1 ), writer["in"].globals() ) + if __name__ == "__main__": unittest.main() From 43e6414d3cf74e46197a9ca2243da8d2c0e4e830 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 24 Jan 2025 15:12:51 +0000 Subject: [PATCH 4/6] SceneWriter : Prefer EditableScope and range-for --- src/GafferScene/SceneWriter.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/GafferScene/SceneWriter.cpp b/src/GafferScene/SceneWriter.cpp index c6adac88fff..75cf32cc9a7 100644 --- a/src/GafferScene/SceneWriter.cpp +++ b/src/GafferScene/SceneWriter.cpp @@ -266,12 +266,11 @@ void SceneWriter::executeSequence( const std::vector &frames ) const SceneInterfacePtr output; tbb::mutex mutex; - ContextPtr context = new Context( *Context::current() ); - Context::Scope scopedContext( context.get() ); + Context::EditableScope scope( Context::current() ); - for( std::vector::const_iterator it = frames.begin(); it != frames.end(); ++it ) + for( auto frame : frames ) { - context->setFrame( *it ); + scope.setFrame( frame ); ConstCompoundDataPtr sets; bool useSetsAPI = true; @@ -284,7 +283,7 @@ void SceneWriter::executeSequence( const std::vector &frames ) const useSetsAPI = SceneReader::useSetsAPI( output.get() ); } - LocationWriter locationWriter( output.get(), !useSetsAPI ? sets.get() : nullptr, context->getTime(), mutex ); + LocationWriter locationWriter( output.get(), !useSetsAPI ? sets.get() : nullptr, scope.context()->getTime(), mutex ); SceneAlgo::parallelProcessLocations( scene, locationWriter ); if( useSetsAPI && sets ) From f81c017dc0f96294e3087c258aba01f4f09d7c55 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Mon, 27 Jan 2025 12:24:08 +0000 Subject: [PATCH 5/6] SceneAlgo : Add `parallelGatherLocations()` function --- Changes.md | 5 ++ include/GafferScene/SceneAlgo.h | 11 +++ include/GafferScene/SceneAlgo.inl | 81 ++++++++++++++++++++++ python/GafferSceneTest/SceneAlgoTest.py | 78 +++++++++++++++++++++ src/GafferSceneModule/SceneAlgoBinding.cpp | 45 ++++++++++++ 5 files changed, 220 insertions(+) diff --git a/Changes.md b/Changes.md index 27ffb02f737..b9dff611ca2 100644 --- a/Changes.md +++ b/Changes.md @@ -6,6 +6,11 @@ Fixes - ContactSheetCore : Fixed bugs handling changes to the input and output image formats. +API +--- + +- SceneAlgo : Added `parallelGatherLocations()` function. + 1.5.4.0 (relative to 1.5.3.0) ======= diff --git a/include/GafferScene/SceneAlgo.h b/include/GafferScene/SceneAlgo.h index db34153973e..15f691e05a2 100644 --- a/include/GafferScene/SceneAlgo.h +++ b/include/GafferScene/SceneAlgo.h @@ -139,6 +139,17 @@ void filteredParallelTraverse( const ScenePlug *scene, const FilterPlug *filterP template void filteredParallelTraverse( const ScenePlug *scene, const IECore::PathMatcher &filter, ThreadableFunctor &f, const ScenePlug::ScenePath &root = ScenePlug::ScenePath() ); +/// Calls `locationFunctor` in parallel for all locations in the scene, passing the results +/// serially to `gatherFunctor`. Parent locations are guaranteed to be passed to `gatherFunctor` +/// before their children, but otherwise the order of execution is unspecified. +template +void parallelGatherLocations( + const ScenePlug *scene, + LocationFunctor &&locationFunctor, // Signature : T locationFunctor( const ScenePlug *scene, const ScenePath &path ) + GatherFunctor &&gatherFunctor, // Signature : void gatherFunctor( T &locationFunctorResult ) + const ScenePlug::ScenePath &root = ScenePlug::ScenePath() +); + /// Searching /// ========= diff --git a/include/GafferScene/SceneAlgo.inl b/include/GafferScene/SceneAlgo.inl index 723ec13b501..4cf21db1af3 100644 --- a/include/GafferScene/SceneAlgo.inl +++ b/include/GafferScene/SceneAlgo.inl @@ -36,8 +36,12 @@ #include "Gaffer/Context.h" +#include "tbb/concurrent_queue.h" #include "tbb/enumerable_thread_specific.h" #include "tbb/parallel_for.h" +#include "tbb/task_arena.h" + +#include namespace GafferScene { @@ -180,6 +184,83 @@ void filteredParallelTraverse( const ScenePlug *scene, const IECore::PathMatcher parallelTraverse( scene, ff, root ); } + +template +void parallelGatherLocations( const ScenePlug *scene, LocationFunctor &&locationFunctor, GatherFunctor &&gatherFunctor, const ScenePlug::ScenePath &root ) +{ + // We use `parallelTraverse()` to run `locationFunctor`, passing the results to + // `gatherFunctor` on the current thread via a queue. In testing, this proved to + // have lower overhead than using TBB's `parallel_pipeline()`. + + using LocationResult = std::invoke_result_t; + using QueueValue = std::variant; + tbb::concurrent_bounded_queue queue; + queue.set_capacity( tbb::this_task_arena::max_concurrency() ); + + IECore::Canceller traverseCanceller; + auto locationFunctorWrapper = [&] ( const ScenePlug *scene, const ScenePlug::ScenePath &path ) { + IECore::Canceller::check( &traverseCanceller ); + queue.push( std::move( locationFunctor( scene, path ) ) ); + return true; + }; + + tbb::task_arena( tbb::task_arena::attach() ).enqueue( + + [&, &threadState = Gaffer::ThreadState::current()] () { + + Gaffer::ThreadState::Scope threadStateScope( threadState ); + try + { + SceneAlgo::parallelTraverse( scene, locationFunctorWrapper, root ); + } + catch( ... ) + { + queue.push( std::current_exception() ); + return; + } + queue.push( std::monostate() ); + } + + ); + + while( true ) + { + QueueValue value; + queue.pop( value ); + if( auto locationResult = std::get_if( &value ) ) + { + try + { + gatherFunctor( *locationResult ); + } + catch( ... ) + { + // We can't rethrow until the `parallelTraverse()` has + // completed, as it references the `queue` and + // `traverseCanceller` from this stack frame. + traverseCanceller.cancel(); + while( true ) + { + queue.pop( value ); + if( std::get_if( &value ) || std::get_if( &value ) ) + { + throw; + } + } + } + } + else if( auto exception = std::get_if( &value ) ) + { + std::rethrow_exception( *exception ); + } + else + { + // We use `monostate` to signal completion. + break; + } + } +} + template IECore::PathMatcher findAll( const ScenePlug *scene, Predicate &&predicate, const ScenePlug::ScenePath &root ) { diff --git a/python/GafferSceneTest/SceneAlgoTest.py b/python/GafferSceneTest/SceneAlgoTest.py index c1c29248d43..71523a840cb 100644 --- a/python/GafferSceneTest/SceneAlgoTest.py +++ b/python/GafferSceneTest/SceneAlgoTest.py @@ -2422,6 +2422,84 @@ def testFindAllWithAttribute( self ) : IECore.PathMatcher( [ "/group/light1" ] ) ) + def testParallelGatherLocations( self ) : + + plane = GafferScene.Plane() + group = GafferScene.Group() + group["in"][0].setInput( plane["out"] ) + + groupFilter = GafferScene.PathFilter() + groupFilter["paths"].setValue( IECore.StringVectorData( [ "/group" ] ) ) + + duplicate = GafferScene.Duplicate() + duplicate["in"].setInput( group["out"] ) + duplicate["filter"].setInput( groupFilter["out"] ) + duplicate["copies"].setValue( 100 ) + + gathered = [] + GafferScene.SceneAlgo.parallelGatherLocations( + + duplicate["out"], + + lambda scene, path : path, + lambda path : gathered.append( path ) + + ) + + # We expect to have visited all locations. + + expected = set( + [ "/", "/group", "/group/plane" ] + + [ f"/group{x}" for x in range( 1, 101 ) ] + + [ f"/group{x}/plane" for x in range( 1, 101 ) ] + ) + self.assertEqual( set( gathered ), expected ) + + # And we expect to have visited parent locations + # before child locations. + + indices = { + value : index + for index, value in enumerate( gathered ) + } + + self.assertEqual( gathered[0], "/" ) + self.assertGreater( indices["/group/plane"], indices["/group"] ) + + for i in range( 1, 101 ) : + self.assertGreater( indices[f"/group{i}/plane"], indices[f"/group{i}"] ) + + def testParallelGatherExceptionHandling( self ) : + + plane = GafferScene.Plane() + + with self.assertRaisesRegex( ZeroDivisionError, "division by zero" ) : + + GafferScene.SceneAlgo.parallelGatherLocations( + + plane["out"], + lambda scene, path : path, + lambda x : 1/0 + + ) + + def testParallelGatherLocationExceptionHandling( self ) : + + plane = GafferScene.Plane() + + gathered = [] + with self.assertRaisesRegex( Exception, "ZeroDivisionError" ) : + + GafferScene.SceneAlgo.parallelGatherLocations( + + plane["out"], + lambda scene, path : 1/0, + lambda x : gathered.append( x ) + + ) + + self.assertEqual( gathered, [] ) + def tearDown( self ) : GafferSceneTest.SceneTestCase.tearDown( self ) diff --git a/src/GafferSceneModule/SceneAlgoBinding.cpp b/src/GafferSceneModule/SceneAlgoBinding.cpp index de9cb95e2c1..c4e5ea4a143 100644 --- a/src/GafferSceneModule/SceneAlgoBinding.cpp +++ b/src/GafferSceneModule/SceneAlgoBinding.cpp @@ -129,6 +129,49 @@ IECore::MurmurHash matchingPathsHashWrapper2( const IECore::PathMatcher &filter, return SceneAlgo::matchingPathsHash( filter, &scene ); } +std::shared_ptr withGILAcquireDeleter( const boost::python::object &o ) +{ + return std::shared_ptr( + new boost::python::object( o ), + []( boost::python::object *o ) { + // Custom deleter. We must hold the GIL when deleting Python + // objects. + IECorePython::ScopedGILLock gilLock; + delete o; + } + ); +} + +void parallelGatherLocationsWrapper( const ScenePlug &scene, object locationFunctor, object gatherFunctor, const ScenePlug::ScenePath &root ) +{ + IECorePython::ScopedGILRelease gilRelease; + + SceneAlgo::parallelGatherLocations( + &scene, + // Per-location functor + [&] ( ConstScenePlugPtr scene, const ScenePlug::ScenePath &path ) { + const std::string pathString = ScenePlug::pathToString( path ); + IECorePython::ScopedGILLock gilLock; + try + { + object result = locationFunctor( boost::const_pointer_cast( scene ), pathString ); + return withGILAcquireDeleter( result ); + } + catch( const boost::python::error_already_set & ) + { + IECorePython::ExceptionAlgo::translatePythonException(); + }; + }, + // Gather functor + [&] ( const std::shared_ptr &locationResult ) { + IECorePython::ScopedGILLock gilLock; + gatherFunctor( *locationResult ); + }, + root + ); + +} + IECore::PathMatcher findAllWrapper( const ScenePlug &scene, object predicate, const ScenePlug::ScenePath &root ) { IECorePython::ScopedGILRelease gilRelease; @@ -383,6 +426,8 @@ void bindSceneAlgo() def( "matchingPathsHash", &matchingPathsHashWrapper1, ( arg( "filter" ), arg( "scene" ), arg( "root" ) = "/" ) ); def( "matchingPathsHash", &matchingPathsHashWrapper2, ( arg( "filter" ), arg( "scene" ) ) ); + def( "parallelGatherLocations", ¶llelGatherLocationsWrapper, ( arg( "scene" ), arg( "locationFunctor" ), arg( "gatherFunctor" ), arg( "root" ) = "/" ) ); + def( "findAll", &findAllWrapper, ( arg( "scene" ), arg( "predicate" ), arg( "root" ) = "/" ) ); def( "findAllWithAttribute", &findAllWithAttributeWrapper, ( arg( "scene" ), arg( "name" ), arg( "value" ) = object(), arg( "root" ) = "/" ) ); From a309dfa0bd39644043cbd17c0d86a19b43bffb6f Mon Sep 17 00:00:00 2001 From: John Haddon Date: Mon, 27 Jan 2025 12:29:11 +0000 Subject: [PATCH 6/6] SceneWriter : Use `parallelGatherLocations()` This is more efficient than the previous approach of using a mutex to protect the output SceneInterface from concurent writes. This gives the following reductions in runtime for a direct SceneReader->SceneWriter process : - ALab : 60% - Moana : 50% - Market : 15% Improvements would be expected to be even greater when heavy processing is done in between the read and the write, as it provides more opportunities for parallelism. --- Changes.md | 5 + src/GafferScene/SceneWriter.cpp | 165 ++++++++++++++------------------ 2 files changed, 76 insertions(+), 94 deletions(-) diff --git a/Changes.md b/Changes.md index b9dff611ca2..c6e41330199 100644 --- a/Changes.md +++ b/Changes.md @@ -6,6 +6,11 @@ Fixes - ContactSheetCore : Fixed bugs handling changes to the input and output image formats. +Improvements +------------ + +- SceneWriter : Improved performance. Benchmarks rewriting complex scenes via a SceneReader->SceneWriter graph show around a 2x speedup. + API --- diff --git a/src/GafferScene/SceneWriter.cpp b/src/GafferScene/SceneWriter.cpp index 75cf32cc9a7..d2dbe7c912b 100644 --- a/src/GafferScene/SceneWriter.cpp +++ b/src/GafferScene/SceneWriter.cpp @@ -44,8 +44,6 @@ #include "IECoreScene/SceneInterface.h" -#include "tbb/mutex.h" - #include #include @@ -58,132 +56,90 @@ using namespace GafferScene; namespace { -struct LocationWriter +struct LocationData { - - LocationWriter( SceneInterface *rootOutput, const CompoundData *sets, float time, tbb::mutex &mutex ) - : m_parent( nullptr ), m_rootOutput( rootOutput ), m_sets( sets ), m_time( time ), m_mutex( mutex ) - { - } - - // Called by `parallelProcessLocations()` to create child functors for each location. - LocationWriter( const LocationWriter &parent ) - : m_parent( &parent ), m_rootOutput( parent.m_rootOutput ), m_sets( parent.m_sets ), m_time( parent.m_time ), m_mutex( parent.m_mutex ) - { - } - - ~LocationWriter() - { - // Some implementations of SceneInterface may perform writing when we release - // the SceneInterfaces, so we need to hold the lock while freeing them. - tbb::mutex::scoped_lock scopedLock( m_mutex ); - m_childOutputs.clear(); - } - - bool operator()( const ScenePlug *scene, const ScenePlug::ScenePath &scenePath ) + LocationData( const ScenePlug *scene, const ScenePlug::ScenePath &path, const CompoundData *setsForTags ) + : m_path( path ), + m_attributes( scene->attributesPlug()->getValue() ), + m_object( scene->objectPlug()->getValue() ), + m_bound( scene->boundPlug()->getValue() ), + m_transform( scene->transformPlug()->getValue() ), + m_childNames( scene->childNamesPlug()->getValue() ) { - // First read all the scene data for this location. We don't need the lock - // for this so we can read multiple locations in parallel. - - ConstCompoundObjectPtr attributes = scene->attributesPlug()->getValue(); - - ConstCompoundObjectPtr globals; - - ConstObjectPtr object = scene->objectPlug()->getValue(); - Imath::Box3f bound = scene->boundPlug()->getValue(); - - IECore::M44dDataPtr transformData; - - if( scenePath.empty() ) - { - globals = scene->globals(); - } - else + if( setsForTags ) { - Imath::M44f t = scene->transformPlug()->getValue(); - transformData = new IECore::M44dData( Imath::M44d ( - t[0][0], t[0][1], t[0][2], t[0][3], - t[1][0], t[1][1], t[1][2], t[1][3], - t[2][0], t[2][1], t[2][2], t[2][3], - t[3][0], t[3][1], t[3][2], t[3][3] - ) ); - } - - SceneInterface::NameList locationSets; - if( m_sets ) - { - const CompoundDataMap &setsMap = m_sets->readable(); - locationSets.reserve( setsMap.size() ); + const CompoundDataMap &setsMap = setsForTags->readable(); + m_tags.reserve( setsMap.size() ); for( const auto &[name, data] : setsMap ) { auto pathMatcher = static_cast( data.get() ); - if( pathMatcher->readable().match( scenePath ) & IECore::PathMatcher::ExactMatch ) + if( pathMatcher->readable().match( path ) & IECore::PathMatcher::ExactMatch ) { - locationSets.push_back( name ); + m_tags.push_back( name ); } } } + } - ConstInternedStringVectorDataPtr childNames = scene->childNamesPlug()->getValue(); - - // Now write the scene data to the output SceneInterface. We require - // a lock for this as SceneInterface writing is not thread-safe. - - tbb::mutex::scoped_lock scopedLock( m_mutex ); - - SceneInterface *output = m_parent ? m_parent->m_childOutputs.at( scenePath.back() ).get() : m_rootOutput; - - if( object->typeId() != IECore::NullObjectTypeId && scenePath.size() > 0 ) + void write( IECoreScene::SceneInterface *root, float time ) const + { + SceneInterfacePtr scene = root; + for( auto &p : m_path ) { - output->writeObject( object.get(), m_time ); + scene = scene->child( p, SceneInterface::CreateIfMissing ); } - output->writeBound( Imath::Box3d( Imath::V3f( bound.min ), Imath::V3f( bound.max ) ), m_time ); - - if( transformData ) + if( m_object->typeId() != IECore::NullObjectTypeId && m_path.size() > 0 ) { - output->writeTransform( transformData.get(), m_time ); + scene->writeObject( m_object.get(), time ); } - for( CompoundObject::ObjectMap::const_iterator it = attributes->members().begin(), eIt = attributes->members().end(); it != eIt; it++ ) + scene->writeBound( Imath::Box3d( Imath::V3f( m_bound.min ), Imath::V3f( m_bound.max ) ), time ); + + if( m_path.size() ) { - output->writeAttribute( it->first, it->second.get(), m_time ); + M44dDataPtr td = new IECore::M44dData( Imath::M44d ( + m_transform[0][0], m_transform[0][1], m_transform[0][2], m_transform[0][3], + m_transform[1][0], m_transform[1][1], m_transform[1][2], m_transform[1][3], + m_transform[2][0], m_transform[2][1], m_transform[2][2], m_transform[2][3], + m_transform[3][0], m_transform[3][1], m_transform[3][2], m_transform[3][3] + ) ); + scene->writeTransform( td.get(), time ); } - if( globals && !globals->members().empty() ) + for( const auto &[name, value] : m_attributes->members() ) { - output->writeAttribute( "gaffer:globals", globals.get(), m_time ); + scene->writeAttribute( name, value.get(), time ); } - if( !locationSets.empty() ) + if( !m_tags.empty() ) { - output->writeTags( locationSets ); + scene->writeTags( m_tags ); } - for( const auto &childName : childNames->readable() ) + for( const auto &childName : m_childNames->readable() ) { - // `SceneAlgo::parallelProcessLocations()` may visit children in any + // `SceneAlgo::parallelGatherLocations()` may visit children in any // order. Pre-create SceneInterface children here so that they are // created in the correct order. - m_childOutputs[childName] = output->child( childName, SceneInterface::CreateIfMissing ); + scene->child( childName, SceneInterface::CreateIfMissing ); } - - return true; } private : - const LocationWriter *m_parent; - SceneInterface *m_rootOutput; - unordered_map m_childOutputs; - const CompoundData *m_sets; - float m_time; - tbb::mutex &m_mutex; + ScenePlug::ScenePath m_path; + ConstCompoundObjectPtr m_attributes; + ConstObjectPtr m_object; + Imath::Box3f m_bound; + Imath::M44f m_transform; + ConstInternedStringVectorDataPtr m_childNames; + SceneInterface::NameList m_tags; }; -} +} // namespace GAFFER_NODE_DEFINE_TYPE( SceneWriter ); @@ -265,7 +221,6 @@ void SceneWriter::executeSequence( const std::vector &frames ) const } SceneInterfacePtr output; - tbb::mutex mutex; Context::EditableScope scope( Context::current() ); for( auto frame : frames ) @@ -283,8 +238,24 @@ void SceneWriter::executeSequence( const std::vector &frames ) const useSetsAPI = SceneReader::useSetsAPI( output.get() ); } - LocationWriter locationWriter( output.get(), !useSetsAPI ? sets.get() : nullptr, scope.context()->getTime(), mutex ); - SceneAlgo::parallelProcessLocations( scene, locationWriter ); + SceneAlgo::parallelGatherLocations( + + scene, + + // Collect LocationData from each location in parallel. + + [&] ( const ScenePlug *scene, const ScenePlug::ScenePath &path ) { + return LocationData( scene, path, useSetsAPI ? nullptr : sets.get() ); + }, + + // Write to output serially, because SceneInterfaces are not + // thread-safe for writing. + + [&] ( const LocationData &locationData ) { + locationData.write( output.get(), scope.context()->getTime() ); + } + + ); if( useSetsAPI && sets ) { @@ -293,6 +264,13 @@ void SceneWriter::executeSequence( const std::vector &frames ) const output->writeSet( name, static_cast( data.get() )->readable() ); } } + + ConstCompoundObjectPtr globals = scene->globals(); + if( !globals->members().empty() ) + { + output->writeAttribute( "gaffer:globals", globals.get(), scope.context()->getTime() ); + } + } } @@ -301,7 +279,6 @@ bool SceneWriter::requiresSequenceExecution() const return true; } - void SceneWriter::createDirectories( const std::string &fileName ) const { const std::filesystem::path filePath( fileName );