improve some docs and apis related to pathfinder

azalea-client/Cargo.toml

@@ -7,7 +7,6 @@ license = { workspace = true }
 repository = { workspace = true }
 
 [dependencies]
-anyhow = { workspace = true }
 azalea-auth = { path = "../azalea-auth", version = "0.11.0" }
 azalea-block = { path = "../azalea-block", version = "0.11.0" }
 azalea-buf = { path = "../azalea-buf", version = "0.11.0" }
@@ -36,6 +35,9 @@ tokio = { workspace = true, features = ["sync"] }
 tracing = { workspace = true }
 uuid = { workspace = true }
 
+[dev-dependencies]
+anyhow = { workspace = true }
+
 [features]
 default = ["log"]
 # enables bevy_log::LogPlugin by default

azalea-client/src/client.rs

@@ -518,6 +518,14 @@ impl Client {
     /// Get a component from this client. This will clone the component and
     /// return it.
     ///
+    ///
+    /// If the component can't be cloned, try [`Self::map_component`] instead.
+    /// If it isn't guaranteed to be present, use [`Self::get_component`] or
+    /// [`Self::map_get_component`].
+    ///
+    /// You may also use [`Self::ecs`] and [`Self::query`] directly if you need
+    /// more control over when the ECS is locked.
+    ///
     /// # Panics
     ///
     /// This will panic if the component doesn't exist on the client.
@@ -534,10 +542,56 @@ impl Client {
     }
 
     /// Get a component from this client, or `None` if it doesn't exist.
+    ///
+    /// If the component can't be cloned, try [`Self::map_component`] instead.
+    /// You may also have to use [`Self::ecs`] and [`Self::query`] directly.
     pub fn get_component<T: Component + Clone>(&self) -> Option<T> {
         self.query::<Option<&T>>(&mut self.ecs.lock()).cloned()
     }
 
+    /// Get a required component for this client and call the given function.
+    ///
+    /// Similar to [`Self::component`], but doesn't clone the component since
+    /// it's passed as a reference. [`Self::ecs`] will remain locked while the
+    /// callback is being run.
+    ///
+    /// If the component is not guaranteed to be present, use
+    /// [`Self::get_component`] instead.
+    ///
+    /// # Panics
+    ///
+    /// This will panic if the component doesn't exist on the client.
+    ///
+    /// ```
+    /// # use azalea_client::{Client, Hunger};
+    /// # fn example(bot: &Client) {
+    /// let hunger = bot.map_component::<Hunger, _>(|h| h.food);
+    /// # }
+    /// ```
+    pub fn map_component<T: Component, R>(&self, f: impl FnOnce(&T) -> R) -> R {
+        let mut ecs = self.ecs.lock();
+        let value = self.query::<&T>(&mut ecs);
+        f(value)
+    }
+
+    /// Optionally get a component for this client and call the given function.
+    ///
+    /// Similar to [`Self::get_component`], but doesn't clone the component
+    /// since it's passed as a reference. [`Self::ecs`] will remain locked
+    /// while the callback is being run.
+    ///
+    /// ```
+    /// # use azalea_client::{Client, mining::Mining};
+    /// # fn example(bot: &Client) {
+    /// let is_mining = bot.map_get_component::<Mining, _>(|m| m.is_some());
+    /// # }
+    /// ```
+    pub fn map_get_component<T: Component, R>(&self, f: impl FnOnce(Option<&T>) -> R) -> R {
+        let mut ecs = self.ecs.lock();
+        let value = self.query::<Option<&T>>(&mut ecs);
+        f(value)
+    }
+
     /// Get an `RwLock` with a reference to our (potentially shared) world.
     ///
     /// This gets the [`Instance`] from the client's [`InstanceHolder`]

azalea-client/src/events.rs

@@ -58,6 +58,9 @@ pub enum Event {
     /// it's actually spawned. This can be useful for setting the client
     /// information with `Client::set_client_information`, so the packet
     /// doesn't have to be sent twice.
+    ///
+    /// You may want to use [`Event::Login`] instead to wait for the bot to be
+    /// in the world.
     Init,
     /// The client is now in the world. Fired when we receive a login packet.
     Login,

azalea-client/src/local_player.rs

@@ -144,7 +144,7 @@ pub enum HandlePacketError {
     #[error(transparent)]
     Io(#[from] io::Error),
     #[error(transparent)]
-    Other(#[from] anyhow::Error),
+    Other(#[from] Box<dyn std::error::Error + Send + Sync>),
     #[error("{0}")]
     Send(#[from] mpsc::error::SendError<AzaleaEvent>),
 }

azalea/src/bot.rs

@@ -104,6 +104,11 @@ impl BotClientExt for azalea_client::Client {
         });
     }
 
+    /// Returns a Receiver that receives a message every game tick.
+    ///
+    /// This is useful if you want to efficiently loop until a certain condition
+    /// is met.
+    ///
     /// ```
     /// # use azalea::prelude::*;
     /// # use azalea::container::WaitingForInventoryOpen;

azalea/src/pathfinder/astar.rs

@@ -23,12 +23,24 @@ const COEFFICIENTS: [f32; 7] = [1.5, 2., 2.5, 3., 4., 5., 10.];
 
 const MIN_IMPROVEMENT: f32 = 0.01;
 
+pub enum PathfinderTimeout {
+    /// Time out after a certain duration has passed. This is a good default so
+    /// you don't waste too much time calculating a path if you're on a slow
+    /// computer.
+    Time(Duration),
+    /// Time out after this many nodes have been considered.
+    ///
+    /// This is useful as an alternative to a time limit if you're doing
+    /// something like running tests where you want consistent results.
+    Nodes(usize),
+}
+
 pub fn a_star<P, M, HeuristicFn, SuccessorsFn, SuccessFn>(
     start: P,
     heuristic: HeuristicFn,
     mut successors: SuccessorsFn,
     success: SuccessFn,
-    timeout: Duration,
+    timeout: PathfinderTimeout,
 ) -> Path<P, M>
 where
     P: Eq + Hash + Copy + Debug,
@@ -104,10 +116,16 @@ where
         }
 
         // check for timeout every ~1ms
-        if num_nodes % 1000 == 0 && start_time.elapsed() > timeout {
+        if num_nodes % 1000 == 0 {
-            // timeout, just return the best path we have so far
+            let timed_out = match timeout {
-            trace!("A* couldn't find a path in time, returning best path");
+                PathfinderTimeout::Time(max_duration) => start_time.elapsed() > max_duration,
-            break;
+                PathfinderTimeout::Nodes(max_nodes) => num_nodes > max_nodes,
+            };
+            if timed_out {
+                // timeout, just return the best path we have so far
+                trace!("A* couldn't find a path in time, returning best path");
+                break;
+            }
         }
     }
 

azalea/src/pathfinder/mod.rs

@@ -16,6 +16,7 @@ use std::sync::atomic::{self, AtomicUsize};
 use std::sync::Arc;
 use std::time::{Duration, Instant};
 
+use astar::PathfinderTimeout;
 use azalea_client::inventory::{Inventory, InventorySet, SetSelectedHotbarSlotEvent};
 use azalea_client::mining::{Mining, StartMiningBlockEvent};
 use azalea_client::movement::MoveEventsSet;
@@ -33,6 +34,7 @@ use bevy_ecs::query::Changed;
 use bevy_ecs::schedule::IntoSystemConfigs;
 use bevy_tasks::{AsyncComputeTaskPool, Task};
 use futures_lite::future;
+use parking_lot::RwLock;
 use tracing::{debug, error, info, trace, warn};
 
 use self::debug::debug_render_path_with_particles;
@@ -49,9 +51,7 @@ use crate::ecs::{
     query::{With, Without},
     system::{Commands, Query, Res},
 };
-use crate::pathfinder::astar::a_star;
+use crate::pathfinder::{astar::a_star, moves::PathfinderCtx, world::CachedWorld};
-use crate::pathfinder::moves::PathfinderCtx;
-use crate::pathfinder::world::CachedWorld;
 use crate::WalkDirection;
 
 #[derive(Clone, Default)]
@@ -103,6 +103,8 @@ pub struct Pathfinder {
     pub is_calculating: bool,
     pub allow_mining: bool,
 
+    pub deterministic_timeout: bool,
+
     pub goto_id: Arc<AtomicUsize>,
 }
 
@@ -117,8 +119,14 @@ pub struct ExecutingPath {
     pub is_path_partial: bool,
 }
 
+/// Send this event to start pathfinding to the given goal.
+///
+/// Also see [`PathfinderClientExt::goto`].
+///
+/// This event is read by [`goto_listener`].
 #[derive(Event)]
 pub struct GotoEvent {
+    /// The local bot entity that will do the pathfinding and execute the path.
     pub entity: Entity,
     pub goal: Arc<dyn Goal + Send + Sync>,
     /// The function that's used for checking what moves are possible. Usually
@@ -127,6 +135,12 @@ pub struct GotoEvent {
 
     /// Whether the bot is allowed to break blocks while pathfinding.
     pub allow_mining: bool,
+
+    /// Whether the timeout should be based on number of nodes considered
+    /// instead of the time passed.
+    ///
+    /// Also see: [`PathfinderTimeout::Nodes`]
+    pub deterministic_timeout: bool,
 }
 #[derive(Event, Clone)]
 pub struct PathFoundEvent {
@@ -155,6 +169,8 @@ pub trait PathfinderClientExt {
 }
 
 impl PathfinderClientExt for azalea_client::Client {
+    /// Start pathfinding to a given goal.
+    ///
     /// ```
     /// # use azalea::prelude::*;
     /// # use azalea::{BlockPos, pathfinder::goals::BlockPosGoal};
@@ -168,6 +184,7 @@ impl PathfinderClientExt for azalea_client::Client {
             goal: Arc::new(goal),
             successors_fn: moves::default_move,
             allow_mining: true,
+            deterministic_timeout: false,
         });
     }
 
@@ -179,6 +196,7 @@ impl PathfinderClientExt for azalea_client::Client {
             goal: Arc::new(goal),
             successors_fn: moves::default_move,
             allow_mining: false,
+            deterministic_timeout: false,
         });
     }
 
@@ -251,7 +269,6 @@ pub fn goto_listener(
         let entity = event.entity;
 
         let goto_id_atomic = pathfinder.goto_id.clone();
-        let goto_id = goto_id_atomic.fetch_add(1, atomic::Ordering::Relaxed) + 1;
 
         let allow_mining = event.allow_mining;
         let mining_cache = MiningCache::new(if allow_mining {
@@ -260,85 +277,133 @@ pub fn goto_listener(
             None
         });
 
-        let task = thread_pool.spawn(async move {
+        let deterministic_timeout = event.deterministic_timeout;
-            debug!("start: {start:?}");
 
-            let cached_world = CachedWorld::new(world_lock);
+        let task = thread_pool.spawn(calculate_path(CalculatePathOpts {
-            let successors = |pos: BlockPos| {
+            entity,
-                call_successors_fn(&cached_world, &mining_cache, successors_fn, pos)
+            start,
-            };
+            goal,
-
+            successors_fn,
-            let mut attempt_number = 0;
+            world_lock,
-
+            goto_id_atomic,
-            let mut path;
+            allow_mining,
-            let mut is_partial: bool;
+            mining_cache,
-
+            deterministic_timeout,
-            'calculate: loop {
+        }));
-                let start_time = std::time::Instant::now();
-                let astar::Path { movements, partial } = a_star(
-                    start,
-                    |n| goal.heuristic(n),
-                    successors,
-                    |n| goal.success(n),
-                    Duration::from_secs(if attempt_number == 0 { 1 } else { 5 }),
-                );
-                let end_time = std::time::Instant::now();
-                debug!("partial: {partial:?}");
-                let duration = end_time - start_time;
-                if partial {
-                    if movements.is_empty() {
-                        info!("Pathfinder took {duration:?} (empty path)");
-                    } else {
-                        info!("Pathfinder took {duration:?} (incomplete path)");
-                    }
-                    // wait a bit so it's not a busy loop
-                    std::thread::sleep(Duration::from_millis(100));
-                } else {
-                    info!("Pathfinder took {duration:?}");
-                }
-
-                debug!("Path:");
-                for movement in &movements {
-                    debug!("  {:?}", movement.target);
-                }
-
-                path = movements.into_iter().collect::<VecDeque<_>>();
-                is_partial = partial;
-
-                let goto_id_now = goto_id_atomic.load(atomic::Ordering::Relaxed);
-                if goto_id != goto_id_now {
-                    // we must've done another goto while calculating this path, so throw it away
-                    warn!("finished calculating a path, but it's outdated");
-                    return None;
-                }
-
-                if path.is_empty() && partial {
-                    if attempt_number == 0 {
-                        debug!("this path is empty, retrying with a higher timeout");
-                        attempt_number += 1;
-                        continue 'calculate;
-                    } else {
-                        debug!("this path is empty, giving up");
-                        break 'calculate;
-                    }
-                }
-                break;
-            }
-
-            Some(PathFoundEvent {
-                entity,
-                start,
-                path: Some(path),
-                is_partial,
-                successors_fn,
-                allow_mining,
-            })
-        });
 
         commands.entity(event.entity).insert(ComputePath(task));
     }
 }
 
+pub struct CalculatePathOpts {
+    pub entity: Entity,
+    pub start: BlockPos,
+    pub goal: Arc<dyn Goal + Send + Sync>,
+    pub successors_fn: SuccessorsFn,
+    pub world_lock: Arc<RwLock<azalea_world::Instance>>,
+    pub goto_id_atomic: Arc<AtomicUsize>,
+    pub allow_mining: bool,
+    pub mining_cache: MiningCache,
+    /// See [`GotoEvent::deterministic_timeout`]
+    pub deterministic_timeout: bool,
+}
+
+/// Calculate the [`PathFoundEvent`] for the given pathfinder options.
+///
+/// You usually want to just use [`PathfinderClientExt::goto`] or send a
+/// [`GotoEvent`] instead of calling this directly.
+///
+/// You are expected to immediately send the `PathFoundEvent` you received after
+/// calling this function. `None` will be returned if the pathfinding was
+/// interrupted by another path calculation.
+pub async fn calculate_path(opts: CalculatePathOpts) -> Option<PathFoundEvent> {
+    debug!("start: {:?}", opts.start);
+
+    let goto_id = opts.goto_id_atomic.fetch_add(1, atomic::Ordering::SeqCst) + 1;
+
+    let cached_world = CachedWorld::new(opts.world_lock);
+    let successors = |pos: BlockPos| {
+        call_successors_fn(&cached_world, &opts.mining_cache, opts.successors_fn, pos)
+    };
+
+    let mut attempt_number = 0;
+
+    let mut path;
+    let mut is_partial: bool;
+
+    'calculate: loop {
+        let start_time = Instant::now();
+
+        let timeout = if opts.deterministic_timeout {
+            PathfinderTimeout::Nodes(if attempt_number == 0 {
+                1_000_000
+            } else {
+                5_000_000
+            })
+        } else {
+            PathfinderTimeout::Time(Duration::from_secs(if attempt_number == 0 { 1 } else { 5 }))
+        };
+
+        let astar::Path { movements, partial } = a_star(
+            opts.start,
+            |n| opts.goal.heuristic(n),
+            successors,
+            |n| opts.goal.success(n),
+            timeout,
+        );
+        let end_time = Instant::now();
+        debug!("partial: {partial:?}");
+        let duration = end_time - start_time;
+        if partial {
+            if movements.is_empty() {
+                info!("Pathfinder took {duration:?} (empty path)");
+            } else {
+                info!("Pathfinder took {duration:?} (incomplete path)");
+            }
+            // wait a bit so it's not a busy loop
+            std::thread::sleep(Duration::from_millis(100));
+        } else {
+            info!("Pathfinder took {duration:?}");
+        }
+
+        debug!("Path:");
+        for movement in &movements {
+            debug!("  {}", movement.target);
+        }
+
+        path = movements.into_iter().collect::<VecDeque<_>>();
+        is_partial = partial;
+
+        let goto_id_now = opts.goto_id_atomic.load(atomic::Ordering::SeqCst);
+        if goto_id != goto_id_now {
+            // we must've done another goto while calculating this path, so throw it away
+            warn!("finished calculating a path, but it's outdated");
+            return None;
+        }
+
+        if path.is_empty() && partial {
+            if attempt_number == 0 {
+                debug!("this path is empty, retrying with a higher timeout");
+                attempt_number += 1;
+                continue 'calculate;
+            } else {
+                debug!("this path is empty, giving up");
+                break 'calculate;
+            }
+        }
+        break;
+    }
+
+    Some(PathFoundEvent {
+        entity: opts.entity,
+        start: opts.start,
+        path: Some(path),
+        is_partial,
+        successors_fn: opts.successors_fn,
+        allow_mining: opts.allow_mining,
+    })
+}
+
 // poll the tasks and send the PathFoundEvent if they're done
 pub fn handle_tasks(
     mut commands: Commands,
@@ -529,6 +594,7 @@ pub fn check_node_reached(
                     executing_path.path = executing_path.path.split_off(i + 1);
                     executing_path.last_reached_node = movement.target;
                     executing_path.last_node_reached_at = Instant::now();
+                    trace!("reached node {:?}", movement.target);
 
                     if let Some(new_path) = executing_path.queued_path.take() {
                         debug!(
@@ -640,6 +706,7 @@ pub fn recalculate_near_end_of_path(
                     goal,
                     successors_fn,
                     allow_mining: pathfinder.allow_mining,
+                    deterministic_timeout: pathfinder.deterministic_timeout,
                 });
                 pathfinder.is_calculating = true;
 
@@ -713,7 +780,11 @@ pub fn tick_execute_path(
                 start_mining_events: &mut start_mining_events,
                 set_selected_hotbar_slot_events: &mut set_selected_hotbar_slot_events,
             };
-            trace!("executing move");
+            trace!(
+                "executing move, position: {}, last_reached_node: {}",
+                **position,
+                executing_path.last_reached_node
+            );
             (movement.data.execute)(ctx);
         }
     }
@@ -732,6 +803,7 @@ pub fn recalculate_if_has_goal_but_no_path(
                     goal,
                     successors_fn: pathfinder.successors_fn.unwrap(),
                     allow_mining: pathfinder.allow_mining,
+                    deterministic_timeout: pathfinder.deterministic_timeout,
                 });
                 pathfinder.is_calculating = true;
             }
@@ -880,6 +952,7 @@ mod tests {
             goal: Arc::new(BlockPosGoal(end_pos)),
             successors_fn: moves::default_move,
             allow_mining: false,
+            deterministic_timeout: false,
         });
         simulation
     }