Merge pull request #3370 from LazyWorksZA/fix/material-new-unresolved-proxy

A material built by Material.New bound an empty texture for the environment DFG lookup
table. Fixes #3369.

Author: xen2

sources/buildengine/Stride.Core.BuildEngine.Common/BuildTransaction.cs

@@ -31,6 +31,41 @@ public IEnumerable<KeyValuePair<string, ObjectId>> SearchValues(Func<KeyValuePai
         }
     }
 
+    public IEnumerable<KeyValuePair<string, ObjectId>> GetMergedIdMap()
+    {
+        var result = new Dictionary<string, ObjectId>();
+
+        lock (transactionOutputObjects)
+        {
+            foreach (var outputObject in transactionOutputObjects)
+            {
+                if (outputObject.Key.Type == UrlType.Content)
+                    result[outputObject.Key.Path] = outputObject.Value;
+            }
+
+            foreach (var outputObjects in outputObjectsGroups)
+            {
+                // Lock underlying EnumerableBuildStep.OutputObjects
+                lock (outputObjects)
+                {
+                    foreach (var outputObject in outputObjects)
+                    {
+                        if (outputObject.Key.Type == UrlType.Content)
+                            result.TryAdd(outputObject.Key.Path, outputObject.Value.ObjectId);
+                    }
+                }
+            }
+
+            if (contentIndexMap != null)
+            {
+                foreach (var entry in contentIndexMap.GetMergedIdMap())
+                    result.TryAdd(entry.Key, entry.Value);
+            }
+        }
+
+        return result;
+    }
+
     public bool TryGetValue(string url, out ObjectId objectId)
     {
         var objUrl = new ObjectUrl(UrlType.Content, url);
@@ -114,8 +149,7 @@ public void WaitPendingOperations()
 
         public IEnumerable<KeyValuePair<string, ObjectId>> GetMergedIdMap()
         {
-            // Shouldn't be used
-            throw new NotImplementedException();
+            return buildTransaction.GetMergedIdMap();
         }
 
         public void Dispose()

sources/core/Stride.Core.Serialization/Serialization/Contents/ContentManager.cs

@@ -670,47 +670,54 @@ private void ThrowAssetNotFound(string url)
     {
         var errorMessage = $"The asset '{url}' could not be found. Asset path should be 'MyFolder/MyAssetName', or '/PackageName/MyFolder/MyAssetName' for an asset from a namespaced package. Check that the path is correct and that the asset has been included into the build.";
 
-        var contentIndexMap = FileProvider?.ContentIndexMap;
-        if (contentIndexMap != null)
+        try
         {
-            var indexMap = contentIndexMap.GetMergedIdMap();
-
-            // First suggest assets differing from the requested URL only by the '/PackageName' root
-            var isRooted = url.StartsWith('/');
-            var candidates = new List<string>();
-            foreach (var entry in indexMap)
-            {
-                if (isRooted ? DiffersByRoot(url, entry.Key) : DiffersByRoot(entry.Key, url))
-                    candidates.Add(entry.Key);
-            }
-            if (candidates.Count > 0)
-            {
-                candidates.Sort();
-                errorMessage = $"The asset '{url}' could not be found. Did you mean:"
-                    + string.Concat(candidates.Select(c => $"{Environment.NewLine}  '{c}'"))
-                    + $"{Environment.NewLine}An asset from a namespaced package is addressed by a rooted URL: '/PackageName/MyFolder/MyAssetName'.";
-            }
-            else
+            var contentIndexMap = FileProvider?.ContentIndexMap;
+            if (contentIndexMap != null)
             {
-                // Otherwise fall back to assets with the same name in other folders
-                var assetName = url.Substring(url.LastIndexOf('/') + 1);
-                var sameNameCandidates = new List<string>();
+                var indexMap = contentIndexMap.GetMergedIdMap();
+
+                // First suggest assets differing from the requested URL only by the '/PackageName' root
+                var isRooted = url.StartsWith('/');
+                var candidates = new List<string>();
                 foreach (var entry in indexMap)
                 {
-                    if (entry.Key.EndsWith(assetName, StringComparison.OrdinalIgnoreCase)
-                        && (entry.Key.Length == assetName.Length || entry.Key[entry.Key.Length - assetName.Length - 1] == '/'))
-                        sameNameCandidates.Add(entry.Key);
+                    if (isRooted ? DiffersByRoot(url, entry.Key) : DiffersByRoot(entry.Key, url))
+                        candidates.Add(entry.Key);
+                }
+                if (candidates.Count > 0)
+                {
+                    candidates.Sort();
+                    errorMessage = $"The asset '{url}' could not be found. Did you mean:"
+                        + string.Concat(candidates.Select(c => $"{Environment.NewLine}  '{c}'"))
+                        + $"{Environment.NewLine}An asset from a namespaced package is addressed by a rooted URL: '/PackageName/MyFolder/MyAssetName'.";
                 }
-                if (sameNameCandidates.Count > 0)
+                else
                 {
-                    sameNameCandidates.Sort();
-                    var more = sameNameCandidates.Count > MaxSameNameCandidates ? $"{Environment.NewLine}  (and {sameNameCandidates.Count - MaxSameNameCandidates} more)" : string.Empty;
-                    errorMessage = $"The asset '{url}' could not be found. Assets with the same name exist at:"
-                        + string.Concat(sameNameCandidates.Take(MaxSameNameCandidates).Select(c => $"{Environment.NewLine}  '{c}'"))
-                        + more;
+                    // Otherwise fall back to assets with the same name in other folders
+                    var assetName = url.Substring(url.LastIndexOf('/') + 1);
+                    var sameNameCandidates = new List<string>();
+                    foreach (var entry in indexMap)
+                    {
+                        if (entry.Key.EndsWith(assetName, StringComparison.OrdinalIgnoreCase)
+                            && (entry.Key.Length == assetName.Length || entry.Key[entry.Key.Length - assetName.Length - 1] == '/'))
+                            sameNameCandidates.Add(entry.Key);
+                    }
+                    if (sameNameCandidates.Count > 0)
+                    {
+                        sameNameCandidates.Sort();
+                        var more = sameNameCandidates.Count > MaxSameNameCandidates ? $"{Environment.NewLine}  (and {sameNameCandidates.Count - MaxSameNameCandidates} more)" : string.Empty;
+                        errorMessage = $"The asset '{url}' could not be found. Assets with the same name exist at:"
+                            + string.Concat(sameNameCandidates.Take(MaxSameNameCandidates).Select(c => $"{Environment.NewLine}  '{c}'"))
+                            + more;
+                    }
                 }
             }
         }
+        catch (Exception)
+        {
+            // Suggestions are best effort; an index map that cannot enumerate must not replace the real error
+        }
 
         // True when <rooted> is exactly '/PackageName' (a single segment) followed by '/<bare>'.
         static bool DiffersByRoot(string rooted, string bare)

sources/editor/Stride.Assets.Presentation/Preview/SkyboxPreview.cs

@@ -1,11 +1,13 @@
 // Copyright (c) .NET Foundation and Contributors (https://dotnetfoundation.org/ & https://stride3d.net) and Silicon Studio Corp. (https://www.siliconstudio.co.jp)
 // Distributed under the MIT license. See the LICENSE.md file in the project root for more information.
 using System.Threading.Tasks;
+using Stride.Core.Assets.Compiler;
 using Stride.Core.Mathematics;
 using Stride.Assets.Skyboxes;
 using Stride.Editor.Annotations;
 using Stride.Editor.Preview;
 using Stride.Engine;
+using Stride.Graphics;
 using Stride.Rendering.Skyboxes;
 using Stride.Rendering;
 using Stride.Rendering.Lights;
@@ -53,6 +55,23 @@ protected override void SetupLighting(Entity camera)
             // No default lighting
         }
 
+        protected override AssetCompilerResult Compile()
+        {
+            var result = base.Compile();
+
+            // The preview material is generated at run time, so the specular lookup table it references
+            // is not a dependency of the skybox asset; compile it too so the preview database can serve it
+            foreach (var profile in new[] { GraphicsProfile.Level_9_1, GraphicsProfile.Level_10_0 })
+            {
+                var lutReference = MaterialSpecularMicrofacetEnvironmentGGXLUT.CreateLookupTableReference(profile);
+                var lutItem = AssetItem.Package.Session.FindAssetFromProxyObject(lutReference);
+                if (lutItem != null)
+                    result.BuildSteps.Add(Builder.Compile(lutItem).BuildSteps);
+            }
+
+            return result;
+        }
+
         protected override PreviewEntity CreatePreviewEntity()
         {
             var skybox = LoadAsset<Skybox>(AssetItem.Location);
@@ -123,7 +142,7 @@ private Material CreateMaterial(float metalness, float glossiness)
                     },
                     SpecularModel = new MaterialSpecularMicrofacetModelFeature()
                 }
