Better builtin support in ResourceReloader

com.unity.render-pipelines.core/CHANGELOG.md

@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Add probe volume influence weight parameter
 - Added support for multiple Baking States to Prove Volumes.
 - Hidding Volume Components not available for the current pipeline on the Volume Profile Inspector.
+- Added error on ResourceReloader when attempting to use [ReloadGroup] on ScriptableObject.
 
 ### Changed
 - Volume Component editor are now specified by `CustomEditorAttribute` instead of `VolumeComponentEditorAttribute`.
@@ -27,6 +28,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Fixed the issue with the special Turkish i, when looking for the m_IsGlobal property in VolumeEditor. (case 1276892)
 - Fixed texture gather macros for GLCore and moved them from target 4.6 to target 4.5.
 - Fixed cubemap array macros for GLCore.
+- Fixed regression on ResourceReloader due to change for supporting built-in resources.
 
 ## [14.0.0] - 2021-11-17
 

com.unity.render-pipelines.core/Runtime/Utilities/ResourceReloader.cs

@@ -55,7 +55,8 @@ public static bool ReloadAllNullIn(System.Object container, string basePath)
                 return false;
 
             var changed = false;
-            foreach (var fieldInfo in container.GetType().GetFields(BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static))
+            foreach (var fieldInfo in container.GetType()
+                .GetFields(BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static))
             {
                 //Recurse on sub-containers
                 if (IsReloadGroup(fieldInfo))
@@ -71,7 +72,7 @@ public static bool ReloadAllNullIn(System.Object container, string basePath)
                     if (attribute.paths.Length == 1)
                     {
                         changed |= SetAndLoadIfNull(container, fieldInfo, GetFullPath(basePath, attribute),
-                            attribute.package == ReloadAttribute.Package.Builtin);
+                            attribute.package);
                     }
                     else if (attribute.paths.Length > 1)
                     {
@@ -89,10 +90,10 @@ public static bool ReloadAllNullIn(System.Object container, string basePath)
                         }
                         else
                         {
-                            bool builtin = attribute.package == ReloadAttribute.Package.Builtin;
                             //Find each null element and reload them
                             for (int index = 0; index < attribute.paths.Length; ++index)
-                                changed |= SetAndLoadIfNull(array, index, GetFullPath(basePath, attribute, index), builtin);
+                                changed |= SetAndLoadIfNull(array, index, GetFullPath(basePath, attribute, index),
+                                    attribute.package);
                         }
                     }
                 }
@@ -103,14 +104,21 @@ public static bool ReloadAllNullIn(System.Object container, string basePath)
             return changed;
         }
 
