The main reason that the function is not being activated from the MouseButton1Up event is because of the following:
The button variable is referring to the Frame object from the initial ScreenGui found within the Tool in the screenshot:
As a result, when the LocalScript calls Ui:Clone() and adds it to the player’s PlayerGui container:
Only the ScreenGui is cloned, meaning the LocalScript is not being brought with it.
No matter what, button.Block would not refer to the TextButton named “Block” within the cloned version of the GUI, unless it specifically referenced the newly created GUI.
If I were to recommend how to restructure it to resolve that issue, I would do the following:
Move the ScreenGui into the ReplicatedStorage so there’s only one original GUI instead of having 1 within every single Sword Tool equipped by players in the game.
The “AttackControl” LocalScript would remain in the Tool object. Its code would be modified to only handle the cloning of the ScreenGui into the PlayerGui container (and the enabling / disabling of that ScreenGui):
-- Revised "AttackControl" LocalScript code
local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local player = Players.LocalPlayer
local PlayerGui = player:WaitForChild("PlayerGui")
local ScreenGui = ReplicatedStorage.ScreenGui --[[ I'd recommend renaming
the ScreenGui to something more specific in case more ScreenGuis
are added there in the future, AND so that the function activated from
Tool.Equipped will be able to properly check if the cloned version of
this ScreenGui has already been given to the player, as to
avoid giving the player multiple clones of the same ScreenGui --]]
ScreenGui.ResetOnSpawn = true -- Ensures that the cloned GUI will automatically be destroyed if the player's Character needs to respawn
local Tool = script.Parent
Tool.Equipped:Connect(function()
local guiCheck = PlayerGui:FindFirstChild(ScreenGui.Name)
if not guiCheck then
local clonedGui = ScreenGui:Clone()
clonedGui.Parent = PlayerGui
else
guiCheck.Enabled = true
end
end)
Tool.Unequipped:Connect(function()
local guiCheck = PlayerGui:FindFirstChild(ScreenGui.Name)
if guiCheck then
guiCheck.Enabled = false -- Instead of destroying it, this will essentially make the ScreenGui invisible. I'll explain why outside of this codeblock, right before the next step
end
end)
The reason I opted to toggle the Enabled property of the ScreenGui rather than to destroy it upon the Tool being unequipped is because it would not have prevented the player from repeatedly equipping / unequipping the Tool to get a brand new ScreenGui before the debounce was set back to false following the 2 second cooldown.
With this implementation, the cooldown will still be in effect because the ScreenGui and its LocalScript would not be destroyed and replaced with a brand new one. However, it would also be necessary to ensure that there is an equivalent debounce included on the server side where it’s listening for the RemoteEvent to be fired to ensure that exploiters couldn’t get around it to attack with no cooldown (but that’s a completely separate change to the combat system within the game, beyond the scope of your original question).
Lastly, create a new LocalScript directly within the ScreenGui that was placed into the ReplicatedStorage; this will handle the button press functionality when it’s cloned into the PlayerGui container.
-- Example of brand new LocalScript code for the ScreenGui
local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local player = Players.LocalPlayer
local Character = player.Character or player.CharacterAdded:Wait()
local Backpack = player:WaitForChild("Backpack")
local ScreenGui = script.Parent
local Frame = ScreenGui:WaitForChild("Frame")
local Block = Frame:WaitForChild("Block")
local Tool
local function initializeToolVariable()
local Sword = Character:WaitForChild("Sword") or Backpack:WaitForChild("Sword")
if Sword then
Tool = Sword
end
end
initializeToolVariable()
local dB = false
local currentlyAttacking = false
Block.Activated:Connect(function()
if
dB == false
and currentlyAttacking == false
and Tool ~= nil
and Tool.Parent == Character
then
dB = true
currentlyAttacking = true
-- Continue with rest of function (create Animation, fire RemoteEvent, etc.)
end
end)
Along with everything brought up above (that I actually wrote last because I only realized what the actual issue was after writing everything below), there’s a couple of things I’d like to mention / have questions about:
To keep it all in a singular function from an individual event without needing separates ones for MouseButton1Up and TouchTap, I’d recommend using either:
Activated (note that the “Active” property for the button must be enabled for this event to fire)
As both of which only fire after the button has been fully activated (as in, the button was directly pressed and then released). These also avoid some of the caveats of the first two events, being:
MouseButton1Up can fire if a player holds down left click before entering the button and then releases it while hovering over the button.
Activated tends to be the event that is recommended for supporting multiple input types, given how it returns the inputObject that was used to activate the button, which can be referenced to easily check a variety of things, including the UserInputType (such as MouseButton1, Touch, Gamepad1, etc.)
A question I have is: Where did the parameters of input, IsTyping, damage, (and) hitboxTime come from? The MouseButton1Up event only returns the X and Y coordinates on the screen where the player’s cursor was when the event fired, meaning that input and IsTyping would return numbers and both damage and hitboxTime would be nil.
None of these, aside from input, appear to be referenced in the function, either. (I also just noticed that the remote variable within the function for a RemoteEvent is also never defined anywhere else in the script.) If you intended to create those as variables specifically to use during that function, it’d be recommended to include it within the function and not as a parameter, for better clarity, instead:
button.Block.MouseButton1Up:Connect(function()
local input
local IsTyping
local damage
local hitboxTime
...
input = "block"
Lastly, unless you omitted / didn’t include the entirety of the code from the LocalScript in the original post, dB and currentlyAttacking and equipped are never assigned a value before the first conditional statement checks for its values in the function (which means that this condition would never be met so it would never run the code within the rest of the function).
My post has a lot of info to process, so if you noticed any mistakes I made and / or if you have questions about anything mentioned here, please feel free to ask!
