From 3a7b341c98a052f529f17fd801185ff52607addc Mon Sep 17 00:00:00 2001 From: Daniel Dresser Date: Thu, 10 Oct 2024 19:23:25 -0700 Subject: [PATCH 1/3] GafferTest::TestRunner : Accept multiple categories to exclude This matches the existing documentation in `gaffer test -help` --- python/GafferTest/TestRunner.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/GafferTest/TestRunner.py b/python/GafferTest/TestRunner.py index 7ed1ad0c889..28c3f9b4666 100644 --- a/python/GafferTest/TestRunner.py +++ b/python/GafferTest/TestRunner.py @@ -190,12 +190,12 @@ def filterCategories( test, inclusions = "*", exclusions = "" ) : testMethod = getattr( test, test._testMethodName ) ## \todo Remove `standard` fallback (breaking change). categories = getattr( testMethod, "categories", { "standard" } ) - if not any( IECore.StringAlgo.match( c, inclusions ) for c in categories ) : + if not any( IECore.StringAlgo.matchMultiple( c, inclusions ) for c in categories ) : setattr( test, test._testMethodName, unittest.skip( f"Categories not included by `{inclusions}`" )( testMethod ) ) - elif any( IECore.StringAlgo.match( c, exclusions ) for c in categories ) : + elif any( IECore.StringAlgo.matchMultiple( c, exclusions ) for c in categories ) : setattr( test, test._testMethodName, unittest.skip( f"Categories excluded by `{exclusions}`" )( testMethod ) From b29be3f543683a61b0cc6c12e29741c4406af686 Mon Sep 17 00:00:00 2001 From: Daniel Dresser Date: Thu, 10 Oct 2024 17:51:51 -0700 Subject: [PATCH 2/3] Instancer : Support Int64VectorData for ids. --- Changes.md | 3 + python/GafferSceneTest/InstancerTest.py | 80 ++++++++++++-- src/GafferScene/Instancer.cpp | 139 +++++++++++++++++------- 3 files changed, 172 insertions(+), 50 deletions(-) diff --git a/Changes.md b/Changes.md index 5217cbb8811..f2d97dd7d4a 100644 --- a/Changes.md +++ b/Changes.md @@ -1,7 +1,10 @@ 1.5.x.x (relative to 1.5.0.1) ======= +Improvements +------------ +- Instancer : Added support for 64 bit ints for ids ( matching what is loaded from USD ). 1.5.0.1 (relative to 1.5.0.0) ======= diff --git a/python/GafferSceneTest/InstancerTest.py b/python/GafferSceneTest/InstancerTest.py index 818fad3dea3..6e70822c760 100644 --- a/python/GafferSceneTest/InstancerTest.py +++ b/python/GafferSceneTest/InstancerTest.py @@ -1498,11 +1498,17 @@ def testSetsWithDeepPrototypeRoots( self ) : ) def testIds( self ) : + with self.subTest( useInt64 = False ): + self.runTestIds( False ) + with self.subTest( useInt64 = True ): + self.runTestIds( True ) + + def runTestIds( self, useInt64 ) : points = IECoreScene.PointsPrimitive( IECore.V3fVectorData( [ imath.V3f( x, 0, 0 ) for x in range( 0, 4 ) ] ) ) points["id"] = IECoreScene.PrimitiveVariable( IECoreScene.PrimitiveVariable.Interpolation.Vertex, - IECore.IntVectorData( [ 10, 100, 111, 5 ] ), + ( IECore.Int64VectorData if useInt64 else IECore.IntVectorData)( [ 10, 100, 111, 5 ] ), ) points["index"] = IECoreScene.PrimitiveVariable( IECoreScene.PrimitiveVariable.Interpolation.Vertex, @@ -1560,16 +1566,16 @@ def testIds( self ) : self.assertEncapsulatedRendersSame( instancer ) - def testNegativeIdsAndIndices( self ) : + def testExtremeIdsAndIndices( self ) : - points = IECoreScene.PointsPrimitive( IECore.V3fVectorData( [ imath.V3f( x, 0, 0 ) for x in range( 0, 2 ) ] ) ) + points = IECoreScene.PointsPrimitive( IECore.V3fVectorData( [ imath.V3f( x, 0, 0 ) for x in range( 0, 4 ) ] ) ) points["id"] = IECoreScene.PrimitiveVariable( IECoreScene.PrimitiveVariable.Interpolation.Vertex, - IECore.IntVectorData( [ -10, -5 ] ), + IECore.Int64VectorData( [ -10, -5, 8000000000, 8000000001 ] ), ) points["index"] = IECoreScene.PrimitiveVariable( IECoreScene.PrimitiveVariable.Interpolation.Vertex, - IECore.IntVectorData( [ -1, -2 ] ), + IECore.IntVectorData( [ -1, -2, 1, 0 ] ), ) objectToScene = GafferScene.ObjectToScene() @@ -1582,24 +1588,57 @@ def testNegativeIdsAndIndices( self ) : instances["children"][0].setInput( cube["out"] ) instances["parent"].setValue( "/" ) + allFilter = GafferScene.PathFilter() + allFilter["paths"].setValue( IECore.StringVectorData( [ '/*' ] ) ) + + customAttributes = GafferScene.CustomAttributes() + customAttributes["in"].setInput( instances["out"] ) + customAttributes["filter"].setInput( allFilter["out"] ) + customAttributes["attributes"].addChild( Gaffer.NameValuePlug( "intAttr", Gaffer.IntPlug( "value", flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ), True, "member1" ) ) + + customAttributes["ReadContextExpression"] = Gaffer.Expression() + customAttributes["ReadContextExpression"].setExpression( + 'parent["attributes"]["member1"]["value"] = context.get( "seed", -1 )' + ) + instancer = GafferScene.Instancer() instancer["in"].setInput( objectToScene["out"] ) - instancer["prototypes"].setInput( instances["out"] ) - instancer["parent"].setValue( "/object" ) + instancer["prototypes"].setInput( customAttributes["out"] ) + instancer["filter"].setInput( allFilter["out"] ) instancer["prototypeIndex"].setValue( "index" ) instancer["id"].setValue( "id" ) + instancer["seedEnabled"].setValue( True ) + instancer["rawSeed"].setValue( True ) self.assertEqual( instancer["out"].childNames( "/object/instances" ), IECore.InternedStringVectorData( [ "sphere", "cube" ] ) ) - self.assertEqual( instancer["out"].childNames( "/object/instances/sphere" ), IECore.InternedStringVectorData( [ "-5" ] ) ) - self.assertEqual( instancer["out"].childNames( "/object/instances/cube" ), IECore.InternedStringVectorData( [ "-10" ] ) ) + self.assertEqual( instancer["out"].childNames( "/object/instances/sphere" ), IECore.InternedStringVectorData( [ "-5", "8000000001" ] ) ) + self.assertEqual( instancer["out"].childNames( "/object/instances/cube" ), IECore.InternedStringVectorData( [ "-10", "8000000000" ] ) ) self.assertEqual( instancer["out"].childNames( "/object/instances/sphere/-5" ), IECore.InternedStringVectorData() ) self.assertEqual( instancer["out"].childNames( "/object/instances/cube/-10" ), IECore.InternedStringVectorData() ) + self.assertEqual( instancer["out"].childNames( "/object/instances/sphere/8000000001" ), IECore.InternedStringVectorData() ) + self.assertEqual( instancer["out"].childNames( "/object/instances/cube/8000000000" ), IECore.InternedStringVectorData() ) self.assertEqual( instancer["out"].object( "/object/instances" ), IECore.NullObject.defaultNullObject() ) self.assertEqual( instancer["out"].object( "/object/instances/sphere" ), IECore.NullObject.defaultNullObject() ) self.assertEqual( instancer["out"].object( "/object/instances/cube" ), IECore.NullObject.defaultNullObject() ) self.assertEqual( instancer["out"].object( "/object/instances/sphere/-5" ), sphere["out"].object( "/sphere" ) ) self.assertEqual( instancer["out"].object( "/object/instances/cube/-10" ), cube["out"].object( "/cube" ) ) + self.assertEqual( instancer["out"].object( "/object/instances/sphere/8000000001" ), sphere["out"].object( "/sphere" ) ) + self.assertEqual( instancer["out"].object( "/object/instances/cube/8000000000" ), cube["out"].object( "/cube" ) ) + + self.assertEqual( instancer["out"].attributes( "/object/instances/sphere/-5" )["intAttr"].value, -5 ) + self.assertEqual( instancer["out"].attributes( "/object/instances/cube/-10" )["intAttr"].value, -10 ) + + # We want to fully support int64 typed ids, but for reasons of backwards compatiblity and OSL support, + # we're still using int32 for the seed context variable, so these ids get wrapped around even in raw seeds + # mode. + self.assertEqual( instancer["out"].attributes( "/object/instances/sphere/8000000001" )["intAttr"].value, -589934591 ) + self.assertEqual( instancer["out"].attributes( "/object/instances/cube/8000000000" )["intAttr"].value, -589934592 ) + + self.assertEqual( + instancer["variations"].getValue(), + IECore.CompoundData( { 'seed' : IECore.IntData( 4 ), '' : IECore.IntData( 4 ) } ) + ) self.assertSceneValid( instancer["out"] ) @@ -2430,6 +2469,29 @@ def quant( x, q ): self.assertEncapsulatedRendersSame( instancer ) + + # We get different results if we change the id the seeds are based on + points["idTest"] = IECoreScene.PrimitiveVariable( + IECoreScene.PrimitiveVariable.Interpolation.Vertex, IECore.IntVectorData( + [ i * 37 for i in range( 100 ) ] + ) + ) + pointsSource["object"].setValue( points ) + instancer["id"].setValue( "idTest" ) + self.assertEqual( uniqueCounts(), { "floatVar" : 5, "color4fVar" : 4, "seed" : 64, "" : 98 } ) + + # Works the same using int64 ids + points["idTest"] = IECoreScene.PrimitiveVariable( + IECoreScene.PrimitiveVariable.Interpolation.Vertex, IECore.Int64VectorData( + [ i * 37 for i in range( 100 ) ] + ) + ) + pointsSource["object"].setValue( points ) + self.assertEqual( uniqueCounts(), { "floatVar" : 5, "color4fVar" : 4, "seed" : 64, "" : 98 } ) + + instancer["id"].setToDefault() + + # Now turn on time offset as well and play with everything together instancer["seeds"].setValue( 10 ) instancer["timeOffset"]["enabled"].setValue( True ) diff --git a/src/GafferScene/Instancer.cpp b/src/GafferScene/Instancer.cpp index 97ed413e554..035001b10ce 100644 --- a/src/GafferScene/Instancer.cpp +++ b/src/GafferScene/Instancer.cpp @@ -264,16 +264,71 @@ struct UniqueHashPrototypeContextVariable } }; +InternedString g_prototypeRootName( "root" ); +ConstInternedStringVectorDataPtr g_emptyNames = new InternedStringVectorData(); + +struct IdData +{ + IdData() : + intElements( nullptr ), int64Elements( nullptr ) + { + } + + void initialize( const Primitive *primitive, const std::string &name ) + { + if( const IntVectorData *intData = primitive->variableData( name ) ) + { + intElements = &intData->readable(); + } + else if( const Int64VectorData *int64Data = primitive->variableData( name ) ) + { + int64Elements = &int64Data->readable(); + } + + } + + size_t size() const + { + if( intElements ) + { + return intElements->size(); + } + else if( int64Elements ) + { + return int64Elements->size(); + } + else + { + return 0; + } + } + + int64_t element( size_t i ) const + { + if( intElements ) + { + return (*intElements)[i]; + } + else + { + return (*int64Elements)[i]; + } + } + + const std::vector *intElements; + const std::vector *int64Elements; + +}; + // We create a seed integer that corresponds to the id by hashing the id and then modulo'ing to // numSeeds, to create seeds in the range 0 .. numSeeds-1 that persistently correspond to the ids, // with a grouping pattern that can be changed with seedScramble -int seedForPoint( int index, const PrimitiveVariable *primVar, int numSeeds, int seedScramble ) +int seedForPoint( size_t index, const IdData &idData, int numSeeds, int seedScramble ) { - int id = index; - if( primVar ) + int64_t id = index; + if( idData.size() ) { - // TODO - the exception this will throw on non-int primvars may not be very clear to users - id = PrimitiveVariable::IndexedView( *primVar )[index]; + id = idData.element( index ); } // numSeeds is set to 0 when we're just passing through the id @@ -293,16 +348,25 @@ int seedForPoint( int index, const PrimitiveVariable *primVar, int numSeeds, int IECore::MurmurHash seedHash; seedHash.append( seedScramble ); - seedHash.append( id ); + + if( id <= INT32_MAX && id >= INT_MIN ) + { + // This branch shouldn't be needed, we'd like to just treat ids as 64 bit now ... + // but if we just took the branch below, that would changing the seeding of existing + // scenes. + seedHash.append( (int)id ); + } + else + { + seedHash.append( id ); + } + id = int( ( double( seedHash.h1() ) / double( UINT64_MAX ) ) * double( numSeeds ) ); id = id % numSeeds; // For the rare case h1 / max == 1.0, make sure we stay in range } return id; } -InternedString g_prototypeRootName( "root" ); -ConstInternedStringVectorDataPtr g_emptyNames = new InternedStringVectorData(); - } ////////////////////////////////////////////////////////////////////////// @@ -337,7 +401,6 @@ class Instancer::EngineData : public Data m_numPrototypes( 0 ), m_numValidPrototypes( 0 ), m_prototypeIndices( nullptr ), - m_ids( nullptr ), m_positions( nullptr ), m_orientations( nullptr ), m_scales( nullptr ), @@ -352,13 +415,10 @@ class Instancer::EngineData : public Data initPrototypes( mode, prototypeIndexName, rootsVariable, rootsList, prototypes ); - if( const IntVectorData *ids = m_primitive->variableData( idName ) ) + m_ids.initialize( m_primitive.get(), idName ); + if( m_ids.size() && m_ids.size() != numPoints() ) { - m_ids = &ids->readable(); - if( m_ids->size() != numPoints() ) - { - throw IECore::Exception( fmt::format( "Id primitive variable \"{}\" has incorrect size", idName ) ); - } + throw IECore::Exception( fmt::format( "Id primitive variable \"{}\" has incorrect size", idName ) ); } if( const V3fVectorData *p = m_primitive->variableData( position ) ) @@ -396,11 +456,11 @@ class Instancer::EngineData : public Data } } - if( m_ids ) + if( m_ids.size() ) { for( size_t i = 0, e = numPoints(); i < e; ++i ) { - int id = (*m_ids)[i]; + int64_t id = m_ids.element(i); auto ins = m_idsToPointIndices.try_emplace( id, i ); if( !ins.second ) { @@ -441,16 +501,16 @@ class Instancer::EngineData : public Data return m_primitive ? m_primitive->variableSize( PrimitiveVariable::Vertex ) : 0; } - size_t instanceId( size_t pointIndex ) const + int64_t instanceId( size_t pointIndex ) const { - return m_ids ? (*m_ids)[pointIndex] : pointIndex; + return m_ids.size() ? m_ids.element( pointIndex ) : pointIndex; } - size_t pointIndex( size_t i ) const + size_t pointIndex( int64_t i ) const { - if( !m_ids ) + if( !m_ids.size() ) { - if( i >= numPoints() ) + if( i >= (int64_t)numPoints() || i < 0 ) { throw IECore::Exception( fmt::format( "Instance id \"{}\" is invalid, instancer produces only {} children. Topology may have changed during shutter.", i, numPoints() ) ); } @@ -488,7 +548,7 @@ class Instancer::EngineData : public Data // If there are duplicates in the id list, then some point indices will be omitted - we // need to check each point index to see if it got assigned an id correctly - int id = (*m_ids)[pointIndex]; + int64_t id = m_ids.element( pointIndex ); if( m_idsToPointIndices.at(id) != pointIndex ) { @@ -632,7 +692,7 @@ class Instancer::EngineData : public Data if( v.seedMode ) { - scope.setAllocated( v.name, seedForPoint( pointIndex, v.primVar, v.numSeeds, v.seedScramble ) ); + scope.setAllocated( v.name, seedForPoint( pointIndex, m_ids, v.numSeeds, v.seedScramble ) ); continue; } @@ -660,7 +720,7 @@ class Instancer::EngineData : public Data { if( v.seedMode ) { - result.append( seedForPoint( pointIndex, v.primVar, v.numSeeds, v.seedScramble ) ); + result.append( seedForPoint( pointIndex, m_ids, v.numSeeds, v.seedScramble ) ); return; } @@ -915,13 +975,13 @@ class Instancer::EngineData : public Data std::vector m_prototypeIndexRemap; std::vector m_prototypeIndicesAlloc; const std::vector *m_prototypeIndices; - const std::vector *m_ids; + IdData m_ids; const std::vector *m_positions; const std::vector *m_orientations; const std::vector *m_scales; const std::vector *m_uniformScales; - using IdsToPointIndices = std::unordered_map ; + using IdsToPointIndices = std::unordered_map ; IdsToPointIndices m_idsToPointIndices; boost::container::flat_map m_attributeCreators; @@ -1695,7 +1755,7 @@ void Instancer::hash( const Gaffer::ValuePlug *output, const Gaffer::Context *co for( size_t i = r.begin(); i != r.end(); ++i ) { const size_t pointIndex = pointIndicesForPrototype[i]; - size_t instanceId = engine->instanceId( pointIndex ); + int64_t instanceId = engine->instanceId( pointIndex ); engine->setPrototypeContextVariables( pointIndex, scope ); IECore::MurmurHash instanceH; instanceH.append( instanceId ); @@ -1791,15 +1851,12 @@ void Instancer::compute( Gaffer::ValuePlug *output, const Gaffer::Context *conte if( seedContextName != "" ) { - const PrimitiveVariable *idPrimVar = findVertexVariable( primitive.get(), idPlug()->getValue() ); - if( idPrimVar && idPrimVar->data->typeId() != IntVectorDataTypeId ) - { - idPrimVar = nullptr; - } - int seeds = rawSeedPlug()->getValue() ? 0 : seedsPlug()->getValue(); int seedScramble = seedPermutationPlug()->getValue(); - prototypeContextVariables.push_back( { seedContextName, idPrimVar, 0, false, true, seeds, seedScramble } ); + + // We set seedMode to true here, which means rather than reading a given primvar, this context + // variable will be driven by whatever is driving id. + prototypeContextVariables.push_back( { seedContextName, nullptr, 0, false, true, seeds, seedScramble } ); } if( timeOffsetEnabled ) @@ -1948,7 +2005,7 @@ void Instancer::compute( Gaffer::ValuePlug *output, const Gaffer::Context *conte for( size_t i = r.begin(); i != r.end(); ++i ) { const size_t pointIndex = pointIndicesForPrototype[i]; - size_t instanceId = engine->instanceId( pointIndex ); + int64_t instanceId = engine->instanceId( pointIndex ); engine->setPrototypeContextVariables( pointIndex, scope ); ConstPathMatcherDataPtr instanceSet = prototypesPlug()->setPlug()->getValue(); PathMatcher pointInstanceSet = instanceSet->readable().subTree( *prototypeRoot ); @@ -2411,7 +2468,7 @@ IECore::ConstInternedStringVectorDataPtr Instancer::computeBranchChildNames( con // the ids, not the point indices, and must be sorted. So we need to allocate a // temp buffer of integer ids, before converting to strings. - std::vector ids; + std::vector ids; ids.reserve( pointIndicesForPrototype.size() ); const EngineData *engineData = esp->engine(); @@ -2427,7 +2484,7 @@ IECore::ConstInternedStringVectorDataPtr Instancer::computeBranchChildNames( con InternedStringVectorDataPtr childNamesData = new InternedStringVectorData; std::vector &childNames = childNamesData->writable(); childNames.reserve( ids.size() ); - for( size_t id : ids ) + for( int64_t id : ids ) { childNames.emplace_back( id ); } @@ -2987,7 +3044,7 @@ void Instancer::InstancerCapsule::render( IECoreScenePreview::Renderer *renderer attribs = proto->m_rendererAttributes.get(); } - int instanceId = engines[0]->instanceId( pointIndex ); + int64_t instanceId = engines[0]->instanceId( pointIndex ); if( !namePrefixLengths[protoIndex] ) @@ -3008,7 +3065,7 @@ void Instancer::InstancerCapsule::render( IECoreScenePreview::Renderer *renderer // up being named when they use the non-encapsulated hierarchy. std::string &name = names[ protoIndex ]; const int prefixLen = namePrefixLengths[ protoIndex ]; - name.resize( namePrefixLengths[protoIndex] + std::numeric_limits< int >::digits10 + 1 ); + name.resize( namePrefixLengths[protoIndex] + std::numeric_limits< int64_t >::digits10 + 1 ); name.resize( std::to_chars( &name[prefixLen], &(*name.end()), instanceId ).ptr - &name[0] ); IECoreScenePreview::Renderer::ObjectInterfacePtr objectInterface; From 85b334a181aef16e288f6f7b6c14ccaf54dba586 Mon Sep 17 00:00:00 2001 From: Daniel Dresser Date: Tue, 15 Oct 2024 19:42:23 -0700 Subject: [PATCH 3/3] Instancer : Document that "parent" plug no longer fully works --- python/GafferSceneUI/InstancerUI.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/python/GafferSceneUI/InstancerUI.py b/python/GafferSceneUI/InstancerUI.py index a6e7287b352..950f3e0a73d 100644 --- a/python/GafferSceneUI/InstancerUI.py +++ b/python/GafferSceneUI/InstancerUI.py @@ -345,12 +345,9 @@ def __init__( self, headings, toolTipOverride = "" ) : "description", """ - The object on which to make the instances. The - position, orientation and scale of the instances - are taken from per-vertex primitive variables on - this object. This is ignored when a filter is - connected, in which case the filter specifies - multiple objects to make the instances from. + Using the `parent` plug to select the source is now deprecated, please use a filter instead. + This plug is still supported for backwards compatibility, but is incompatible with recent features, + like accurately reporting variation counts. """, "layout:section", "Settings.General",