How do i improve these scripts? its kinda messy and buggy

Help and Feedback Code Review
#1

I’m making a game inspired by Battle cats. I’m trying to rework the entity script by making the NPC into client sided, adding the part for the moving.
Though i did make the system for that and it works, i’ve reworking some of my script with the system addition. However it has a lot of issues, such as lag (prob due to animation) and bugs to the entity scripts.


To make it kinda not confusing, you can see here the blue and red NPC has lotsa issues, and for longer the game gets laggy.

Now i need to make these all to be fixed, can u help me? thanks

Script 1 -- EnemyEntityScript (same with the blue one but with different variable and bugged even though its almost the same script) 
local npc = script.Parent
local stats = require(npc.Stats)
local NPCSystem = require(game.ReplicatedStorage.NPCSystem)
local EntityModule = require(game.ReplicatedStorage.EntityModule)
local curHP = npc:WaitForChild('Health')

local baseunit = game.Workspace.Enviroment["Base-unit"]
local baseenemy = game.Workspace.Enviroment["Base-Enemy"]

local hitbox = Instance.new("Part")
local weld = Instance.new("WeldConstraint")

local ClientModelEvent = game.ReplicatedStorage.ClientModelEvent

local RunService = game:GetService("RunService")

weld.Part0 = npc
weld.Part1 = hitbox; weld.Parent = hitbox

hitbox.Name = 'Hitbox'
hitbox.Anchored = false; hitbox.CanCollide = true
hitbox.Massless = true
hitbox.CanCollide = false
hitbox.Transparency = 0.5
hitbox.Size = Vector3.new(15, 5.8, 4.5)
hitbox.CFrame = (npc.CFrame + Vector3.new(-hitbox.Size.Z * 0.6, 0, 0))
hitbox.Parent = npc

local animation = {
	Idle = {'Idle',false,0.5},
	Run = {'Run',false,0.5},
	Attack = {'Attack',false,0.5}
}

local animstring = 'Run'

function updateAnim(string)
	for i,v in pairs(animation) do
		if i ~= string then
			ClientModelEvent:FireAllClients('stopAnim',stats.ClientModelName,{Parent = workspace.Enemy,animName = animation[i][1]})
			animation[i][2] = false
		end
	end
	ClientModelEvent:FireAllClients('playAnim',stats.ClientModelName,{Parent = workspace.Enemy,animName = animation[string][1],fadetime = animation[string][3],force = false})
	animation[string][2] = true
end

debounce = {
	walking = true,
	attacking = false,
	atkended = false,
	isblockingside = false
}


while task.wait() do
	updateAnim(animstring)
	if curHP.Value <= 0 then
		--onDeath()
		--task.wait(2)
		ClientModelEvent:FireAllClients('delete',stats.ClientModelName)
		npc.Parent = nil
		script.Enabled = false
	end

	if debounce.walking and not NPCSystem.IsMoving(npc) then
		NPCSystem.MoveTo(npc,baseunit.Position + Vector3.new(8,0,stats.zPos))
		animstring = 'Run'
	elseif animstring == 'Idle' or animstring == 'Attack' then
		NPCSystem.StopMoving(npc)
	end

	local basepos = baseunit.Position + Vector3.new(9,0,stats.zPos)

	local unitfolder = game.Workspace.Unit
	for i,entity in pairs(unitfolder:GetChildren()) do
		if entity:IsA('Part') then
			local relativePos = hitbox.CFrame:PointToObjectSpace(entity.Position)
			local entityCurHP = entity:WaitForChild('Health')
			if debounce.attacking == false and entityCurHP.Value > 0 then
				if math.abs(relativePos.X) <= npc.Hitbox.Size.X * 1 * 0.6 and math.abs(relativePos.Z) <= npc.Hitbox.Size.Z * 1 * 0.6 then
					local atktimer = stats.atktimer
					local idletimer = stats.idletimer
					local length = stats.lengthofAtk
					animstring = 'Attack'
					debounce.walking = false
					task.delay(atktimer,function()
						EntityModule.onHit(true,npc,hitbox,entityCurHP,stats)
					end)
					task.delay(length,function()
						if curHP.Value > 0 and math.abs(relativePos.X) <= npc.Hitbox.Size.X * 1 * 0.6 and math.abs(relativePos.Z) <= npc.Hitbox.Size.Z * 1 * 0.6 then
							animstring = 'Idle'
							debounce.walking = false
						else
							debounce.walking = true
						end
						debounce.atkended = true
						task.delay(idletimer,function()
							debounce.atkended = false
							debounce.attacking = false
						end)
					end)
					debounce.attacking = true
					--end)
				end
			elseif entityCurHP.Value > 0 and (not debounce.attacking or debounce.atkended) and math.abs(relativePos.X) <= npc.Hitbox.Size.X * 1 * 0.6 and math.abs(relativePos.Z) <= npc.Hitbox.Size.Z * 1 * 0.6 then
				animstring = 'Idle'
				debounce.attacking = true
				debounce.walking = false
			elseif debounce.atkended then
				debounce.attacking = true
				debounce.walking = true
				--elseif debounce.attacking == true then
			end	
		end
	end
