Switch to a much more stable method for fastUseItem

Turns out there is a forge event right next to the spot in vanilla that makes you go slowly while holding a shield. Wonder how I missed it before, I as read that code to figure out how much to make you faster by. Probably caused by having a solution in mind and ignoring anything unrelated to that solution
As part of this, simplify the modifier clearing logic. Will be even more simple if the forge hook gets merged, I should consider implementing a modifier hook for that ahead of time and just call it in the 6 different locations for now

src/main/java/slimeknights/tconstruct/library/tools/helper/ModifierUtil.java

@@ -12,10 +12,8 @@
 import net.minecraft.world.entity.EquipmentSlot;
 import net.minecraft.world.entity.EquipmentSlot.Type;
 import net.minecraft.world.entity.LivingEntity;
-import net.minecraft.world.entity.ai.attributes.AttributeInstance;
 import net.minecraft.world.entity.ai.attributes.AttributeModifier;
 import net.minecraft.world.entity.ai.attributes.AttributeModifier.Operation;
-import net.minecraft.world.entity.ai.attributes.Attributes;
 import net.minecraft.world.entity.item.ItemEntity;
 import net.minecraft.world.entity.player.Player;
 import net.minecraft.world.item.ItemStack;
@@ -33,7 +31,6 @@
 import slimeknights.tconstruct.library.tools.capability.TinkerDataKeys;
 import slimeknights.tconstruct.library.tools.context.EquipmentChangeContext;
 import slimeknights.tconstruct.library.tools.context.ToolHarvestContext;
-import slimeknights.tconstruct.library.tools.item.IModifiable;
 import slimeknights.tconstruct.library.tools.nbt.IModDataView;
 import slimeknights.tconstruct.library.tools.nbt.IToolStackView;
 import slimeknights.tconstruct.library.tools.nbt.ModifierNBT;
