Merge branch 'imbris/fix-possesion-comp-sync' into 'master'

Improve possession edge cases

See merge request veloren/veloren!3233
This commit is contained in:
Imbris 2022-02-27 20:10:16 +00:00
commit 7be5b4c051
13 changed files with 335 additions and 195 deletions

View File

@ -1828,9 +1828,31 @@ impl Client {
},
ServerGeneral::SetPlayerEntity(uid) => {
if let Some(entity) = self.state.ecs().entity_from_uid(uid.0) {
*self.state.ecs_mut().write_resource() = PlayerEntity(Some(entity));
let old_player_entity = core::mem::replace(
&mut *self.state.ecs_mut().write_resource(),
PlayerEntity(Some(entity)),
);
if let Some(old_entity) = old_player_entity.0 {
// Transfer controller to the new entity.
let mut controllers = self.state.ecs().write_storage::<Controller>();
if let Some(controller) = controllers.remove(old_entity) {
if let Err(e) = controllers.insert(entity, controller) {
error!(
?e,
"Failed to insert controller when setting new player entity!"
);
}
}
}
if let Some(presence) = self.presence {
self.presence = Some(match presence {
PresenceKind::Spectator => PresenceKind::Spectator,
PresenceKind::Character(_) => PresenceKind::Possessor,
PresenceKind::Possessor => PresenceKind::Possessor,
});
}
} else {
return Err(Error::Other("Failed to find entity from uid.".to_owned()));
return Err(Error::Other("Failed to find entity from uid.".into()));
}
},
ServerGeneral::TimeOfDay(time_of_day, calendar) => {

View File

@ -26,6 +26,14 @@ use serde::{Deserialize, Serialize};
pub enum PresenceKind {
Spectator,
Character(CharacterId),
Possessor,
}
impl PresenceKind {
/// Check if the presence represents a control of a character, and thus
/// certain in-game messages from the client such as control inputs
/// should be handled.
pub fn controlling_char(&self) -> bool { matches!(self, Self::Character(_) | Self::Possessor) }
}
#[derive(Debug, Clone, Copy, PartialEq, Serialize, Deserialize)]

View File

@ -223,11 +223,6 @@ impl NetSync for Combo {
}
impl NetSync for ActiveAbilities {
// When in debug, sync from all entities. This allows animation work to be done
// when possessing entities
#[cfg(debug_assertions)]
const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity;
#[cfg(not(debug_assertions))]
const SYNC_FROM: SyncFrom = SyncFrom::ClientEntity;
}

View File

@ -1,5 +1,4 @@
use specs::{world::WorldExt, Builder, Entity as EcsEntity, Join};
use tracing::error;
use vek::*;
use common::{
@ -9,8 +8,6 @@ use common::{
agent::{AgentEvent, Sound, SoundKind},
dialogue::Subject,
inventory::slot::EquipSlot,
item,
slot::Slot,
tool::ToolKind,
Inventory, Pos, SkillGroupKind,
},
@ -22,14 +19,9 @@ use common::{
uid::Uid,
vol::ReadVol,
};
use common_net::{msg::ServerGeneral, sync::WorldSyncExt};
use common_net::sync::WorldSyncExt;
use crate::{
client::Client,
presence::{Presence, RegionSubscription},
state_ext::StateExt,
Server,
};
use crate::{state_ext::StateExt, Server};
use crate::pet::tame_pet;
use hashbrown::{HashMap, HashSet};
@ -143,144 +135,6 @@ pub fn handle_unmount(server: &mut Server, rider: EcsEntity) {
state.ecs().write_storage::<Is<Rider>>().remove(rider);
}
/// FIXME: This code is dangerous and needs to be refactored. We can't just
/// comment it out, but it needs to be fixed for a variety of reasons. Get rid
/// of this ASAP!
pub fn handle_possess(server: &mut Server, possessor_uid: Uid, possesse_uid: Uid) {
let ecs = server.state.ecs();
if let (Some(possessor), Some(possesse)) = (
ecs.entity_from_uid(possessor_uid.into()),
ecs.entity_from_uid(possesse_uid.into()),
) {
// Check that entities still exist
if !possessor.gen().is_alive()
|| !ecs.is_alive(possessor)
|| !possesse.gen().is_alive()
|| !ecs.is_alive(possesse)
{
error!(
"Error possessing! either the possessor entity or possesse entity no longer exists"
);
return;
}
if ecs.read_storage::<Client>().get(possesse).is_some() {
error!("can't possess other players");
return;
}
match (|| -> Option<Result<(), specs::error::Error>> {
let mut clients = ecs.write_storage::<Client>();
let c = clients.remove(possessor)?;
clients.insert(possesse, c).ok()?;
let playerlist_messages = if let Some(client) = clients.get(possesse) {
client.send_fallible(ServerGeneral::SetPlayerEntity(possesse_uid));
// If a player is posessing non player, add possesse to playerlist as player and
// remove old player
if let Some(possessor_player) = ecs.read_storage::<comp::Player>().get(possessor) {
let admins = ecs.read_storage::<comp::Admin>();
let entity_possession_msg = ServerGeneral::PlayerListUpdate(
common_net::msg::server::PlayerListUpdate::Add(
possesse_uid,
common_net::msg::server::PlayerInfo {
player_alias: possessor_player.alias.clone(),
is_online: true,
is_moderator: admins.get(possessor).is_some(),
character: ecs.read_storage::<comp::Stats>().get(possesse).map(
|s| common_net::msg::CharacterInfo {
name: s.name.clone(),
},
),
},
),
);
let remove_old_player_msg = ServerGeneral::PlayerListUpdate(
common_net::msg::server::PlayerListUpdate::Remove(possessor_uid),
);
// Send msg to new possesse client now because it is not yet considered a player
// and will be missed by notify_players
client.send_fallible(entity_possession_msg.clone());
client.send_fallible(remove_old_player_msg.clone());
Some((remove_old_player_msg, entity_possession_msg))
} else {
None
}
} else {
None
};
drop(clients);
if let Some((remove_player, possess_entity)) = playerlist_messages {
server.state().notify_players(possess_entity);
server.state().notify_players(remove_player);
}
//optional entities
let mut players = ecs.write_storage::<comp::Player>();
let mut presence = ecs.write_storage::<Presence>();
let mut subscriptions = ecs.write_storage::<RegionSubscription>();
let mut admins = ecs.write_storage::<comp::Admin>();
let mut waypoints = ecs.write_storage::<comp::Waypoint>();
players
.remove(possessor)
.map(|p| players.insert(possesse, p).ok()?);
presence
.remove(possessor)
.map(|p| presence.insert(possesse, p).ok()?);
subscriptions
.remove(possessor)
.map(|s| subscriptions.insert(possesse, s).ok()?);
admins
.remove(possessor)
.map(|a| admins.insert(possesse, a).ok()?);
waypoints
.remove(possessor)
.map(|w| waypoints.insert(possesse, w).ok()?);
Some(Ok(()))
})() {
Some(Ok(())) => (),
Some(Err(e)) => {
error!(?e, ?possesse, "Error inserting component during possession");
return;
},
None => {
error!(?possessor, "Error removing component during possession");
return;
},
}
// Put possess item into loadout
let mut inventories = ecs.write_storage::<Inventory>();
let mut inventory = inventories
.entry(possesse)
.expect("Nobody has &mut World, so there's no way to delete an entity.")
.or_insert(Inventory::new_empty());
let debug_item = comp::Item::new_from_asset_expect("common.items.debug.admin_stick");
if let item::ItemKind::Tool(_) = debug_item.kind() {
assert!(
inventory
.swap(
Slot::Equip(EquipSlot::ActiveMainhand),
Slot::Equip(EquipSlot::InactiveMainhand),
)
.first()
.is_none(),
"Swapping active and inactive mainhands never results in leftover items",
);
inventory.replace_loadout_item(EquipSlot::ActiveMainhand, Some(debug_item));
}
// Remove will of the entity
ecs.write_storage::<comp::Agent>().remove(possesse);
// Reset controller of former shell
ecs.write_storage::<comp::Controller>()
.get_mut(possessor)
.map(|c| c.reset());
}
}
fn within_mounting_range(player_position: Option<&Pos>, mount_position: Option<&Pos>) -> bool {
match (player_position, mount_position) {
(Some(ppos), Some(ipos)) => ppos.0.distance_squared(ipos.0) < MAX_MOUNT_RANGE.powi(2),

View File

@ -18,11 +18,11 @@ use group_manip::handle_group;
use information::handle_site_info;
use interaction::{
handle_create_sprite, handle_lantern, handle_mine_block, handle_mount, handle_npc_interaction,
handle_possess, handle_sound, handle_unmount,
handle_sound, handle_unmount,
};
use inventory_manip::handle_inventory;
use invite::{handle_invite, handle_invite_response};
use player::{handle_client_disconnect, handle_exit_ingame};
use player::{handle_client_disconnect, handle_exit_ingame, handle_possess};
use specs::{Builder, Entity as EcsEntity, WorldExt};
use trade::{cancel_trade_for, handle_process_trade_action};

View File

@ -271,8 +271,235 @@ fn persist_entity(state: &mut State, entity: EcsEntity) -> EcsEntity {
);
},
PresenceKind::Spectator => { /* Do nothing, spectators do not need persisting */ },
PresenceKind::Possessor => { /* Do nothing, possessor's are not persisted */ },
};
}
entity
}
/// FIXME: This code is dangerous and needs to be refactored. We can't just
/// comment it out, but it needs to be fixed for a variety of reasons. Get rid
/// of this ASAP!
pub fn handle_possess(server: &mut Server, possessor_uid: Uid, possessee_uid: Uid) {
use crate::presence::RegionSubscription;
use common::{
comp::{inventory::slot::EquipSlot, item, slot::Slot, Inventory},
region::RegionMap,
};
use common_net::sync::WorldSyncExt;
let state = server.state_mut();
let mut delete_entity = None;
if let (Some(possessor), Some(possessee)) = (
state.ecs().entity_from_uid(possessor_uid.into()),
state.ecs().entity_from_uid(possessee_uid.into()),
) {
// In this section we check various invariants and can return early if any of
// them are not met.
{
let ecs = state.ecs();
// Check that entities still exist
if !possessor.gen().is_alive()
|| !ecs.is_alive(possessor)
|| !possessee.gen().is_alive()
|| !ecs.is_alive(possessee)
{
error!(
"Error possessing! either the possessor entity or possessee entity no longer \
exists"
);
return;
}
let clients = ecs.read_storage::<Client>();
let players = ecs.read_storage::<comp::Player>();
if clients.contains(possessee) || players.contains(possessee) {
error!("Can't possess other players!");
return;
}
if !clients.contains(possessor) {
error!("Error posessing, no `Client` component on the possessor!");
return;
}
// Limit possessible entities to those in the client's subscribed regions (so
// that the entity already exists on the client, this reduces the
// amount of syncing edge cases to consider).
let subscriptions = ecs.read_storage::<RegionSubscription>();
let region_map = ecs.read_resource::<RegionMap>();
let possessee_in_subscribed_region = subscriptions
.get(possessor)
.iter()
.flat_map(|s| s.regions.iter())
.filter_map(|key| region_map.get(*key))
.any(|region| region.entities().contains(possessee.id()));
if !possessee_in_subscribed_region {
return;
}
// No early returns allowed after this.
}
// Sync the player's character data to the database. This must be done before
// moving any components from the entity.
//
// NOTE: Below we delete old entity (if PresenceKind::Character) as if logging
// out. This is to prevent any potential for item duplication (although
// it would only be possible if the player could repossess their entity,
// hand off some items, and then crash the server in a particular time
// window, and only admins should have access to the item with this ability
// in the first place (though that isn't foolproof)). We could potentially fix
// this but it would require some tweaks to the CharacterUpdater code
// (to be able to deque the pending persistence request issued here if
// repossesing the original character), and it seems prudent to be more
// conservative with making changes there to support this feature.
let possessor = persist_entity(state, possessor);
let ecs = state.ecs();
let mut clients = ecs.write_storage::<Client>();
// Transfer client component. Note: we require this component for possession.
let client = clients
.remove(possessor)
.expect("Checked client component was present above!");
client.send_fallible(ServerGeneral::SetPlayerEntity(possessee_uid));
// Note: we check that the `possessor` and `possessee` entities exist above, so
// this should never panic.
clients
.insert(possessee, client)
.expect("Checked entity was alive!");
// Other components to transfer if they exist.
fn transfer_component<C: specs::Component>(
storage: &mut specs::WriteStorage<'_, C>,
possessor: EcsEntity,
possessee: EcsEntity,
transform: impl FnOnce(C) -> C,
) {
if let Some(c) = storage.remove(possessor) {
// Note: we check that the `possessor` and `possessee` entities exist above, so
// this should never panic.
storage
.insert(possessee, transform(c))
.expect("Checked entity was alive!");
}
}
let mut players = ecs.write_storage::<comp::Player>();
let mut presence = ecs.write_storage::<Presence>();
let mut subscriptions = ecs.write_storage::<RegionSubscription>();
let mut admins = ecs.write_storage::<comp::Admin>();
let mut waypoints = ecs.write_storage::<comp::Waypoint>();
transfer_component(&mut players, possessor, possessee, |x| x);
transfer_component(&mut presence, possessor, possessee, |mut presence| {
presence.kind = match presence.kind {
PresenceKind::Spectator => PresenceKind::Spectator,
// This prevents persistence from overwriting original character info with stuff
// from the new character.
PresenceKind::Character(_) => {
delete_entity = Some(possessor);
PresenceKind::Possessor
},
PresenceKind::Possessor => PresenceKind::Possessor,
};
presence
});
transfer_component(&mut subscriptions, possessor, possessee, |x| x);
transfer_component(&mut admins, possessor, possessee, |x| x);
transfer_component(&mut waypoints, possessor, possessee, |x| x);
// If a player is posessing, add possessee to playerlist as player and remove
// old player.
// Fetches from possessee entity here since we have transferred over the
// `Player` component.
if let Some(player) = players.get(possessee) {
use common_net::msg;
let add_player_msg = ServerGeneral::PlayerListUpdate(
msg::server::PlayerListUpdate::Add(possessee_uid, msg::server::PlayerInfo {
player_alias: player.alias.clone(),
is_online: true,
is_moderator: admins.contains(possessee),
character: ecs.read_storage::<comp::Stats>().get(possessee).map(|s| {
msg::CharacterInfo {
name: s.name.clone(),
}
}),
}),
);
let remove_player_msg = ServerGeneral::PlayerListUpdate(
msg::server::PlayerListUpdate::Remove(possessor_uid),
);
drop((clients, players)); // need to drop so we can use `notify_players` below
state.notify_players(remove_player_msg);
state.notify_players(add_player_msg);
}
// Put possess item into loadout
let mut inventories = ecs.write_storage::<Inventory>();
let mut inventory = inventories
.entry(possessee)
.expect("Nobody has &mut World, so there's no way to delete an entity.")
.or_insert(Inventory::new_empty());
let debug_item = comp::Item::new_from_asset_expect("common.items.debug.admin_stick");
if let item::ItemKind::Tool(_) = debug_item.kind() {
let leftover_items = inventory.swap(
Slot::Equip(EquipSlot::ActiveMainhand),
Slot::Equip(EquipSlot::InactiveMainhand),
);
assert!(
leftover_items.is_empty(),
"Swapping active and inactive mainhands never results in leftover items"
);
inventory.replace_loadout_item(EquipSlot::ActiveMainhand, Some(debug_item));
}
drop(inventories);
// Remove will of the entity
ecs.write_storage::<comp::Agent>().remove(possessee);
// Reset controller of former shell
if let Some(c) = ecs.write_storage::<comp::Controller>().get_mut(possessor) {
*c = Default::default();
}
// Send client new `SyncFrom::ClientEntity` components and tell it to
// deletes these on the old entity.
let clients = ecs.read_storage::<Client>();
let client = clients
.get(possessee)
.expect("We insert this component above and have exclusive access to the world.");
use crate::sys::sentinel::TrackedStorages;
use specs::SystemData;
let tracked_storages = TrackedStorages::fetch(ecs);
let comp_sync_package = tracked_storages.create_sync_from_client_entity_switch(
possessor_uid,
possessee_uid,
possessee,
);
if !comp_sync_package.is_empty() {
client.send_fallible(ServerGeneral::CompSync(comp_sync_package));
}
}
// Outside block above to prevent borrow conflicts (i.e. convenient to let
// everything drop at the end of the block rather than doing it manually for
// this). See note on `persist_entity` call above for why we do this.
if let Some(entity) = delete_entity {
// Delete old entity
if let Err(e) = state.delete_entity_recorded(entity) {
error!(
?e,
?entity,
"Failed to delete entity when removing character during possession."
);
}
}
}

