Add broadcast command #148

van-sprundel · 2024-10-18T22:06:37Z

Implementation for #15.

Supports target selectors.
Also added functions to get all_players and nearest_player.

Snowiiii · 2024-10-19T08:35:17Z

pumpkin/src/world/mod.rs

+    pub fn get_players(&self) -> Vec<Arc<Player>> {
+        self.current_players
+            .lock()
+            .values()
+            .cloned()
+            .collect::<Vec<_>>()
+    }


I don't like copying the Vec here

Got it. Changed it to clone just the name string instead of the entire player

Snowiiii · 2024-10-19T08:38:38Z

pumpkin/src/commands/cmd_say.rs

+fn parse_selectors(content: &str, sender: &CommandSender, server: &Server) -> String {
+    let mut final_message = String::new();
+
+    let tokens: Vec<&str> = content.split_whitespace().collect();
+
+    for token in tokens {
+        // TODO impl @e
+        let result = match token {
+            "@p" => {
+                let position = match sender {
+                    CommandSender::Player(p) => p.last_position.load(),
+                    _ => Vector3::new(0., 0., 0.),
+                };
+                vec![server
+                    .get_nearest_player(&position)
+                    .map_or_else(|| String::from("nobody"), |p| p.gameprofile.name.clone())]
+            }
+            "@r" => {
+                let online_players: Vec<String> = server
+                    .get_online_players()
+                    .map(|p| p.gameprofile.name.clone())
+                    .collect();
+
+                if online_players.is_empty() {
+                    vec![String::from("nobody")]
+                } else {
+                    vec![online_players[rand::random::<usize>() % online_players.len()].clone()]
+                }
+            }
+            "@s" => match sender {
+                CommandSender::Player(p) => vec![p.gameprofile.name.clone()],
+                _ => vec![String::from("console")],
+            },
+            "@a" => server
+                .get_online_players()
+                .map(|p| p.gameprofile.name.clone())
+                .collect::<Vec<_>>(),
+            "@here" => server
+                .get_online_players()
+                .map(|p| p.gameprofile.name.clone())
+                .collect::<Vec<_>>(),
+            _ => vec![token.to_string()],
+        };
+
+        // formatted player names
+        final_message.push_str(&format_player_names(&result));
+        final_message.push(' ');
+    }
+
+    final_message.trim_end().to_string()
+}


