Merge pull request #1433 from johnhaddon/ignoreUSDLightCollections

USDScene : Don't treat `lightLink` and `shadowLink` as sets

Changes

@@ -1,8 +1,10 @@
 10.5.x.x (relative to 10.5.9.2)
 ========
 
+Fixes
+-----
 
-
+- USDScene : `lightLink` and `shadowLink` collections on UsdLuxLightAPI are no longer treated as sets.
 
 10.5.9.2 (relative to 10.5.9.1)
 ========

contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp

@@ -283,6 +283,33 @@ boost::container::flat_map<pxr::TfToken, PrimPredicate> g_schemaTypeSetPredicate
 	{ pxr::TfToken( "usd:pointInstancers" ), &pxr::UsdPrim::IsA<pxr::UsdGeomPointInstancer> }
 };
 
+bool collectionIsSet( const pxr::UsdCollectionAPI &collection )
+{
+	if(
+		collection.GetPrim().HasAPI<pxr::UsdLuxLightAPI>() &&
+		( collection.GetName() == pxr::UsdLuxTokens->lightLink || collection.GetName() == pxr::UsdLuxTokens->shadowLink )
+	)
+	{
+		// These collections are problematic :
+		//
+		// - They define the objects that _this_ light is linked to. So it makes
+		//   no sense to combine them from multiple prims into a single set as
+		//   we do when loading recursively.
+		// - The UsdLuxLightAPI defaults the collection to have
+		//   `includeRoot=true`, which in conjunction with the default expansion
+		//   rule means that it will include every single prim in the scene.
+		//   That's not only a lot of prims, but most of them won't be below the
+		//   collection's prim, and every single one of those will trigger a
+		//   warning.
+		// - Gaffer has a different light-linking mechanism anyway.
+		//
+		// For these reasons, we do not treat them as sets.
+		return false;
+	}
+
+	return true;
+}
+
 // If `predicate` is non-null then it is called to determine if _this_ prim is in the set. If null,
 // then the set is loaded from a UsdCollection called `name`.
 IECore::PathMatcher localSet( const pxr::UsdPrim &prim, const pxr::TfToken &name, PrimPredicate predicate, const Canceller *canceller )
@@ -298,7 +325,9 @@ IECore::PathMatcher localSet( const pxr::UsdPrim &prim, const pxr::TfToken &name
 	}
 
 	const size_t prefixSize = prim.GetPath().GetPathElementCount();
-	if( auto collection = pxr::UsdCollectionAPI( prim, name ) )
+
+	auto collection = pxr::UsdCollectionAPI( prim, name );
+	if( collection && collectionIsSet( collection ) )
 	{
 		Canceller::check( canceller );
 		pxr::UsdCollectionAPI::MembershipQuery membershipQuery = collection.ComputeMembershipQuery();
@@ -406,7 +435,10 @@ SceneInterface::NameList localSetNames( const pxr::UsdPrim &prim )
 		result.reserve( allCollections.size() );
 		for( const pxr::UsdCollectionAPI &collection : allCollections )
 		{
-			result.push_back( collection.GetName().GetString() );
+			if( collectionIsSet( collection ) )
+			{
+				result.push_back( collection.GetName().GetString() );
+			}
 		}
 	}
 	else

contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py

@@ -3347,6 +3347,37 @@ def testLightAttribute( self ) :
 			} )
 		)
 
+	@unittest.skipIf( pxr.Usd.GetVersion() < ( 0, 21, 11 ), "UsdLuxLightAPI not available" )
+	def testLightAndShadowLinkCollections( self ) :
+
+		# We ignore `lightLink` and `shadowLink` on lights, because they have
+		# a specific meaning in USD that doesn't translate to our definition of a set.
+
+		root = IECoreScene.SceneInterface.create(
+			os.path.join( os.path.dirname( __file__ ), "data", "sphereLight.usda" ),
+			IECore.IndexedIO.OpenMode.Read
+		)
+		self.assertNotIn( "lightLink", root.setNames() )
+		self.assertNotIn( "shadowLink", root.setNames() )
+		self.assertEqual( root.readSet( "lightLink" ), IECore.PathMatcher() )
+		self.assertEqual( root.readSet( "shadowLink" ), IECore.PathMatcher() )
+
+		# But that doesn't mean folks can't use those names for sets elsewhere if they
+		# want to.
+
+		fileName = os.path.join( self.temporaryDirectory(), "test.usda" )
+		root = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Write )
+
+		root.writeSet( "lightLink", IECore.PathMatcher( [ "/test1" ] ) )
+		root.writeSet( "shadowLink", IECore.PathMatcher( [ "/test2" ] ) )
+		del root
+
+		root = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Read )
+		self.assertIn( "lightLink", root.setNames() )
+		self.assertIn( "shadowLink", root.setNames() )
+		self.assertEqual( root.readSet( "lightLink" ), IECore.PathMatcher( [ "/test1" ] ) )
+		self.assertEqual( root.readSet( "shadowLink" ), IECore.PathMatcher( [ "/test2" ] ) )
+
 	def testReadDoubleSidedAttribute( self ) :
 
 		root = IECoreScene.SceneInterface.create(