From a14c2f054ca8e3b3a03f224f0695cdab4c5fc948 Mon Sep 17 00:00:00 2001 From: crabman Date: Tue, 27 Feb 2024 08:37:40 +0000 Subject: [PATCH] addressed review comments --- assets/voxygen/i18n/en/command.ftl | 1 + common/src/states/transform.rs | 2 + server/src/cmd.rs | 7 +-- server/src/events/entity_manipulation.rs | 56 +++++++++++++++++++----- server/src/events/mod.rs | 1 + server/src/events/player.rs | 13 +++--- 6 files changed, 60 insertions(+), 20 deletions(-) diff --git a/assets/voxygen/i18n/en/command.ftl b/assets/voxygen/i18n/en/command.ftl index 4b76ba0b6a..824382c4f7 100644 --- a/assets/voxygen/i18n/en/command.ftl +++ b/assets/voxygen/i18n/en/command.ftl @@ -84,6 +84,7 @@ command-repaired-items = Repaired all equipped items command-message-group-missing = You are using group chat but do not belong to a group. Use /world or /region to change chat. command-tell-request = { $sender } wants to talk to you. +command-transform-invalid-presence = Cannot transform in the current presence # Unreachable/untestable but added for consistency diff --git a/common/src/states/transform.rs b/common/src/states/transform.rs index b8a98cbfc3..cebcce5ae2 100644 --- a/common/src/states/transform.rs +++ b/common/src/states/transform.rs @@ -3,6 +3,7 @@ use std::time::Duration; use common_assets::AssetExt; use rand::thread_rng; use serde::{Deserialize, Serialize}; +use tracing::error; use vek::Vec3; use crate::{ @@ -65,6 +66,7 @@ impl CharacterBehavior for Data { // Buildup finished, start transformation } else { let Ok(entity_config) = EntityConfig::load(&self.static_data.target) else { + error!(?self.static_data.target, "Failed to load entity configuration"); end_ability(data, &mut update); return update; }; diff --git a/server/src/cmd.rs b/server/src/cmd.rs index 55eebf444f..02a10350b4 100644 --- a/server/src/cmd.rs +++ b/server/src/cmd.rs @@ -672,9 +672,10 @@ fn handle_into_npc( TransformEntityError::UnexpectedNpcTeleporter => { Content::localized("command-unimplemented-teleporter-spawn") }, - })?; - - Ok(()) + TransformEntityError::LoadingCharacter => { + Content::localized("command-transform-invalid-presence") + }, + }) } fn handle_make_npc( diff --git a/server/src/events/entity_manipulation.rs b/server/src/events/entity_manipulation.rs index ec753a66a3..d7827ad60e 100644 --- a/server/src/events/entity_manipulation.rs +++ b/server/src/events/entity_manipulation.rs @@ -22,7 +22,7 @@ use common::{ item::flatten_counted_items, loot_owner::LootOwnerKind, Alignment, Auras, Body, CharacterState, Energy, Group, Health, Inventory, Object, Player, - Poise, Pos, Presence, PresenceKind, SkillSet, Stats, + Poise, Pos, Presence, PresenceKind, SkillSet, Stats, BASE_ABILITY_LIMIT, }, consts::TELEPORTER_RADIUS, event::{ @@ -2106,13 +2106,17 @@ pub fn handle_transform(server: &mut Server, TransformEvent(uid, info): Transfor return; }; - let _ = transform_entity(server, entity, info); + if let Err(error) = transform_entity(server, entity, info) { + error!(?error, ?uid, "Failed transform entity"); + } } +#[derive(Debug)] pub enum TransformEntityError { EntityDead, UnexpectedNpcWaypoint, UnexpectedNpcTeleporter, + LoadingCharacter, } pub fn transform_entity( @@ -2158,20 +2162,38 @@ pub fn transform_entity( } // Disable persistence - { + 'persist: { + match server + .state + .ecs() + .read_storage::() + .get(entity) + .map(|presence| presence.kind) + { + // Transforming while the character is being loaded or is spectating is invalid! + Some(PresenceKind::Spectator | PresenceKind::LoadingCharacter(_)) => { + return Err(TransformEntityError::LoadingCharacter); + }, + Some(PresenceKind::Possessor | PresenceKind::Character(_)) => {}, + None => break 'persist, + } + // Run persistence once before disabling it + // + // We must NOT early return between persist_entity() being called and + // persistence being set to Possessor super::player::persist_entity(server.state_mut(), entity); - // TODO: let Imbris work out some edge-cases: - // - error on PresenseKind::LoadingCharacter - // - handle active inventory actions - let ecs = server.state.ecs(); - let mut presences = ecs.write_storage::(); - let presence = presences.get_mut(entity); + // We re-fetch presence here as mutable, because checking for a valid + // [`PresenceKind`] must be done BEFORE persist_entity but persist_entity needs + // exclusive mutable access to the server's state + let mut presences = server.state.ecs().write_storage::(); + let Some(presence) = presences.get_mut(entity) else { + // Checked above + unreachable!("We already know this entity has a Presence"); + }; - if let Some(presence) = presence - && let PresenceKind::Character(id) = presence.kind - { + if let PresenceKind::Character(id) = presence.kind { server.state.ecs().write_resource::().remove_entity( Some(entity), None, @@ -2194,6 +2216,16 @@ pub fn transform_entity( set_or_remove_component(server, entity, Some(body.density()))?; set_or_remove_component(server, entity, Some(body.collider()))?; set_or_remove_component(server, entity, Some(scale))?; + // Reset active abilities + set_or_remove_component( + server, + entity, + Some(if body.is_humanoid() { + comp::ActiveAbilities::default_limited(BASE_ABILITY_LIMIT) + } else { + comp::ActiveAbilities::default() + }), + )?; // Don't add Agent or ItemDrops to players if !is_player { diff --git a/server/src/events/mod.rs b/server/src/events/mod.rs index 9967ab680f..6bcb708f68 100644 --- a/server/src/events/mod.rs +++ b/server/src/events/mod.rs @@ -37,6 +37,7 @@ mod mounting; mod player; mod trade; +/// Shared utilities used by other code **in this crate** pub(crate) mod shared { pub(crate) use super::{ entity_manipulation::{transform_entity, TransformEntityError}, diff --git a/server/src/events/player.rs b/server/src/events/player.rs index ab210bdef0..db8fcd1b9b 100644 --- a/server/src/events/player.rs +++ b/server/src/events/player.rs @@ -243,11 +243,14 @@ pub fn handle_client_disconnect( Event::ClientDisconnected { entity } } -// When a player logs out, their data is queued for persistence in the next tick -// of the persistence batch update. The player will be -// temporarily unable to log in during this period to avoid -// the race condition of their login fetching their old data -// and overwriting the data saved here. +/// When a player logs out, their data is queued for persistence in the next +/// tick of the persistence batch update. The player will be +/// temporarily unable to log in during this period to avoid +/// the race condition of their login fetching their old data +/// and overwriting the data saved here. +/// +/// This function is also used by the Transform event and MUST NOT assume that +/// the persisting entity is deleted afterwards. pub(super) fn persist_entity(state: &mut State, entity: EcsEntity) -> EcsEntity { if let ( Some(presence),