How could I make my automatic cannon script more efficient?

Help and Feedback Code Review
#1

What are you not satisfied with?

This code is for my automatic cannon I’m working on. I want to make it as efficient as possible, and I am looking for some feedback on it.

How does the code work?

  1. The player clicks a button on the cannon that says “Fire”
  2. A part is created just outside of the barrel.
  3. An explosion is created for the ball to move
  4. A CannonBallExplosionScript is cloned to the ball
  5. Every time the cannon fires, there is a counter script that counts how many cannon balls have been fired, this number is stored in an IntValue and is displayed on the cannon.
  6. After a set number of cannon ball fires, the cannon destroys.

What potential improvements have you considered?

  • The cannon currently only works in four directions, and I was trying to figure out an easy way to make it work in any direction.
  • There is usually a lot of lag whenever I run it.
  • There is an invisible barrier around the cannon so players don’t get launched. A non-collision constraint is created in the cannonball to prevent it from colliding with the barrier.

Code:

LaunchScript:

local minPower = script.Parent.Parent:GetAttribute("MinPower")
local maxPower = script.Parent.Parent:GetAttribute("MaxPower")

local stop = script.Parent.Parent:GetAttribute("Break")
local initialWaitTime = script.Parent.Parent:GetAttribute("InitialWaitTime")

local started = false

local click = script.Parent.ClickDetector

function shoot()
	local power = math.random(minPower,maxPower)
	
	local oLeft = script.Parent.Parent:GetAttribute("OffsetLeft")
	local oRight = script.Parent.Parent:GetAttribute("OffsetRight")
	
	script.Parent.Parent.Counter.CannonBalls.Value = script.Parent.Parent.Counter.CannonBalls.Value + 1
	
	local ball = Instance.new("Part",script.Parent.Parent)
	ball.Name = "CannonBall"
	ball.Shape = "Ball"
	ball.Size = Vector3.new(2,2,2)
	ball.BrickColor = BrickColor.Black()
	ball.CastShadow = false
	ball.Material = "Metal"
	ball.AssemblyLinearVelocity = Vector3.new(0,0,math.random(oLeft,oRight))
	
	local barrel = script.Parent.Parent.Barrel.Position
	local base = script.Parent.Parent.Base
	
	local explosion = Instance.new("Explosion",ball)
	explosion.ExplosionType = "NoCraters"
	explosion.DestroyJointRadiusPercent = 0
	explosion.BlastRadius = 2
	explosion.BlastPressure = power*10000000
	
	if base.Orientation == Vector3.new(0,0,0) then
		ball.Position = Vector3.new(barrel.X+7.05,barrel.Y+2.6,barrel.Z)
		explosion.Position = Vector3.new(ball.Position.X-0.1,ball.Position.Y-0.1,ball.Position.Z)
	elseif base.Orientation == Vector3.new(0,180,0) then
		ball.Position = Vector3.new(barrel.X-7.05,barrel.Y+2.6,barrel.Z)
		explosion.Position = Vector3.new(ball.Position.X+0.1,ball.Position.Y-0.1,ball.Position.Z)
	elseif base.Orientation == Vector3.new(0,-90,0) then
		ball.Position = Vector3.new(barrel.X+0.5,barrel.Y+2.6,barrel.Z+10)
		explosion.Position = Vector3.new(ball.Position.X,ball.Position.Y-0.1,ball.Position.Z-0.1)
	elseif base.Orientation == Vector3.new(0,90,0) then
		ball.Position = Vector3.new(barrel.X+0.5,barrel.Y+2.6,barrel.Z-10)
		explosion.Position = Vector3.new(ball.Position.X,ball.Position.Y-0.1,ball.Position.Z+0.1)
	else
		error("Incompatible orientation! Orientation.X and Orientation.Z must be set to 0, while Orienation.Y must be either 0, 180, -90, or 90!")
	end
	
	local noCollisionConstraint = Instance.new("NoCollisionConstraint")
	noCollisionConstraint.Parent = ball
	noCollisionConstraint.Part0 = ball
	noCollisionConstraint.Part1 = script.Parent.Parent.Barrier.Front
	
	local cloneScript = script.Parent.Parent.CannonBallExplosionScript:Clone()
	cloneScript.Disabled = false
	cloneScript.Parent = ball

end

click.MouseClick:Connect(function()
	
	if started == true then return end
	started = true
	
	wait(initialWaitTime)
	
	local delayTime = script.Parent.Parent:GetAttribute("DelayTime")
	local maxBullets = script.Parent.Parent:GetAttribute("MaxBullets")
	
	while wait(delayTime) do
		shoot()
		if script.Parent.Parent.Counter.CannonBalls.Value > maxBullets and stop == true then
			
			script.Parent.BrickColor = BrickColor.Red()
			
			
			script.Parent.Parent.Barrier:Destroy()
			
			for _, part in pairs(script.Parent.Parent:GetChildren()) do
				if part.ClassName == "Part" or part.ClassName == "UnionOperation" then
					part.Anchored = false
				end
			end
			
			wait(3)
			
			script.Parent.Parent:Destroy()
			
			break
		end
	end
end)

CannonBallExplosionScript:

local ball = script.Parent

wait(0.3)

script.Parent.Touched:Connect(function(t)
	if t.Name ~= "CannonBall" then
		local explosion = Instance.new("Explosion",game.Workspace)

		explosion.Position = ball.Position
		explosion.ExplosionType = "NoCraters"
		explosion.BlastRadius = 12
		explosion.BlastPressure = 100000
			
		ball:Destroy()
	end
end)

CounterScript:

local v = script.Parent.CannonBalls
script.Parent.SurfaceGui.TextLabel.Text = v.Value

v.Changed:Connect(function()
	script.Parent.SurfaceGui.TextLabel.Text = v.Value
end)

Any suggestions would be strongly appreciated, feel free to ask any questions!

#2

Some stuff that I would personally do is for 1 condense everything into one script. You can separate parts by using either coroutines or spawn(function(). The one thing to note is using coroutines is just like using another script (according to wiki at least). 2 instead :Destroy(), I would say using game.Debris:AddItem(part,0) is better > additionally you could create a function at the top to remove things. Iv done that when I have larger scripts containing a lot of information.

function removepart(part)
     game.Debris:AddItem(part,0)
end

^something like that and then you’d just pass the part through. Aside from that I would also suggest creating the instance before hand and cloning it, instead of making it inside the script. Like scripts in general try to run everything as fast as possible if you’re trying to run on efficiency I think you’d want to cut down on as much waste as possible. Of course these are all things that I would personally do. Hopefully this helps at least somewhat.

1 Like