How can i improve my magic attack scripts?

Help and Feedback Code Review
#1

I’ve made some sorcery scripts, allowing the user to execute 2 attacks: Fireball and Heatwave.

I wanted to know what i did of good and of bad regarding the scripts, and how to fix any issues with it.

I’m fascinated with this sort of thing since ever

There are 4 scripts in total:
LocalScript “FireSpellcast” inside of StarterPlayerScripts
ServerScript “FireSpells” inside of ServerScriptService
ModuleScript “Spells” inside of FireSpells
ServerScript “TouchManager” inside of the projectiles, inside of ServerStorage

FireSpellcast

CAS = game:GetService("ContextActionService")

Player = game.Players.LocalPlayer
Character = Player.Character or Player.CharacterAdded:Wait()
Humanoid = Character:WaitForChild("Humanoid")
HumanoidRootPart = Character:WaitForChild("HumanoidRootPart")

FireballCastingAnim = Humanoid:LoadAnimation(script.FireballCasting)
HeatwaveCastingAnim = Humanoid:LoadAnimation(script.HeatwaveCasting)

function Fireball()
	FireballCastingAnim:Play()
	delay(0.5, function()
		game.ReplicatedStorage.SpellRemotes.Fireball:FireServer(HumanoidRootPart)
	end)
	CAS:UnbindAction("Fireball")
	wait(2)
	print("Fireball Ready")
	CAS:BindAction("Fireball",Fireball,false,Enum.KeyCode.E)
end

function Heatwave()
	HeatwaveCastingAnim:Play()
	delay(0.8,function()
		game.ReplicatedStorage.SpellRemotes.Heatwave:FireServer(HumanoidRootPart)
	end)
	CAS:UnbindAction("Heatwave")
	wait(4)
	print("Heatwave Ready")
	CAS:BindAction("Heatwave",Heatwave,false,Enum.KeyCode.Q)
end

CAS:BindAction("Fireball",Fireball,false,Enum.KeyCode.E)
CAS:BindAction("Heatwave",Heatwave,false,Enum.KeyCode.Q)

FireSpells

Spells = require(script:WaitForChild("Spells"))

game.ReplicatedStorage.SpellRemotes.Fireball.OnServerEvent:Connect(Spells.Fireball)
game.ReplicatedStorage.SpellRemotes.Heatwave.OnServerEvent:Connect(Spells.Heatwave)

Spells

local Spells = {}

local SpellObjects = {
	Fireball = game.ServerStorage.FireProjectiles.Fireball,
	Heatwave = game.ServerStorage.FireProjectiles.Heatwave
}

function Spells.Fireball(Player,HRP) -- Direct attack with high damage and small hitbox
	local StartPos = HRP.CFrame:ToWorldSpace(CFrame.new(0,0,-2))
	local Projectile = SpellObjects.Fireball:Clone()
	
	local Vel = Projectile:WaitForChild("Vel")
	local Fire = Projectile:WaitForChild("Fire")
	local TouchManager = Projectile:WaitForChild("TouchManager")
	
	Projectile.Parent = workspace:WaitForChild("Projectiles")
	Projectile.CFrame = StartPos
	Vel.Velocity = Projectile.CFrame.lookVector*30
	Fire.Enabled = true
	TouchManager.Disabled = not true
	
	delay(5,function()
		Projectile:Destroy()
	end)
	
	TouchManager.OnTouched.Event:Connect(function(Hit)
		if not Hit:IsDescendantOf(Player.Character) then
			local Humanoid = Hit.Parent:FindFirstChild("Humanoid")
			if Humanoid then
				if Humanoid.Health > 0 then
					Humanoid:TakeDamage(34)
					Projectile:Destroy()
				end
			end
		end
	end)
end

