Merge branch 'imbris/misc_fix' into 'master'

Trade canceling related tweaks, make kill_npcs not leave clutter (and actually remove entities in the first place), and misc tweaks

See merge request veloren/veloren!3555

CHANGELOG.md

@@ -17,11 +17,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Use fluent for translations
 - First tab on Login screen triggers username focus
 - Certain NPCs will now attack when alone with victim
+- /kill_npcs no longer leaves drops behind and also has bug causing it to not destroy entities
+  fixed.
 
 ### Removed
 
 ### Fixed
 - Fixed crash due to zooming out very far
+- Client properly knows trade was cancelled when exiting to the character screen (and no longer
+  tries to display the trade window when rejoining)
+- Cancel trades for an entity when it is deleted (note this doesn't effect trades between players
+  since their entities are not removed).
 
 ## [0.13.0] - 2022-07-23
 

client/src/lib.rs

@@ -2065,6 +2065,8 @@ impl Client {
                             PresenceKind::Possessor => PresenceKind::Possessor,
                         });
                     }
+                    // Clear pending trade
+                    self.pending_trade = None;
                 } else {
                     return Err(Error::Other("Failed to find entity from uid.".into()));
                 }
@@ -2543,6 +2545,9 @@ impl Client {
 
     /// Clean client ECS state
     fn clean_state(&mut self) {
+        // Clear pending trade
+        self.pending_trade = None;
+
         let client_uid = self
             .uid()
             .map(|u| u.into())

common/src/comp/health.rs

@@ -195,10 +195,7 @@ impl Health {
 
     pub fn should_die(&self) -> bool { self.current == 0 }
 
-    pub fn kill(&mut self) {
-        self.current = 0;
-        self.is_dead = true;
-    }
+    pub fn kill(&mut self) { self.current = 0; }
 
     #[cfg(not(target_arch = "wasm32"))]
     pub fn revive(&mut self) {

server/src/client.rs

@@ -140,6 +140,7 @@ impl Client {
         }*/
     }
 
+    /// Like `send` but any errors are explicitly ignored.
     pub(crate) fn send_fallible<M: Into<ServerMsg>>(&self, msg: M) { let _ = self.send(msg); }
 
     pub(crate) fn send_prepared(&self, msg: &PreparedMsg) -> Result<(), StreamError> {

server/src/cmd.rs

@@ -1826,27 +1826,35 @@ fn handle_kill_npcs(
         false
     };
 
-    let ecs = server.state.ecs();
-    let mut healths = ecs.write_storage::<comp::Health>();
-    let players = ecs.read_storage::<comp::Player>();
-    let alignments = ecs.read_storage::<Alignment>();
-    let mut count = 0;
+    let to_kill = {
+        let ecs = server.state.ecs();
+        let entities = ecs.entities();
+        let healths = ecs.write_storage::<comp::Health>();
+        let players = ecs.read_storage::<comp::Player>();
+        let alignments = ecs.read_storage::<Alignment>();
 
-    for (mut health, (), alignment) in (&mut healths, !&players, alignments.maybe()).join() {
-        let should_kill = kill_pets
-            || if let Some(Alignment::Owned(owned)) = alignment {
-                ecs.entity_from_uid(owned.0)
-                    .map_or(true, |owner| !players.contains(owner))
-            } else {
-                true
-            };
+        (&entities, &healths, !&players, alignments.maybe())
+            .join()
+            .filter_map(|(entity, _health, (), alignment)| {
+                let should_kill = kill_pets
+                    || if let Some(Alignment::Owned(owned)) = alignment {
+                        ecs.entity_from_uid(owned.0)
+                            .map_or(true, |owner| !players.contains(owner))
+                    } else {
+                        true
+                    };
 
-        if should_kill {
-            count += 1;
-            health.kill();
+                should_kill.then(|| entity)
+            })
+            .collect::<Vec<_>>()
+    };
+    let count = to_kill.len();
+    for entity in to_kill {
+        // Directly remove entities instead of modifying health to avoid loot drops.
+        if let Err(e) = server.state.delete_entity_recorded(entity) {
+            error!(?e, ?entity, "Failed to delete entity");
         }
     }
-
     let text = if count > 0 {
         format!("Destroyed {} NPCs.", count)
     } else {

server/src/events/entity_manipulation.rs

@@ -129,15 +129,6 @@ pub fn handle_knockback(server: &Server, entity: EcsEntity, impulse: Vec3<f32>)
 /// other players. If the entity that killed it had stats, then give it exp for
 /// the kill. Experience given is equal to the level of the entity that was
 /// killed times 10.
-// NOTE: Clippy incorrectly warns about a needless collect here because it does
-// not understand that the pet count (which is computed during the first
-// iteration over the members in range) is actually used by the second iteration
-// over the members in range; since we have no way of knowing the pet count
-// before the first loop finishes, we definitely need at least two loops.   Then
-// (currently) our only options are to store the member list in temporary space
-// (e.g. by collecting to a vector), or to repeat the loop; but repeating the
-// loop would currently be very inefficient since it has to rescan every entity
-// on the server again.
 pub fn handle_destroy(server: &mut Server, entity: EcsEntity, last_change: HealthChange) {
     let state = server.state_mut();
 
@@ -531,18 +522,11 @@ pub fn handle_destroy(server: &mut Server, entity: EcsEntity, last_change: Healt
                 .destroy_entity(rtsim_entity.0);
         }
 
-        let _ = state
-            .delete_entity_recorded(entity)
-            .map_err(|e| error!(?e, ?entity, "Failed to delete destroyed entity"));
+        println!("deleting");
+        if let Err(e) = state.delete_entity_recorded(entity) {
+            error!(?e, ?entity, "Failed to delete destroyed entity");
+        }
     }
-
-    // TODO: Add Delete(time_left: Duration) component
-    /*
-    // If not a player delete the entity
-    if let Err(err) = state.delete_entity_recorded(entity) {
-        error!(?e, "Failed to delete destroyed entity");
-    }
-    */
 }
 
 /// Delete an entity without any special actions (this is generally used for

server/src/events/mod.rs

@@ -25,9 +25,10 @@ use inventory_manip::handle_inventory;
 use invite::{handle_invite, handle_invite_response};
 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};
+use trade::handle_process_trade_action;
 
 pub use group_manip::update_map_markers;
+pub(crate) use trade::cancel_trades_for;
 
 mod entity_creation;
 mod entity_manipulation;
@@ -165,7 +166,6 @@ impl Server {
                     handle_loaded_character_data(self, entity, components);
                 },
                 ServerEvent::ExitIngame { entity } => {
-                    cancel_trade_for(self, entity);
                     handle_exit_ingame(self, entity);
                 },
                 ServerEvent::CreateNpc {

server/src/events/player.rs

@@ -1,8 +1,7 @@
 use super::Event;
 use crate::{
-    client::Client, events::trade::cancel_trade_for, metrics::PlayerMetrics,
-    persistence::character_updater::CharacterUpdater, presence::Presence, state_ext::StateExt,
-    BattleModeBuffer, Server,
+    client::Client, metrics::PlayerMetrics, persistence::character_updater::CharacterUpdater,
+    presence::Presence, state_ext::StateExt, BattleModeBuffer, Server,
 };
 use common::{
     comp,
@@ -24,9 +23,17 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity) {
     let entity = persist_entity(state, entity);
 
     // Create new entity with just `Client`, `Uid`, `Player`, and `...Stream`
-    // components Easier than checking and removing all other known components
+    // components.
+    //
+    // Easier than checking and removing all other known components.
+    //
     // Note: If other `ServerEvent`s are referring to this entity they will be
-    // disrupted
+    // disrupted.
+
+    // Since we remove `Uid` below, any trades won't be cancelled by
+    // `delete_entity_recorded`. So we cancel the trade here. (maybe the trade
+    // key could be switched from `Uid` to `Entity`)
+    super::cancel_trades_for(state, entity);
 
     let maybe_admin = state.ecs().write_storage::<comp::Admin>().remove(entity);
     let maybe_group = state
@@ -115,7 +122,6 @@ pub fn handle_client_disconnect(
     skip_persistence: bool,
 ) -> Event {
     span!(_guard, "handle_client_disconnect");
-    cancel_trade_for(server, entity);
     if let Some(client) = server
         .state()
         .ecs()

server/src/events/trade.rs

@@ -55,7 +55,7 @@ fn notify_agent_prices(
 }
 
 /// Invoked when the trade UI is up, handling item changes, accepts, etc
-pub fn handle_process_trade_action(
+pub(super) fn handle_process_trade_action(
     server: &mut Server,
     entity: EcsEntity,
     trade_id: TradeId,
@@ -164,29 +164,34 @@ pub fn handle_process_trade_action(
     }
 }
 
-//Cancel all trades registered for a given UID.
-pub fn cancel_trade_for(server: &mut Server, entity: EcsEntity) {
-    if let Some(uid) = server.state().ecs().uid_from_entity(entity) {
-        let mut trades = server.state.ecs().write_resource::<Trades>();
+/// Cancel all trades registered for a given UID.
+///
+/// Note: This doesn't send any notification to the provided entity (only other
+/// participants in the trade). It is assumed that the supplied entity either no
+/// longer exists or is awareof this cancellation through other means (e.g.
+/// client getting ExitInGameSuccess message knows that it should clear any
+/// trades).
+pub(crate) fn cancel_trades_for(state: &mut common_state::State, entity: EcsEntity) {
+    let ecs = state.ecs();
+    if let Some(uid) = ecs.uid_from_entity(entity) {
+        let mut trades = ecs.write_resource::<Trades>();
 
         let active_trade = match trades.entity_trades.get(&uid) {
             Some(n) => *n,
-            None => {
-                return;
-            },
+            None => return,
         };
 
         let to_notify = trades.decline_trade(active_trade, uid);
-        to_notify
-            .and_then(|u| server.state.ecs().entity_from_uid(u.0))
-            .map(|e| {
-                server.notify_client(e, ServerGeneral::FinishedTrade(TradeResult::Declined));
-                notify_agent_simple(
-                    server.state.ecs().write_storage::<Agent>(),
-                    e,
-                    AgentEvent::FinishedTrade(TradeResult::Declined),
-                );
-            });
+        to_notify.and_then(|u| ecs.entity_from_uid(u.0)).map(|e| {
+            if let Some(c) = ecs.read_storage::<crate::Client>().get(e) {
+                c.send_fallible(ServerGeneral::FinishedTrade(TradeResult::Declined));
+            }
+            notify_agent_simple(
+                ecs.write_storage::<Agent>(),
+                e,
+                AgentEvent::FinishedTrade(TradeResult::Declined),
+            );
+        });
     }
 }
 

server/src/lib.rs

@@ -1172,11 +1172,9 @@ impl Server {
     where
         S: Into<ServerMsg>,
     {
-        self.state
-            .ecs()
-            .read_storage::<Client>()
-            .get(entity)
-            .map(|c| c.send(msg));
+        if let Some(client) = self.state.ecs().read_storage::<Client>().get(entity) {
+            client.send_fallible(msg);
+        }
     }
 
     pub fn notify_players(&mut self, msg: ServerGeneral) { self.state.notify_players(msg); }

server/src/state_ext.rs

@@ -1,7 +1,7 @@
 use crate::{
     automod::AutoMod,
     client::Client,
-    events::update_map_markers,
+    events::{self, update_map_markers},
     persistence::PersistedComponents,
     pet::restore_pet,
     presence::{Presence, RepositionOnChunkLoad},
@@ -1000,10 +1000,14 @@ impl StateExt for State {
             );
         }
 
+        // Cancel extant trades
+        events::cancel_trades_for(self, entity);
+
         let (maybe_uid, maybe_pos) = (
             self.ecs().read_storage::<Uid>().get(entity).copied(),
             self.ecs().read_storage::<comp::Pos>().get(entity).copied(),
         );
+
         let res = self.ecs_mut().delete_entity(entity);
         if res.is_ok() {
             if let (Some(uid), Some(pos)) = (maybe_uid, maybe_pos) {