Fixed shader and material "Failed to find builder dependency" errors

Fixed shader and material "Failed to find builder dependency" errors Merge pull request #1462 from aws-lumberyard-dev/santorac/stabilization/2106/MissingDependencyWarnings-ATOM-15136 Fixed a couple builders to avoid adding job dependencies on source files that don't exist. This removes mostly benign (but noisy) messages about "Failed to find builder dependency". Changed AssetUtils::GetPossibleDepenencyPaths to return all possible source paths, rather than stopping when one is found. This function is now used to report a list of all possible source dependencies, so that CreateJobs will get called by the AP whenever a file shows up at one of those locations. If a file was missing before and then appears, this will cause the builders to wake up and add the appropriate job dependencies on the new files. In ShaderVariantAssetBuilder and MaterialBuilder, we now use GetPossibleDepenencyPaths to report source dependencies rather than job dependencies. We only report a job dependency when the actual source file has been identified. This should all now be consistent with the intended design of the AP's dependency systems (the prior approach was a hack based on misunderstanding of what source dependencies are). SrgLayoutBuilder's change is a bit tricky. The above changes did not fix all of the "Failed to find builder dependency" messages because AzslBuilder sometimes skips particular files in CreateJobs. When this happens, it is invalid to report an AzslBuilder job dependency on that file. So I copied the same conditional code that is used to skip an azsli file in CreateJobs, and used that to skip AddAzslBuilderJobDependency() in SrgLayoutBuilder as well. With all these changes combined, I do think we've solved the issue where jobs fail to evict outdated jobs, as described in ATOM-15134. However, we are not yet seeing the iteration time improvements we were hoping for. Before these changes I was seeing roughly a 0.5 minute delay for the initial change, and a 2 minute delay for a subsequent change. With these changes it's more like 0.5 mibutes and 1.5 minutes. It appears that the AP scan is being starved by all the AssetBuilder processing going on, and perhaps IO contention. I suspect that this will be greatly improved on the development branch where we no longer have AzslBuilder and SrgLayoutBuilder slowing things down. ATOM-15136 Builder dependency errors reported in mainline ATOM-15134 Replace GetPossibleDepenencyPaths Approach with Source Dependencies Testing: - Deleted and rebuild the ASV cache a few times. - Ran through a few different scenarios of adding and removing referenced files in the project folder and gem folder. Covering... - Parent materials - Material types - Material functors - I discovered a particular scenario that produces incorrect results, but I verified the scenario fails without my changes too: LYN-4757 - Opened a few levels in AtomTest.
2021-06-22 20:12:40 -07:00
parent bc3aa45ed9 71525ce601
commit b3eb54e9c9
5 changed files with 102 additions and 43 deletions
@@ -72,22 +72,43 @@ namespace AZ
        static constexpr uint32_t ShaderVariantJobVariantParam = 3;
        static constexpr uint32_t ShouldExitEarlyFromProcessJobParam = 4;

-        static void AddShaderAssetJobDependency(
-            AssetBuilderSDK::JobDescriptor& jobDescriptor,
-            const AssetBuilderSDK::PlatformInfo& platformInfo,
-            const AZStd::string& shaderVariantListFilePath,
-            const AZStd::string& shaderFilePath)
+        //! Adds source file dependencies for every place a referenced file may appear, and detects if one of
+        //! those possible paths resolves to the expected file.
+        //! @param currentFilePath - the full path to the file being processed
+        //! @param referencedParentPath - the path to a reference file, which may be relative to the @currentFilePath, or may be a full asset path.
+        //! @param sourceFileDependencies - new source file dependencies will be added to this list
+        //! @param foundSourceFile - if one of the source file dependencies is found, the highest priority one will be indicated here, otherwise this will be empty.
+        //! @return true if the referenced file was found and @foundSourceFile was set
+        bool LocateReferencedSourceFile(
+            AZStd::string_view currentFilePath, AZStd::string_view referencedParentPath,
+            AZStd::vector<AssetBuilderSDK::SourceFileDependency>& sourceFileDependencies,
+            AZStd::string& foundSourceFile)
        {
-            AZStd::vector<AZStd::string> possibleDependencies = AZ::RPI::AssetUtils::GetPossibleDepenencyPaths(shaderVariantListFilePath, shaderFilePath);
+            foundSourceFile.clear();
+
+            bool found = false;
+
+            AZStd::vector<AZStd::string> possibleDependencies = RPI::AssetUtils::GetPossibleDepenencyPaths(currentFilePath, referencedParentPath);
            for (auto& file : possibleDependencies)
            {
-                AssetBuilderSDK::JobDependency jobDependency;
-                jobDependency.m_jobKey = ShaderAssetBuilder::ShaderAssetBuilderJobKey;
-                jobDependency.m_platformIdentifier = platformInfo.m_identifier;
-                jobDependency.m_type = AssetBuilderSDK::JobDependencyType::Order;
-                jobDependency.m_sourceFile.m_sourceFileDependencyPath = file;
-                jobDescriptor.m_jobDependencyList.push_back(jobDependency);
+                AssetBuilderSDK::SourceFileDependency sourceFileDependency;
+                sourceFileDependency.m_sourceFileDependencyPath = file;
+                sourceFileDependencies.push_back(sourceFileDependency);
+
+                if (!found)
+                {
+                    AZ::Data::AssetInfo sourceInfo;
+                    AZStd::string watchFolder;
+                    AzToolsFramework::AssetSystemRequestBus::BroadcastResult(found, &AzToolsFramework::AssetSystem::AssetSystemRequest::GetSourceInfoBySourcePath, file.c_str(), sourceInfo, watchFolder);
+
+                    if (found)
+                    {
+                        foundSourceFile = file;
+                    }
+                }
            }
+
+            return found;
        }

        //! Returns true if @sourceFileFullPath starts with a valid asset processor scan folder, false otherwise.
@@ -334,6 +355,9 @@ namespace AZ
                response.m_result = AssetBuilderSDK::CreateJobsResultCode::Success;
                return;
            }
+            
+            AZStd::string foundShaderFile;
+            LocateReferencedSourceFile(variantListFullPath, shaderVariantList.m_shaderFilePath, response.m_sourceFileDependencyList, foundShaderFile);

            for (const AssetBuilderSDK::PlatformInfo& info : request.m_enabledPlatforms)
            {
@@ -349,8 +373,16 @@ namespace AZ
                
                    jobDescriptor.m_jobKey = GetShaderVariantTreeAssetJobKey();
                    jobDescriptor.SetPlatformIdentifier(info.m_identifier.data());
-                
-                    AddShaderAssetJobDependency(jobDescriptor, info, variantListFullPath, shaderVariantList.m_shaderFilePath);
+
+                    if (!foundShaderFile.empty())
+                    {
+                        AssetBuilderSDK::JobDependency jobDependency;
+                        jobDependency.m_jobKey = ShaderAssetBuilder::ShaderAssetBuilderJobKey;
+                        jobDependency.m_platformIdentifier = info.m_identifier;
+                        jobDependency.m_type = AssetBuilderSDK::JobDependencyType::Order;
+                        jobDependency.m_sourceFile.m_sourceFileDependencyPath = foundShaderFile;
+                        jobDescriptor.m_jobDependencyList.push_back(jobDependency);
+                    }
                
                    jobDescriptor.m_jobParameters.emplace(ShaderSourceFilePathJobParam, shaderSourceFileFullPath);
                
@@ -203,6 +203,16 @@ namespace AZ
                // queue up AzslBuilder dependencies:
                for (RHI::ShaderPlatformInterface* shaderPlatformInterface : platformInterfaces)
                {
+                    const bool isAzsli = AzFramework::StringFunc::Path::IsExtension(fullPath.c_str(), "azsli");
+                    if (isAzsli)
+                    {
+                        auto skipCheck = ShaderBuilderUtility::ShouldSkipFileForSrgProcessing(SrgLayoutBuilderName, fullPath);
+                        if (skipCheck != ShaderBuilderUtility::SrgSkipFileResult::ContinueProcess)
+                        {
+                            continue;
+                        }
+                    }
+
                    AddAzslBuilderJobDependency(jobDescriptor, info.m_identifier, shaderPlatformInterface->GetAPIName().GetCStr(), fullPath);
                }
                response.m_createJobOutputs.push_back(jobDescriptor);
@@ -66,6 +66,7 @@ namespace AZ
            //! it's possible that b.json could be found in either MyGem/Assets/Foo/Bar/a.json or in MyGem/Assets/Bar/a.json.
            //! @param originatingSourceFilePath  Path to a file that references referencedSourceFilePath. May be absolute or relative to asset-root.
            //! @param referencedSourceFilePath   The referenced path as it appears in the originating file. May be relative to the originating file location or relative to asset-root.
+            //! @return the list of possible paths, ordered from highest priority to lowest priority
            AZStd::vector<AZStd::string> GetPossibleDepenencyPaths(const AZStd::string& originatingSourceFilePath, const AZStd::string& referencedSourceFilePath);

            // Definitions...
@@ -67,16 +67,41 @@ namespace AZ
            BusDisconnect();
        }

-        void AddPossibleJobDependencies(const char* jobKey, AZStd::string_view currentFilePath, AZStd::string_view referencedParentPath, AZStd::vector<AssetBuilderSDK::JobDependency>& jobDependencies)
+        //! Adds all relevant dependencies for a referenced source file, considering that the path might be relative to the original file location or a full asset path.
+        //! This will usually include multiple source dependencies and a single job dependency, but will include only source dependencies if the file is not found.
+        //! Note the AssetBuilderSDK::JobDependency::m_platformIdentifier will not be set by this function. The calling code must set this value before passing back
+        //! to the AssetBuilderSDK::CreateJobsResponse.
+        void AddPossibleDependencies(
+            AZStd::string_view currentFilePath, AZStd::string_view referencedParentPath,
+            AZStd::vector<AssetBuilderSDK::SourceFileDependency>& sourceFileDependencies,
+            const char* jobKey, AZStd::vector<AssetBuilderSDK::JobDependency>& jobDependencies)
        {
-            AZStd::vector<AZStd::string> possibleDependencies = AssetUtils::GetPossibleDepenencyPaths(currentFilePath, referencedParentPath);
+            bool dependencyFileFound = false;
+
+            AZStd::vector<AZStd::string> possibleDependencies = RPI::AssetUtils::GetPossibleDepenencyPaths(currentFilePath, referencedParentPath);
            for (auto& file : possibleDependencies)
            {
-                AssetBuilderSDK::JobDependency jobDependency;
-                jobDependency.m_jobKey = jobKey;
-                jobDependency.m_type = AssetBuilderSDK::JobDependencyType::Order;
-                jobDependency.m_sourceFile.m_sourceFileDependencyPath = file;
-                jobDependencies.push_back(jobDependency);
+                AssetBuilderSDK::SourceFileDependency sourceFileDependency;
+                sourceFileDependency.m_sourceFileDependencyPath = file;
+                sourceFileDependencies.push_back(sourceFileDependency);
+
+                // The first path found is the highest priority, and will have a job dependency, as this is the one
+                // the builder will actually use
+                if (!dependencyFileFound)
+                {
+                    AZ::Data::AssetInfo sourceInfo;
+                    AZStd::string watchFolder;
+                    AzToolsFramework::AssetSystemRequestBus::BroadcastResult(dependencyFileFound, &AzToolsFramework::AssetSystem::AssetSystemRequest::GetSourceInfoBySourcePath, file.c_str(), sourceInfo, watchFolder);
+
+                    if (dependencyFileFound)
+                    {
+                        AssetBuilderSDK::JobDependency jobDependency;
+                        jobDependency.m_jobKey = jobKey;
+                        jobDependency.m_type = AssetBuilderSDK::JobDependencyType::Order;
+                        jobDependency.m_sourceFile.m_sourceFileDependencyPath = file;
+                        jobDependencies.push_back(jobDependency);
+                    }
+                }
            }
        }

@@ -123,7 +148,7 @@ namespace AZ
            // We'll build up this one JobDescriptor and reuse it to register each of the platforms
            AssetBuilderSDK::JobDescriptor outputJobDescriptor;
            outputJobDescriptor.m_jobKey = JobKey;
-
+            
            // Load the file so we can detect and report dependencies.
            // If the file is a .materialtype, report dependencies on the .shader files.
            // If the file is a .material, report a dependency on the .materialtype and parent .material file
@@ -152,7 +177,9 @@ namespace AZ

                    for (auto& shader : materialTypeSourceData.GetValue().m_shaderCollection)
                    {
-                        AddPossibleJobDependencies("Shader Asset", request.m_sourceFile, shader.m_shaderFilePath, outputJobDescriptor.m_jobDependencyList);
+                        AddPossibleDependencies(request.m_sourceFile, shader.m_shaderFilePath,
+                            response.m_sourceFileDependencyList, "Shader Asset",
+                            outputJobDescriptor.m_jobDependencyList);
                    }

                    for (auto& functor : materialTypeSourceData.GetValue().m_materialFunctorSourceData)
@@ -161,7 +188,9 @@ namespace AZ

                        for (const MaterialFunctorSourceData::AssetDependency& dependency : dependencies)
                        {
-                            AddPossibleJobDependencies(dependency.m_jobKey.c_str(), request.m_sourceFile, dependency.m_sourceFilePath, outputJobDescriptor.m_jobDependencyList);
+                            AddPossibleDependencies(request.m_sourceFile, dependency.m_sourceFilePath,
+                                response.m_sourceFileDependencyList,
+                                dependency.m_jobKey.c_str(), outputJobDescriptor.m_jobDependencyList);
                        }
                    }
                }
@@ -196,7 +225,9 @@ namespace AZ

                    // Register dependency on the parent material source file so we can load it and use it's data to build this variant material.
                    // Note, we don't need a direct dependency on the material type because the parent material will depend on it.
-                    AddPossibleJobDependencies(JobKey, request.m_sourceFile, parentMaterialPath, outputJobDescriptor.m_jobDependencyList);
+                    AddPossibleDependencies(request.m_sourceFile, parentMaterialPath,
+                        response.m_sourceFileDependencyList,
+                        JobKey, outputJobDescriptor.m_jobDependencyList);
                }
            }
            
@@ -109,31 +109,16 @@ namespace AZ

            AZStd::vector<AZStd::string> GetPossibleDepenencyPaths(const AZStd::string& originatingSourceFilePath, const AZStd::string& referencedSourceFilePath)
            {
-                // We potentially add the parent dependency as both a direct path and a relative path rather than use AssetUtils::ResolvePathReference
-                // because there is no guarantee that the Asset Processor has seen the parent file yet (which ResolvePathReference requires).
-                // In that case, we have to add both possible locations because we don't know where it will show up.
-
                AZStd::vector<AZStd::string> results;

-                // The first dependency we add is using the referencedSourceFilePath as a relative path. This gives relative paths priority over asset-root paths.
+                // Use the referencedSourceFilePath as a relative path starting at originatingSourceFilePath
                AZStd::string combinedPath = originatingSourceFilePath;
                AzFramework::StringFunc::Path::StripFullName(combinedPath);
                AzFramework::StringFunc::Path::Join(combinedPath.c_str(), referencedSourceFilePath.c_str(), combinedPath);
                results.push_back(combinedPath);

-                // If the parent file exists at the relative path, then there is no need to report a dependency on the asset-root path.
-                bool assetFound = false;
-                AZ::Data::AssetInfo sourceInfo;
-                AZStd::string watchFolder;
-                AzToolsFramework::AssetSystemRequestBus::BroadcastResult(assetFound, &AzToolsFramework::AssetSystem::AssetSystemRequest::GetSourceInfoBySourcePath, combinedPath.c_str(), sourceInfo, watchFolder);
-
-                if (!assetFound)
-                {
-                    // The parent file wasn't found at the relative path, so we need a dependency on the asset-root path in case the file
-                    // exists there. Note, we still keep the relative path dependency above because we don't know whether it's missing because
-                    // it doesn't exist, or just because the AP hasn't found it yet.
-                    results.push_back(referencedSourceFilePath);
-                }
+                // Use the referencedSourceFilePath as a standard asset path
+                results.push_back(referencedSourceFilePath);

                return results;
            }