diff --git a/include/GafferScene/Instancer.h b/include/GafferScene/Instancer.h index b10c1dc8777..d6b7fccce4a 100644 --- a/include/GafferScene/Instancer.h +++ b/include/GafferScene/Instancer.h @@ -126,6 +126,9 @@ class GAFFERSCENE_API Instancer : public BranchCreator Gaffer::StringPlug *idPlug(); const Gaffer::StringPlug *idPlug() const; + Gaffer::BoolPlug *omitDuplicateIdsPlug(); + const Gaffer::BoolPlug *omitDuplicateIdsPlug() const; + Gaffer::StringPlug *positionPlug(); const Gaffer::StringPlug *positionPlug() const; diff --git a/python/GafferSceneTest/InstancerTest.py b/python/GafferSceneTest/InstancerTest.py index 22e5b7d654e..52705412f25 100644 --- a/python/GafferSceneTest/InstancerTest.py +++ b/python/GafferSceneTest/InstancerTest.py @@ -1592,32 +1592,69 @@ def testDuplicateIds( self ) : points = IECoreScene.PointsPrimitive( IECore.V3fVectorData( [ imath.V3f( x, 0, 0 ) for x in range( 6 ) ] ) ) points["id"] = IECoreScene.PrimitiveVariable( IECoreScene.PrimitiveVariable.Interpolation.Vertex, - IECore.IntVectorData( [ 0, 0, 2, 2, 4, 4 ] ), + IECore.IntVectorData( [ 0, 1, 2, 2, 3, 4 ] ), ) objectToScene = GafferScene.ObjectToScene() objectToScene["object"].setValue( points ) sphere = GafferScene.Sphere() + parent = GafferScene.Parent() + parent["parent"].setValue( "/" ) + parent["in"].setInput( sphere["out"] ) + parent["children"][0].setInput( sphere["out"] ) instancer = GafferScene.Instancer() instancer["in"].setInput( objectToScene["out"] ) - instancer["prototypes"].setInput( sphere["out"] ) + instancer["prototypes"].setInput( parent["out"] ) instancer["parent"].setValue( "/object" ) instancer["id"].setValue( "id" ) + instancer["omitDuplicateIds"].setValue( False ) + + with self.assertRaisesRegex( RuntimeError, 'Instancer.__engine : Instance id "2" is duplicated at index 2 and 3. This probably indicates invalid source data, if you want to hack around it, you can set "omitDuplicateIds"' ) : + self.assertSceneValid( instancer["out"] ) + + instancer["omitDuplicateIds"].setValue( True ) self.assertSceneValid( instancer["out"] ) - self.assertEqual( instancer["out"].childNames( "/object/instances/sphere" ), IECore.InternedStringVectorData( [ "0", "2", "4" ] ) ) + self.assertEqual( instancer["out"].childNames( "/object/instances/sphere" ), IECore.InternedStringVectorData( [ "0", "1", "3", "4" ] ) ) self.assertEqual( instancer["out"].transform( "/object/instances/sphere/0" ), imath.M44f().translate( imath.V3f( 0, 0, 0 ) ) ) - self.assertEqual( instancer["out"].transform( "/object/instances/sphere/2" ), imath.M44f().translate( imath.V3f( 2, 0, 0 ) ) ) - self.assertEqual( instancer["out"].transform( "/object/instances/sphere/4" ), imath.M44f().translate( imath.V3f( 4, 0, 0 ) ) ) + self.assertEqual( instancer["out"].transform( "/object/instances/sphere/1" ), imath.M44f().translate( imath.V3f( 1, 0, 0 ) ) ) + self.assertEqual( instancer["out"].transform( "/object/instances/sphere/3" ), imath.M44f().translate( imath.V3f( 4, 0, 0 ) ) ) + self.assertEqual( instancer["out"].transform( "/object/instances/sphere/4" ), imath.M44f().translate( imath.V3f( 5, 0, 0 ) ) ) + + self.assertEncapsulatedRendersSame( instancer ) + + instancer["prototypeIndex"].setValue( "prototypeIndex" ) + + # Test duplicate ids between different prototypes - the handling of this is now pretty consistent, but + # it used to be treated quite differently + + points["prototypeIndex"] = IECoreScene.PrimitiveVariable( + IECoreScene.PrimitiveVariable.Interpolation.Vertex, + IECore.IntVectorData( [ 0, 1, 0, 1, 0, 1 ] ), + ) + objectToScene["object"].setValue( points ) + + instancer["omitDuplicateIds"].setValue( False ) + + with self.assertRaisesRegex( RuntimeError, 'Instancer.__engine : Instance id "2" is duplicated at index 2 and 3. This probably indicates invalid source data, if you want to hack around it, you can set "omitDuplicateIds"' ) : + self.assertSceneValid( instancer["out"] ) + + instancer["omitDuplicateIds"].setValue( True ) + + self.assertEqual( instancer["out"].childNames( "/object/instances" ), IECore.InternedStringVectorData( [ "sphere", "sphere1" ] ) ) + self.assertEqual( instancer["out"].childNames( "/object/instances/sphere" ), IECore.InternedStringVectorData( [ "0", "3" ] ) ) + self.assertEqual( instancer["out"].childNames( "/object/instances/sphere1" ), IECore.InternedStringVectorData( [ "1", "4" ] ) ) + + self.assertEqual( instancer["out"].transform( "/object/instances/sphere/0" ), imath.M44f().translate( imath.V3f( 0, 0, 0 ) ) ) + self.assertEqual( instancer["out"].transform( "/object/instances/sphere/3" ), imath.M44f().translate( imath.V3f( 4, 0, 0 ) ) ) + + self.assertEqual( instancer["out"].transform( "/object/instances/sphere1/1" ), imath.M44f().translate( imath.V3f( 1, 0, 0 ) ) ) + self.assertEqual( instancer["out"].transform( "/object/instances/sphere1/4" ), imath.M44f().translate( imath.V3f( 5, 0, 0 ) ) ) - # TODO - test duplicate ids for different prototypes - this behaviour has changed. - # Currently, we take the first instance with the id, but John pointed out that if order - # changes during the shutter, this could cause weirdness. Perhaps the best solution - # is if two elements have the same id, both of them are omitted, and a warning is printed. def testAttributes( self ) : diff --git a/python/GafferSceneUI/InstancerUI.py b/python/GafferSceneUI/InstancerUI.py index 8efb5de64bc..3f82298cbb1 100644 --- a/python/GafferSceneUI/InstancerUI.py +++ b/python/GafferSceneUI/InstancerUI.py @@ -286,50 +286,50 @@ def __init__( self, headings, toolTipOverride = "" ) : "layout:customWidget:seedColumnHeadings:widgetType", "GafferSceneUI.InstancerUI._SeedColumnHeadings", "layout:customWidget:seedColumnHeadings:section", "Context Variations", - "layout:customWidget:seedColumnHeadings:index", 18, + "layout:customWidget:seedColumnHeadings:index", 19, "layout:customWidget:idContextCountSpacer:widgetType", "GafferSceneUI.InstancerUI._SeedCountSpacer", "layout:customWidget:idContextCountSpacer:section", "Context Variations", - "layout:customWidget:idContextCountSpacer:index", 19, + "layout:customWidget:idContextCountSpacer:index", 20, "layout:customWidget:idContextCountSpacer:accessory", True, "layout:customWidget:idContextCount:widgetType", "GafferSceneUI.InstancerUI._SeedCountWidget", "layout:customWidget:idContextCount:section", "Context Variations", - "layout:customWidget:idContextCount:index", 19, + "layout:customWidget:idContextCount:index", 20, "layout:customWidget:idContextCount:accessory", True, "layout:customWidget:seedVariableSpacer:widgetType", "GafferSceneUI.InstancerUI._VariationSpacer", "layout:customWidget:seedVariableSpacer:section", "Context Variations", - "layout:customWidget:seedVariableSpacer:index", 20, + "layout:customWidget:seedVariableSpacer:index", 21, "layout:customWidget:seedVariableSpacer:accessory", True, "layout:customWidget:seedsSpacer:widgetType", "GafferSceneUI.InstancerUI._VariationSpacer", "layout:customWidget:seedsSpacer:section", "Context Variations", - "layout:customWidget:seedsSpacer:index", 21, + "layout:customWidget:seedsSpacer:index", 22, "layout:customWidget:seedsSpacer:accessory", True, "layout:customWidget:seedPermutationSpacer:widgetType", "GafferSceneUI.InstancerUI._VariationSpacer", "layout:customWidget:seedPermutationSpacer:section", "Context Variations", - "layout:customWidget:seedPermutationSpacer:index", 22, + "layout:customWidget:seedPermutationSpacer:index", 23, "layout:customWidget:seedPermutationSpacer:accessory", True, "layout:customWidget:seedSpacer:widgetType", "GafferSceneUI.InstancerUI._SectionSpacer", "layout:customWidget:seedSpacer:section", "Context Variations", - "layout:customWidget:seedSpacer:index", 23, + "layout:customWidget:seedSpacer:index", 24, "layout:customWidget:timeOffsetHeadings:widgetType", "GafferSceneUI.InstancerUI._TimeOffsetColumnHeadings", "layout:customWidget:timeOffsetHeadings:section", "Context Variations", - "layout:customWidget:timeOffsetHeadings:index", 24, + "layout:customWidget:timeOffsetHeadings:index", 25, "layout:customWidget:timeOffsetHeadings:description", "Testing description", "layout:customWidget:timeOffsetSpacer:widgetType", "GafferSceneUI.InstancerUI._SectionSpacer", "layout:customWidget:timeOffsetSpacer:section", "Context Variations", - "layout:customWidget:timeOffsetSpacer:index", 25, + "layout:customWidget:timeOffsetSpacer:index", 26, "layout:customWidget:timeOffsetSpacer:divider", True, "layout:customWidget:totalSpacer:widgetType", "GafferSceneUI.InstancerUI._SectionSpacer", "layout:customWidget:totalSpacer:section", "Context Variations", - "layout:customWidget:totalSpacer:index", 26, + "layout:customWidget:totalSpacer:index", 27, plugs = { @@ -483,6 +483,22 @@ def __init__( self, headings, toolTipOverride = "" ) : ], + "omitDuplicateIds" : [ + + "description", + """ + When false, having the same ids on multiple points is considered + an error. Setting to True will allow a render to proceed, with any + points that share an id all removed. + """, + + "layout:section", "Settings.General", + + # \todo : Uncomment for Gaffer 1.4 + # "userDefault", False, + + ], + "position" : [ "description", diff --git a/src/GafferScene/Instancer.cpp b/src/GafferScene/Instancer.cpp index a9f1ddd23fd..6d603c4a7c8 100644 --- a/src/GafferScene/Instancer.cpp +++ b/src/GafferScene/Instancer.cpp @@ -364,6 +364,7 @@ class Instancer::EngineData : public Data const StringVectorData *rootsList, const ScenePlug *prototypes, const std::string &idName, + bool omitDuplicateIds, const std::string &position, const std::string &orientation, const std::string &scale, @@ -468,8 +469,8 @@ class Instancer::EngineData : public Data } } + bool hasDuplicates = false; std::vector< std::vector > instancesForPrototypeIndex( m_numPrototypes ); - if( constantPrototypeIndex == -1 || m_ids ) { if( m_ids ) @@ -482,18 +483,63 @@ class Instancer::EngineData : public Data for( size_t i = 0, e = numPoints(); i < e; ++i ) { int id = (*m_ids)[i]; - if( m_idsToPointIndices.try_emplace( id, i ).second ) + auto ins = m_idsToPointIndices.try_emplace( id, i ); + if( !ins.second ) { - if( constantPrototypeIndex != -1 ) + if( !omitDuplicateIds ) { - instancesForPrototypeIndex[ constantPrototypeIndex ].push_back( i ); - continue; + throw IECore::Exception( fmt::format( "Instance id \"{}\" is duplicated at index {} and {}. This probably indicates invalid source data, if you want to hack around it, you can set \"omitDuplicateIds\".", id, m_idsToPointIndices[id], i ) ); } + hasDuplicates = true; + ins.first->second = std::numeric_limits::max(); + } + + if( hasDuplicates ) + { + continue; + } + + if( constantPrototypeIndex != -1 ) + { + instancesForPrototypeIndex[ constantPrototypeIndex ].push_back( i ); + continue; + } + + int prototypeIndex = m_prototypeIndexRemap[ (*m_prototypeIndices)[ i ] % m_numPrototypes ]; + if( prototypeIndex != -1 ) + { + instancesForPrototypeIndex[ prototypeIndex ].push_back( i ); + } + } - int prototypeIndex = m_prototypeIndexRemap[ (*m_prototypeIndices)[ i ] % m_numPrototypes ]; - if( prototypeIndex != -1 ) + if( hasDuplicates ) + { + // We have duplicates, and haven't thrown an exception, so we need to omit the duplicates. + // To accomplish this, we need to start over on instancesForPrototypeIndex + for( auto &i : instancesForPrototypeIndex ) + { + i.resize( 0 ); + } + + // Now rebuild, only including indices with valid entries in m_idsToPointIndices + for( size_t i = 0, e = numPoints(); i < e; ++i ) + { + int id = (*m_ids)[i]; + + if( m_idsToPointIndices[id] == i ) { - instancesForPrototypeIndex[ prototypeIndex ].push_back( i ); + if( constantPrototypeIndex != -1 ) + { + instancesForPrototypeIndex[ constantPrototypeIndex ].push_back( i ); + continue; + } + + int prototypeIndex = m_prototypeIndexRemap[ (*m_prototypeIndices)[ i ] % m_numPrototypes ]; + if( prototypeIndex != -1 ) + { + instancesForPrototypeIndex[ prototypeIndex ].push_back( i ); + continue; + } } } } @@ -1162,6 +1208,7 @@ Instancer::Instancer( const std::string &name ) addChild( new StringPlug( "prototypeRoots", Plug::In, "prototypeRoots" ) ); addChild( new StringVectorDataPlug( "prototypeRootsList", Plug::In, new StringVectorData ) ); addChild( new StringPlug( "id", Plug::In, "instanceId" ) ); + addChild( new BoolPlug( "omitDuplicateIds", Plug::In, true ) ); addChild( new StringPlug( "position", Plug::In, "P" ) ); addChild( new StringPlug( "orientation", Plug::In ) ); addChild( new StringPlug( "scale", Plug::In ) ); @@ -1265,174 +1312,184 @@ const Gaffer::StringPlug *Instancer::idPlug() const return getChild( g_firstPlugIndex + 6 ); } +Gaffer::BoolPlug *Instancer::omitDuplicateIdsPlug() +{ + return getChild( g_firstPlugIndex + 7 ); +} + +const Gaffer::BoolPlug *Instancer::omitDuplicateIdsPlug() const +{ + return getChild( g_firstPlugIndex + 7 ); +} + Gaffer::StringPlug *Instancer::positionPlug() { - return getChild( g_firstPlugIndex + 7 ); + return getChild( g_firstPlugIndex + 8 ); } const Gaffer::StringPlug *Instancer::positionPlug() const { - return getChild( g_firstPlugIndex + 7 ); + return getChild( g_firstPlugIndex + 8 ); } Gaffer::StringPlug *Instancer::orientationPlug() { - return getChild( g_firstPlugIndex + 8 ); + return getChild( g_firstPlugIndex + 9 ); } const Gaffer::StringPlug *Instancer::orientationPlug() const { - return getChild( g_firstPlugIndex + 8 ); + return getChild( g_firstPlugIndex + 9 ); } Gaffer::StringPlug *Instancer::scalePlug() { - return getChild( g_firstPlugIndex + 9 ); + return getChild( g_firstPlugIndex + 10 ); } const Gaffer::StringPlug *Instancer::scalePlug() const { - return getChild( g_firstPlugIndex + 9 ); + return getChild( g_firstPlugIndex + 10 ); } Gaffer::StringPlug *Instancer::attributesPlug() { - return getChild( g_firstPlugIndex + 10 ); + return getChild( g_firstPlugIndex + 11 ); } const Gaffer::StringPlug *Instancer::attributesPlug() const { - return getChild( g_firstPlugIndex + 10 ); + return getChild( g_firstPlugIndex + 11 ); } Gaffer::StringPlug *Instancer::attributePrefixPlug() { - return getChild( g_firstPlugIndex + 11 ); + return getChild( g_firstPlugIndex + 12 ); } const Gaffer::StringPlug *Instancer::attributePrefixPlug() const { - return getChild( g_firstPlugIndex + 11 ); + return getChild( g_firstPlugIndex + 12 ); } Gaffer::BoolPlug *Instancer::encapsulateInstanceGroupsPlug() { - return getChild( g_firstPlugIndex + 12 ); + return getChild( g_firstPlugIndex + 13 ); } const Gaffer::BoolPlug *Instancer::encapsulateInstanceGroupsPlug() const { - return getChild( g_firstPlugIndex + 12 ); + return getChild( g_firstPlugIndex + 13 ); } Gaffer::BoolPlug *Instancer::seedEnabledPlug() { - return getChild( g_firstPlugIndex + 13 ); + return getChild( g_firstPlugIndex + 14 ); } const Gaffer::BoolPlug *Instancer::seedEnabledPlug() const { - return getChild( g_firstPlugIndex + 13 ); + return getChild( g_firstPlugIndex + 14 ); } Gaffer::StringPlug *Instancer::seedVariablePlug() { - return getChild( g_firstPlugIndex + 14 ); + return getChild( g_firstPlugIndex + 15 ); } const Gaffer::StringPlug *Instancer::seedVariablePlug() const { - return getChild( g_firstPlugIndex + 14 ); + return getChild( g_firstPlugIndex + 15 ); } Gaffer::IntPlug *Instancer::seedsPlug() { - return getChild( g_firstPlugIndex + 15 ); + return getChild( g_firstPlugIndex + 16 ); } const Gaffer::IntPlug *Instancer::seedsPlug() const { - return getChild( g_firstPlugIndex + 15 ); + return getChild( g_firstPlugIndex + 16 ); } Gaffer::IntPlug *Instancer::seedPermutationPlug() { - return getChild( g_firstPlugIndex + 16 ); + return getChild( g_firstPlugIndex + 17 ); } const Gaffer::IntPlug *Instancer::seedPermutationPlug() const { - return getChild( g_firstPlugIndex + 16 ); + return getChild( g_firstPlugIndex + 17 ); } Gaffer::BoolPlug *Instancer::rawSeedPlug() { - return getChild( g_firstPlugIndex + 17 ); + return getChild( g_firstPlugIndex + 18 ); } const Gaffer::BoolPlug *Instancer::rawSeedPlug() const { - return getChild( g_firstPlugIndex + 17 ); + return getChild( g_firstPlugIndex + 18 ); } Gaffer::ValuePlug *Instancer::contextVariablesPlug() { - return getChild( g_firstPlugIndex + 18 ); + return getChild( g_firstPlugIndex + 19 ); } const Gaffer::ValuePlug *Instancer::contextVariablesPlug() const { - return getChild( g_firstPlugIndex + 18 ); + return getChild( g_firstPlugIndex + 19 ); } GafferScene::Instancer::ContextVariablePlug *Instancer::timeOffsetPlug() { - return getChild( g_firstPlugIndex + 19 ); + return getChild( g_firstPlugIndex + 20 ); } const GafferScene::Instancer::ContextVariablePlug *Instancer::timeOffsetPlug() const { - return getChild( g_firstPlugIndex + 19 ); + return getChild( g_firstPlugIndex + 20 ); } Gaffer::AtomicCompoundDataPlug *Instancer::variationsPlug() { - return getChild( g_firstPlugIndex + 20 ); + return getChild( g_firstPlugIndex + 21 ); } const Gaffer::AtomicCompoundDataPlug *Instancer::variationsPlug() const { - return getChild( g_firstPlugIndex + 20 ); + return getChild( g_firstPlugIndex + 21 ); } Gaffer::ObjectPlug *Instancer::enginePlug() { - return getChild( g_firstPlugIndex + 21 ); + return getChild( g_firstPlugIndex + 22 ); } const Gaffer::ObjectPlug *Instancer::enginePlug() const { - return getChild( g_firstPlugIndex + 21 ); + return getChild( g_firstPlugIndex + 22 ); } GafferScene::ScenePlug *Instancer::capsuleScenePlug() { - return getChild( g_firstPlugIndex + 22 ); + return getChild( g_firstPlugIndex + 23 ); } const GafferScene::ScenePlug *Instancer::capsuleScenePlug() const { - return getChild( g_firstPlugIndex + 22 ); + return getChild( g_firstPlugIndex + 23 ); } Gaffer::PathMatcherDataPlug *Instancer::setCollaboratePlug() { - return getChild( g_firstPlugIndex + 23 ); + return getChild( g_firstPlugIndex + 24 ); } const Gaffer::PathMatcherDataPlug *Instancer::setCollaboratePlug() const { - return getChild( g_firstPlugIndex + 23 ); + return getChild( g_firstPlugIndex + 24 ); } void Instancer::affects( const Plug *input, AffectedPlugsContainer &outputs ) const @@ -1448,6 +1505,7 @@ void Instancer::affects( const Plug *input, AffectedPlugsContainer &outputs ) co input == prototypesPlug()->childNamesPlug() || input == prototypesPlug()->existsPlug() || input == idPlug() || + input == omitDuplicateIdsPlug() || input == positionPlug() || input == orientationPlug() || input == scalePlug() || @@ -1526,6 +1584,7 @@ void Instancer::hash( const Gaffer::ValuePlug *output, const Gaffer::Context *co h.append( prototypesPlug()->childNamesHash( ScenePath() ) ); idPlug()->hash( h ); + omitDuplicateIdsPlug()->hash( h ); positionPlug()->hash( h ); orientationPlug()->hash( h ); scalePlug()->hash( h ); @@ -1748,6 +1807,7 @@ void Instancer::compute( Gaffer::ValuePlug *output, const Gaffer::Context *conte prototypeRootsList.get(), prototypesPlug(), idPlug()->getValue(), + omitDuplicateIdsPlug()->getValue(), positionPlug()->getValue(), orientationPlug()->getValue(), scalePlug()->getValue(),