View File

@ -14,7 +14,7 @@ use common::{
vol::ReadVol,
};
use common_ecs::{Job, Origin, Phase, System};
use common_net::msg::{ClientGeneral, PresenceKind, ServerGeneral};
use common_net::msg::{ClientGeneral, ServerGeneral};
use common_state::{BlockChange, BuildAreas};
use specs::{Entities, Join, Read, ReadExpect, ReadStorage, Write, WriteStorage};
use tracing::{debug, trace, warn};
@ -84,14 +84,14 @@ impl Sys {
}
},
ClientGeneral::ControllerInputs(inputs) => {
if matches!(presence.kind, PresenceKind::Character(_)) {
if presence.kind.controlling_char() {
if let Some(controller) = controllers.get_mut(entity) {
controller.inputs.update_with_new(*inputs);
}
}
},
ClientGeneral::ControlEvent(event) => {
if matches!(presence.kind, PresenceKind::Character(_)) {
if presence.kind.controlling_char() {
// Skip respawn if client entity is alive
if let ControlEvent::Respawn = event {
if healths.get(entity).map_or(true, |h| !h.is_dead) {
@ -105,7 +105,7 @@ impl Sys {
}
},
ClientGeneral::ControlAction(event) => {
if matches!(presence.kind, PresenceKind::Character(_)) {
if presence.kind.controlling_char() {
if let Some(controller) = controllers.get_mut(entity) {
controller.push_action(event);
}
@ -119,7 +119,7 @@ impl Sys {
.or_default()
});
if matches!(presence.kind, PresenceKind::Character(_))
if presence.kind.controlling_char()
&& force_updates.get(entity).is_none()
&& healths.get(entity).map_or(true, |h| !h.is_dead)
&& is_rider.get(entity).is_none()

View File

@ -100,7 +100,7 @@ impl<'a> System<'a> for Sys {
map_marker,
))
},
PresenceKind::Spectator => None,
PresenceKind::Spectator | PresenceKind::Possessor => None,
},
),
);

View File

@ -96,6 +96,31 @@ macro_rules! trackers {
Some(EntityPackage { uid, comps })
}
/// Create sync package for switching a client to another entity specifically to
/// remove/add components that are only synced for the client's entity.
pub fn create_sync_from_client_entity_switch(
&self,
old_uid: Uid,
new_uid: Uid,
new_entity: specs::Entity,
) -> CompSyncPackage<EcsCompPacket> {
let mut comp_sync_package = CompSyncPackage::new();
$(
if matches!(
<$component_type as NetSync>::SYNC_FROM,
SyncFrom::ClientEntity,
) {
comp_sync_package.comp_removed::<$component_type>(old_uid);
if let Some(comp) = self.$component_name.get(new_entity).cloned() {
comp_sync_package.comp_inserted(new_uid, comp);
}
}
)*
comp_sync_package
}
}
/// Contains an [`UpdateTracker`] for every synced component (that uses this method of
@ -225,6 +250,7 @@ macro_rules! trackers {
comp_sync_package
}
}
}
}