+        static void CheckReloadGroupSupportedType(Type type)
+        {
+            if (type.IsSubclassOf(typeof(ScriptableObject)))
+                throw new Exception(@$"ReloadGroup attribute must not be used on {nameof(ScriptableObject)}.
+If {nameof(ResourceReloader)} create an instance of it, it will be not saved as a file, resulting in corrupted ID when building.");
+        }
+
         static bool FixGroupIfNeeded(System.Object container, FieldInfo info)
         {
+            var type = info.FieldType;
+            CheckReloadGroupSupportedType(type);
+
             if (IsNull(container, info))
             {
-                var type = info.FieldType;
-                var value = type.IsSubclassOf(typeof(ScriptableObject))
-                    ? ScriptableObject.CreateInstance(type)
-                    : Activator.CreateInstance(type);
+                var value = Activator.CreateInstance(type);
 
                 info.SetValue(
                     container,
@@ -126,17 +134,16 @@ static bool FixGroupIfNeeded(Array array, int index)
         {
             Assert.IsNotNull(array);
 
+            var type = array.GetType().GetElementType();
+            CheckReloadGroupSupportedType(type);
+
             if (IsNull(array.GetValue(index)))
             {
-                var type = array.GetType().GetElementType();
                 var value = type.IsSubclassOf(typeof(ScriptableObject))
                     ? ScriptableObject.CreateInstance(type)
                     : Activator.CreateInstance(type);
 
-                array.SetValue(
-                    value,
-                    index
-                );
+                array.SetValue(value, index);
                 return true;
             }
 
@@ -147,10 +154,7 @@ static bool FixArrayIfNeeded(System.Object container, FieldInfo info, int length
         {
             if (IsNull(container, info) || ((Array)info.GetValue(container)).Length < length)
             {
-                info.SetValue(
-                    container,
-                    Activator.CreateInstance(info.FieldType, length)
-                );
+                info.SetValue(container, Activator.CreateInstance(info.FieldType, length));
                 return true;
             }
 
@@ -180,29 +184,35 @@ static bool IsNull(System.Object container, FieldInfo info)
         static bool IsNull(System.Object field)
             => field == null || field.Equals(null);
 
-        static UnityEngine.Object Load(string path, Type type, bool builtin)
+        static UnityEngine.Object Load(string path, Type type, ReloadAttribute.Package location)
         {
             // Check if asset exist.
             // Direct loading can be prevented by AssetDatabase being reloading.
             var guid = AssetDatabase.AssetPathToGUID(path);
-            if (!builtin && String.IsNullOrEmpty(guid))
+            if (location == ReloadAttribute.Package.Root && String.IsNullOrEmpty(guid))
                 throw new Exception($"Cannot load. Incorrect path: {path}");
 
             // Else the path is good. Attempt loading resource if AssetDatabase available.
             UnityEngine.Object result;
-            if (builtin && type == typeof(Shader))
+            switch (location)
             {
-                result = Shader.Find(path);
-            }
-            else
-            {
-                result = AssetDatabase.LoadAssetAtPath(path, type);
-
-                if (IsNull(result))
-                    result = Resources.GetBuiltinResource(type, path);
-
-                if (IsNull(result))
-                    result = AssetDatabase.GetBuiltinExtraResource(type, path);
+                case ReloadAttribute.Package.Builtin:
+                    if (type == typeof(Shader))
+                        result = Shader.Find(path);
+                    else
+                        result = Resources.GetBuiltinResource(type, path); //handle wrong path error
+                    break;
+                case ReloadAttribute.Package.BuiltinExtra:
+                    if (type == typeof(Shader))
+                        result = Shader.Find(path);
+                    else
+                        result = AssetDatabase.GetBuiltinExtraResource(type, path); //handle wrong path error
+                    break;
+                case ReloadAttribute.Package.Root:
+                    result = AssetDatabase.LoadAssetAtPath(path, type);
+                    break;
+                default:
+                    throw new NotImplementedException($"Unknown {location}");
             }
 
             if (IsNull(result))
@@ -215,23 +225,23 @@ static UnityEngine.Object Load(string path, Type type, bool builtin)
         }
 
         static bool SetAndLoadIfNull(System.Object container, FieldInfo info,
-            string path, bool builtin)
+            string path, ReloadAttribute.Package location)
         {
             if (IsNull(container, info))
             {
-                info.SetValue(container, Load(path, info.FieldType, builtin));
+                info.SetValue(container, Load(path, info.FieldType, location));
                 return true;
             }
 
             return false;
         }
 
-        static bool SetAndLoadIfNull(Array array, int index, string path, bool builtin)
+        static bool SetAndLoadIfNull(Array array, int index, string path, ReloadAttribute.Package location)
         {
             var element = array.GetValue(index);
             if (IsNull(element))
             {
-                array.SetValue(Load(path, array.GetType().GetElementType(), builtin), index);
+                array.SetValue(Load(path, array.GetType().GetElementType(), location), index);
                 return true;
             }
 
@@ -280,7 +290,13 @@ public enum Package
             /// <summary>
             /// Used for resources inside the package.
             /// </summary>
-            Root
+            Root,
+
+            /// <summary>
+            /// Used for builtin extra resources when the resource isn't part of the package (i.e. builtin
+            /// extra Sprite).
+            /// </summary>
+            BuiltinExtra,
         };
 
 #if UNITY_EDITOR