function Spells.Heatwave(Player,HRP) -- AOE attack with low damage and speed with a large hitbox
	local StartPos = HRP.CFrame:ToWorldSpace(CFrame.new(0,0,-2))
	local Wave = SpellObjects.Heatwave:Clone()
	
	local Vel = Wave:WaitForChild("Vel")
	local Fire = Wave:WaitForChild("Fire")
	local TouchManager = Wave:WaitForChild("TouchManager")
	
	local AlreadyHitTargets = {}
	
	Wave.CFrame = StartPos
	Wave.Parent = workspace.Projectiles
	Vel.Velocity = Wave.CFrame.lookVector*15 - Vector3.new(0,1,0)
	Fire.Enabled = true
	TouchManager.Disabled = not true
	
	delay(5,function()
		Wave:Destroy()
		for Index, Value in ipairs(AlreadyHitTargets) do
			table.remove(AlreadyHitTargets,Index)
		end
	end)
	
	TouchManager.OnTouched.Event:Connect(function(Hit) -- check if its not player, if its human and whether or not it has already been damaged before
		if not Hit:IsDescendantOf(Player.Character) then
			local Humanoid = Hit.Parent:FindFirstChild("Humanoid")
			if Humanoid then
				local EnemyCharacter = table.find(AlreadyHitTargets,Humanoid.Parent)
				if not EnemyCharacter then
					if Humanoid.Health > 0 then
						table.insert(AlreadyHitTargets,#AlreadyHitTargets+1,Humanoid.Parent)
						Humanoid:TakeDamage(20)
					end
				end
			end
		end
	end)
	
end

return Spells

TouchManager

script.Parent.Touched:Connect(function(Hit)
	script.OnTouched:Fire(Hit)
end)

My first post in Code Review!!!

1 Like
#2

Hello Windsonnes, I looked over your code and found some imperfections, let’s start.
Starting off with the FireSpellcase :

  1. One major error with your code is the lack of use of locals. Locals in Lua make said variable “local” to the scope. This allows all local variables to be gced (garbaged collected) once the scope ends. Locals are also stored in the stack, while globals are stored in the heap. Referencing memory in the heap is much slower than the stack. Memory in the heap is not managed automatically. To declare a variable as local, you simply just do local VarName = Value

  2. In recent updates, Humanoid:LoadAnimation() has been deprecated. Instead use Animator:LoadAnimation

  3. delay is a function with a lot of controversy in the community. Eryn made an article highlighting the negatives of spawn() and delay(). They are both in the 30hz pipeline of roblox, meaning they are updated every 1/30 of a second. It is not guaranteed that the wait will resume when wanted. Also in this case you can just do wait(0.8); local function run() .... end; run().

  4. The last big thing about this script is you not cache game.ReplicatedStorage, at the top of the script you can simply just do local ReplicatedStorage = game:GetService("ReplicatedStorage"), instead of getting RepStorage when needed.

That is all for that script, lets move onto FireSpells
(This is a smaller script so not that much on it)

  1. Same problem as I previously highlighted, cache Replicated Storage. This is a common bad habit I see many beginners fall victim of.

Now onto the Spells script :

  1. There are more of the same problems of the last two scripts; not caching RepStorage and using :WFC this commonly

  2. One small thing i saw when looking over your script is the use of TouchManager.Disabled = not true, you simply could of done = false

  3. On the hit detection part, you could of done :

local Humanoid = Hit.Parent....... -- complete this part
if Humanoid and Humanoid.Health > 0 then 
    Humanoid.Health = Humanoid.Health - x
end

The use of :TakeDamage() is unneeded, since it is just calling an auxiliary function.

  1. In your for loop, it is a good idea to use the throwaway variable “_” when the corresponding variable is of no use of the script.
for Index, _ in ipairs(AlreadyHitTargets) do
  1. Personally, I disagree with this use of table.insert(); This function can be avoided by doing : table[#table+1] = value. This avoid calling a function.

Lastly, TouchManager is unneeded, there is already an event for .Touched. It’s not needed for you to use an event to fire another one. This is unneeded.

Overall, Great script. :+1:. Happy Holidays!

6 Likes