end
Script 2 -- The EntityModule ( some is a bit dumb ) 
math.randomseed(tick())

local module = {}
module.CurrentUnit = {server = {},client = {}}
module.CurrentEnemy = {server = {},client = {}}
local scriptType = {
	default = {unit = game.ReplicatedStorage.UnitEntityScript,enemy = game.ReplicatedStorage.EnemyEntityScript}
}

local globalVar = require(game.ReplicatedStorage.GlobalVar)
local Upgrades = require(game.ReplicatedStorage.UnitUpgradeData)
local Util = require(game.ReplicatedStorage.Utilities)
local NPCSystem = require(game.ReplicatedStorage.NPCSystem)

local ClientModelEvent = game.ReplicatedStorage.ClientModelEvent
local StoreEvent = game.ReplicatedStorage.StoreRunninSystem

function module.SpawnClientModel(isEnemy,randomname,id,part,offset)
	local codename = Util.getValueByIdinMap(isEnemy and globalVar.EnemyID or globalVar.UnitID,'id',id,'codename')

	local folder = isEnemy and game.ReplicatedStorage.EnemyFolder or game.ReplicatedStorage.UnitFolder
	local clone = folder:FindFirstChild(codename):Clone()
	local randomZ = Random.new():NextNumber(-2,2)
	local pos = (isEnemy and game.Workspace.Enviroment["Base-Enemy"].Position or game.Workspace.Enviroment["Base-unit"].Position) + Vector3.new(isEnemy and -6 or 6,0, randomZ)
	clone:SetPrimaryPartCFrame(CFrame.new(pos,pos + Vector3.new(-5,0,0)))
	clone.Parent = isEnemy and workspace.Enemy or workspace.Unit
	local entitype = isEnemy and '_ENEMYMODEL_' or '_UNITMODEL_'
	--clone.Name = clone.Name..entitype..math.random(-999999999,999999999)
	clone.Name = randomname
	NPCSystem.ClientModelConnect(clone,part,offset)
end

