Fix ability cooldowns being triggered when GCD results in failure (#72)

* Fix ability cooldown triggered when GCD results in failure

* Missed some &

* Update RELEASES.md

README.md

@@ -16,7 +16,7 @@ use leafwing_input_manager::prelude::*;
 
 // We're modelling https://leagueoflegends.fandom.com/wiki/Zyra/LoL
 // to show off this crate's features!
-#[derive(Actionlike, Abilitylike, Clone, Copy, Hash, PartialEq, Eq, Reflect)]
+#[derive(Actionlike, Abilitylike, Debug, Clone, Copy, Hash, PartialEq, Eq, Reflect)]
 pub enum ZyraAbility {
     GardenOfThorns,
     DeadlySpines,

RELEASES.md

@@ -1,5 +1,16 @@
 # Release Notes
 
+## Version 0.11 (unreleased)
+
+## Bugs (0.11)
+
+- Fixed `CooldownState::trigger` triggering an ability cooldown when the global cooldown was not ready.
+
+### Usability (0.11)
+
+- `AbilityState`, `CooldownState`, and `ChargeState` now take a reference to an `AbilityLike` where possible.
+- Added an `OnGlobalCooldown` error variant to indicate whether `CooldownState::ready` or `CooldownState::trigger` failed due to the global cooldown or the abilities cooldown.
+
 ## Version 0.10
 
 ## Dependencies (0.10)

examples/cooldown.rs

@@ -139,7 +139,7 @@ fn handle_add_one_ability(
     if actions.just_pressed(&CookieAbility::AddOne) {
         // Calling .trigger checks if the cooldown can be used, then triggers it if so
         // Note that this may miss other important limitations on when abilities can be used
-        if cooldowns.trigger(CookieAbility::AddOne).is_ok() {
+        if cooldowns.trigger(&CookieAbility::AddOne).is_ok() {
             // The result returned should be checked to decide how to respond
             score.0 += 1;
         }
@@ -154,7 +154,7 @@ fn handle_double_cookies_ability(
     // Checks whether the action is pressed, and if it is ready.
     // If so, triggers the ability, resetting its cooldown.
     if cookie_ability_state
-        .trigger_if_just_pressed(CookieAbility::DoubleCookies)
+        .trigger_if_just_pressed(&CookieAbility::DoubleCookies)
         .is_ok()
     {
         score.0 *= 2;
@@ -166,7 +166,7 @@ fn change_cookie_color_when_clicked(
 ) {
     let (mut color, ability_state) = query.single_mut();
     if ability_state
-        .ready_and_just_pressed(CookieAbility::AddOne)
+        .ready_and_just_pressed(&CookieAbility::AddOne)
         .is_ok()
     {
         *color = CookieBundle::COOKIE_CLICKED_COLOR.into();

src/ability_state.rs

@@ -61,7 +61,7 @@ impl<A: Abilitylike, P: Pool + Component> AbilityStateItem<'_, A, P> {
     ///
     /// Calls [`Abilitylike::ready`] on the specified action.
     #[inline]
-    pub fn ready(&self, action: A) -> Result<(), CannotUseAbility> {
+    pub fn ready(&self, action: &A) -> Result<(), CannotUseAbility> {
         let maybe_pool = self.pool.as_deref();
         let maybe_ability_costs = self.ability_costs.as_deref();
 
@@ -77,8 +77,8 @@ impl<A: Abilitylike, P: Pool + Component> AbilityStateItem<'_, A, P> {
     ///
     /// The error value for "this ability is not pressed" will be prioritized over "this ability is not ready".
     #[inline]
-    pub fn ready_and_pressed(&self, action: A) -> Result<(), CannotUseAbility> {
-        if self.action_state.pressed(&action) {
+    pub fn ready_and_pressed(&self, action: &A) -> Result<(), CannotUseAbility> {
+        if self.action_state.pressed(action) {
             self.ready(action)?;
             Ok(())
         } else {
@@ -90,8 +90,8 @@ impl<A: Abilitylike, P: Pool + Component> AbilityStateItem<'_, A, P> {
     ///
     /// The error value for "this ability is not pressed" will be prioritized over "this ability is not ready".
     #[inline]
-    pub fn ready_and_just_pressed(&self, action: A) -> Result<(), CannotUseAbility> {
-        if self.action_state.just_pressed(&action) {
+    pub fn ready_and_just_pressed(&self, action: &A) -> Result<(), CannotUseAbility> {
+        if self.action_state.just_pressed(action) {
             self.ready(action)?;
             Ok(())
         } else {
@@ -103,7 +103,7 @@ impl<A: Abilitylike, P: Pool + Component> AbilityStateItem<'_, A, P> {
     ///
     /// Calls [`Abilitylike::trigger`] on the specified action.
     #[inline]
-    pub fn trigger(&mut self, action: A) -> Result<(), CannotUseAbility> {
+    pub fn trigger(&mut self, action: &A) -> Result<(), CannotUseAbility> {
         let maybe_pool = self.pool.as_deref_mut();
         let maybe_ability_costs = self.ability_costs.as_deref();
 
@@ -119,8 +119,8 @@ impl<A: Abilitylike, P: Pool + Component> AbilityStateItem<'_, A, P> {
     ///
     /// Calls [`Abilitylike::trigger`] on the specified action.
     #[inline]
-    pub fn trigger_if_pressed(&mut self, action: A) -> Result<(), CannotUseAbility> {
-        if self.action_state.just_pressed(&action) {
+    pub fn trigger_if_pressed(&mut self, action: &A) -> Result<(), CannotUseAbility> {
+        if self.action_state.just_pressed(action) {
             let maybe_pool = self.pool.as_deref_mut();
             let maybe_ability_costs = self.ability_costs.as_deref();
 
@@ -139,8 +139,8 @@ impl<A: Abilitylike, P: Pool + Component> AbilityStateItem<'_, A, P> {
     ///
     /// Calls [`Abilitylike::trigger`] on the specified action.
     #[inline]
-    pub fn trigger_if_just_pressed(&mut self, action: A) -> Result<(), CannotUseAbility> {
-        if self.action_state.just_pressed(&action) {
+    pub fn trigger_if_just_pressed(&mut self, action: &A) -> Result<(), CannotUseAbility> {
+        if self.action_state.just_pressed(action) {
             let maybe_pool = self.pool.as_deref_mut();
             let maybe_ability_costs = self.ability_costs.as_deref();
 
@@ -161,16 +161,16 @@ impl<A: Abilitylike, P: Pool + Component> AbilityStateReadOnlyItem<'_, A, P> {
     ///
     /// Calls [`Abilitylike::ready`] on the specified action.
     #[inline]
-    pub fn ready(&self, action: A) -> Result<(), CannotUseAbility> {
+    pub fn ready(&self, action: &A) -> Result<(), CannotUseAbility> {
         action.ready(self.charges, self.cooldowns, self.pool, self.ability_costs)
     }
 
     /// Is this ability both ready and pressed?
     ///
     /// The error value for "this ability is not pressed" will be prioritized over "this ability is not ready".
     #[inline]
-    pub fn ready_and_pressed(&self, action: A) -> Result<(), CannotUseAbility> {
-        if self.action_state.pressed(&action) {
+    pub fn ready_and_pressed(&self, action: &A) -> Result<(), CannotUseAbility> {
+        if self.action_state.pressed(action) {
             self.ready(action)?;
             Ok(())
         } else {
@@ -182,8 +182,8 @@ impl<A: Abilitylike, P: Pool + Component> AbilityStateReadOnlyItem<'_, A, P> {
     ///
     /// The error value for "this ability is not pressed" will be prioritized over "this ability is not ready".
     #[inline]
-    pub fn ready_and_just_pressed(&self, action: A) -> Result<(), CannotUseAbility> {
-        if self.action_state.just_pressed(&action) {
+    pub fn ready_and_just_pressed(&self, action: &A) -> Result<(), CannotUseAbility> {
+        if self.action_state.just_pressed(action) {
             self.ready(action)?;
             Ok(())
         } else {
@@ -209,7 +209,7 @@ mod tests {
     fn ability_state_methods_are_visible_from_query() {
         fn simple_system(mut query: Query<AbilityState<TestAction>>) {
             let mut ability_state = query.single_mut();
-            let _triggered = ability_state.trigger(TestAction::Duck);
+            let _triggered = ability_state.trigger(&TestAction::Duck);
         }
 
         let mut app = App::new();

src/charges.rs

@@ -21,7 +21,7 @@ use std::collections::HashMap;
 /// use leafwing_abilities::premade_pools::mana::{Mana, ManaPool};
 /// use leafwing_input_manager::Actionlike;
 ///
-/// #[derive(Actionlike, Abilitylike, Clone, Reflect, Hash, PartialEq, Eq)]
+/// #[derive(Actionlike, Abilitylike, Debug, Clone, Reflect, Hash, PartialEq, Eq)]
 /// enum Action {
 ///     // Neither cooldowns nor charges
 ///     Move,
@@ -163,7 +163,7 @@ impl<A: Abilitylike> ChargeState<A> {
     /// use leafwing_abilities::prelude::*;
     /// use leafwing_input_manager::Actionlike;
     ///
-    /// #[derive(Actionlike, Abilitylike, Clone, Copy, PartialEq, Eq, Hash, Reflect)]
+    /// #[derive(Actionlike, Abilitylike, Debug, Clone, Copy, PartialEq, Eq, Hash, Reflect)]
     /// enum Action {
     ///     Run,
     ///     Jump,
@@ -190,7 +190,7 @@ impl<A: Abilitylike> ChargeState<A> {
     /// Returns `true` if the underlying [`Charges`] is [`None`].
     #[inline]
     #[must_use]
-    pub fn available(&self, action: A) -> bool {
+    pub fn available(&self, action: &A) -> bool {
         if let Some(charges) = self.get(action) {
             charges.available()
         } else {
@@ -205,7 +205,7 @@ impl<A: Abilitylike> ChargeState<A> {
     ///
     /// Returns `true` if the underlying [`Charges`] is [`None`].
     #[inline]
-    pub fn expend(&mut self, action: A) -> Result<(), CannotUseAbility> {
+    pub fn expend(&mut self, action: &A) -> Result<(), CannotUseAbility> {
         if let Some(charges) = self.get_mut(action) {
             charges.expend()
         } else {
@@ -218,7 +218,7 @@ impl<A: Abilitylike> ChargeState<A> {
     /// The exact effect is determined by the [`Charges`]'s [`ReplenishStrategy`].
     /// If the `action` is not associated with a [`Charges`], this has no effect.
     #[inline]
-    pub fn replenish(&mut self, action: A) {
+    pub fn replenish(&mut self, action: &A) {
         if let Some(charges) = self.get_mut(action) {
             charges.replenish();
         }
@@ -227,15 +227,15 @@ impl<A: Abilitylike> ChargeState<A> {
     /// Returns a reference to the underlying [`Charges`] for `action`, if set.
     #[inline]
     #[must_use]
-    pub fn get(&self, action: A) -> Option<&Charges> {
-        self.charges_map.get(&action)
+    pub fn get(&self, action: &A) -> Option<&Charges> {
+        self.charges_map.get(action)
     }
 
     /// Returns a mutable reference to the underlying [`Charges`] for `action`, if set.
     #[inline]
     #[must_use]
-    pub fn get_mut(&mut self, action: A) -> Option<&mut Charges> {
-        self.charges_map.get_mut(&action)
+    pub fn get_mut(&mut self, action: &A) -> Option<&mut Charges> {
+        self.charges_map.get_mut(action)
     }
 
     /// Sets the underlying [`Charges`] for `action` to the provided value.

src/cooldown.rs

@@ -43,14 +43,14 @@ use std::{collections::HashMap, fmt::Display, marker::PhantomData};
 /// action_state.press(&Action::Jump);
 ///
 /// // This will only perform a limited check; consider using the `Abilitylike::ready` method instead
-/// if action_state.just_pressed(&Action::Jump) && cooldowns.ready(Action::Jump).is_ok() {
+/// if action_state.just_pressed(&Action::Jump) && cooldowns.ready(&Action::Jump).is_ok() {
 ///    // Actually do the jumping thing here
 ///    // Remember to actually begin the cooldown if you jumped!
-///    cooldowns.trigger(Action::Jump);
+///    cooldowns.trigger(&Action::Jump);
 /// }
 ///
 /// // We just jumped, so the cooldown isn't ready yet
-/// assert_eq!(cooldowns.ready(Action::Jump), Err(CannotUseAbility::OnCooldown));
+/// assert_eq!(cooldowns.ready(&Action::Jump), Err(CannotUseAbility::OnCooldown));
 /// ```
 #[derive(Resource, Component, Debug, Clone, PartialEq, Eq, Reflect)]
 pub struct CooldownState<A: Abilitylike> {
@@ -122,7 +122,10 @@ impl<A: Abilitylike> CooldownState<A> {
     /// or this can be used on its own,
     /// reading the returned [`Result`] to determine if the ability was used.
     #[inline]
-    pub fn trigger(&mut self, action: A) -> Result<(), CannotUseAbility> {
+    pub fn trigger(&mut self, action: &A) -> Result<(), CannotUseAbility> {
+        // Call `ready` here so that we don't trigger the actions cooldown when the GCD might fail
+        self.ready(action)?;
+
         if let Some(cooldown) = self.get_mut(action) {
             cooldown.trigger()?;
         }
@@ -139,14 +142,12 @@ impl<A: Abilitylike> CooldownState<A> {
     /// This will be `Ok` if the underlying [`Cooldown::ready`] call is true,
     /// or if no cooldown is stored for this action.
     #[inline]
-    pub fn ready(&self, action: A) -> Result<(), CannotUseAbility> {
-        self.gcd_ready()?;
-
+    pub fn ready(&self, action: &A) -> Result<(), CannotUseAbility> {
         if let Some(cooldown) = self.get(action) {
-            cooldown.ready()
-        } else {
-            Ok(())
+            cooldown.ready()?;
         }
+
+        self.gcd_ready()
     }
 
     /// Has the global cooldown for actions of type `A` expired?
@@ -155,7 +156,9 @@ impl<A: Abilitylike> CooldownState<A> {
     #[inline]
     pub fn gcd_ready(&self) -> Result<(), CannotUseAbility> {
         if let Some(global_cooldown) = self.global_cooldown.as_ref() {
-            global_cooldown.ready()
+            global_cooldown
+                .ready()
+                .map_err(|_| CannotUseAbility::OnGlobalCooldown)
         } else {
             Ok(())
         }
@@ -168,7 +171,7 @@ impl<A: Abilitylike> CooldownState<A> {
     pub fn tick(&mut self, delta_time: Duration, maybe_charges: Option<&mut ChargeState<A>>) {
         if let Some(charge_state) = maybe_charges {
             for (action, cooldown) in self.cooldown_map.iter_mut() {
-                let charges = charge_state.get_mut(action.clone());
+                let charges = charge_state.get_mut(action);
                 cooldown.tick(delta_time, charges);
             }
         } else {
@@ -185,15 +188,15 @@ impl<A: Abilitylike> CooldownState<A> {
     /// The cooldown associated with the specified `action`, if any.
     #[inline]
     #[must_use]
-    pub fn get(&self, action: A) -> Option<&Cooldown> {
-        self.cooldown_map.get(&action)
+    pub fn get(&self, action: &A) -> Option<&Cooldown> {
+        self.cooldown_map.get(action)
     }
 
     /// A mutable reference to the cooldown associated with the specified `action`, if any.
     #[inline]
     #[must_use]
-    pub fn get_mut(&mut self, action: A) -> Option<&mut Cooldown> {
-        self.cooldown_map.get_mut(&action)
+    pub fn get_mut(&mut self, action: &A) -> Option<&mut Cooldown> {
+        self.cooldown_map.get_mut(action)
     }
 
     /// Set a cooldown for the specified `action`.
@@ -460,10 +463,10 @@ mod tick_tests {
         let cooldown = Cooldown::new(Duration::from_millis(1000));
         let mut cloned_cooldown = cooldown.clone();
         let _ = cloned_cooldown.trigger();
-        assert!(cooldown != cloned_cooldown);
+        assert_ne!(cooldown, cloned_cooldown);
 
         cloned_cooldown.tick(Duration::from_millis(123), None);
-        assert!(cooldown != cloned_cooldown);
+        assert_ne!(cooldown, cloned_cooldown);
     }
 
     #[test]

src/lib.rs

@@ -52,7 +52,7 @@ pub mod prelude {
 /// use leafwing_input_manager::Actionlike;
 /// use bevy::reflect::Reflect;
 ///
-/// #[derive(Actionlike, PartialEq, Eq, Clone, Copy, Hash, Reflect)]
+/// #[derive(Actionlike, Debug, PartialEq, Eq, Clone, Copy, Hash, Reflect)]
 /// enum PlayerAction {
 ///    // Movement
 ///    Up,
@@ -82,16 +82,14 @@ pub trait Abilitylike: Actionlike {
         maybe_pool: Option<&P>,
         maybe_costs: Option<&AbilityCosts<Self, P>>,
     ) -> Result<(), CannotUseAbility> {
-        let charges = charges.get(self.clone());
-        let cooldown = cooldowns.get(self.clone());
+        let charges = charges.get(self);
+        let cooldown = cooldowns.get(self);
 
         ability_ready(
             charges,
             cooldown,
             maybe_pool,
-            maybe_costs
-                .and_then(|costs| costs.get(self.clone()))
-                .copied(),
+            maybe_costs.and_then(|costs| costs.get(self)).copied(),
         )
     }
 
@@ -108,16 +106,14 @@ pub trait Abilitylike: Actionlike {
         maybe_pool: Option<&mut P>,
         maybe_costs: Option<&AbilityCosts<Self, P>>,
     ) -> Result<(), CannotUseAbility> {
-        let charges = charges.get_mut(self.clone());
-        let cooldown = cooldowns.get_mut(self.clone());
+        let charges = charges.get_mut(self);
+        let cooldown = cooldowns.get_mut(self);
 
         trigger_ability(
             charges,
             cooldown,
             maybe_pool,
-            maybe_costs
-                .and_then(|costs| costs.get(self.clone()))
-                .copied(),
+            maybe_costs.and_then(|costs| costs.get(self)).copied(),
         )
     }
 
@@ -172,6 +168,9 @@ pub enum CannotUseAbility {
     /// The [`Cooldown`] of this ability was not ready
     #[error("Cooldown not ready.")]
     OnCooldown,
+    /// The Global [`Cooldown`] for this [`CooldownState`] was not ready
+    #[error("Global cooldown not ready.")]
+    OnGlobalCooldown,
     /// Not enough resources from the corresponding [`Pool`]s are available
     #[error("Not enough resources.")]
     PoolInsufficient,