Is there a more efficient way to do this?

Help and Feedback Scripting Support
#1

So I’m working on a weapon system, and I have this sword. Right now, whenever a player swings (they can do this every 0.25 seconds), the player will fire a remote event. In the server code, I currently loop through the character’s children, to see which tool is equipped, then loop through every player in the game and check if they’re in range, and if so, damage.

However, I’m wondering if there is a more efficient way to do this because I’m aiming to have 100 players in the server, and I’d imagine looping through all the players to check the distance every time a player swings will get very costly.

I’d appreciate any input, thanks! :smiley:

Here is the code

Attack.OnServerEvent:Connect(function(player)
	local Character = game.Workspace[player.Name]
	local PlayerContents = Character:GetChildren()
	for i = 1,#PlayerContents,1 do
		if PlayerContents[i].Name == 'MiniSword' then
			for _, players in pairs(game.Players:GetPlayers()) do
  			local Position = ((players.Character.HumanoidRootPart.Position-player.Character.HumanoidRootPart.Position).magnitude)
			if Position < 4 then
				if players.Name ~= player.Name then
				players.Character.Humanoid:TakeDamage(15)
				end
			end
		end

		end
	end
end)
1 Like
#2

you can just attach the touched event to part of sword that will damaged the player. like this

local swordTip = --insert the part of sword that should damage the player
swordTip.Touched:Connect(function(hit)
    if hit.Parent:FindFirstChild("Humanoid") then
        local character = hit.Parent
        character.Humanoid:TakeDamage(15)
    end
end)

this way you can just run this code on server and do the animations,if any, on the client.
also, i would recommend adding a debounce to this script to prevent any spamming of the weapon like this:

local swordTip = --insert the part of sword that should damage the player
local COOL_DOWN_TIME = 3 -- this is how long in seconds the player should have to wait before using the sword to attack a player again
local debounce = false
swordTip.Touched:Connect(function(hit)
    if hit.Parent:FindFirstChild("Humanoid") and not debounce then
        debounce = true
        local character = hit.Parent
        character.Humanoid:TakeDamage(15)
        wait(COOL_DOWN_TIME)
        debounce = false
    end
end)

In addition, i would advise against the current method you are using because players can damage players that they should not be able to. For example, one player may not be facing the direction of the other player, so therefore they should not be able to damage the player because the sword might not actually hit them , but there difference in magnitude might be less then 4 causing the player to get damaged anyway (i know its a bit confusing but get what im saying)

2 Likes
#3

There isn’t much you can do to speed it up. You could implement a spatial partitioning system, but it’s probably not worth it.

But you can make your code a bit better by using FindFirstChild instead of looping.

Attack.OnServerEvent:Connect(function(player)
	local Character = player.Character
	if Character:FindFirstChild('MiniSword') then
		for _, players in pairs(game.Players:GetPlayers()) do
  			local Position = ((players.Character.HumanoidRootPart.Position-player.Character.HumanoidRootPart.Position).magnitude)
			if Position < 4 then
				if players.Name ~= player.Name then
					players.Character.Humanoid:TakeDamage(15)
				end
			end
		end
	end
end)
1 Like
#4

Right off the bat, you can instead send the tool instance as a variable to the remote function. For example, a localscript in the sword could parse itself to the RemoteEvent like so:

local tool = script.Parent
local attackEvent = game:GetService("ReplicatedStorage"):WaitForChild("Attack")

tool.Activated:Connect(function()
   attackEvent:FireServer(tool)
end)

And your server script would look something like this:

game:GetService("ReplicatedStorage"):WaitForChild("Attack").OnServerEvent:Connect(function(player, tool)
    if tool.Name == "MiniSword" then
        -- Code goes here
    end
end)

This removes one of the for loops in your script already, making it cheaper by a power of n.

This seems like an incredibly inefficient way to do it, and there are a number of alternatives.

You could create a Region3 around the player when they activate the weapon, use workspace:FindPartsInRegion3(yourRegion) to find all the BaseParts in that Region3 and then check if their parents are Characters, however this would limit you to a square attack area.

You could alternatively use a hitbox - simply create a Part of the desired size and shape and keep it in either ReplicatedStorage or ServerStorage, and then when the weapon is activated :Clone() it and center it on the player. Then, use hitbox:GetTouchingParts() and again check if their parents are Characters.

I think this latter option is more expensive, however it isn’t unreasonably so and allows finer control over the hitbox size and shape. Some example code:

Weapon:

local RS = game:GetService("ReplicatedStorage")
local attackEvent = RS:WaitForChild("Attack")

local tool = script.Parent
tool.Activated:Connect(function()
    attackEvent:FireServer(tool)
end)

Server script:

local RS = game:GetService("ReplicatedStorage")
local Players = game:GetService("Players")
local attackEvent = RS:WaitForChild("Attack")

local weaponHitbox = RS:WaitForChild("MiniSwordHitbox")
local weaponName = "MiniSword"

attackEvent.OnServerEvent:Connect(function(player, tool)
    if tool.Name == weaponName then
        local hitbox = weaponHitbox:Clone()
        hitbox.CFrame = player.character.CFrame
        hitbox.Parent = workspace

        local parts = hitbox:GetTouchingParts()
        for _,v in ipairs(parts) do
            local targetPlayer = Players:FindFirstChild(v.Parent.Name)
            if targetPlayer then
                -- Damage code or whatever
                print("Killing "..targetPlayer.Name.."!")
            end
        end
    end
end)

I wrote this up in the post editor at 1am so it might not work perfectly, but give it a go and PM me if you need help debugging it.

From a design perspective, this code is relatively secure, however the feedback to the client is pretty awful, as they don’t know if they’ve hit until the server does, introducing some real potential latency issues - this would be resolved by having both the client and the server doing the check, but only actually damaging the other player once the server has confirmed there is a hit.

4 Likes
#5

Thank you @RomeoEDD and @tlr22 for your help. I will definitely take your feedback into consideration, especially the FindFirstChild. I was a bit reluctant to use a touched event, for probably some unfounded reasons.

Thank you for the detailed response @plasmascreen, I’ll definitely either use the hitbox or region3 solution, though most likely the region3 option as it seems more elegant. I was a bit worried with setting the tool as a parameter, because I figured an exploiter could change that to a different, better sword, and get longer range/better damage, which is why I opted for checking the tool on the server with a loop. I’ll definitely use FindFirstChild instead though.

#6

This is a very valid concern, and one I failed to consider. However, after some testing of my own it seems that if you parse the Tool as a parameter, it is effectively a pointer to the Tool on the server.
Therefore any changes made to the Tool on the client are still unrecognised by the server.

You can test this yourself too.
Create a Tool in StarterPack and name it “Original”, insert a LocalScript and paste in the following code:

local tool = script.Parent

local p = Instance.new("Part")
p.Name = "Handle"
p.Size = Vector3.new(1, 1, 1)
p.Parent = tool

tool.Activated:Connect(function()
	script.Parent.Name = tostring(math.random(1, 100))
	game:GetService("ReplicatedStorage"):WaitForChild("RemoteEvent"):FireServer(tool)
	print(tool.Name)
end)

Create a RemoteEvent in ReplicatedStorage - do not rename it.
In the ServerScriptService, create a script and paste in the following code:

game:GetService("ReplicatedStorage"):WaitForChild("RemoteEvent").OnServerEvent:Connect(function(player, tool)
	print(tool.Name)
end)

Now load in and click with the tool - you’ll notice the name of the Tool changes on every click, but is not effected on the server!

1 Like
#7

It seems more elegant because you’re not using physical parts, but I don’t think I would recommend it over the other options due to the restrictions it places on you.
:GetTouchingParts() is a very consistent function, and shouldn’t let you down.

1 Like
#8

(Sorry for the late response)
Thanks so much, you’ve been a lot of help! However, I’m wondering, do you know if there is any way to combat the latency. Currently, because of the inherent delay from the remote event, when the player is moving the hitbox end up spawning behind them. Do you know any ways to combat this?

Attack.OnServerEvent:Connect(function(player,tool)
	local Character = game.Workspace[player.Name]
	if tool.Name == 'MiniSword' then
		local Part = Instance.new('Part')
		Part.Size = Vector3.new(4,5,2)
		Part.Anchored = true
		Part.CanCollide = false
		Part.Transparency = 1
		Part.CFrame = Character.HumanoidRootPart.CFrame * CFrame.new(0,0,-2)
		Part.Parent = game.Workspace
		delay(1,function()
			Part:Destroy()
		end)
		Part.Touched:Connect(function()end)
		local Parts = Part:GetTouchingParts()
		for _,v in ipairs(Parts) do
			if v.Parent:FindFirstChild('Humanoid') then
				if v.Parent.Name ~= player.Name then
					v.Parent.Humanoid:TakeDamage(10)
					break
				end
			end
    	end
	end
end)
1 Like