function module.SpawnEnemy(id,buff)
	local percent = buff/100
	local codename = Util.getValueByIdinMap(globalVar.EnemyID,'id',id,'codename')

	--enemyClone.Name = enemyClone.Name..math.random(1000000,9999999)
	local randomZ = Random.new():NextNumber(-2,2)
	local pos = game.Workspace.Enviroment["Base-Enemy"].Position + Vector3.new(-6,0, randomZ)
	local leName = codename..'_ENEMYPART_'..math.random(-99999999,99999999)
	Util.MakePart(100,leName,workspace.Enemy,pos,Vector3.new(1,1,1),true,false,Color3.fromHex('FFFFFF'))
	local lePart = Util.FindInstance(Util.instances,100,leName)

	local cloneCollision = game.ReplicatedStorage.Collision:Clone()	
	cloneCollision.Parent = lePart

	local statClone = game.ReplicatedStorage.DefaultStats:Clone()
	statClone.Parent = lePart
	statClone.Name = 'Stats'
	local stats = require(statClone)
	for i,v in pairs(Util.getValueByIdinMap(globalVar.EnemyID,'id',id,'stats')) do
		stats[i] = v
	end
	
	local cloneScript = scriptType[stats.typescript].enemy:Clone()
	if not cloneScript then cloneScript = scriptType.default.enemy:Clone(); warn('Type of Script is not found!!') end
	cloneScript.Parent = lePart
	
	local HP = Instance.new('NumberValue')
	HP.Name = 'Health'
	stats.maxhp *= percent
	HP.Value = stats.maxhp
	HP.Parent = lePart

	stats.atk *= percent
	stats.zPos = randomZ
	
	local entitype = '_ENEMYMODEL_'
	local thename = codename..entitype..math.random(-999999999,999999999)
	stats.ClientModelName = thename
	ClientModelEvent:FireAllClients('spawn',lePart,{isEnemy = true,name = thename,id = id,offset = stats.modeloffset})
end
 
function module.SpawnUnit(id)
	local codename = Util.getValueByIdinMap(globalVar.UnitID,'id',id,'codename')
	
	local pos = game.Workspace.Enviroment["Base-unit"].Position + Vector3.new(6,0,0)
	local leName = codename..'_UNITPART_'..math.random(-99999999,99999999)
	Util.MakePart(101,leName,workspace.Unit,pos,Vector3.new(1,1,1),true,false,Color3.fromHex('FFFFFF'))
	local lePart = Util.FindInstance(Util.instances,101,leName)
	
	local cloneCollision = game.ReplicatedStorage.Collision:Clone()
	cloneCollision.Parent = lePart
	
	local randomZ = Random.new():NextNumber(-2,2)
	local statClone = game.ReplicatedStorage.DefaultStats:Clone()
	statClone.Parent = lePart
	statClone.Name = 'Stats'
	local stats = require(statClone)
	for i2,v2 in pairs(Util.getValueByIdinMap(Upgrades,'id',id,'stats')) do
		if v2 ~= nil then
			stats[i2] = v2
		end
	end
	local cloneScript = scriptType[stats.typescript].unit:Clone()
	if not cloneScript then cloneScript = scriptType.default.unit:Clone(); warn('Type of Script is not found!!') end
	cloneScript.Parent = lePart
	
	local HP = Instance.new('NumberValue')
	HP.Name = 'Health'
	HP.Value = stats.maxhp
	HP.Parent = lePart
	
	stats.zPos = randomZ
	
	local entitype = '_UNITMODEL_'
	local thename = codename..entitype..math.random(-999999999,999999999)
	stats.ClientModelName = thename
	ClientModelEvent:FireAllClients('spawn',lePart,{isEnemy = false,name = thename,id = id,offset = stats.modeloffset})
end

function module.onHit(isEnemy,npc,hitbox,curHP,stats)
	local folder = game.Workspace.Unit
	if isEnemy then folder = game.Workspace.Enemy end
	--if atktimer <= 0 then
	--if not debounced3 then
	for i,entity in pairs(folder:GetChildren()) do
		local relativePos = hitbox.CFrame:PointToObjectSpace(entity.Position)
		if curHP.Value > 0 and math.abs(relativePos.X) <= npc.Hitbox.Size.X * 1 * 0.6 and math.abs(relativePos.Z) <= npc.Hitbox.Size.Z * 1 * 0.6 then
			curHP.Value -= stats.atk
			--print(tostring(entity.Name)..' HIT!')
		end
	end
	--end
	--end
end

