Basic hot reload support for mod development #160
Conversation
It seemed to work before but I think calling unity APIs from background threads is undefined behaviour
| /// <para><b>Limitations:</b></para> | ||
| /// <list type="bullet"> | ||
| /// <item><description> | ||
| /// The old assembly does not get unloaded. If you have created any unity game objects or components, |
There was a problem hiding this comment.
This seems rough, not because of needing to destroy stuff in unload but because it makes a reload costly.
There was a problem hiding this comment.
Costly compared to what?
The regular use case should be unchanged, even with EnableHotReload enabled, and only subsequent changes will incur the extra assembly overhead.
And I think even that overhead is not too bad, when developing with scriptengine in nine sols I rarely had to restart the game because it became laggy after too many hot reload cycles.
| string mods = Path.Combine(ManagedPath, "Mods"); | ||
|
|
||
| string[] modDirectories = Directory.GetDirectories(mods) | ||
| .Except([Path.Combine(mods, "Disabled")]) |
There was a problem hiding this comment.
Guessing we are not using modern enough build tooling for c#13 to be supported here
There was a problem hiding this comment.
I can of course revert to the old syntax, it's not a big deal.
However, I think that this is a syntax-only feature and I've been using it locally without problem, so if I were to simply bump dotnet from 6.0 (end of life since November 12, 2024) to 8.0 in the github actions, everything should work even on the old net472 runtime.
There was a problem hiding this comment.
Right, I can't actually make that change in this PR, since the action from main gets used.
|
If you referenced bepinex.scriptengine heavily, make sure everything is on board in terms of licensing. It looks like it probably isn't because
|
I don't think I did? I briefly read their code, but wrote the code here myself. I can of course still add comment crediting the original code for the inspiration. |
jakobhellermann
force-pushed
the
hot-reload
branch
from
26da64d to
0325ec9
Compare
|
My main concern with this would just be people not disposing of resources properly but that's their problem I suppose? We could introduce a hook context for this I think like tmodloader et al do, but I'll take a look at that myself ig. If anyone else has also tested this I'm fine with it though. I would love if the formatting was like the normal c# style though. Sorry for the wait on reviewing this one as well. |
| <PropertyGroup> | ||
| <OutputDir>$(SolutionDir)/OutputFinal</OutputDir> | ||
| <!-- todo macOS --> | ||
| <GamePath Condition="'$(OS)' == 'Windows_NT'">C:/Program Files (x86)/Steam/steamapps/common/Hollow Knight</GamePath> |
There was a problem hiding this comment.
I'd ideally give an env variable for this because these paths aren't universal (external drive steam library, flatpak, etc). Or Directory.Build.props. Can still default if it's unset but then people with less standard paths don't have to mess with the checked in csproj
There was a problem hiding this comment.
If I understand correctly, Directory.Build.Props automatically overrides the solution properties, so putting
<Project>
<PropertyGroup>
<GamePath>D:/Games/Hollow Knight</GamePath>
</PropertyGroup>
</Project>next to the solution will just work.
Of course you have to know that this exists. I guess you could leave a comment mentioning it here? Or have a Directory.Build.Props.example checked into git.
| string directory = Path.GetDirectoryName(location); | ||
| string globalSettingsOverride = Path.Combine(directory, globalSettingsFileName); | ||
| if (string.IsNullOrEmpty(location)) | ||
| { // TODO: not set while hot reloading |
There was a problem hiding this comment.
This is kind of annoying. If I'm understanding this feature, if you have a DebugMod.GlobalSettings.json in GameDir/Managed/Mods/DebugMod/, then it will be loaded with priority over SavesDir/DebugMod.GlobalSettings.json.
Of course, while reloading, we can't look up the path of the assembly. I think lumafly puts each mod into a directory of the same name, but if you manually copy MyCoolFolder/DebugMod.dll into your Managed, then hot reloading has no idea about this settings override file.
Maybe this is fine for a niche use case in a development feature.
There was a problem hiding this comment.
Settings overrides are generally intended for mod pack distribution, personally I don't mind if we just say this isn't supported with hot reloading but I have no decision power
fifty-six
force-pushed
the
master
branch
3 times, most recently
from
53e753f to
f51a18b
Compare
3 participants
Similar to how BepInEx/ScriptEngine works.
When a file is changed, the old loaded mod will be
Unloaded (only works forIToggleablemods) and the new version of the assembly is loaded next to it. To fix conflicts, the assemblies name gets modified to$originalName-$currentTimestampusing Cecil.Configurable via
EnableHotReload.