ProwlEngine / Prowl

An Open Source C# 3D Game Engine under MIT license, inspired by Unity and featuring a complete editor and built on Silk.NET
MIT License
367 stars 29 forks source link

[Editor] Crash on Recompiling project scripts #7

Closed michaelsakharov closed 9 months ago

michaelsakharov commented 10 months ago

Describe the bug The Editor crashes when recompiling project scripts after a Custom component has been added to a GameObject

To Reproduce

  1. Create .cs MonoBehvaiour script file in your project
  2. Add the MonoBehaviour to a gameobject
  3. Modify the script again or press Recompile Scripts
  4. Crash
10xJosh commented 9 months ago

Ill look into this a bit more tomorrow

michaelsakharov commented 9 months ago

Thanks, recompiling is supposed to delete all gameobjects and their components and then put them back after, but it seems something is holding onto them so it cant unload the script assembly.

michaelsakharov commented 9 months ago

If it doesn't already just delete all objects before recompiling, don't worry about bringing them back just yet, we don't have a scene system yet so there's no proper way to reload the world/scene

10xJosh commented 9 months ago

Howdy! I'm having trouble trying to figure out how to add a .cs script to a component. Could you walk me through how to do it? Also, I noticed while I had the editor opened and I'd change a file I'd get an error, is this similar to what you're getting when you recompile?

2023-11-28 21_34_11-Prowl (Debugging) - Microsoft Visual Studio

michaelsakharov commented 9 months ago

Hey!, You should be able to just add a .cs file from File Explorer (I pushed a commit that implements the ShowInExplorer option in the Asset Browser):

https://github.com/michaelsakharov/Prowl/assets/8621606/cd4235f6-25a2-4a01-b6b1-0d6f65a138cd

michaelsakharov commented 9 months ago

Howdy! I'm having trouble trying to figure out how to add a .cs script to a component. Could you walk me through how to do it? Also, I noticed while I had the editor opened and I'd change a file I'd get an error, is this similar to what you're getting when you recompile?

2023-11-28 21_34_11-Prowl (Debugging) - Microsoft Visual Studio

Looks like this is a Differant issue

10xJosh commented 9 months ago

Hey!, You should be able to just add a .cs file from File Explorer (I pushed a commit that implements the ShowInExplorer option in the Asset Browser): Untitled.Project.mp4

I was so close to figuring it out! Thanks for the walkthrough!

10xJosh commented 9 months ago

Howdy! I'm having trouble trying to figure out how to add a .cs script to a component. Could you walk me through how to do it? Also, I noticed while I had the editor opened and I'd change a file I'd get an error, is this similar to what you're getting when you recompile? 2023-11-28 21_34_11-Prowl (Debugging) - Microsoft Visual Studio

Looks like this is a Differant issue

Ok ill create an issue for this one

michaelsakharov commented 9 months ago

Was working on something else and skimmed over this code, seems like i already have the GameObject reload stuff, so something else is holding onto the Component, as long as that component is in memory (Garbage collector cant dispose of it) then we cant unload the script assembly.

10xJosh commented 9 months ago

So it looks like the error is being caused by the CSharp-Editor.dll and CSharp.dll in the temp folder. Apparently they're being locked by another thread preventing them from being deleted from temp when the project is recompiled.

The code that's causing the issue can be found in the Prowl/Prowl.Editor/EditorApplication.cs Line 143

// Delete everything under Temp\Bin
if (Directory.Exists(Path.Combine(Project.TempDirectory, "bin")))
    Directory.Delete(Path.Combine(Project.TempDirectory, "bin"), true);
Directory.CreateDirectory(Path.Combine(Project.TempDirectory, "bin"));

Some of the debugging screenshots:

2023-11-28 23_27_20-Prowl (Debugging) - Microsoft Visual Studio

2023-11-28 23_29_08-Microsoft Visual Studio Debug Console

michaelsakharov commented 9 months ago

Yeah we need to delete the old assemblies so that we can recompile new ones in its place It works as intended if you don't use the Component.

I don't think threads are involved since we don't really use threads anywhere at the moment, the project is almost entirely single threaded, which needs to change but thats another issue.

I'm pretty sure the problem is that a Reference to the GameObject which has the component attached to it, still exists somewhere in the code even after deleting all GameObjects.

10xJosh commented 9 months ago

I'm pretty sure the problem is that a Reference to the GameObject which has the component attached to it, still exists somewhere in the code even after deleting all GameObjects.

Yeah looks like that is the case, im gonna have to look into a bit more. Still not sure where the issue is coming from.

10xJosh commented 9 months ago

Ok, little update, I checked _componentCache, _components, cachedObjectTypes and allObjects lists and everything looks like its being deleted properly. I think the issue is stemming from the way assemblies are unloaded. It seems like they aren't unloading at all.

Under Prowl.Runtime/ExternalAssemblyLoadContextManager.cs on line 71 it seems like the function call _externalAssemblyLoadContext.Unload(); isn't unloading. The _assemblyDependencyResolvers parameter never gets updated after it runs unlike the other unloads and disposes in the rest of the code.

My theory is that since the assembly is still in use/alive, when the code goes to delete the folder, we get the "System.UnauthorizedAccessException", because its still "in use" causing the crash. Thats where I'm at so far.

michaelsakharov commented 9 months ago

The assembly should be getting unloaded, since if you create the script but don't assign the component, you can keep recompiling the project and changing the script.

And the Unload, Assembly deletion, Compile and Load all works fine. It only breaks once you add a component.,

michaelsakharov commented 9 months ago

Ill take a look at this issue today.

michaelsakharov commented 9 months ago

https://learn.microsoft.com/en-us/dotnet/standard/assembly/unloadability#troubleshoot-unloadability-issues

Seems like there can be a lot of issues for this Newtonsoft our serializer might not support unloading assemblies it has serialized/deserialized stuff for, i guess cause it holds information for them in a cache.

10xJosh commented 9 months ago

I tried setting up the debugger and comparing the memory when it was unloaded with and without the component added and the only difference is that theres a ExternalAssemblyLoadContext1 object attached still which we already know is the case.

2023-12-02 21_47_59-Prowl Editor exe - PID_ 19948 - WinDbg 1 2308 2002 0

I don't know enough about this so I'll have to look into it later on or after you create the serialization you mentioned on discord. 🤷🏾‍♂️

michaelsakharov commented 9 months ago

Yeah, if the memory in out project no longer has the Component then it might be an external library like Newtonsoft that's caching a copy for some reason.

Im almost finished with the new serializer so once I get that in we can come back to this see if anything has changed,

michaelsakharov commented 9 months ago

Replacing Newtonsoft i think fixed this issue.

michaelsakharov commented 9 months ago

Yup, I think its fixed! :D

10xJosh commented 9 months ago

Awesome! Good work!