From d88d8ff91ae949a48d6ec18cf22db978ed2da13d Mon Sep 17 00:00:00 2001 From: Bit-Crust <166267954+Bit-Crust@users.noreply.github.com> Date: Mon, 20 Jan 2025 15:58:43 -0600 Subject: [PATCH 1/2] Thinking ordered scripts, previously scripts were recorded with an unordered map from script name to enabled state, however this doesn't guarantee maintained order, causing script failure. An ordered map wouldn't work because it requires a comparison function, when the order needs to be determined by insertion order. It seems like it may as well be handled with a vector of script names, while maintaining the unordered map of script name to enabled state. However, each script name is recorded twice there (this is also what I have implemented, it works). Probably the best solution would be to just keep a vector of script name and a vector of corresponding enabled states, however the efficiency of searching goes from log of size to linear with position, which seems bad, but nothing should ever have enough scripts on it to be a problem, is what I imagine. Feels like I'm missing the optimal solution. --- Source/Entities/MovableObject.cpp | 28 +++++++++++++++------------- Source/Entities/MovableObject.h | 9 +++++---- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/Source/Entities/MovableObject.cpp b/Source/Entities/MovableObject.cpp index 7ef5ba1b35..690edafb0b 100644 --- a/Source/Entities/MovableObject.cpp +++ b/Source/Entities/MovableObject.cpp @@ -79,6 +79,7 @@ void MovableObject::Clear() { m_HUDVisible = true; m_IsTraveling = false; m_AllLoadedScripts.clear(); + m_EnabledScripts.clear(); m_FunctionsAndScripts.clear(); m_StringValueMap.clear(); m_NumberValueMap.clear(); @@ -225,8 +226,8 @@ int MovableObject::Create(const MovableObject& reference) { m_PostEffectEnabled = reference.m_PostEffectEnabled; m_ForceIntoMasterLuaState = reference.m_ForceIntoMasterLuaState; - for (auto& [scriptPath, scriptEnabled]: reference.m_AllLoadedScripts) { - LoadScript(scriptPath, scriptEnabled); + for (const auto& scriptPath: reference.m_AllLoadedScripts) { + LoadScript(scriptPath, reference.m_EnabledScripts.at(scriptPath)); } if (reference.m_pScreenEffect) { @@ -442,7 +443,7 @@ int MovableObject::Save(Writer& writer) const { writer << m_CanBeSquished; writer.NewProperty("HUDVisible"); writer << m_HUDVisible; - for (const auto& [scriptPath, scriptEnabled]: m_AllLoadedScripts) { + for (const auto& [scriptPath, scriptEnabled]: m_EnabledScripts) { if (!scriptPath.empty()) { writer.NewProperty("ScriptPath"); writer << scriptPath; @@ -545,7 +546,8 @@ int MovableObject::LoadScript(const std::string& scriptPath, bool loadAsEnabledS } } - m_AllLoadedScripts.try_emplace(scriptPath, loadAsEnabledScript); + m_EnabledScripts.try_emplace(scriptPath, loadAsEnabledScript); + m_AllLoadedScripts.push_back(scriptPath); std::unordered_map scriptFileFunctions; if (usedState.RunScriptFileAndRetrieveFunctions(scriptPath, GetSupportedScriptFunctionNames(), scriptFileFunctions) < 0) { @@ -580,14 +582,16 @@ int MovableObject::ReloadScripts() { movableObjectPreset->ReloadScripts(); } - std::unordered_map loadedScriptsCopy = m_AllLoadedScripts; + std::unordered_map enabledScriptsCopy = m_EnabledScripts; + std::vector loadedScriptsCopy = m_AllLoadedScripts; + m_EnabledScripts.clear(); m_AllLoadedScripts.clear(); m_FunctionsAndScripts.clear(); - for (const auto& [scriptPath, scriptEnabled]: loadedScriptsCopy) { - status = LoadScript(scriptPath, scriptEnabled); + for (const auto& scriptPath: loadedScriptsCopy) { + status = LoadScript(scriptPath, enabledScriptsCopy.at(scriptPath)); // If the script fails to load because of an error in its Lua, we need to manually add the script path so it's not lost forever. if (status == -4) { - m_AllLoadedScripts.try_emplace(scriptPath, scriptEnabled); + m_EnabledScripts.try_emplace(scriptPath, enabledScriptsCopy.at(scriptPath)); } else if (status < 0) { break; } @@ -618,7 +622,7 @@ bool MovableObject::EnableOrDisableScript(const std::string& scriptPath, bool en return false; } - if (auto scriptEntryIterator = m_AllLoadedScripts.find(scriptPath); scriptEntryIterator != m_AllLoadedScripts.end() && scriptEntryIterator->second == !enableScript) { + if (auto scriptEntryIterator = m_EnabledScripts.find(scriptPath); scriptEntryIterator != m_EnabledScripts.end() && scriptEntryIterator->second == !enableScript) { if (ObjectScriptsInitialized() && RunFunctionOfScript(scriptPath, enableScript ? "OnScriptEnable" : "OnScriptDisable") < 0) { return false; } @@ -640,7 +644,7 @@ bool MovableObject::EnableOrDisableScript(const std::string& scriptPath, bool en } void MovableObject::EnableOrDisableAllScripts(bool enableScripts) { - for (const auto& [scriptPath, scriptIsEnabled]: m_AllLoadedScripts) { + for (const auto& [scriptPath, scriptIsEnabled]: m_EnabledScripts) { if (enableScripts != scriptIsEnabled) { EnableOrDisableScript(scriptPath, enableScripts); } @@ -688,9 +692,7 @@ int MovableObject::RunFunctionOfScript(const std::string& scriptPath, const std: for (const LuaFunction& luaFunction: m_FunctionsAndScripts.at(functionName)) { const LuabindObjectWrapper* luabindObjectWrapper = luaFunction.m_LuaFunction.get(); if (scriptPath == luabindObjectWrapper->GetFilePath() && usedState.RunScriptFunctionObject(luabindObjectWrapper, "_ScriptedObjects", std::to_string(m_UniqueID), functionEntityArguments, functionLiteralArguments) < 0) { - if (m_AllLoadedScripts.size() > 1) { - g_ConsoleMan.PrintString("ERROR: An error occured while trying to run the " + functionName + " function for script at path " + scriptPath); - } + g_ConsoleMan.PrintString("ERROR: An error occured while trying to run the " + functionName + " function for script at path " + scriptPath); return -2; } } diff --git a/Source/Entities/MovableObject.h b/Source/Entities/MovableObject.h index 6bbc73c0de..3595571195 100644 --- a/Source/Entities/MovableObject.h +++ b/Source/Entities/MovableObject.h @@ -112,14 +112,14 @@ namespace RTE { /// Checks if the script at the given path is one of the scripts on this MO. /// @param scriptPath The path to the script to check. /// @return Whether or not the script is on this MO. - bool HasScript(const std::string& scriptPath) const { return m_AllLoadedScripts.find(scriptPath) != m_AllLoadedScripts.end(); } + bool HasScript(const std::string& scriptPath) const { return std::find(m_AllLoadedScripts.begin(), m_AllLoadedScripts.end(), scriptPath) != m_AllLoadedScripts.end(); } /// Checks if the script at the given path is one of the enabled scripts on this MO. /// @param scriptPath The path to the script to check. /// @return Whether or not the script is enabled on this MO. bool ScriptEnabled(const std::string& scriptPath) const { - auto scriptPathIterator = m_AllLoadedScripts.find(scriptPath); - return scriptPathIterator != m_AllLoadedScripts.end() && scriptPathIterator->second == true; + auto scriptPathIterator = m_EnabledScripts.find(scriptPath); + return scriptPathIterator != m_EnabledScripts.end() && scriptPathIterator->second == true; } /// Enables or dsiableds the script at the given path on this MO. @@ -1235,7 +1235,8 @@ namespace RTE { }; std::string m_ScriptObjectName; //!< The name of this object for script usage. - std::unordered_map m_AllLoadedScripts; //!< A map of script paths to the enabled state of the given script. + std::vector m_AllLoadedScripts; //!< A map of script paths to the enabled state of the given script. + std::unordered_map m_EnabledScripts; //!< A map of script paths to the enabled state of the given script. std::unordered_map> m_FunctionsAndScripts; //!< A map of function names to vectors of Lua functions. Used to maintain script execution order and avoid extraneous Lua calls. volatile bool m_RequestedSyncedUpdate; //!< For optimisation purposes, scripts explicitly request a synced update if they want one. From ab8b18b7427fe444ce8e04c8427de8502c810e4c Mon Sep 17 00:00:00 2001 From: Bit-Crust <166267954+Bit-Crust@users.noreply.github.com> Date: Mon, 20 Jan 2025 16:06:30 -0600 Subject: [PATCH 2/2] Changelog and comment. --- CHANGELOG.md | 2 ++ Source/Entities/MovableObject.h | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 814ed247f0..641471455f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -187,6 +187,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fixed several issues with the way pie menus and aiming interacts between players, such as opening the pie menu always resetting the M&KB player's aim and pie selection, as well as another issue where the pie menu would fail to appear entirely for some players. +- Fixed issue where scripts applied to `MovableObject`s could become disordered in certain circumstances. +
Removed diff --git a/Source/Entities/MovableObject.h b/Source/Entities/MovableObject.h index 3595571195..a749240b75 100644 --- a/Source/Entities/MovableObject.h +++ b/Source/Entities/MovableObject.h @@ -1235,7 +1235,7 @@ namespace RTE { }; std::string m_ScriptObjectName; //!< The name of this object for script usage. - std::vector m_AllLoadedScripts; //!< A map of script paths to the enabled state of the given script. + std::vector m_AllLoadedScripts; //!< A vector of script for scripts applied to this object, in order of insertion. std::unordered_map m_EnabledScripts; //!< A map of script paths to the enabled state of the given script. std::unordered_map> m_FunctionsAndScripts; //!< A map of function names to vectors of Lua functions. Used to maintain script execution order and avoid extraneous Lua calls.