Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Change MaterialAssetCompilers build dependency type to other materials (#1477) #1478

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

laske185
Copy link

PR Details

Set in the MaterialAssetCompiler the BuildDependencyType to materials to Runtime to resolve the issue described in #1477.

Description

Change the BuildDependencyType to materials to BuildDependencyType.Runtime (as it is in the ModelAssetCompiler). So the AssetDependenciesCompiler also compiles materials referenced by material layers.

Related Issue

Closes #1477

Motivation and Context

I'm not able to compile my project with the explained asset setup. With that change it works.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@CLAassistant
Copy link

CLAassistant commented Jul 25, 2022

CLA assistant check
All committers have signed the CLA.

@@ -23,7 +23,7 @@ internal class MaterialAssetCompiler : AssetCompilerBase
public override IEnumerable<BuildDependencyInfo> GetInputTypes(AssetItem assetItem)
{
yield return new BuildDependencyInfo(typeof(TextureAsset), typeof(AssetCompilationContext), BuildDependencyType.Runtime);
yield return new BuildDependencyInfo(typeof(MaterialAsset), typeof(AssetCompilationContext), BuildDependencyType.CompileAsset);
yield return new BuildDependencyInfo(typeof(MaterialAsset), typeof(AssetCompilationContext), BuildDependencyType.Runtime);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand the effect of this change to determine if it's fine to merge this PR and resolve the issue referenced.

I managed to find the following deeper explanation:

  • BuildDependencyType.Runtime - the referenced asset is not needed to compile this asset, but it's needed at runtime to use the compiled content (eg. Models need Materials, who need Textures. But you can compile Models, Materials and Textures independently).
  • BuildDependencyType.CompileContent - the referenced asset needs to be compiled before this asset, and the compiler of this asset needs to load the corresponding content generated from the referenced asset (eg. A prefab model, which aggregates multiple models together, needs the compiled version of each model it's referencing to be able to merge them).
  • BuildDependencyType.CompileAsset - the referenced asset is read when compiling this asset because it depends on some of its parameter, but the referenced asset itself doesn't need to be compiled first (eg. Navigation Meshes need to read the scene asset they are related to in order to gather static colliders it contains, but they don't need to compile the scene itself).

From this I'm understanding that currently when you build a material asset it may look into other referenced material assets and read some of their parameters. Switching to Runtime mode will potentially change it in a way where those parameters aren't always available (not sure, tbh).

Something I've seen done is a combination BuildDependencyType.Runtime | BuildDependencyType.CompileAsset - if the issue would still be resolved with that I think it would be preferred with a comment added to investigate it further and referencing the issue and PR for context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the uses of CompileAsset I see is in the dependency analysis up-to-date check. I'm not sure when this can occur (referenced material changes after a first compilation pass which results in the recompilation of referencing material), but we should try to preserve this behavior.

foreach (var node in References)
{
if (node.HasOne(BuildDependencyType.CompileAsset))
{
if (node.Target.Analyze(context))
upToDate = false;
}
}

Copy link
Author

@laske185 laske185 Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is also fixed with the combination BuildDependencyType.Runtime | BuildDependencyType.CompileAsset.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AssetCompiler is unable to resolve material references from layers of materials
3 participants