How can I improve my script's efficiency? sometimes it has damage delays

Hi, I am currently making a sword and it has some noticeable delays between the strikes and damage that is actually dealt. I read an article saying that FindFirstChild takes more time than some other methods, so I am wonder how I can improve this sword script. Thanks for reading my post.

This is the client script

UIS.InputBegan:Connect(function(input, gameProcessed)
	if input.UserInputType == Enum.UserInputType.MouseButton1 and not gameProcessed and debounce == true then
		
		print("left clicked!")
		print(rankValue)
		wait(0.1)
		
		if char.RightHand:FindFirstChild("Weld") and debounce == true then
			debounce =  false
			print("sword found ready for slash")
			loadedAnim:Play()
			
			dmgpart.Touched:Connect(function(hit)
				if db2 == true then
					db2 = false
					if hit.Parent:FindFirstChild("Humanoid")then

						activateSwordDmg:FireServer(hit, sideValue.Value)
	
					else
						print('hit the wrong thing')
					end
				end
			end)

			wait(0.5)
			db2 = true
			debounce = true
			wait()

		else
			print("okay m8")
		end
	end
end)

This is the server script


activateSwordDmg.OnServerEvent:Connect(function(player, hit, sideValue)

		print("not nil")
	else
		print("npc hit")
		game.Workspace[hit.Parent.Name].Humanoid:TakeDamage(10)
		print("Dmg DEALED")
	end
end)
1 Like

This might be a good old case of latency. The time it takes for the server to get the request and then do the damage, then send that information back to the client is always a bit noticeable. Although, you also have a wait(0.1) at the start that probably contributes to the feel of it.

First though, I’ve made some changes to the code to explain a different way to approach this (untested):

local ready, swinging = false, false

-- the event is only created once, as otherwise you end up with damage stacking
-- if your game features equipping/unequipping weapons, you'll want to
-- disconnect the old event and connect a new one when that happens
dmgpart.Touched:Connect(function(hit)
	if swinging and hit.Parent:FindFirstChild("Humanoid") then
		-- this is vulnerable to exploiting, see more in the explanation below
		activateSwordDmg:FireServer(hit, sideValue.Value)
	end
end)

UIS.InputBegan:Connect(function(input, gameProcessed)
	-- long conditions which just exit out can be made into a guard clause
	if ready or gameProcessed or input.UserInputType ~= Enum.UserInputType.MouseButton1 then return end

	print("left clicked")
	print(rankValue)
	-- wait(0.1) -- This wait is not necessary and should be removed.

	if not char.RightHand:FindFirstChild('Weld') then
		print('no sword equipped')

		return
	end

	-- these flags tell the event when it can hit
	ready = true
	swinging = true

	-- instead of waiting about 0.5 seconds (which is inaccurate anyways), you can use
	-- the `.Stopped` event from the AnimationTrack to `:Wait()` for it to finish
	loadedAnim:Play()
	loadedAnim.Stopped:Wait()

	swinging = false

	-- you can add some yield here to have a cooldown between swings
	ready = false
end)

Alongside the comments explaining some of the changes, I’d like to go into how events work in Roblox. There is a trailing wait() in your client script; it happens at the end of the UIS event. Events run asynchronously, so waiting inside one of them won’t actually stop code anywhere else. Trailing waits just make the thread stick around for a little longer but offer no observable functionality.

On top of that, you shouldn’t use wait() to solve problems that it seemingly does. This thread goes more in depth with that:

In most cases, there is an equivalent Roblox event that you can listen to via Connection:Wait() (different from wait()) which more efficiently does the trick. I assumed in your example that you wanted to wait until after the animation was over, but this might not be the case. Maybe you want it to wait until a certain keyframe is reached, for example. There is also an event for that, and it’s always worth checking the DevHub if it can make the code a little nicer.

Next up is the event and debouncing. I changed the code as explained in the comment. The problem is that once you :Connect() an event, it’s not disconnected until you explicitly call :Disconnect(), or the object it was connected to is :Destroy()ed. As such, every time the player swings a new connection is made, and more damage is tacked on to the hit.

The swinging debounce controls when the sword can do damage, and the ready controls when the sword is ready to be swung again. This avoids multiple connections.

Lastly, I want to address the RemoteEvent. You are giving a bit too much authority to the player. The server code should also keep track of its own debounce regarding the player’s last hit. It shouldn’t be set by the client, as this is exploitable. You’ll also want to make sure that hit (or hit.Parent) is actually an instance (via typeof()), as people can pass tables that look like instances. And, of course, you’ll want some sort of distance check. Simply testing if the player’s character is within some small radius from their target (with some leniency to compensate for lag) will mitigate the issue.

4 Likes

Thanks for the detailed reply! I will get back to testing on studios later at night. I will get back to you later!

Hi, I just messed around with more testing and the code you provided seems to hit the humanoid more than once. I am not super advanced in scripting and I would need more help. Could you please help me fix this behavior? Thanks!

My bad. Toggle the swinging to false within the hit event.

2 Likes

Thanks! Now it works now! As you mentioned, I did give the player a lot of power how can I also do debounce on the server side? Thanks again!