From 75539b4f897934e9c93238dc4d0130f4709b1627 Mon Sep 17 00:00:00 2001 From: hg <4683435+hgtw@users.noreply.github.com> Date: Sat, 10 Feb 2024 04:25:03 -0500 Subject: [PATCH] [Tasks] Avoid removing client tasks from memory on load (#4052) If a task was deleted or had new elements added to it without updating character states to match changes, client state for the task was not loaded into memory and a "contact a GM" warning was sent to the client. This caused an issue if a client later accepted a new task because it could be placed in the same client state slot as a non-loaded task. The table does not have constraints so this was also inserted into the db. The next time client task state was reloaded after zoning, the new task would not be loaded since it occupied a used slot, even though that used slot was also not being loaded. The client could not remove or reacquire the original task without GM intervention and an entry was left in the db for the new task. This makes the following changes to client task state loading: - If a task id no longer exists it is deleted from client state tables instead of only being removed from memory. - If a task occupies the same client state slot as another task, it is deleted from client state tables instead of being ignored. - If new elements have been added to a task, client state will keep the task in memory. The new activity states will be inserted into the db when necessary for updates (may not be immediate). These changes also fix two smaller bugs as a consequence: - If a character was at the 20 quest limit the last quest wasn't being processed for activity count changes. The task would continue to show to clients but any added new elements couldn't be completed. - Deleted tasks that occupied slot 0 in client state would fallback to loading it as a solo task of type 0. This prevented a client's real solo task from being loaded if the deleted task was processed first. Note clients may receive or lose credit for completed elements if new ones are added in the middle of tasks. Server ops will still need to update character state tables manually on task changes to prevent this. --- zone/task_manager.cpp | 69 +++++++++++++++---------------------------- 1 file changed, 23 insertions(+), 46 deletions(-) diff --git a/zone/task_manager.cpp b/zone/task_manager.cpp index 059dd390d..169e06fd9 100644 --- a/zone/task_manager.cpp +++ b/zone/task_manager.cpp @@ -1309,13 +1309,23 @@ bool TaskManager::LoadClientState(Client *client, ClientTaskState *cts) fmt::format("charid = {} ORDER BY acceptedtime", character_id) ); + std::vector remove; + for (auto &character_task: character_tasks) { int task_id = character_task.taskid; int slot = character_task.slot; + const TaskInformation* task = GetTaskData(task_id); + if (!task) + { + LogError("Character [{}] has task [{}] which does not exist", character_id, task_id); + remove.push_back(task_id); + continue; + } + // this used to be loaded from character_tasks // this should just load from the tasks table - auto type = task_manager->GetTaskType(character_task.taskid); + auto type = task->type; if (task_id < 0) { LogTasks( @@ -1339,6 +1349,7 @@ bool TaskManager::LoadClientState(Client *client, ClientTaskState *cts) if (task_info->task_id != TASKSLOTEMPTY) { LogTasks("Error: slot [{}] for task [{}] is already occupied", slot, task_id); + remove.push_back(task_id); continue; } @@ -1347,8 +1358,10 @@ bool TaskManager::LoadClientState(Client *client, ClientTaskState *cts) task_info->updated = false; task_info->was_rewarded = character_task.was_rewarded; - for (auto &i : task_info->activity) { - i.activity_id = -1; + // enable all states in memory, any missing from db get inserted if updated + for (int i = 0; i < MAXACTIVITIESPERTASK; ++i) + { + task_info->activity[i].activity_id = i < task->activity_count ? i : -1; } // this check keeps a lot of core task updating code from working properly (shared or otherwise) @@ -1366,6 +1379,13 @@ bool TaskManager::LoadClientState(Client *client, ClientTaskState *cts) ); } + if (!remove.empty()) + { + std::string filter = fmt::format("charid = {} AND taskid IN ({})", character_id, fmt::join(remove, ",")); + CharacterTasksRepository::DeleteWhere(database, filter); + CharacterActivitiesRepository::DeleteWhere(database, filter); + } + // Load Activities LogTasks("Loading activities for character_id [{}]", character_id); @@ -1536,49 +1556,6 @@ bool TaskManager::LoadClientState(Client *client, ClientTaskState *cts) } } - // Check that there is an entry in the client task state for every activity_information in each task - // This should only break if a ServerOP adds or deletes activites for a task that players already - // have active, or due to a bug. - for (int task_index = 0; task_index < MAXACTIVEQUESTS + 1; task_index++) { - int task_id = cts->m_active_tasks[task_index].task_id; - if (task_id == TASKSLOTEMPTY) { - continue; - } - const auto task_data = GetTaskData(task_id); - if (!task_data) { - client->Message( - Chat::Red, - "Active Task Slot %i, references a task (%i), that does not exist. " - "Removing from memory. Contact a GM to resolve this.", - task_index, - task_id - ); - - LogError("Character [{}] has task [{}] which does not exist", character_id, task_id); - cts->m_active_tasks[task_index].task_id = TASKSLOTEMPTY; - continue; - } - for (int activity_index = 0; activity_index < task_data->activity_count; activity_index++) { - if (cts->m_active_tasks[task_index].activity[activity_index].activity_id != activity_index) { - client->Message( - Chat::Red, - "Active Task %i, %s. activity_information count does not match expected value." - "Removing from memory. Contact a GM to resolve this.", - task_id, task_data->title.c_str() - ); - - LogTasks( - "Fatal error in character [{}] task state. activity_information [{}] for Task [{}] either missing from client state or from task", - character_id, - activity_index, - task_id - ); - cts->m_active_tasks[task_index].task_id = TASKSLOTEMPTY; - break; - } - } - } - LogTasksDetail( "m_active_task task_id is [{}] slot [{}]", cts->m_active_task.task_id,