Feedback on my fight combo script

Hello everyone I have recently attempted a punching combo script. It works and all, however i am wondering how to make it more clean and if i can fix anything that may be exploited.

Code:

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Server_Check = ReplicatedStorage:WaitForChild("Server_Check")

local Animations = {
	["1"] = {
		ID = 121787988837103,
		Sound = script:WaitForChild("hit_punch_l"),
		Parts = {"LeftHand", "LeftLowerArm", "LeftUpperArm"}
	},
	["2"] = {
		ID = 108417296994768,
		Sound = script:WaitForChild("hit_kick_l"),
		Parts = {"LeftLowerLeg", "LeftUpperLeg"}
	}
}

local CanSwing = true

local Character = player.Character or player.CharacterAdded:Wait()
local Humanoid = Character:WaitForChild("Humanoid")
local Animator = Humanoid:WaitForChild("Animator")
local Tool = script.Parent
local Currentm1 = 1
local MaxCycle = 2
local CanHit = true

local function isInPart(part, Partlist)
	for i, v in pairs(Partlist) do
		if part.Name == v then
			return true
		end
	end
	return false
end

Tool.Activated:Connect(function()
	CanHit = true
	if CanSwing then
		CanSwing = false

		local animData = Animations[tostring(Currentm1)]
		if not animData then
			warn("Invalid animation cycle index: " .. tostring(Currentm1))
			return
		end

		local AnimationInstance = Instance.new("Animation")
		AnimationInstance.AnimationId = "rbxassetid://" .. animData.ID
		AnimationInstance.Parent = script

		local Loaded_Anim = Animator:LoadAnimation(AnimationInstance)
		Loaded_Anim:Play()
		animData.Sound:Play()

		for i, v in pairs(Character:GetChildren()) do
			if v:IsA("MeshPart") and isInPart(v, animData.Parts) then
				v.Touched:Connect(function(otherPart)
					local HitParent = otherPart.Parent
					if HitParent:FindFirstChildWhichIsA("Humanoid") then
						if CanHit == true then
							CanHit = false
							Server_Check:FireServer(otherPart.Parent)
							print("Hit a player")
						end
					end
				end)
			end
		end

		Loaded_Anim.Stopped:Wait()

		
		Currentm1 += 1
		if Currentm1 > MaxCycle then
			Currentm1 = 1
		end

		CanSwing = true
	end
end)
2 Likes

To make it more clean in my opinion,

  1. Don’t run animations on the server if it was meant to be ran instantly without a delay.

for this make a client side animation preloader so that the server does not have to sacrifice a lot of resources to run animation.

Client sided animations are also good for those on lower end bandwidth, internet connections.

  1. For the client to server remotes. Simply place your “important” punching system in serverscriptservice because it could be stolen by people that copies games and basically steals it

something like client fire remote to server
server makes the player punch

also dont forget animations that needs to be run on client

1 Like

Yeah the animations are being run on client.

2 Likes

i recommend using UserInputService with/or ContextActionService, since Tool.Activated can easily just be skidded by doing Tool:Activate(). you also get more freedom with your security since there is no need to handle it all in each tool and will make it easier to handle in general. that’s just my 1 cent though, looks good for now.

Sorry for late reply, using UIS how would i implement it?

depends on your approach. if you want complete control without any additional instructions then use UserInputService, which can be done like this:

local UIS = game:GetService("UserInputService")
local remote = ReplicatedStorage.TEMPLATE_EVENT

UIS.InputBegan:Connect(function(input, gameProcessed)
    if 
        input.UserInputType == Enum.UserInputType.MouseButton1 -- the main problem, you have to declare inputs manually
        and not gameProcessed 
    then
        remote:FireServer("MouseButton1 pressed!")
    end
end)

if you want full input controls (which i recommend personally since you can customize inputs from different devices) you should use ContextActionService, which can be done like this:

local CAS= game:GetService("ContextActionService")
local remote = ReplicatedStorage.TEMPLATE_EVENT

local function onMouseAction(actionName, inputState, inputObj)
-- passes 3 arguments: the action name, the current state (pressed or Begin, and released or End), and the key that was pressed
    if 
        inputState == Enum.UserInputState.Begin 
        and inputObj.UserInputType == Enum.UserInputType.MouseButton1 
    then
        remote:FireServer("MouseButton1 pressed via ContextAction!")
    end
end

CAS:BindAction(
    "MouseButton1Action", -- the action name
    onMouseAction, -- the function you want to call when the input objects are pressed
    false, -- true = creates a mobile button. if you're parenting the button somewhere use CAS:GetButton(ActionName)
    Enum.UserInputType.MouseButton1, -- arguments past this are additional input objects
    Enum.Keycode.ButtonR2 -- right trigger on controller
)

the main downside of CAS over UIS is that it’s a lot more intensive work than UIS. i recommend using UIS if you want an easy to work with system (similar to what you have here). however, take this as a grain of salt as i don’t work with either one, and i created this purely in the devforum comment formatting, so it’s not optimized and might have a lot of errors. apologies for not giving a straight answer, but unfortunately i can’t write an entire system. you can just pass what i said to claude or chatgpt for them to finish it for you

2 Likes

Okay, thank you i understand now!

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.