How can I make this script more efficient?

Help and Feedback Code Review
#1

Hello.
I have a working plane script but it seems to be quite inefficient. Please help make it more efficient.
This is the code.

wait(5)
local model = script.Parent
local t = script.Parent.protect.Value
for i,v in pairs(workspace:GetDescendants()) do
	if v.Name == t then
		home = v
	end
end
local e = Instance.new('BodyGyro')
e.Parent = script.Parent.Engine
local engine = script.Parent.Engine
local a = home.part1
local b = home.part2
local c = home.part3
local d = home.part4
local currentPos = d
local nextPos = a
local function FireGuns(target)
	for _,v in pairs(script.Parent:GetDescendants()) do
		if v.Name == "MachineGun" then		
			local b = game:GetService("ReplicatedStorage").Ammo.AB
			local bullet = b:Clone()
			bullet.Position = v.Position
			bullet.Orientation = v.Orientation
			bullet.Parent = workspace
		end
	end
end
local function BOOM(target)
	if target and (script.Parent.BluePlane.Position - target.RedPlane.Position).magnitude < 51 and (target.Name == "RedHeavyFighter" or target.Name == "RedFighter" or target.Mission.Value == "Patrol") then
		target.Health.Value = 0
		script.Parent.Health.Value = 0
		script.Disabled = true
	end
end
local function getTargets()
	local targets = workspace.RedPlanes
	for v,t in pairs(workspace:GetDescendants()) do
		local enemyPlane = t:FindFirstChild("RedPlane")
		if enemyPlane and enemyPlane.Parent.Health.Value > 0 then
			enemyPlane.Parent.Parent = targets
		end
	end
	return targets
end
local function findNearestEnemy(targets)
	local target = nil
	local distance = nil
	local previousDistance = 99999
	for i,v in pairs(targets:GetChildren()) do
		if v:IsA("Model") and v.RedPlane and v.Health.Value > 0 then
			distance = (script.Parent.BluePlane.Position - v.RedPlane.Position).magnitude
			if distance < previousDistance and v.Health.Value > 0 and (script.Parent.BluePlane.Position - v.RedPlane.Position).magnitude < 1501 then
				previousDistance = distance
				target = v
			end
		end
	end
	return target
end
local function adjustSpeed(target)
	if (script.Parent.BluePlane.Position - target.RedPlane.Position).magnitude < 100 and target.Health.Value > 0 then
		script.Parent.Speed.Value = target.Speed.Value
	elseif not target or (script.Parent.BluePlane.Position - target.RedPlane.Position).magnitude > 100 and target.Health.Value > 0 then
		script.Parent.Speed.Value = -40
	end
end
local function shoot(target)
	if (script.Parent.BluePlane.Position - target.RedPlane.Position).magnitude < 1001 and target.Health.Value > 0 then
		FireGuns(target)
	end
end
local function moveTo(part)
	local origincframe = engine.BodyGyro.cframe
	local dir = (script.Parent.BluePlane.Position - part.Position).unit
	local spawnPos = script.Parent.BluePlane.Position
	local pos = spawnPos + dir 	
	engine:findFirstChild("BodyGyro").maxTorque = Vector3.new(10000,10000,10000)
	engine:findFirstChild("BodyGyro").cframe = CFrame.new(pos, pos+dir)
end
local function getNextL(currentPos)
	if currentPos == a then
		nextPos = b
	end
	if currentPos == b then
		nextPos = c
	end
	if currentPos == c then
		nextPos = d
	end
	if currentPos == d then
		nextPos = a
	end
	return nextPos
end
local function orbit()
	script.Parent.Speed.Value = -40
	if (script.Parent.BluePlane.Position - nextPos.Position).magnitude > 100 	then
		moveTo(nextPos)	 
	else
		local loc = getNextL(nextPos)
		nextPos = loc
		moveTo(nextPos)		
	end
end
local function attack(target)
	if target and target.RedPlane and target.Health.Value > 0 then
		local origincframe = engine.BodyGyro.cframe
		local dir = (script.Parent.BluePlane.Position - target.RedPlane.Position).unit
		local spawnPos = script.Parent.BluePlane.Position
		local pos = spawnPos + dir 	
		engine:findFirstChild("BodyGyro").maxTorque = Vector3.new(10000,10000,10000)
		engine:findFirstChild("BodyGyro").cframe = CFrame.new(pos, pos+dir)
		adjustSpeed(target)
		shoot(target)
		BOOM(target)
	end
end
local function patrolAi()
	local targets = getTargets()
	local target = findNearestEnemy(targets)
	if target and target.Health.Value > 0 then
		attack(target)
	elseif not target then
		orbit(target)
	end
end
while wait(0.01) do
	patrolAi()
end

Thanks for reading
Please tell me how to make this script more efficent

#2

I’d start with this function:

This is being called quite regularly so needs to be as light as possible. Calling workspace:GetDescendants() every frame and looping through them all seems unnecessary to me.

You could improve this by making sure that all planes are parented to a single folder within workspace. That way you only need to check the children of that folder rather than all of workspace.

An alternative is to use CollectionService and tag each plane when they are created. You can then use CollectionService:GetTagged(PLANE_TAG) to get all the planes.

1 Like
#3

@McThor2 Thanks so much! Will do!

1 Like
#4

Some other mentions. In short, there is a crap ton of redundancies to be found in your script, all of which more or less impact performance.

If applicable, using attributes is generally better and faster than value instances.

This is redundant. Request ReplicatedStorage once at the top of the script and use the stored variable instead.

This probably stands out the most to me. You should be aliasing commonly accessed instances to local variables at the top of the script and use the said variables instead. Not only will doing so help make coding easier (fewer words typed out), it will also boost performance. Trust me, I did some benchmarking and it does make a performance difference.

Declaration of local variables is redundant if you’re only going to use them once. Directly assign the nextPos global to what’s returned by the function.

This is redundant. You do not need to create a new Vector3 on each function call. You can create it once at the top of the script and simply reuse it.

This makes no sense and is what alarms me the most. FindFirstChild does not guarantee the return of an instance, and by directly indexing what’s returned you’re assuming that it will always return an instance, which doesn’t happen. These kinds of lines are prone to error in addition to being redundant. Consider using just dot operators if you know for a fact that they won’t error, or split the line for sanity checks.

This is also redundant. You can return the nextPos immediately after it’s found, in the opposite order. Or just use a table.

You’re sending over parameters and yet the function doesn’t take any?

The parameter is pointless. wait() will never be fast enough to achieve a 10-millisecond yield. You should be using task.wait() nonetheless which is faster than standard wait and will thus make your functions update faster.

1 Like
#5

make variables make more sense. d or “f” is inunderstandable