-            });
+            }, Game.Content);
         }
     }
 }

sources/editor/Stride.Assets.Presentation/Themes/Generic.xaml

@@ -145,7 +145,7 @@
                   <Slider Grid.Column="1" Value="{Binding Metalness}" Height="16" Minimum="0" Maximum="1" HorizontalAlignment="Stretch"
                           TickFrequency="0.0333" TickPlacement="BottomRight" IsMoveToPointEnabled="True"/>
                 </Grid>
-                <Grid HorizontalAlignment="Stretch">
+                <Grid HorizontalAlignment="Stretch" Margin="0,3,0,0">
                   <Grid.ColumnDefinitions>
                     <ColumnDefinition Width="70" />
                     <ColumnDefinition Width="*" />

sources/engine/Stride.Assets.Tests/TestMaterialGenerator.cs

@@ -6,7 +6,9 @@
 using System.IO;
 using Xunit;
 using Stride.Core.Mathematics;
+using Stride.Core.Serialization;
 using Stride.Core.Yaml;
+using Stride.Graphics;
 using Stride.Rendering;
 using Stride.Rendering.Materials;
 using Stride.Rendering.Materials.ComputeColors;
@@ -914,6 +916,51 @@ public void Test2Layers3ShadingModels()
         }
 
 
+        /// <summary>
+        /// The generator on its own leaves texture references as proxies.
+        /// </summary>
+        /// <remarks>
+        /// This is what the asset compiler serializes: the proxy carries the reference, and a content
+        /// load turns it back into the texture. A caller that generates a material at run time is outside
+        /// that path and has to resolve the references itself, which is what <c>Material.New</c> does.
+        /// Pinning it here keeps a change made for the run-time path from breaking the build-time one.
+        /// </remarks>
+        [Fact]
+        public void TestGeneratorLeavesTextureReferencesAsProxies()
+        {
+            // The graphics profile decides between the LUT16 and LUT8 lookup tables; pin it
+            // so the URL asserted below does not depend on the context's default.
+            var context = new MaterialGeneratorContextExtended { GraphicsProfile = GraphicsProfile.Level_10_0 };
+            var materialDesc = new MaterialDescriptor
+            {
+                Attributes =
+                {
+                    Diffuse = new MaterialDiffuseMapFeature(new ComputeColor(Color.White)),
+                    DiffuseModel = new MaterialDiffuseLambertModelFeature(),
+                    Specular = new MaterialMetalnessMapFeature(new ComputeFloat(1.0f)),
+                    MicroSurface = new MaterialGlossinessMapFeature(new ComputeFloat(0.9f)),
+
+                    // Defaults its environment function to MaterialSpecularMicrofacetEnvironmentGGXLUT,
+                    // which attaches a reference to a lookup table texture.
+                    SpecularModel = new MaterialSpecularMicrofacetModelFeature(),
+                },
+            };
+
+            var result = MaterialGenerator.Generate(materialDesc, context, "test_material");
+            Assert.False(result.HasErrors, result.ToText());
+
+            var lookupTable = result.Material.Passes[0].Parameters
+                .Get(MaterialSpecularMicrofacetEnvironmentGGXLUTKeys.EnvironmentLightingDFG_LUT);
+
+            Assert.NotNull(lookupTable);
+
+            var reference = AttachedReferenceManager.GetAttachedReference(lookupTable);
+
+            Assert.NotNull(reference);
+            Assert.True(reference.IsProxy, "The generator resolved the reference, which the asset compiler relies on staying a proxy.");
+            Assert.Equal("/Stride.Engine/StrideEnvironmentLightingDFGLUT16", reference.Url);
+        }
+
         private class MaterialGeneratorContextExtended : MaterialGeneratorContext
         {
             private readonly Dictionary<object, object> assetMap = new Dictionary<object, object>();

sources/engine/Stride.Engine.Tests/ParameterCollectionAttachedReferenceTest.cs

@@ -0,0 +1,99 @@
+// Copyright (c) .NET Foundation and Contributors (https://dotnetfoundation.org/ & https://stride3d.net)
+// Distributed under the MIT license. See the LICENSE.md file in the project root for more information.
+
+using System;
+using System.Collections.Generic;
+
+using Xunit;
+
+using Stride.Core.Assets;
+using Stride.Core.Diagnostics;
+using Stride.Core.Serialization;
+using Stride.Rendering;
+
+namespace Stride.Engine.Tests
+{
+    /// <summary>
+    /// Tests how a <see cref="ParameterCollection"/> handles object values that are unresolved
+    /// attached references (proxies made by <see cref="AttachedReferenceManager.CreateProxyObject{T}(AssetId, string)"/>).
+    /// </summary>
+    public class ParameterCollectionAttachedReferenceTest
+    {
+        public static readonly ObjectParameterKey<object> ObjectKey = ParameterKeys.NewObject<object>();
+
+        [Fact]
+        public void TestResolveWithoutContentManagerWarnsAndKeepsProxy()
+        {
+            var parameters = new ParameterCollection();
+            var url = $"test/unresolved/{Guid.NewGuid()}";
+            var proxy = AttachedReferenceManager.CreateProxyObject<TestReferencedContent>(AssetId.New(), url);
+            parameters.Set(ObjectKey, proxy);
+
+            var log = new LoggerResult();
+            parameters.ResolveAttachedReferences(null, log);
+
+            Assert.Contains(log.Messages, message => message.Type == LogMessageType.Warning && message.Text.Contains(url));
+            Assert.Same(proxy, parameters.Get(ObjectKey));
+        }
+
+        [Fact]
+        public void TestUpdateLayoutReportsUnresolvedReference()
+        {
+            var parameters = new ParameterCollection();
+            // The report deduplicates per URL for the process lifetime, so make the URL unique
+            var url = $"test/unresolved/{Guid.NewGuid()}";
+            parameters.Set(ObjectKey, AttachedReferenceManager.CreateProxyObject<TestReferencedContent>(AssetId.New(), url));
+
+            var warnings = CollectGlobalWarningsDuring(() => parameters.UpdateLayout(LayoutWith(ObjectKey)));
+
+            Assert.Contains(warnings, text => text.Contains(url));
+        }
+
+        [Fact]
+        public void TestUpdateLayoutIgnoresResolvedObjects()
+        {
+            var parameters = new ParameterCollection();
+            parameters.Set(ObjectKey, new TestReferencedContent());
+
+            var warnings = CollectGlobalWarningsDuring(() => parameters.UpdateLayout(LayoutWith(ObjectKey)));
+
+            Assert.DoesNotContain(warnings, text => text.Contains("unloaded content"));
+        }
+
+        private static ParameterCollectionLayout LayoutWith(ParameterKey key)
+        {
+            var layout = new ParameterCollectionLayout { ResourceCount = 1 };
+            layout.LayoutParameterKeyInfos.Add(new ParameterKeyInfo(key, 0));
+            return layout;
+        }
+
+        private static List<string> CollectGlobalWarningsDuring(Action action)
+        {
+            var warnings = new List<string>();
+
+            void Collect(ILogMessage message)
+            {
+                if (message.Type >= LogMessageType.Warning)
+                {
+                    lock (warnings) warnings.Add(message.Text);
+                }
+            }
+
+            GlobalLogger.GlobalMessageLogged += Collect;
+            try
+            {
+                action();
+            }
+            finally
+            {
+                GlobalLogger.GlobalMessageLogged -= Collect;
+            }
+
+            return warnings;
+        }
+
+        private class TestReferencedContent
+        {
+        }
+    }
+}

sources/engine/Stride.Engine.Tests/Stride.Engine.Tests.csproj

@@ -23,6 +23,7 @@
     <Compile Include="Build\TestBuilder.cs" />
     <Compile Include="EngineTestBase.cs" />
     <Compile Include="ForwardRendererTransparentTargetsTest.cs" />
+    <Compile Include="ParameterCollectionAttachedReferenceTest.cs" />
     <Compile Include="ParameterCollectionUpdateEngineTest.cs" />
     <Compile Include="EntityUpdateEngineTest.cs" />
     <Compile Include="AnimatedModelTests.cs" />

sources/engine/Stride.Engine/Stride.Engine.sdpkg

@@ -9,3 +9,5 @@ RootAssets:
     - c90f3988-0544-4cbe-993f-13af7d9c23c6:/Stride.Engine/StrideDefaultFont
     - d26edb11-10bd-403c-b3c2-9c7fcccf25e5:/Stride.Engine/StrideDefaultSplashScreen
     - FF02239B-3697-4EBB-9F37-FE880659E64B:/Stride.Engine/StrideDebugSpriteFont
+    - a49995f8-2380-4baa-a03e-f8d1da35b79a:/Stride.Engine/StrideEnvironmentLightingDFGLUT16
+    - 87540190-ab97-4b4e-b3c2-d57d2fbb1ff3:/Stride.Engine/StrideEnvironmentLightingDFGLUT8