stride icon indicating copy to clipboard operation
stride copied to clipboard

feat: Assembly Processor Module Initializer to Roslyn

Open IXLLEGACYIXL opened this issue 1 year ago • 5 comments

PR Details

Entry point to convert assembly processor to roslyn.

  1. ModuleInitializers are source generated through roslyn not mono cecil
  2. ModuleInitializerProcessor is gone
  3. converted all mono.cecil processors to directly write into the < Module > instead of writing to ModuleInitalizer which then gets scanned just for the sake to be scanned again

Related Issue

didnt find yet the tracking

Types of changes

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

Checklist

  • [x] My change requires a change to the documentation. if anything existed about this in docs you cant get anymore a higher priority than Assemblyscan or Serializationprocessor updatables have always the lowest priority for their registration ( for now ) shaders have a random registration timing based on what C# decides
  • [ ] I have added tests to cover my changes.
  • [x] All new and existing tests passed.
  • [x] I have built and run the editor to try this change out.

Remarks

Order of ModuleInitializers is a horrible concept, the reason why it was done from my analysis AssemblyScan and SerializationProcessor had to be registered before anything else, IF these arent the first ones to be registered stride just dies at multiple points, which means if you give a higher priority of -999 and you rely on serialization then it just breaks

There is no other usage of it, so if the rest gets translated to roslyn, we can generate first the assemblyscan and serialization register AND then add all others in whatever order

IXLLEGACYIXL avatar Jul 31 '24 00:07 IXLLEGACYIXL

ready for review, it works no idea about hte assemblyprocessor change in the json? should that be ? didnt do that intentionally

until the attribute question isnt solved icant continue with roslyn, so this is as far as it gets

IXLLEGACYIXL avatar Jul 31 '24 21:07 IXLLEGACYIXL

When referencing a source generator from a project there are some additional parameters you need:

<ProjectReference Include="SourceGenerator.csproj" OutputItemType="Analyzer" ReferenceOutputAssembly="false" />

That's probably why the .deps.json is changing.

fydar avatar Jul 31 '24 22:07 fydar

When referencing a source generator from a project there are some additional parameters you need:

<ProjectReference Include="SourceGenerator.csproj" OutputItemType="Analyzer" ReferenceOutputAssembly="false" />

That's probably why the .deps.json is changing.

the assembly processor doesnt project reference stride.core which is currently the way its implemented that if you reference stride.core you get the compilerservices see Stride.Core.CompilerServices.props

IXLLEGACYIXL avatar Jul 31 '24 22:07 IXLLEGACYIXL

@tebjan could you try if it affects vvvv or if you see shader problems? Tests are passing but not sure if tests cover that much

IXLLEGACYIXL avatar Aug 01 '24 07:08 IXLLEGACYIXL

This PR is important for the Avalonia port (in the area of loading the editor "plugins" and discovering the types for views and viewmodels).

Since Avalonia 11.1.x, the old module initializer doesn't work anymore because Avalonia own IL post-processing seems to break our generated <Module> class. It didn't occur with Avalonia 11.0.x and older versions.

I tried this PR on top of Avalonia 11.1.x and it seems to work again.

Kryptos-FR avatar Aug 27 '24 09:08 Kryptos-FR

can this be reviewed so avalonia and roslyn transition can continue?

IXLLEGACYIXL avatar Sep 24 '24 15:09 IXLLEGACYIXL

Overall it looks good to me. However, I tried to run it on Teamcity, and it seems incremental build is broken.

image As you can see on this image, if I rebuild master twice in a row, the second build takes only 1 min. Looking at the log, I can see lot of stuff skipped away like this:

Skipping target "CoreCompile" because all output files are up-to-date with respect to the input files.

However, this doesn't happen anymore with this branch.

Let me know if you need help looking into it.

is this now an issue with this branch or is it better ?

i dont know yet what i can do about it, the asemblyprocessor has to be rebuilt and wasnt there an issue with rebuilding it? Like i dont know what to do as teh tests of the engine passed

IXLLEGACYIXL avatar Sep 26 '24 11:09 IXLLEGACYIXL

Sorry if I wasn't clear. It is a regression introduced in this branch.

It seems using this incremental generator breaks the "rebuild" optimizations. If I was to not change any file (or only in GameStudio.dll), it would skip most of the rebuild as C# input files are same. But with this branch, it seems it rebuilds everything, even if there are no changes.

I assume it's because it needs to know the "dependencies" (file read) when generating code. I will take a quick look at if/how it can be controlled with source generators.

xen2 avatar Sep 27 '24 03:09 xen2

FYI, it seems to work on my computer. It might be just the build agent. Updating to latest VS2022, and if not enough I will investigate/debug on the build agent itself.

xen2 avatar Sep 27 '24 03:09 xen2

Please disregard previous comments, it was simply due to the agent doing a git clean when not on master (on TeamCity, I used "Clean On Branch Change", it shouldn't have happened with two builds of this branch in a row, somehow master is treated differently....)

xen2 avatar Sep 27 '24 07:09 xen2

merging this also means nugets using the stride libs need to update their stride core and need to be recompiled so the genreator can be applied

IXLLEGACYIXL avatar Sep 27 '24 11:09 IXLLEGACYIXL

someone could try to measure build time if it got faster or slower if someone is bored but its not too important

IXLLEGACYIXL avatar Sep 28 '24 19:09 IXLLEGACYIXL

Looks like this PR is ready to be merge ?

Eideren avatar Sep 29 '24 14:09 Eideren

seems so yes

IXLLEGACYIXL avatar Sep 29 '24 14:09 IXLLEGACYIXL

Thanks !

Eideren avatar Sep 29 '24 14:09 Eideren