I’m experimenting with tools, and I made a script that detects when a player who is holding a tool clicks another player. It works, but each time one player clicks another player the print statement inside the activated function prints multiple times. The number of times it prints increases each time I click the player.
It should be printing only once per click, but it instead prints multiple times per click, with the number of prints increasing each click.
I could not figure out what I what I am doing wrong. Can I please get assistance with this?
There are no relevant errors in the output.
Tool Script (It’s a Local Script):
local Players = game:GetService("Players")
local tool = script.Parent
local function isPlayer(target)
for _, player in ipairs(Players:GetPlayers()) do
if player.Character and player.Character:FindFirstChild(target.Name) then
return true
end
end
return false
end
local function onToolActivated()
local character = tool.Parent
local player = Players:GetPlayerFromCharacter(character)
if not player then return end
player:GetMouse().TargetFilter = character
player:GetMouse().Button1Down:Connect(function()
local target = player:GetMouse().Target
if target and isPlayer(target) then
-- This should print once per click, but it prints multiple times
print(player.Name .. " clicked on another player: " .. target.Parent.Name)
end
end)
end
tool.Activated:Connect(onToolActivated)
--[[
please note this isn't as optimised
as it could be because
I'm a little bit short on time
but it is still optimised and
I hope this helps.
I have made the code nicer to look at,
at least.
]]
local players = game:GetService("Players")
local tool = script.Parent
local player = players.LocalPlayer
local character = player.Character
local mouse = player:GetMouse()
mouse.TargetFilter = character
local function isPlayer(target)
for _, player in ipairs(players:GetPlayers()) do
if player.Character and player.Character:FindFirstChild(target.Name) then
return true
end
end
return false
end
local function onToolActivated()
if tool.Parent ~= character then return nil end
mouse.Button1Down:Once(function()
local target = mouse.Target
if target and isPlayer(target) then
print(player.Name .. " clicked on another player: " .. target.Parent.Name)
end
end)
end
tool.Activated:Connect(onToolActivated)
You’re nesting connections. This line subscribes a function to Button1Down every time tool.Activated fires:
Firstly, I believe you mean to use Equipped instead of Activated since I assume most people want to listen for mouse clicks while the tool is equipped. However, using just Activated may be outright better for your case since Activated fires on mouse click only while the tool is equipped. For the sake of argument, I will continue on as if Activated doesn’t exist.
There are multiple ways to go about this issue, but to save yourself the hassle of connecting and disconnecting from Button1Down repeatedly, you can make the two events independent of one another and cause the click listener to disregard all clicks unless a condition dictated by the equip event is met:
local equipped: boolean;
tool.Equipped:Connect(function()
equipped = true;
end)
tool.Unequipped:Connect(function()
equipped = false;
end)
mouse.Button1Down:Connect(function() -- consider userinputservice instead
if not equipped then return; end -- escape if tool isn't equipped
-- functionality
end)
This is not a quick fix because :Once() will continue to subscribe to the event until the first trigger, so the player can equip the tool and establish a one-time subscription via :Once(). The connection does not void if the player unequips the tool, meaning the script still listens for mouse clicks even after unequipping.
local plrs = game:GetService'Players';
local function getPlayerFromBasePart(obj: BasePart)
return plrs:GetPlayerFromCharacter(obj:FindFirstAncestorOfClass'Model'); -- returns the player whos character possesses the basepart, otherwise returns nil (similar to false in luau boolean logic)
end
Yeah, like I said, I was a bit short on time. Thanks for expanding on the simplification and optimisation of the script. @ValiantWind you should try some of the things @anime_bb suggested, it would really help your script a lot, especially implementing GetPlayerFromCharacter to find which character contains the BasePart.
I think this should be Unequipped instead of Deactivated otherwise the player would have to re-equip the tool each time. For the sake of robust code I think you should also initialise the equipped variable at the top of the script, even though it is nil and the code would still work, it would make more sense to someone reading it for it to be false.
I’ve decided to use UserInputService instead. I got the base logic for how I would set up UserInputService for this, but how would I get the player that the tool holder clicked on?
Mouse.Target should work still for getting the mouse’s current target (the player that was clicked). You’re just detecting a mouse click from the UserInputService rather than the mouse object.
An alternative (that really isn’t efficient at all for this situation but I’ll say it anyway) is using raycasting with Camera:ScreenPointToRay.
First, let me reiterate that Activated might be the way to go depending on your use case here. With that said, UserInputService is a newer API feature than the mouse object. I haven’t been keeping up with recent developments, but I believe some mouse features are deprecated.
This function already returns the player if they exist, otherwise it returns nil. It does so by finding the first model in the parental hierarchy and passing the result into Players:GetPlayerFromCharacter(), as specified in the comments.
For anyone who comes across this thread with the same problem, here is my final script (local script):
local Players = game:GetService("Players")
local UserInputService = game:GetService("UserInputService")
local toolHolder = Players.LocalPlayer
local tool: Tool = script.Parent
local equipped = false;
local function getPlayerFromBasePart(obj: BasePart)
return Players:GetPlayerFromCharacter(obj:FindFirstAncestorOfClass("Model")); -- returns the player whos character possesses the basepart, otherwise returns nil (similar to false in luau boolean logic)
end
tool.Equipped:Connect(function()
equipped = true;
end)
tool.Unequipped:Connect(function()
equipped = false;
end)
UserInputService.InputBegan:Connect(function(input: InputObject, gameProcessed: boolean) -- consider userinputservice instead
if not equipped then return end -- escape if tool isn't equipped
if not gameProcessed and input.UserInputType == Enum.UserInputType.MouseButton1 then
local mouse = toolHolder:GetMouse()
local target = mouse.Target
if target then
local targetPlayer = getPlayerFromBasePart(target)
if targetPlayer then
print(`{toolHolder.Name} clicked on {targetPlayer.Name}`)
end
end
end
end)
Just a couple of things, I might be being a bit picky now, but all for the sake of good practice…
Try to minimise your use of nested if statements here. This statement could be if not gameProcessed and input.UserInputType == Enum.UserInputType.MouseButton1 then, it would make it nicer to look at. Functionality and how robust code is both matter.
You don’t need to define variable type here, also for the sake of readability and understanding the code you might want to consider initialising it with false.
The tool is always going to be parented to the character when in use, you don’t need to call this every time MouseButton1 is pressed. You can just do this at the top of the script:
local toolHolder = Players.LocalPlayer
--and for the character (if needed):
local character = toolHolder.Character or toolHolder.CharacterAdded:Wait()
