don't store group id in ChatMode

common/src/comp/chat.rs

@@ -18,7 +18,7 @@ pub enum ChatMode {
     /// Talk to players in your region of the world
     Region,
     /// Talk to your current group of players
-    Group(Group),
+    Group,
     /// Talk to your faction
     Faction(String),
     /// Talk to every player on the server
@@ -31,19 +31,31 @@ impl Component for ChatMode {
 
 impl ChatMode {
     /// Create a plain message from your current chat mode and uuid.
-    pub fn to_plain_msg(&self, from: Uid, text: impl ToString) -> UnresolvedChatMsg {
+    pub fn to_plain_msg(
+        &self,
+        from: Uid,
+        text: impl ToString,
+        group: Option<Group>,
+    ) -> Result<UnresolvedChatMsg, &'static str> {
         let chat_type = match self {
             ChatMode::Tell(to) => ChatType::Tell(from, *to),
             ChatMode::Say => ChatType::Say(from),
             ChatMode::Region => ChatType::Region(from),
-            ChatMode::Group(group) => ChatType::Group(from, *group),
+            ChatMode::Group => ChatType::Group(
+                from,
+                group.ok_or(
+                    "You tried sending a group message while not belonging to a group. Use /world \
+                     or /region to change your chat mode.",
+                )?,
+            ),
             ChatMode::Faction(faction) => ChatType::Faction(from, faction.clone()),
             ChatMode::World => ChatType::World(from),
         };
-        UnresolvedChatMsg {
+
+        Ok(UnresolvedChatMsg {
             chat_type,
             content: Content::Plain(text.to_string()),
-        }
+        })
     }
 }
 
@@ -346,10 +358,10 @@ impl<G> GenericChatMsg<G> {
         }
     }
 
-    pub fn get_group_mut_sender(&mut self) -> Option<(&mut G, Option<&Uid>)> {
-        match &mut self.chat_type {
-            ChatType::GroupMeta(g) => Some((g, None)),
-            ChatType::Group(sender, g) => Some((g, Some(sender))),
+    pub fn get_group(&self) -> Option<&G> {
+        match &self.chat_type {
+            ChatType::GroupMeta(g) => Some(g),
+            ChatType::Group(_, g) => Some(g),
             _ => None,
         }
     }

server/src/cmd.rs

@@ -2888,7 +2888,9 @@ fn handle_tell(
         } else {
             message_opt.join(" ")
         };
-        server.state.send_chat(mode.to_plain_msg(target_uid, msg));
+        server
+            .state
+            .send_chat(mode.to_plain_msg(target_uid, msg, None)?);
         server.notify_client(target, ServerGeneral::ChatMode(mode));
         Ok(())
     } else {
@@ -2913,7 +2915,7 @@ fn handle_faction(
         let msg = args.join(" ");
         if !msg.is_empty() {
             if let Some(uid) = server.state.ecs().read_storage().get(target) {
-                server.state.send_chat(mode.to_plain_msg(*uid, msg));
+                server.state.send_chat(mode.to_plain_msg(*uid, msg, None)?);
             }
         }
         server.notify_client(target, ServerGeneral::ChatMode(mode));
@@ -2933,14 +2935,16 @@ fn handle_group(
     no_sudo(client, target)?;
 
     let groups = server.state.ecs().read_storage::<comp::Group>();
-    if let Some(group) = groups.get(target) {
-        let mode = comp::ChatMode::Group(*group);
+    if let Some(group) = groups.get(target).copied() {
+        let mode = comp::ChatMode::Group;
         drop(groups);
         insert_or_replace_component(server, target, mode.clone(), "target")?;
         let msg = args.join(" ");
         if !msg.is_empty() {
             if let Some(uid) = server.state.ecs().read_storage().get(target) {
-                server.state.send_chat(mode.to_plain_msg(*uid, msg));
+                server
+                    .state
+                    .send_chat(mode.to_plain_msg(*uid, msg, Some(group))?);
             }
         }
         server.notify_client(target, ServerGeneral::ChatMode(mode));
@@ -3064,7 +3068,7 @@ fn handle_region(
     let msg = args.join(" ");
     if !msg.is_empty() {
         if let Some(uid) = server.state.ecs().read_storage().get(target) {
-            server.state.send_chat(mode.to_plain_msg(*uid, msg));
+            server.state.send_chat(mode.to_plain_msg(*uid, msg, None)?);
         }
     }
     server.notify_client(target, ServerGeneral::ChatMode(mode));
@@ -3085,7 +3089,7 @@ fn handle_say(
     let msg = args.join(" ");
     if !msg.is_empty() {
         if let Some(uid) = server.state.ecs().read_storage().get(target) {
-            server.state.send_chat(mode.to_plain_msg(*uid, msg));
+            server.state.send_chat(mode.to_plain_msg(*uid, msg, None)?);
         }
     }
     server.notify_client(target, ServerGeneral::ChatMode(mode));
@@ -3106,7 +3110,7 @@ fn handle_world(
     let msg = args.join(" ");
     if !msg.is_empty() {
         if let Some(uid) = server.state.ecs().read_storage().get(target) {
-            server.state.send_chat(mode.to_plain_msg(*uid, msg));
+            server.state.send_chat(mode.to_plain_msg(*uid, msg, None)?);
         }
     }
     server.notify_client(target, ServerGeneral::ChatMode(mode));

server/src/state_ext.rs

@@ -908,7 +908,7 @@ impl StateExt for State {
 
     /// Send the chat message to the proper players. Say and region are limited
     /// by location. Faction and group are limited by component.
-    fn send_chat(&self, mut msg: comp::UnresolvedChatMsg) {
+    fn send_chat(&self, msg: comp::UnresolvedChatMsg) {
         let ecs = self.ecs();
         let is_within =
             |target, a: &comp::Pos, b: &comp::Pos| a.0.distance_squared(b.0) < target * target;
@@ -918,31 +918,7 @@ impl StateExt for State {
 
         let group_manager = ecs.read_resource::<comp::group::GroupManager>();
 
-        let group_info = msg.get_group_mut_sender().and_then(|(g, from)| {
-            // Check if the sender is in the group he is trying to send the message to. If
-            // it doesn't match, update the message's group accordingly.
-            // There currently is a bug that can cause group messages to appear in other
-            // groups. This commonly happens when a player has his [ChatMode] set
-            // to group and is kicked from his current group or joins another,
-            // any message sent afterwards appears in the previous group as long
-            // as his chatmode isn't updated.
-            // TODO: Find out why sometimes messages from players never previously in the
-            // group leak.
-            // FIXME: Another pontential fix could be updating the [ChatMode] as the player
-            // leaves and joins a new group. Possibly a better solution?
-            if let Some(from) = from {
-                let group = ecs
-                    .read_storage::<comp::Group>()
-                    .get(entity_from_uid(*from)?)
-                    .copied()?;
-
-                if g != &group {
-                    *g = group;
-                }
-            }
-
-            group_manager.group_info(*g)
-        });
+        let group_info = msg.get_group().and_then(|g| group_manager.group_info(*g));
 
         let resolved_msg = msg
             .clone()
@@ -1100,7 +1076,12 @@ impl StateExt for State {
                 },
                 comp::ChatType::Group(from, g) => {
                     if group_info.is_none() {
-                        // group not found, reply with command error
+                        // Group not found, reply with command error
+                        // This should usually NEVER happen since now it is checked whether the
+                        // sender is still in the group upon emitting the message (TODO: Can this be
+                        // triggered if the message is sent in the same tick as the sender is
+                        // removed from the group?)
+
                         let reply = comp::ChatType::CommandError.into_plain_msg(
                             "You are using group chat but do not belong to a group. Use /world or \
                              /region to change chat.",

server/src/sys/msg/general.rs

@@ -1,12 +1,12 @@
 use crate::client::Client;
 use common::{
-    comp::{ChatMode, Player},
+    comp::{ChatMode, ChatType, Group, Player},
     event::{EventBus, ServerEvent},
     resources::Time,
     uid::Uid,
 };
 use common_ecs::{Job, Origin, Phase, System};
-use common_net::msg::ClientGeneral;
+use common_net::msg::{ClientGeneral, ServerGeneral};
 use rayon::prelude::*;
 use specs::{Entities, Join, ParJoin, Read, ReadStorage, WriteStorage};
 use tracing::{debug, error, warn};
@@ -15,10 +15,11 @@ impl Sys {
     fn handle_general_msg(
         server_emitter: &mut common::event::Emitter<'_, ServerEvent>,
         entity: specs::Entity,
-        _client: &Client,
+        client: &Client,
         player: Option<&Player>,
         uids: &ReadStorage<'_, Uid>,
         chat_modes: &ReadStorage<'_, ChatMode>,
+        groups: &ReadStorage<'_, Group>,
         msg: ClientGeneral,
     ) -> Result<(), crate::error::Error> {
         match msg {
@@ -27,8 +28,17 @@ impl Sys {
                     if let Some(from) = uids.get(entity) {
                         const CHAT_MODE_DEFAULT: &ChatMode = &ChatMode::default();
                         let mode = chat_modes.get(entity).unwrap_or(CHAT_MODE_DEFAULT);
-                        // Send chat message
-                        server_emitter.emit(ServerEvent::Chat(mode.to_plain_msg(*from, message)));
+                        // Try sending the chat message
+                        match mode.to_plain_msg(*from, message, groups.get(entity).copied()) {
+                            Ok(message) => {
+                                server_emitter.emit(ServerEvent::Chat(message));
+                            },
+                            Err(error) => {
+                                client.send_fallible(ServerGeneral::ChatMsg(
+                                    ChatType::CommandError.into_plain_msg(error),
+                                ));
+                            },
+                        }
                     } else {
                         error!("Could not send message. Missing player uid");
                     }
@@ -71,6 +81,7 @@ impl<'a> System<'a> for Sys {
         ReadStorage<'a, Uid>,
         ReadStorage<'a, ChatMode>,
         ReadStorage<'a, Player>,
+        ReadStorage<'a, Group>,
         WriteStorage<'a, Client>,
     );
 
@@ -80,7 +91,7 @@ impl<'a> System<'a> for Sys {
 
     fn run(
         _job: &mut Job<Self>,
-        (entities, server_event_bus, time, uids, chat_modes, players, mut clients): Self::SystemData,
+        (entities, server_event_bus, time, uids, chat_modes, players, groups, mut clients): Self::SystemData,
     ) {
         (&entities, &mut clients, players.maybe())
             .par_join()
@@ -95,6 +106,7 @@ impl<'a> System<'a> for Sys {
                             player,
                             &uids,
                             &chat_modes,
+                            &groups,
                             msg,
                         )
                     });

voxygen/src/hud/chat.rs

@@ -801,7 +801,7 @@ fn render_chat_mode(chat_mode: &ChatMode, imgs: &Imgs) -> (Color, conrod_core::i
         ChatMode::Say => (SAY_COLOR, imgs.chat_say_small),
         ChatMode::Region => (REGION_COLOR, imgs.chat_region_small),
         ChatMode::Faction(_) => (FACTION_COLOR, imgs.chat_faction_small),
-        ChatMode::Group(_) => (GROUP_COLOR, imgs.chat_group_small),
+        ChatMode::Group => (GROUP_COLOR, imgs.chat_group_small),
         ChatMode::Tell(_) => (TELL_COLOR, imgs.chat_tell_small),
     }
 }