View File

@ -1022,8 +1022,9 @@ impl Hud {
// point.
let character_id = match client.presence().unwrap() {
PresenceKind::Character(id) => id,
PresenceKind::Character(id) => Some(id),
PresenceKind::Spectator => unreachable!("HUD creation in Spectator mode!"),
PresenceKind::Possessor => None,
};
// Create a new HotbarState from the persisted slots.

View File

@ -8,7 +8,8 @@
bool_to_option,
drain_filter,
once_cell,
trait_alias
trait_alias,
option_get_or_insert_default
)]
#![recursion_limit = "2048"]

View File

@ -35,7 +35,7 @@ impl Default for CharacterProfile {
pub struct ServerProfile {
/// A map of character's by id to their CharacterProfile.
pub characters: HashMap<CharacterId, CharacterProfile>,
// Selected character in the chararacter selection screen
/// Selected character in the chararacter selection screen
pub selected_character: Option<CharacterId>,
}
@ -53,18 +53,14 @@ impl Default for ServerProfile {
/// Initially it is just for persisting things that don't belong in
/// settings.ron - like the state of hotbar and any other character level
/// configuration.
#[derive(Clone, Debug, Serialize, Deserialize)]
#[derive(Default, Clone, Debug, Serialize, Deserialize)]
#[serde(default)]
pub struct Profile {
pub servers: HashMap<String, ServerProfile>,
}
impl Default for Profile {
fn default() -> Self {
Profile {
servers: HashMap::new(),
}
}
/// Temporary character profile, used when it should
/// not be persisted to the disk.
#[serde(skip)]
pub transient_character: Option<CharacterProfile>,
}
impl Profile {
@ -112,17 +108,22 @@ impl Profile {
/// # Arguments
///
/// * server - current server the character is on.
/// * character_id - id of the character.
/// * character_id - id of the character, passing `None` indicates the
/// transient character profile should be used.
pub fn get_hotbar_slots(
&self,
server: &str,
character_id: CharacterId,
character_id: Option<CharacterId>,
) -> [Option<hud::HotbarSlotContents>; 10] {
self.servers
.get(server)
.and_then(|s| s.characters.get(&character_id))
.map(|c| c.hotbar_slots.clone())
.unwrap_or_else(default_slots)
match character_id {
Some(character_id) => self
.servers
.get(server)
.and_then(|s| s.characters.get(&character_id)),
None => self.transient_character.as_ref(),
}
.map(|c| c.hotbar_slots.clone())
.unwrap_or_else(default_slots)
}
/// Set the hotbar_slots for the requested character_id.
@ -133,22 +134,26 @@ impl Profile {
/// # Arguments
///
/// * server - current server the character is on.
/// * character_id - id of the character.
/// * character_id - id of the character, passing `None` indicates the
/// transient character profile should be used.
/// * slots - array of hotbar_slots to save.
pub fn set_hotbar_slots(
&mut self,
server: &str,
character_id: CharacterId,
character_id: Option<CharacterId>,
slots: [Option<hud::HotbarSlotContents>; 10],
) {
self.servers
.entry(server.to_string())
.or_insert(ServerProfile::default())
// Get or update the CharacterProfile.
.characters
.entry(character_id)
.or_insert(CharacterProfile::default())
.hotbar_slots = slots;
match character_id {
Some(character_id) => self.servers
.entry(server.to_string())
.or_insert(ServerProfile::default())
// Get or update the CharacterProfile.
.characters
.entry(character_id)
.or_default(),
None => self.transient_character.get_or_insert_default(),
}
.hotbar_slots = slots;
}
/// Get the selected_character for the provided server.
@ -209,7 +214,7 @@ mod tests {
#[test]
fn test_get_slots_with_empty_profile() {
let profile = Profile::default();
let slots = profile.get_hotbar_slots("TestServer", 12345);
let slots = profile.get_hotbar_slots("TestServer", Some(12345));
assert_eq!(slots, [(); 10].map(|()| None))
}
@ -217,6 +222,6 @@ mod tests {
fn test_set_slots_with_empty_profile() {
let mut profile = Profile::default();
let slots = [(); 10].map(|()| None);
profile.set_hotbar_slots("TestServer", 12345, slots);
profile.set_hotbar_slots("TestServer", Some(12345), slots);
}
}

View File

@ -1361,10 +1361,11 @@ impl PlayState for SessionState {
let server_name = &client.server_info().name;
// If we are changing the hotbar state this CANNOT be None.
let character_id = match client.presence().unwrap() {
PresenceKind::Character(id) => id,
PresenceKind::Character(id) => Some(id),
PresenceKind::Spectator => {
unreachable!("HUD adaption in Spectator mode!")
},
PresenceKind::Possessor => None,
};
// Get or update the ServerProfile.