ExtendRealityLtd / Malimbe

[Obsolete - No longer maintained] A collection of tools to simplify writing public API components for the Unity software.
MIT License
51 stars 11 forks source link

Use RequestScriptReload #69

Closed simonferquel closed 2 years ago

simonferquel commented 2 years ago

WeaveAllAssemblies() currently depends on the ordering of pre-compiled assembly loading / vs. assets loading. Built assemblies (aside from the pre-defined assemblies) are actually not tracked by ADB. So if the Weaver integration dll is loaded after everything has been imported (and after the project has been compiled a first time) - which is the case with Unity 2020.3+, AssetDatabase.Refresh() won't do anything.

thestonefox commented 2 years ago

Thanks for the info on this, I should be able to test that locally

thestonefox commented 2 years ago

This is failing with:

Sources\FodyRunner.UnityIntegration\EditorWeaver.cs(53,35): Error CS0117: 'EditorUtility' does not contain a definition for 'RequestScriptReload'

Is RequestScriptReload a new feature that isn't backward compatible with older versions of Unity?

simonferquel commented 2 years ago

It is introduced in 2019.4. The PR could be modified to build a "pre-2019.4" version and a new one, and use version define constraints to load the correct one depending on the editor version. Otherwise, if the package is reworked to include integration source code, it can be done via a simple #if/#else block

thestonefox commented 2 years ago

What about this for a solution?

if (didChangeAnyAssembly)
{
    AssetDatabase.Refresh();
    Reflection.MethodInfo checkMethod = typeof(EditorUtility).GetMethod("RequestScriptReload", Reflection.BindingFlags.Public | Reflection.BindingFlags.Static);
    if (checkMethod != null)
    {
        checkMethod.Invoke(null, null);
    }
}

Do the refresh anyway as that will work on older versions of unity, then look up at runtime (i.e. when the editor starts) to see if the new method exists via reflection, if it exists then invoke it.

This way, Malimbe will support the older versions of unity and the newer versions all within one version.

simonferquel commented 2 years ago

Should do it yes

thestonefox commented 2 years ago

All looks good. Can i request a tidy up to the pull request before it's merged?

There only needs to be the 1 commit (the last one) so if you can just rebase and delete the previous commit. then amend the commit message so it adheres to our conventional commits, this would do:

feat(UnityIntegration): invoke RequestScriptReload by reflection

The EditorUnity class now has a RequestScriptReload method that can be used
to ensure the weavers are run on first compilation. This will resolve the issue
where weavers are not run first time on Unity 2020 or above.

Signed-off-by: Simon Ferquel <simon.ferquel@unity3d.com>

If you don't have time to do this, I can raise the PR and put you down as the co-author of it for credit if you prefer.

simonferquel commented 2 years ago

done! thanks

ExtendReality-Bot commented 2 years ago

:tada: This PR is included in version 9.7.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: