Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BPModLoaderMod: Fix problem with out-of-scope variables in async call and ensure that LoadMods is always called in a game thread #752

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

igromanru
Copy link
Contributor

@igromanru igromanru commented Jan 11, 2025

Description

  • Removed ExecuteInGameThread call from the LoadMod function.
  • Ensured that LoadMods is always called in a game thread instead.
  • Added error messages in case the World is nil in either the LoadMods or LoadMod function.

According to the current mod logic with the new UEHelpers v3, the parameter World shouldn't be nil in either LoadMods or LoadMod. It can be an invalid UObject, but never nil. Therefore, if it should happen, we should assume that it is an error in BPModLoaderMod's code.

Quality control

Tested in Palworld v0.4.12 with Mod Config Menu (UI) and Basic MiniMap mods.
Here is a snippet from the log file without timestamps for better visibility:

Starting Lua mod 'BPModLoaderMod'
[Lua] [BPModLoaderMod] Mods/BPModLoaderMod/load_order.txt not present or no matching mods, loading all BP mods in random order.
[Lua] [BPModLoaderMod] DekBasicMinimap_P == table: 000001D4366428B0
[Lua] [BPModLoaderMod]     AssetName == ModActor_C
[Lua] [BPModLoaderMod]     AssetNameAsFName == FNameUserdata: 000001D436575B88
[Lua] [BPModLoaderMod]     Name == DekBasicMinimap_P
[Lua] [BPModLoaderMod]     AssetPath == /Game/Mods/DekBasicMinimap_P/ModActor
[Lua] [BPModLoaderMod] DekModConfigMenu_P == table: 000001D436643230
[Lua] [BPModLoaderMod]     AssetName == ModActor_C
[Lua] [BPModLoaderMod]     AssetNameAsFName == FNameUserdata: 000001D436575B88
[Lua] [BPModLoaderMod]     Name == DekModConfigMenu_P
[Lua] [BPModLoaderMod]     AssetPath == /Game/Mods/DekModConfigMenu_P/ModActor
Mod 'jsbLuaProfilerMod' disabled in mods.txt.
Starting Lua mod 'Keybinds'
Starting mods (from enabled.txt, no defined load order)...
Mod 'DekModConfigMenu' has enabled.txt, starting mod.
Event loop start
[Lua] [BPModLoaderMod] Loading mod: DekBasicMinimap_P
[Lua] [BPModLoaderMod] Actor: ModActor_C /Temp/Untitled_0.Untitled:PersistentLevel.ModActor_C_2147482480
[Lua] [BPModLoaderMod] Loading mod: DekModConfigMenu_P
[Lua] [BPModLoaderMod] Actor: ModActor_C /Temp/Untitled_0.Untitled:PersistentLevel.ModActor_C_2147482479
[Lua] [BPModLoaderMod] Loading mod: DekBasicMinimap_P
[Lua] [BPModLoaderMod] Actor: ModActor_C /Game/Pal/Maps/Login/PL_Login.PL_Login:PersistentLevel.ModActor_C_2147482144
[Lua] [BPModLoaderMod] Loading mod: DekModConfigMenu_P
[Lua] [BPModLoaderMod] Actor: ModActor_C /Game/Pal/Maps/Login/PL_Login.PL_Login:PersistentLevel.ModActor_C_2147482143
[Lua] [DekModConfigMenu] -RegisteringCustomEvent:SetupModConfigBindings
[Lua] [DekModConfigMenu] ModActor_C_2147482144:OnSavedModConfig found!
[Lua] [DekModConfigMenu] -RegisteringHook
[RegisterHook] Registered script hook (3, 3) for Function /Game/Mods/DekModConfigMenu_P/Widgets/WBP_ModConfigMenuUI.WBP_ModConfigMenuUI_C:AfterClickSavedButton
[Lua] [DekModConfigMenu] -RegisteredHook(3, 3) With CB: DekBasicMinimap_P:OnSavedModConfig
[Lua] [DekModConfigMenu] ModActor_C_2147482144:OnUpdatedModConfig found!
[Lua] [DekModConfigMenu] -RegisteringHook
[RegisterHook] Registered script hook (4, 4) for Function /Game/Mods/DekModConfigMenu_P/Widgets/WBP_ModConfigMenuUI.WBP_ModConfigMenuUI_C:AfterUpdatedConfig
[Lua] [DekModConfigMenu] -RegisteredHook(4, 4) With CB: DekBasicMinimap_P:OnUpdatedModConfig
[Lua] [DekBasicMinimap_P] scanning for player ui
[Lua] [BPModLoaderMod] Loading mod: DekBasicMinimap_P
[Lua] [BPModLoaderMod] Actor: ModActor_C /Game/Pal/Maps/Title/PL_Title.PL_Title:PersistentLevel.ModActor_C_2147482031
[Lua] [BPModLoaderMod] Loading mod: DekModConfigMenu_P
[Lua] [BPModLoaderMod] Actor: ModActor_C /Game/Pal/Maps/Title/PL_Title.PL_Title:PersistentLevel.ModActor_C_2147482030
[Lua] [DekModConfigMenu] -RegisteringCustomEvent:SetupModConfigBindings
[Lua] [DekModConfigMenu] ModActor_C_2147482031:OnSavedModConfig found!
[Lua] [DekModConfigMenu] -UnregisteringHook(3, 3) With CB: DekBasicMinimap_P:OnSavedModConfig
Unregistering script hook with id: 3, FunctionName: /Game/Mods/DekModConfigMenu_P/Widgets/WBP_ModConfigMenuUI.WBP_ModConfigMenuUI_C:AfterClickSavedButton
[Lua] [DekModConfigMenu] -RegisteringHook
[RegisterHook] Registered script hook (5, 5) for Function /Game/Mods/DekModConfigMenu_P/Widgets/WBP_ModConfigMenuUI.WBP_ModConfigMenuUI_C:AfterClickSavedButton
[Lua] [DekModConfigMenu] -RegisteredHook(5, 5) With CB: DekBasicMinimap_P:OnSavedModConfig
[Lua] [DekModConfigMenu] ModActor_C_2147482031:OnUpdatedModConfig found!
[Lua] [DekModConfigMenu] -UnregisteringHook(4, 4) With CB: DekBasicMinimap_P:OnUpdatedModConfig
Unregistering script hook with id: 4, FunctionName: /Game/Mods/DekModConfigMenu_P/Widgets/WBP_ModConfigMenuUI.WBP_ModConfigMenuUI_C:AfterUpdatedConfig
[Lua] [DekModConfigMenu] -RegisteringHook
[RegisterHook] Registered script hook (6, 6) for Function /Game/Mods/DekModConfigMenu_P/Widgets/WBP_ModConfigMenuUI.WBP_ModConfigMenuUI_C:AfterUpdatedConfig
[Lua] [DekModConfigMenu] -RegisteredHook(6, 6) With CB: DekBasicMinimap_P:OnUpdatedModConfig
[Lua] [DekModConfigMenu_P] req mod config menu
[Lua] [DekModConfigMenu] Reading LUA Mod config from: Win64!
[Lua] [DekModConfigMenu] Reading LUA Mod config from: ue4ss subfolder (latest exp release!
[Lua] [DekModConfigMenu] FOUND ModFolder: BPModLoaderMod
[Lua] [DekModConfigMenu] IGNORING: Mods/BPModLoaderMod/.luarc.json
[Lua] [DekModConfigMenu] IGNORING: Mods/BPModLoaderMod/load_order.txt
[Lua] [DekModConfigMenu] FOUND ModFolder: jsbLuaProfilerMod
[Lua] [DekModConfigMenu] FOUND ModFolder: ActorDumperMod
[Lua] [DekModConfigMenu] FOUND ModFolder: ConsoleEnablerMod
[Lua] [DekModConfigMenu] FOUND ModFolder: SplitScreenMod
[Lua] [DekModConfigMenu] FOUND ModFolder: Keybinds
[Lua] [DekModConfigMenu] FOUND ModFolder: ConsoleCommandsMod
[Lua] [DekModConfigMenu] FOUND ModFolder: KismetDebuggerMod
[Lua] [DekModConfigMenu] FOUND ModFolder: BPML_GenericFunctions
[Lua] [DekModConfigMenu] FOUND ModFolder: shared
[Lua] [DekModConfigMenu] IGNORING: Mods/shared/Types.lua
[Lua] [DekModConfigMenu] FOUND ModFolder: CheatManagerEnablerMod
[Lua] [DekModConfigMenu] FOUND ModFolder: LineTraceMod
[Lua] [DekModConfigMenu] FOUND ModFolder: DekModConfigMenu
[Lua] [DekModConfigMenu] IGNORING: Mods/DekModConfigMenu/enabled.txt
[Lua] [DekModConfigMenu] FOUND BP MOD CONFIG: DekBasicMinimap_P.modconfig.json
[Lua] [DekModConfigMenu] IGNORING: LogicMods/DekBasicMinimap_P.pak
[Lua] [DekModConfigMenu] IGNORING: LogicMods/DekBasicMinimap_P.png
[Lua] [DekModConfigMenu] IGNORING: LogicMods/DekModConfigMenu_P.pak
[Lua] [DekModConfigMenu] ADDED: D:\Data\qLoad\Games\Palworld-RuTracker\Pal\Content\Paks\LogicMods\DekBasicMinimap_P.modconfig.json
[Lua] [Widgets] D:\Data\qLoad\Games\Palworld-RuTracker\Pal\Content\Paks\LogicMods\DekBasicMinimap_P.modconfig.json
[Lua] [DekModConfigMenu_P] mod config closed

Review

Please review the code, then when the code is approved, I'll write the Changelog and make a PR out of the Draft.

feat: Make sure that LoadMods gets executed in a game thread
refactor: Change code order to make sure that functions are over the code the gets executed on mod load
…. It can only happen if there is a bug in BPModLoaderMod's code.
@UE4SS
Copy link
Collaborator

UE4SS commented Jan 11, 2025

The code looks good but I don't link the changes to Log.
Manually specifying when a line should end is the way we have always done it so I don't like changing it just in this one instance.
I think the commit making changes to Log should be reverted and a line break should be added wherever one is missing.

@igromanru
Copy link
Contributor Author

Why? It's the whole point of a function. To take code that is repetitive and do it once.
Each single print line should end with a new line and exactly because it didn't, someone forgot to add the \n at the end of one of the logs.

Log(string.format("Mods/BPModLoaderMod/load_order.txt not present or no matching mods, loading all BP mods in random order."))

we have always done it so I don't like changing it just in this one instance.

Each of the default Lua mods has its own implementation of the Log function, but most of them don't even have such a function and simply use print.
There is no consistency...
Or maybe you want me rather remove the Log function completely and just write [BPModLoader] in front of each print text? Then it would match 80% of other mods.

If you want consistency and do it right, it would be better to make a sort of Utils library that has such functions that every mod can use.
https://github.com/igromanru/BaseUtils-UE4SS/blob/5c4b15e4535038d49d831ae4c4c8a618a060746c/BaseUtils.lua#L27

@UE4SS
Copy link
Collaborator

UE4SS commented Jan 11, 2025

Why? It's the whole point of a function. To take code that is repetitive and do it once. Each single print line should end with a new line and exactly because it didn't, someone forgot to add the \n at the end of one of the logs.

Log(string.format("Mods/BPModLoaderMod/load_order.txt not present or no matching mods, loading all BP mods in random order."))

we have always done it so I don't like changing it just in this one instance.

Each of the default Lua mods has its own implementation of the Log function, but most of them don't even have such a function and simply use print. There is no consistency... Or maybe you want me rather remove the Log function completely and just write [BPModLoader] in front of each print text? Then it would match 80% of other mods.

If you want consistency and do it right, it would be better to make a sort of Utils library that has such functions that every mod can use. https://github.com/igromanru/BaseUtils-UE4SS/blob/5c4b15e4535038d49d831ae4c4c8a618a060746c/BaseUtils.lua#L27

As far as I can tell, there's only one outlier and that's ConsoleCommandsMod.
We use explicit \n throughout all our systems, and I don't think we should change that, and certainly not as part of random other PR that has nothing to do with it.
A new println global function can certainly be introduced in a new PR however.

We also shouldn't be creating a wrapper for print in some kind of library either because we already override the print function from Lua for that purpose.
There are two reasons we have Log wrappers, because our print wrapper doesn't add the mod name (which we can easily fix globally), or because we also want to print to the UE console.

Making changes to the log function isn't within the scope of what this PR is trying to do, which is fix a bug with unsafely accessing variables asynchronously, it's unnecessarily causing bloat in the diff.
Making a small change like adding a missing \n is something I can accept even if it's unrelated, but making sweeping changes to a mod just makes it more complicated to review, and yes I realize this particular diff isn't that difficult but it's the principle.

@igromanru igromanru force-pushed the BPModLoaderMod_World_scope_fix branch from 9031c8d to ada6a92 Compare January 11, 2025 19:16
@igromanru igromanru marked this pull request as ready for review January 11, 2025 22:19
@igromanru
Copy link
Contributor Author

Log changes reverted, Changelog edited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants