[ds5678]

PR Details

This pull request replaces ParameterKeyProcessor in the AssemblyProcessor with additional Roslyn source generation in EffectCodeWriter.

Related Issue

Part of #1835

Types of changes

• Docs change / refactoring / dependency upgrade

Checklist

• All new and existing tests passed.
• I have built and run the editor to try this change out.

Notes

• My code assumes that shader classes cannot be nested, which I think is disallowed, but I wanted to note it anyway.
• I had to modify a bunch of shader names in the RenderTests folder because there were conflicting names in the same namespace. Previously, the partial classes for the source generated Keys classes were just being silently merged.
• I'm not sure what the cadence is for updating the files in deps/AssemblyProcessor, but I included my compiled files.
• deps/AssemblyProcessor has a folder for .NET 8 assemblies. I think that should be deleted, but I did not do it in this pull request.
• I did not remove the command line argument because I encountered build issues when I tried removing a different argument earlier today.

[github-actions]

🤖 Draft PR — automatic CI is skipped to save runner minutes.

• Mark the PR ready for review to run the full automatic CI — or add a ci-run-on-draft label to run it now without leaving draft.
• Or arm a specific opt-in suite: ci-enduser, ci-editor, ci-ios, ci-android.

[ds5678]

Based on Discord feedback, I attempted to revert my changes to the shaders and disable the generator in Stride.Shaders.Tests, but I ran into a few issues. For starters, a few recent commits explicitly depend on generated code:

• 33016b3
• aa337af

However, even if I comment out the lines for the new tests from those commits while disabling the generator, I still get test failures. With the generator, there are no failed tests.

For reference, here is how I'm disabling the generator:

  <Target Name="RemoveGeneratorAnalyzer" BeforeTargets="CoreCompile">
    <ItemGroup>
      <Analyzer Remove="@(Analyzer)" Condition="'%(Analyzer.Filename)' == 'Stride.Shaders.Generators'" />
    </ItemGroup>
  </Target>

In light of this, I think that modifying the shaders is necessary. The only alternative I can think of is to move the generated __Initialize__ methods from the partial classes to a file static class. I am investigating this alternative and will report back.

[ds5678]

The only alternative I can think of is to move the generated __Initialize__ methods from the partial classes to a file static class. I am investigating this alternative and will report back.

This worked well and has been pushed, so this pull request no longer makes any changes to the test shaders.

[xen2]

The file static approach for the test shaders is good, let's do the namespace stuff later.

A few things before merging:

1. Hand-written keys in user code don't get a name anymore. ParameterKeyProcessor was patching every assembly, including user games, but now only generated keys and the engine files you edited get named. For example ParticlesSample has this in user code:

public static readonly ValueParameterKey<Color4> EmissiveValue = ParameterKeys.NewValue<Color4>();

and it will silently fail any lookup by name.
Could we have a small generator for that instead? Find static ParameterKey fields, emit a module initializer that runs the cctor and calls ParameterKeys.Merge(field, typeof(Owner), "Owner.Field") (UnsafeAccessor can deal with private fields). That way users just need to rebuild like before, and you can revert all the manual edits on the engine key files, which would make the PR much smaller.
If you don't want to do it in this PR, we could also keep ParameterKeyProcessor for hand-written keys until then.

Also for the engine side, a lot of those hand-written key classes shouldn't exist at all. For example FlareArtifactKeys.Count is only used by FlareArtifactEffect.sdfx (using params FlareArtifactKeys), so it could just be declared in a params block in the sdfx and be generated, and the .cs file goes away. Could be done as a follow-up.

Related: the processor was also running on generated code, and it removed the Keys suffix from the name (params FooKeys → Foo.Bar). The generator now uses the params block name as is, so it gives FooKeys.Bar. Nothing in the repo is affected (all params blocks end with Parameters/Params), but user effects with params XxxKeys would get different key names. Can you strip Keys like the processor did? Then moving FlareArtifactKeys into a params block also keeps FlareArtifact.Count.

2. I would avoid throwing in Merge. When GameStudio reloads the game assembly, the new one registers the same key names with a new owner Type, so I think it will throw there (didn't test it). Better to keep last-one-wins like before, and maybe just log an error if assembly name + type full name are different.

3. The default values you added in SpriteBase.sdsl probably make the embedded bytecodes stale, can you check EmbeddedEffectsAreCurrent (and regenerate with the command in the test message if needed)?

Longer term (not for this PR): I'd like registration to be per assembly, with Register/Unregister done by the host like DataSerializerFactory during reload and undo/redo. We need that for plugin unload anyway. The generator from 1. could then just output that list instead of calling Merge, no change needed in user code.

[ds5678]

@xen2 I implemented all your requested changes except for the new generator. I have a few follow-up questions about that:

• Searching every assembly for static fields inheriting from ParameterKey would be very inefficient. Are you okay with a marker attribute? I can make it work at both the type level and assembly level.
• Do you have a preference for the name of said attribute?
• Should the attribute be in Stride.Foundation or Stride.Shaders?
• Should the generator be in Stride.Core.CompilerServices or Stride.Shaders.Generators?

[ds5678]

@xen2 I implemented all your requested changes except for the new generator. I have a few follow-up questions about that:

• Searching every assembly for static fields inheriting from ParameterKey would be very inefficient. Are you okay with a marker attribute? I can make it work at both the type level and assembly level.
• Do you have a preference for the name of said attribute?
• Should the attribute be in Stride.Foundation or Stride.Shaders?
• Should the generator be in Stride.Core.CompilerServices or Stride.Shaders.Generators?

I rebased my changes, implemented the new generator, and made the Stride codebase use that generator. Since I didn't get any answers to the above questions, I made some decisions during development:

• There is a marker attribute. It can be applied on types or assemblies. In the Stride codebase, the attribute is applied to the Stride.Rendering assembly and the Stride.Voxels assembly. Types outside those two assemblies have the attribute applied directly.
• It is named EffectKeysAttribute, which is similar to the previously used AssemblyEffectKeysAttribute that ParameterKeyProcessor applied to processed assemblies.
• The attribute is in the same directory as ParameterKey, ie the Stride.Foundation assembly.
• The new generator is in Stride.Core.CompilerServices, which enables it to share code with the assembly scan generator.