From 06cb72e3f6f0cad892e3b1408f58150e663889c0 Mon Sep 17 00:00:00 2001 From: Pavel Solodovnikov Date: Mon, 15 Apr 2024 16:56:46 +0300 Subject: [PATCH] wzapi: change `removeObject` API function to defer object removal Introduce `scriptQueuedObjectRemovals()` vector to hold deferred object removal requests from `wzapi::removeObject` API. `wzapi::removeObject` API call adds the game objects to this list. They will eventually be released during calls to `processScriptQueuedObjectRemovals()` during the following stages of the main game loop: 1. `recvMessage()` - there are a few functions which distribute resources of the defeated players among others and may trigger script events. 2. `updateScripts()` - processes queued timer functions. 3. `droidUpdate()`, `missionDroidUpdate()`, `structureUpdate()` - main routines for updating the state of in-game objects. These would potentially trigger the majority of script events. Each pair represents `` tuple. The sole reason for this breaking API change is to guard against cases like this one: 1. `gameStateUpdate()` processes droid state updates inside a `mutating_list_iterate`. 2. A droid update triggers some user script, which calls `wzapi::removeObject` several times in a row. 3. This may potentially destroy `itNext` iterator saved inside the `mutating_list_iterate` from of the upper stack frames, which would lead to invalid memory access. So, instead of removing multiple objects inside of `mutating_list_iterate`, which is unsafe, we just queue removal requests and execute actual destruction routines once we are out of the `mutating_list_iterate` function. Signed-off-by: Pavel Solodovnikov --- src/loop.cpp | 54 +++++++++++++++++++++++------------- src/wzapi.cpp | 76 +++++++++++++++++++++++++++++++++++---------------- src/wzapi.h | 30 +++++++++++++++++++- 3 files changed, 116 insertions(+), 44 deletions(-) diff --git a/src/loop.cpp b/src/loop.cpp index da0ac6e0de9..d618c2e432b 100644 --- a/src/loop.cpp +++ b/src/loop.cpp @@ -87,6 +87,7 @@ #include "clparse.h" #include "gamehistorylogger.h" #include "profiling.h" +#include "wzapi.h" #include "warzoneconfig.h" @@ -490,6 +491,14 @@ void countUpdate(bool synch) } } +template +static void executeFnAndProcessScriptQueuedRemovals(Fn fn) +{ + ASSERT(wzapi::scriptQueuedObjectRemovals().empty(), "Leftover script-queued object removals detected!"); + fn(); + wzapi::processScriptQueuedObjectRemovals(); +} + static void gameStateUpdate() { WZ_PROFILE_SCOPE(gameStateUpdate); @@ -515,7 +524,7 @@ static void gameStateUpdate() if (!paused && !scriptPaused()) { - updateScripts(); + executeFnAndProcessScriptQueuedRemovals([]() { updateScripts(); }); } // Update abandoned structures @@ -544,27 +553,34 @@ static void gameStateUpdate() //update the current power available for a player updatePlayerPower(i); - mutating_list_iterate(apsDroidLists[i], [](DROID* d) - { - droidUpdate(d); - return IterationResult::CONTINUE_ITERATION; + executeFnAndProcessScriptQueuedRemovals([i]() { + mutating_list_iterate(apsDroidLists[i], [](DROID* d) + { + droidUpdate(d); + return IterationResult::CONTINUE_ITERATION; + }); }); - mutating_list_iterate(mission.apsDroidLists[i], [](DROID* d) - { - missionDroidUpdate(d); - return IterationResult::CONTINUE_ITERATION; + executeFnAndProcessScriptQueuedRemovals([i]() { + mutating_list_iterate(mission.apsDroidLists[i], [](DROID* d) + { + missionDroidUpdate(d); + return IterationResult::CONTINUE_ITERATION; + }); }); - // FIXME: These for-loops are code duplication - mutating_list_iterate(apsStructLists[i], [](STRUCTURE* s) - { - structureUpdate(s, false); - return IterationResult::CONTINUE_ITERATION; + executeFnAndProcessScriptQueuedRemovals([i]() { + mutating_list_iterate(apsStructLists[i], [](STRUCTURE* s) + { + structureUpdate(s, false); + return IterationResult::CONTINUE_ITERATION; + }); }); - mutating_list_iterate(mission.apsStructLists[i], [](STRUCTURE* s) - { - structureUpdate(s, true); // update for mission - return IterationResult::CONTINUE_ITERATION; + executeFnAndProcessScriptQueuedRemovals([i]() { + mutating_list_iterate(mission.apsStructLists[i], [](STRUCTURE* s) + { + structureUpdate(s, true); // update for mission + return IterationResult::CONTINUE_ITERATION; + }); }); } @@ -641,7 +657,7 @@ GAMECODE gameLoop() { // Receive NET_BLAH messages. // Receive GAME_BLAH messages, and if it's time, process exactly as many GAME_BLAH messages as required to be able to tick the gameTime. - recvMessage(); + executeFnAndProcessScriptQueuedRemovals([]() { recvMessage(); }); bool selectedPlayerIsSpectator = bMultiPlayer && NetPlay.players[selectedPlayer].isSpectator; bool multiplayerHostDisconnected = bMultiPlayer && !NetPlay.isHostAlive && NetPlay.bComms && !NetPlay.isHost; // do not fast-forward after the host has disconnected diff --git a/src/wzapi.cpp b/src/wzapi.cpp index 556a2534ff7..f8065821b04 100644 --- a/src/wzapi.cpp +++ b/src/wzapi.cpp @@ -2930,36 +2930,22 @@ bool wzapi::removeStruct(WZAPI_PARAMS(STRUCTURE *psStruct)) WZAPI_DEPRECATED //-- ## removeObject(gameObject[, sfx]) //-- -//-- Remove the given game object with special effects. Returns a boolean that is true on success. +//-- Queue the given game object for removal with or without special effects. Returns a boolean that is true on success. //-- A second, optional boolean parameter specifies whether special effects are to be applied. (3.2+ only) //-- +//-- BREAKING CHANGE (4.5+): the effect of this function is not immediate anymore, the object will be +//-- queued for later removal instead of destroying it right away. +//-- User scripts should not rely on this function having immediate side-effects. +//-- bool wzapi::removeObject(WZAPI_PARAMS(BASE_OBJECT *psObj, optional _sfx)) { SCRIPT_ASSERT(false, context, psObj, "No valid object provided"); - bool sfx = _sfx.value_or(false); + SCRIPT_ASSERT(false, context, + psObj->type == OBJ_STRUCTURE || psObj->type == OBJ_DROID || psObj->type == OBJ_FEATURE, + "Wrong game object type"); - bool retval = false; - if (sfx) - { - switch (psObj->type) - { - case OBJ_STRUCTURE: destroyStruct((STRUCTURE *)psObj, gameTime); break; - case OBJ_DROID: retval = destroyDroid((DROID *)psObj, gameTime); break; - case OBJ_FEATURE: retval = destroyFeature((FEATURE *)psObj, gameTime); break; - default: SCRIPT_ASSERT(false, context, false, "Wrong game object type"); break; - } - } - else - { - switch (psObj->type) - { - case OBJ_STRUCTURE: retval = removeStruct((STRUCTURE *)psObj, true); break; - case OBJ_DROID: retval = removeDroidBase((DROID *)psObj); break; - case OBJ_FEATURE: retval = removeFeature((FEATURE *)psObj); break; - default: SCRIPT_ASSERT(false, context, false, "Wrong game object type"); break; - } - } - return retval; + scriptQueuedObjectRemovals().emplace_back(psObj, _sfx.value_or(false)); + return true; } //-- ## setScrollLimits(x1, y1, x2, y2) @@ -4600,3 +4586,45 @@ nlohmann::json wzapi::constructMapTilesArray() } return mapTileArray; } + +wzapi::QueuedObjectRemovalsVector& wzapi::scriptQueuedObjectRemovals() +{ + static QueuedObjectRemovalsVector instance = []() + { + static constexpr size_t initialCapacity = 16; + QueuedObjectRemovalsVector ret; + ret.reserve(initialCapacity); + return ret; + }(); + return instance; +} + +void wzapi::processScriptQueuedObjectRemovals() +{ + auto& queuedObjRemovals = scriptQueuedObjectRemovals(); + for (auto& objWithSfxFlag : queuedObjRemovals) + { + BASE_OBJECT* psObj = objWithSfxFlag.first; + if (objWithSfxFlag.second) + { + switch (psObj->type) + { + case OBJ_STRUCTURE: destroyStruct((STRUCTURE*)psObj, gameTime); break; + case OBJ_DROID: destroyDroid((DROID*)psObj, gameTime); break; + case OBJ_FEATURE: destroyFeature((FEATURE*)psObj, gameTime); break; + default: ASSERT(false, "Wrong game object type"); break; + } + } + else + { + switch (psObj->type) + { + case OBJ_STRUCTURE: removeStruct((STRUCTURE*)psObj, true); break; + case OBJ_DROID: removeDroidBase((DROID*)psObj); break; + case OBJ_FEATURE: removeFeature((FEATURE*)psObj); break; + default: ASSERT(false, "Wrong game object type"); break; + } + } + } + queuedObjRemovals.clear(); +} diff --git a/src/wzapi.h b/src/wzapi.h index 21f958efd79..d5216ed347b 100644 --- a/src/wzapi.h +++ b/src/wzapi.h @@ -1098,6 +1098,14 @@ namespace wzapi no_return_value makeComponentAvailable(WZAPI_PARAMS(std::string componentName, int player)); bool allianceExistsBetween(WZAPI_PARAMS(int player1, int player2)); bool removeStruct(WZAPI_PARAMS(STRUCTURE *psStruct)) WZAPI_DEPRECATED; + /// + /// Queues the given game object for removal with or without special effects. + /// + /// The current behavior is in effect since version 4.5+. + /// + /// Game object to be queued for removal. + /// Optional boolean parameter that specifies whether special effects are to be applied. + /// `true` if `psObj` has been successfully queued for destruction. bool removeObject(WZAPI_PARAMS(BASE_OBJECT *psObj, optional _sfx)); no_return_value setScrollLimits(WZAPI_PARAMS(int x1, int y1, int x2, int y2)); scr_area getScrollLimits(WZAPI_NO_PARAMS); @@ -1126,6 +1134,26 @@ namespace wzapi nlohmann::json constructStaticPlayerData(); std::vector getUpgradesObject(); nlohmann::json constructMapTilesArray(); -} + + // `wzapi::removeObject` API call adds the game objects to this list. + // They will eventually be released during calls to `processScriptQueuedObjectRemovals()` + // during the following stages of the main game loop: + // 1. `recvMessage()` - there are a few functions which distribute + // resources of the defeated players among others and may trigger script events. + // 2. `updateScripts()` - processes queued timer functions. + // 3. `droidUpdate()`, `missionDroidUpdate()`, `structureUpdate()` - main + // routines for updating the state of in-game objects. These would potentially + // trigger the majority of script events. + // + // Each pair represents tuple. + using QueuedObjectRemovalsVector = std::vector>; + + QueuedObjectRemovalsVector& scriptQueuedObjectRemovals(); + /// + /// Walks `scriptQueuedObjectRemovals()` list, destroys every object in that list + /// and clears the container. + /// + void processScriptQueuedObjectRemovals(); +} // namespace wzapi #endif