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
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sources/engine/Stride.Assets/Materials/MaterialAssetCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

stride/sources/assets/Stride.Core.Assets/Analysis/BuildAssetNode.cs

Lines 148 to 155 in a4985e6

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 •
edited
Loading

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.

yield return new BuildDependencyInfo(typeof(GameSettingsAsset), typeof(AssetCompilationContext), BuildDependencyType.CompileAsset);
}

Expand Down