function module.onDeath(npc)
	task.delay(0.6,function()
		local bom = game.ReplicatedStorage.explosion:Clone()
		bom.Parent = npc
		bom.CFrame = npc:GetPrimaryPartCFrame()
		bom.Attachment.Explode1:Emit(10)
		bom.Attachment.Explode2:Emit(25)
		bom.Attachment.Explode3:Emit(25)
		bom.Attachment.Explode4:Emit(10)
		task.delay(0.2,function()
			for i,v in pairs(npc:GetChildren()) do
				if v:IsA('MeshPart') or v:IsA('Part') then
					v.Transparency = 1
				end
			end 
		end)
	end)
end

return module
Script 3 -- LocalScript containing Client sided model that could be triggered from serverside, i think this is where the lag came from 
local EntityModule = require(game.ReplicatedStorage.EntityModule)
local NPCSystem = require(game.ReplicatedStorage.NPCSystem)
local remote = game.ReplicatedStorage.ClientModelEvent

local currentAnimPlaying = {}

function PlayAnimation(obj,parent,animation,fadetime,force)
	if parent:FindFirstChild(obj) == nil then return end
	local instanz = parent:FindFirstChild(obj)
	local track = instanz:WaitForChild(animation)
	local anim = instanz.Humanoid.Animator:LoadAnimation(track)
	if force then
		anim:Play(fadetime or 0.2)
		currentAnimPlaying[obj] = {[animation] = anim}
	else
		if (currentAnimPlaying[obj] and currentAnimPlaying[obj][animation] or nil) == nil then
			currentAnimPlaying[obj] = {[animation] = anim}
			anim:Play(fadetime or 0.2)
		elseif currentAnimPlaying[obj][animation].TimePosition >= currentAnimPlaying[obj][animation].Length then
			currentAnimPlaying[obj] = {[animation] = anim}
			anim:Play(fadetime or 0.2)
		end 
	end
end

function StopAnimation(obj,parent,animation,fadetime)
	if parent:FindFirstChild(obj) == nil then return end
	local instanz = parent:FindFirstChild(obj)
	for i,v in pairs(instanz.Humanoid.Animator:GetPlayingAnimationTracks()) do
		if v.Name:lower() == animation:lower() then
			v:Stop(fadetime or 0.2)
		end
	end
end

remote.OnClientEvent:Connect(function(funcname,obj,tbl)
	if funcname == 'spawn' then
		EntityModule.SpawnClientModel(tbl.isEnemy,tbl.name,tbl.id,obj,tbl.offset)
	elseif funcname == 'delete' then
		NPCSystem.StopClientModel(obj)
		if game.Workspace.Enemy:FindFirstChild(obj) then game.Workspace.Enemy:FindFirstChild(obj).Parent = nil end
		if game.Workspace.Unit:FindFirstChild(obj) then game.Workspace.Unit:FindFirstChild(obj).Parent = nil end
	elseif funcname == 'playAnim' then
		PlayAnimation(obj,tbl.Parent,tbl.animName,tbl.fadetime,tbl.force)
	elseif funcname == 'stopAnim' then
		StopAnimation(obj,tbl.Parent,tbl.animName,tbl.fadetime)
	end
end)
Script 4 -- NPCSystem, copied from someone a bit. Though there is a problem that when it reached the destination and trigger the same coords, it just gone 
local System = {}
System.CurrentFunctionRunnin = {}
System.ClientFunctionRunnin = {}
local RS = game:GetService('RunService')
local Util = require(game.ReplicatedStorage.Utilities)
local Store = game.ReplicatedStorage.StoreRunninSystem
function System.MoveTo(obj,pos)
	pos = Vector3.new(pos.X,pos.Y,pos.Z)
	for i,v in pairs(System.CurrentFunctionRunnin) do
		if i:find(obj.Name) then
			System.CurrentFunctionRunnin[i] = nil
			Store:FireAllClients({script,'ClientFunctionRunnin'},i,nil)
		end
	end
	local nam = obj.Name..math.random(100000,999999)
	System.CurrentFunctionRunnin[nam] = true
	Store:FireAllClients({script,'ClientFunctionRunnin'},nam,true)

	local Values = require(obj:WaitForChild('Stats'))
	local durTimer = 0
	local startPos = obj.Position
	local duration = (pos - startPos).Magnitude / Values.walkspeed
	local dir = (pos - startPos).Unit
	local curDir = obj.CFrame.LookVector
	local durMult = 1 / duration
	local hb
	hb = RS.Heartbeat:Connect(function(dt)
		durTimer += dt
		if durTimer >= duration then
			durTimer = duration
			System.CurrentFunctionRunnin[nam] = nil
			Store:FireAllClients({script,'ClientFunctionRunnin'},nam,nil)
			hb:Disconnect()
		end
		local ratio = durTimer * durMult
		local pos = startPos:Lerp(pos,ratio)
		obj.CFrame = CFrame.new(pos,pos + curDir:Lerp(dir,math.min(durTimer * 10,1)))

		if System.CurrentFunctionRunnin[nam] then
			hb:Disconnect()
		end
	end)
end

function System.ClientModelConnect(obj,part,offsetpos)
	local function Show(inst,bool)
		for i,v in ipairs(inst:GetChildren()) do
			if (v:IsA('Part') or v:IsA('MeshPart')) and v.Name ~= 'HumanoidRootPart' then
				v.Transparency = bool and 0 or 1
			end
		end
	end
	for i,v in pairs(System.ClientFunctionRunnin) do
		if i:find(obj.Name) then
			System.ClientFunctionRunnin[i] = nil
		end
	end
	local nam = obj.Name..math.random(1000000,9999999)
	System.ClientFunctionRunnin[nam] = true

	local objpos = obj.PrimaryPart.Position + offsetpos
	local x, y, z = obj.PrimaryPart.CFrame:ToEulerAnglesYXZ()

	local camera = workspace.CurrentCamera
	local hb
	hb = RS.Heartbeat:Connect(function(dt)
		objpos = objpos:Lerp(part.Position + offsetpos,dt * 10)
		local x2, y2, z2 = part.CFrame:ToEulerAnglesYXZ()
		x,y,z = Util.Lerp(x,x2,dt * 10),Util.Lerp(y,y2,dt * 10),Util.Lerp(z,z2,dt * 10)
		obj:SetPrimaryPartCFrame(CFrame.new(objpos) * CFrame.Angles(x, y, z))
		local vector, onScreen = camera:WorldToScreenPoint(obj.PrimaryPart.Position)
		if onScreen then Show(obj,true) else Show(obj,false) end

		if not System.ClientFunctionRunnin[nam] then
			obj = nil
			hb:Disconnect()
		end
	end)
end
function System.StopClientModel(obj)
	for i,v in pairs(System.ClientFunctionRunnin) do
		if i:find(obj) then
			System.ClientFunctionRunnin[i] = nil
		end
	end
end

function System.IsMoving(obj)
	for i,v in pairs(System.CurrentFunctionRunnin) do
		if i:find(obj.Name) and v then
			return true
		end
	end
	return false 
end

function System.StopMoving(obj)
	for i,v in pairs(System.CurrentFunctionRunnin) do
		if i:find(obj.Name) and v then
			print(obj.Name,'STOP!!')
			System.CurrentFunctionRunnin[i] = nil
			Store:FireAllClients({script,'ClientFunctionRunnin'},i,nil)
		end
	end
end

return System
1 Like
How do i improve these scripts? its kinda messy and buggy
#2

You can improve them by adding more comments. When I read this:

function System.MoveTo(obj,pos)

My immediate thought is that this is going to move a Part to a Vector3 position. But then what actually happens in the function is completely indecipherable. The most important parts of your documentation is the function signature (function name and parameter names, including type annotations if you choose to use them), and any comment explanation above the function which explains what it is expected to do.