We already have player arguments implemented, See for example gamemode command or kick command...
You can implement is easily by providing the target as an argument .with_child(argument(ARG_TARGET, consume_arg_player) and then parsing it let target = parse_arg_player(sender, server, ARG_TARGET, args)?;

The say command supports multiple target specifiers, it's not a command argument, but a way the content gets formatted

pumpkin/src/commands/cmd_say.rs

Snowiiii · 2024-10-19T08:45:18Z

pumpkin/src/server/mod.rs

+
+    /// Gets the nearest player from the world position
+    pub fn get_nearest_player(&self, target: &Vector3<f64>) -> Option<Arc<Player>> {
+        // TODO respect which world the player is in


You can easily get the World from a player by calling player.living_enitity.entity.world. But i the current plan is to search for Player on all world (e.g. Gamemode, Kick Command...)

In minecraft, the nearest player has to be in the same world. If that's the plan though, then I can change it

pumpkin/src/server/mod.rs

…odule This function is specific to the command, we don't want to check any other world since a target specifier only checks the world the player is in

StripedMonkey

Thanks for your efforts trying to implement this! I do have some quibbles and things that would be good to see changed.

StripedMonkey · 2024-10-19T15:40:20Z

pumpkin-core/src/math/distance.rs

@@ -0,0 +1,9 @@
+use super::vector3::Vector3;


I'm not sure I'm a fan of having something as trivial as "distance" be its own file unless we have a larger set of traits. Is there a reason this isn't in Vector3?

Something like:

pub fn distance_to(&self, other: &Vector3<T>) -> T { (self - other).length() }

StripedMonkey · 2024-10-19T15:44:15Z

pumpkin/src/world/mod.rs

+    pub fn broadcast_message(&self, content: &TextComponent) {
+        self.current_players.lock().values().for_each(|player| {
+            player.send_system_message(content.clone());
+        });
+    }


Nit: Can we make this generic over an arbitrary ChatMessage trait implemented for strings, textcomponents, etc? Or make it generic over something like Into<TextComponent>?

StripedMonkey · 2024-10-19T16:07:08Z

pumpkin/src/commands/mod.rs

@@ -77,6 +78,7 @@ pub fn default_dispatcher<'a>() -> CommandDispatcher<'a> {
    dispatcher.register(cmd_echest::init_command_tree());
    dispatcher.register(cmd_kill::init_command_tree());
    dispatcher.register(cmd_kick::init_command_tree());
+    dispatcher.register(cmd_say::init_command_tree());


This isn't your problem per-se, but When I see large blocks of repetitive code like this my eyes start to glaze over and I stop being able to read.

It sets off alarm bells that the code could be simplified some. It might not be getting to that point right now, but with how large and repetitive this is getting, another abstraction would be nice.

List Option

Something like

fn register_commands(&mut self, commands: impl IntoIterator<C>) where C: CommandTree { for command in commands { dispatcher.register(command::default()); } } // in default_dispatcher dispatcher.register_commands(vec![ cmd_pumpkin, cmd_gamemode, ... ]);

Macro Option

macro_rules! init_commands { (($command:ident),*) => { $( dispatcher.register($command::init_command_tree()); )*} } // In default_dispatcher init_commands!(cmd_pumpkin, cmd_gamemode, cmd_stop, cmd_help, cmd_echest, cmd_kill);

Say what you will about macros, but as long as they're explained, they make the boilerplate much more legible.

In the current code, dispatcher.register(...::init_command_tree()); is all noise, and what you're actually reading needs to be that tiny cmd_say

The macro option can be implemented as-is, but the list list one requires you to do some more significant refactors. Neither has to be implemented right now, but it's getting there.

StripedMonkey · 2024-10-19T16:12:47Z

pumpkin/src/world/mod.rs

+            .collect::<Vec<_>>()
+    }
+
+    pub fn get_nearest_player_name(&self, target: &Vector3<f64>) -> Option<String> {


I understand this PR is not intended to give us the functionality for everything, but this should not be get_nearest_player_name, but

let Some(name) = get_nearest_player().map(Player::get_name) else { return Err(...) }

or something.

for a single get_nearest_... it works, but implementing this functionality for EVERYTHING you can get from a "nearest player" would be terrifying

van-sprundel added 5 commits October 18, 2024 20:42

Add say command

c8df113

Use map instead of map_or

118da0d

Update impl

b3561a5

Add target selector logic

ca376b7

Use tokens instead, remove regex dependency

fa4f1bb

van-sprundel changed the title ~~Add say command~~ Add broadcast command Oct 18, 2024

Moved distance function to core

9b624db

Snowiiii reviewed Oct 19, 2024

View reviewed changes

van-sprundel added 9 commits October 19, 2024 11:41

Use reference for broadcast_message

36aba13

Cargo fmt

b2415af

Only allow player for get_nearest_player

3b03ebe

Move to get_player_names to avoid arc cloning, updated func

1b3e040

cargo fmt

7ac0d12

Move get_nearest_player_name down to world struct, move func to cmd m…

07542b1

…odule This function is specific to the command, we don't want to check any other world since a target specifier only checks the world the player is in

Only send console message when not a player

d80a481

Put broadcast_message iter in closure

7b1b427

cargo fmt

2d249c5

StripedMonkey suggested changes Oct 19, 2024

View reviewed changes

van-sprundel closed this by deleting the head repository Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add broadcast command #148

Add broadcast command #148

van-sprundel commented Oct 18, 2024 •

edited

Loading

Snowiiii Oct 19, 2024

van-sprundel Oct 19, 2024

Snowiiii Oct 19, 2024

van-sprundel Oct 19, 2024

Snowiiii Oct 19, 2024

van-sprundel Oct 19, 2024

StripedMonkey left a comment

StripedMonkey Oct 19, 2024

StripedMonkey Oct 19, 2024

StripedMonkey Oct 19, 2024

StripedMonkey Oct 19, 2024

Add broadcast command #148

Add broadcast command #148

Conversation

van-sprundel commented Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StripedMonkey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

List Option

Macro Option

Choose a reason for hiding this comment

van-sprundel commented Oct 18, 2024 •

edited

Loading