@@ -51,9 +48,6 @@ public final class ModifierUtil {
   /** Vanilla enchantments tag */
   public static final String TAG_ENCHANTMENTS = "Enchantments";
 
-  /** Attribute to mostly cancel out use item slowdown. Use item puts you at 20% speed, so 1+3 leaves you at 80% speed */
-  private static final AttributeModifier FAST_USE_ITEM = new AttributeModifier("tconstruct.fast_use_item", 3f, Operation.MULTIPLY_TOTAL);
-
   /** Key for marking a modifier in use */
   private static final ResourceLocation ACTIVE_MODIFIER = TConstruct.getResource("active_modifier");
 
@@ -362,33 +356,22 @@ public static ModifierEntry getActiveModifier(IToolStackView tool) {
     return null;
   }
 
-  /** Called to apply fast using item if we have that ability */
-  public static void checkFastUsingItem(IToolStackView tool, LivingEntity living) {
-    if (living.isUsingItem() && tool.getVolatileData().getBoolean(IModifiable.FAST_USE_ITEM)) {
-      AttributeInstance instance = living.getAttribute(Attributes.MOVEMENT_SPEED);
-      if (instance != null && !instance.hasModifier(FAST_USE_ITEM)) {
-        instance.addTransientModifier(FAST_USE_ITEM);
-      }
-      if (living.level.isClientSide) {
-        living.getCapability(TinkerDataCapability.CAPABILITY).ifPresent(data -> data.computeIfAbsent(TinkerDataKeys.SCALED_FOV_MODIFIER).set(IModifiable.FAST_USE_ITEM, 0.4f));
-      }
-    }
-  }
+  /** @deprecated No longer needed, will be removed in 1.19. */
+  @Deprecated
+  public static void checkFastUsingItem(IToolStackView tool, LivingEntity living) {}
 
-  /** Called to clear any data modifiers set when usage starts, does not clear active modifier, use {@link #finishUsingItem(LivingEntity, IToolStackView)} for tools supporting right click modifiers */
-  public static void finishUsingItem(LivingEntity living) {
-    AttributeInstance instance = living.getAttribute(Attributes.MOVEMENT_SPEED);
-    if (instance != null) {
-      instance.removeModifier(FAST_USE_ITEM);
-    }
-    if (living.level.isClientSide) {
-      living.getCapability(TinkerDataCapability.CAPABILITY).ifPresent(data -> data.computeIfAbsent(TinkerDataKeys.SCALED_FOV_MODIFIER).remove(IModifiable.FAST_USE_ITEM));
-    }
+  /** @deprecated No longer needed. Use {@link #finishUsingItem(IToolStackView)} when you stop using a modifier */
+  @Deprecated
+  public static void finishUsingItem(LivingEntity living) {}
+
+  /** @deprecated No longer needed. Use {@link #finishUsingItem(IToolStackView)} when you stop using a modifier */
+  @Deprecated
+  public static void finishUsingItem(LivingEntity living, IToolStackView tool) {
+    finishUsingItem(tool);
   }
 
   /** Called to clear any data modifiers set when usage starts */
-  public static void finishUsingItem(LivingEntity living, IToolStackView tool) {
+  public static void finishUsingItem(IToolStackView tool) {
     tool.getPersistentData().remove(ACTIVE_MODIFIER);
-    finishUsingItem(living);
   }
 }

src/main/java/slimeknights/tconstruct/library/tools/item/ModifiableItem.java

@@ -361,11 +361,21 @@ public void onUseTick(Level pLevel, LivingEntity entityLiving, ItemStack stack,
     }
   }
 
+  @Override
+  public boolean canContinueUsing(ItemStack oldStack, ItemStack newStack) {
+    if (super.canContinueUsing(oldStack, newStack)) {
+      if (oldStack != newStack) {
+        ModifierUtil.finishUsingItem(ToolStack.from(oldStack));
+      }
+    }
+    return super.canContinueUsing(oldStack, newStack);
+  }
+
   @Override
   public ItemStack finishUsingItem(ItemStack stack, Level worldIn, LivingEntity entityLiving) {
     ToolStack tool = ToolStack.from(stack);
     ModifierEntry activeModifier = ModifierUtil.getActiveModifier(tool);
-    ModifierUtil.finishUsingItem(entityLiving, tool);
+    ModifierUtil.finishUsingItem(tool);
     if (activeModifier != null) {
       activeModifier.getHook(TinkerHooks.CHARGEABLE_INTERACT).onFinishUsing(tool, activeModifier, entityLiving);
       return stack;
@@ -383,7 +393,7 @@ public ItemStack finishUsingItem(ItemStack stack, Level worldIn, LivingEntity en
   public void releaseUsing(ItemStack stack, Level worldIn, LivingEntity entityLiving, int timeLeft) {
     ToolStack tool = ToolStack.from(stack);
     ModifierEntry activeModifier = ModifierUtil.getActiveModifier(tool);
-    ModifierUtil.finishUsingItem(entityLiving, tool);
+    ModifierUtil.finishUsingItem(tool);
     if (activeModifier != null) {
       activeModifier.getHook(TinkerHooks.CHARGEABLE_INTERACT).onStoppedUsing(tool, activeModifier, entityLiving, timeLeft);
       return;

src/main/java/slimeknights/tconstruct/tools/ToolClientEvents.java

@@ -5,14 +5,18 @@
 import net.minecraft.client.Minecraft;
 import net.minecraft.client.color.item.ItemColors;
 import net.minecraft.client.gui.screens.MenuScreens;
+import net.minecraft.client.player.Input;
 import net.minecraft.client.renderer.entity.ItemEntityRenderer;
 import net.minecraft.server.packs.PackType;
 import net.minecraft.world.entity.EquipmentSlot;
+import net.minecraft.world.entity.player.Player;
+import net.minecraft.world.item.ItemStack;
 import net.minecraftforge.api.distmarker.Dist;
 import net.minecraftforge.client.ClientRegistry;
 import net.minecraftforge.client.event.ColorHandlerEvent;
 import net.minecraftforge.client.event.EntityRenderersEvent;
 import net.minecraftforge.client.event.ModelRegistryEvent;
+import net.minecraftforge.client.event.MovementInputUpdateEvent;
 import net.minecraftforge.client.event.ParticleFactoryRegisterEvent;
 import net.minecraftforge.client.event.RegisterClientReloadListenersEvent;
 import net.minecraftforge.client.model.ModelLoaderRegistry;
@@ -31,6 +35,7 @@
 import slimeknights.mantle.data.ISafeManagerReloadListener;
 import slimeknights.tconstruct.TConstruct;
 import slimeknights.tconstruct.common.ClientEventBase;
+import slimeknights.tconstruct.common.TinkerTags;
 import slimeknights.tconstruct.common.network.TinkerNetwork;
 import slimeknights.tconstruct.library.client.materials.MaterialTooltipCache;
 import slimeknights.tconstruct.library.client.model.DynamicTextureLoader;
@@ -47,6 +52,8 @@
 import slimeknights.tconstruct.library.client.modifiers.TankModifierModel;
 import slimeknights.tconstruct.library.modifiers.ModifierId;
 import slimeknights.tconstruct.library.modifiers.ModifierManager;
+import slimeknights.tconstruct.library.tools.helper.ModifierUtil;
+import slimeknights.tconstruct.library.tools.item.IModifiable;
 import slimeknights.tconstruct.library.utils.HarvestTiers;
 import slimeknights.tconstruct.library.utils.Util;
 import slimeknights.tconstruct.tools.client.ArmorModelHelper;
@@ -116,6 +123,7 @@ static void registerRenderers(EntityRenderersEvent.RegisterRenderers event) {
   @SubscribeEvent
   static void clientSetupEvent(FMLClientSetupEvent event) {
     MinecraftForge.EVENT_BUS.addListener(ToolClientEvents::handleKeyBindings);
+    MinecraftForge.EVENT_BUS.addListener(ToolClientEvents::handleInput);
     ArmorModelHelper.init();
 
     // keybinds
@@ -253,4 +261,17 @@ private static void handleKeyBindings(PlayerTickEvent event) {
       wasLeggingsInteracting = isLeggingsInteract;
     }
   }
+
+  private static void handleInput(MovementInputUpdateEvent event) {
+    Player player = event.getPlayer();
+    if (player.isUsingItem() && !player.isPassenger()) {
+      ItemStack using = player.getUseItem();
+      if (using.is(TinkerTags.Items.MODIFIABLE) && ModifierUtil.checkVolatileFlag(using, IModifiable.FAST_USE_ITEM)) {
+        Input input = event.getInput();
+        // mostly cancel out future 0.2x, want 80% move speed instead of 20%
+        input.leftImpulse *= 4;
+        input.forwardImpulse *= 4;
+      }
+    }
+  }
 }

src/main/java/slimeknights/tconstruct/tools/item/ModifiableBowItem.java

@@ -82,7 +82,6 @@ public InteractionResultHolder<ItemStack> use(Level level, Player player, Intera
       return InteractionResultHolder.fail(bow);
     }
     player.startUsingItem(hand);
-    ModifierUtil.checkFastUsingItem(tool, player);
     // property for scope, release, and item model
     float drawspeed = ConditionalStatModifierHook.getModifiedStat(tool, player, ToolStats.DRAW_SPEED) / 20f;
     player.getCapability(TinkerDataCapability.CAPABILITY).ifPresent(data -> data.put(DRAWSPEED, drawspeed));
@@ -94,19 +93,12 @@ public InteractionResultHolder<ItemStack> use(Level level, Player player, Intera
     return InteractionResultHolder.consume(bow);
   }
 
-  @Override
-  public ItemStack finishUsingItem(ItemStack stack, Level level, LivingEntity living) {
-    ModifierUtil.finishUsingItem(living);
-    return super.finishUsingItem(stack, level, living);
-  }
-
   @Override
   public void releaseUsing(ItemStack bow, Level level, LivingEntity living, int timeLeft) {
     // clear zoom regardless, does not matter if the tool broke, we should not be zooming
     if (level.isClientSide) {
       living.getCapability(TinkerDataCapability.CAPABILITY).ifPresent(data -> data.computeIfAbsent(TinkerDataKeys.FOV_MODIFIER).remove(SCOPE));
     }
-    ModifierUtil.finishUsingItem(living);
 
     // need player
     if (!(living instanceof Player player)) {

src/main/java/slimeknights/tconstruct/tools/item/ModifiableCrossbowItem.java

@@ -128,7 +128,6 @@ public InteractionResultHolder<ItemStack> use(Level level, Player player, Intera
       // if we have ammo, start charging
       if (BowAmmoModifierHook.hasAmmo(tool, bow, player, getSupportedHeldProjectiles())) {
         player.startUsingItem(hand);
-        ModifierUtil.checkFastUsingItem(tool, player);
         float drawspeed = ConditionalStatModifierHook.getModifiedStat(tool, player, ToolStats.DRAW_SPEED) / 20f;
         player.getCapability(TinkerDataCapability.CAPABILITY).ifPresent(data -> data.put(DRAWSPEED, drawspeed));
         // we want an int version to make sounds more precise
@@ -243,15 +242,8 @@ public static void fireCrossbow(IToolStackView tool, Player player, InteractionH
     }
   }
 
-  @Override
-  public ItemStack finishUsingItem(ItemStack stack, Level level, LivingEntity living) {
-    ModifierUtil.finishUsingItem(living);
-    return super.finishUsingItem(stack, level, living);
-  }
-
   @Override
   public void releaseUsing(ItemStack bow, Level level, LivingEntity living, int chargeRemaining) {
-    ModifierUtil.finishUsingItem(living);
     if (!(living instanceof Player player)) {
       return;
     }

src/main/java/slimeknights/tconstruct/tools/logic/EquipmentChangeWatcher.java

@@ -99,10 +99,10 @@ private static void runModifierHooks(LivingEntity entity, EquipmentSlot changedS
       for (ModifierEntry entry : tool.getModifierList()) {
         entry.getModifier().onUnequip(tool, entry.getLevel(), context);
       }
-      // if you scrolled away, we really don't know whether you stopped using an item or not, luckily everything in this method can be safely run multiple times
+      // only path that should bring you here that did not already call the modifier method is when your shield breaks. ideally we will switch to a forge onStoppedUsing method instead
       // TODO 1.19: consider simplier check, such as the tool having the active modifier tag set. Will need to do a bit of work for bows which don't set modifiers though
       if (!entity.isUsingItem() || entity.getItemBySlot(changedSlot) != entity.getUseItem()) {
-        ModifierUtil.finishUsingItem(entity, tool);
+        ModifierUtil.finishUsingItem(tool);
       }
     }
 

src/main/java/slimeknights/tconstruct/tools/modifiers/ability/interaction/BlockingModifier.java

@@ -25,7 +25,6 @@ protected void registerHooks(Builder hookBuilder) {
   public InteractionResult onToolUse(IToolStackView tool, ModifierEntry modifier, Player player, InteractionHand hand, InteractionSource source) {
     if (source == InteractionSource.RIGHT_CLICK && !tool.isBroken()) {
       ModifierUtil.startUsingItem(tool, modifier.getId(), player, hand);
-      ModifierUtil.checkFastUsingItem(tool, player);
       return InteractionResult.CONSUME;
     }
     return InteractionResult.PASS;