This is my game script for my Pirate Era wave defense game… Any tips on how to improve, organize or make the code more efficient?
local Players = game:GetService("Players")
local TeamService = game:GetService("Teams")
local baseHealth = game.Workspace.Base.Health
local CurrentWave = 1
local alive = {}
local npcs = {}
local waveCopy = game.ReplicatedStorage.Wave1:Clone()
waveCopy.Parent = game.Workspace.Wave
local function Wave(player)
player.PlayerGui.CoreUI.WaveLabel.Text = "Wave "..tostring(CurrentWave)
for i, v in pairs(game.Workspace.Wave:GetChildren()) do
v:Destroy()
end
if CurrentWave == 1 then
local waveCopy = game.ReplicatedStorage.Wave1:Clone()
waveCopy.Parent = game.Workspace.Wave
elseif CurrentWave == 2 then
print('20 enemies')
elseif CurrentWave == 3 then
print('30 enemies')
elseif CurrentWave == 4 then
print('40 enemies')
end
end
local function Restart(val, player)
for i, v in pairs(game.Workspace.Wave:GetChildren()) do
v:Destroy()
end
if val == 'Lost' then
player.PlayerGui.CoreUI.GameLabel.Text = "You lost!"
wait(3)
player.PlayerGui.CoreUI.GameLabel.Text = "Restarting..."
wait(3)
else
player.PlayerGui.CoreUi.GameLabel.Text = "Victorious!"
-- give win reward
end
CurrentWave = 1
baseHealth.Value = 2000
for i, v in pairs(Players:GetChildren()) do
v.Team = TeamService.Pirates
v:LoadCharacter()
end
Wave(player)
end
game.Players.PlayerAdded:Connect(function(player)
player.CharacterAdded:Connect(function()
player.Character.Humanoid.Died:Connect(function()
player.Team = game.Teams["[Dead]"]
table.remove(alive)
if #alive == 0 then
Restart('Lost', player)
end
end)
end)
table.insert(alive, player)
for i, v in pairs(waveCopy:GetChildren()) do
table.insert(npcs, v)
v.Humanoid.Died:Connect(function()
table.remove(npcs)
if #npcs == 0 then
CurrentWave += 1
Wave(player)
end
end)
end
baseHealth:GetPropertyChangedSignal("Value"):Connect(function()
if baseHealth.Value <= 0 then
Restart('Lost', player)
end
end)
end)
Hello dockyboy, I looked over your script and i found some problems, let’s start.
Something small is your use of game.Workspace, Roblox added an environmental variable, workspace which achieves the same reward.
One major problem with your script is the misuse of pairs, pairs should only be used on dictionaries. Use of it on arrays is discouraged. Pairs iterates over a table without sequence. The source code of it looks like :
-- pairs --
function pairs(Table)
return next, Table, nil
end
while ipairs’ source code is more of a numerical for loop.
-- ipairs -- (https://www.lua.org/pil/7.3.html)
function iter (a, i)
i = i + 1
local v = a[i]
if v then
return i, v
end
end
function ipairs (a)
return iter, a, 0
end
You should use ipairs for arrays, and pairs for dictionaries/hash tables.
Your if statement is quite lengthy, we can reduce it by doing :
if CurrentWave == 1 then
-- your code
elseif CurrentWave <= 4 then
print(CurrentWave.."0 enemies")
end
Right here, you are declaring a Global. Globals are stored in the Heap. Accessing the globals are quite slower than the traditional stack for locals . You can simply just do : local CurrentValue = 1
Here, You are not using the variable Players, which you allocated for this reason. change it to
Players.PlayerAdded:Connect(function(player)
I also see you are commonly using table.insert() and table.remove. These are unneeded. You could simply just do :
-- your code : table.insert(alive, player) --
Alive[#Alive+1] = player
-- your code : table.insert(npcs, v) --
npcs[#npcs+1] = v
Lastly, I commonly see you have inconsistent variable naming. It’s always a good idea to keep consistent conventions.
Edit : I missed out on one problem:
In your for loops, it seems like you are not using the value “i”. When a value retains no value to be used, you should use the name “_” for it. This signifies to the reader that said variable is useless.
if #npcs == 0 then
CurrentWave += 1
Wave(player)
end
The CurrentWave increments after every wave once all NPC’s are killed.
Then the CurrentWave global variable is updated to the next wave, then the Wave() function is called.
local function Wave(player)
player.PlayerGui.CoreUI.WaveLabel.Text = "Wave "..tostring(CurrentWave)
for i, v in pairs(game.Workspace.Wave:GetChildren()) do
v:Destroy()
end
if CurrentWave == 1 then
local waveCopy = game.ReplicatedStorage.Wave1:Clone()
waveCopy.Parent = game.Workspace.Wave
elseif CurrentWave == 2 then
print('20 enemies')
elseif CurrentWave == 3 then
print('30 enemies')
elseif CurrentWave == 4 then
print('40 enemies')
end
end
I don’t quite get what you are trying to ask. The code i just suggested should only replace
with
if CurrentWave == 1 then
local waveCopy = game.ReplicatedStorage.Wave1:Clone()
waveCopy.Parent = game.Workspace.Wave
elseif CurrentWave <= 4 then
print(CurrentWave.."0 enemies")
end
It is not relating to the incrementing of CurrentWave. One other small this is that CurrentWave is not a global, it is a local. Globals are initialized like so : Var = Value. The major difference is how they are stored in memory; as I highlighted in the main post above.
onedar gave you that code, since the code that you gave him to review, contained print statements to print the amount of enemies in that wave, how ever the number of enemies increments 10 every round so if it is the round 2, it will have 20 enemies, so thats why onedar suggested you that piece of code, since that, with that code it will just concanate the number of wave with 0, again if the wave is 2 it will be 20, so thats why onedar suggested the code to shorten the prints, now if you were just doing that for testing purposes you should have been put a comment or something like that, since that, when you put a code in Code Review, the people that reviews it, will review the current code, not knowing if you will change it, so they will take it very literally, if your goal is to make something different every wave just keep it like that, how ever if you will do some sort of pattern just like those prints, try to implement the code that onedar gave to you.
One major problem with your script is the misuse of pairs, pairs should only be used on dictionaries. Use of it on arrays is discouraged. Pairs iterates over a table without sequence.
What? You can use any of those 2, using pairs on arrays is not discouraged. You need to know the difference between Ipairs and pairs so you know when to use them. An use case of pairs on arrays would be to loop through them indefinitely and pick a random value.
Right here, you are declaring a Global. Globals are stored in the Heap. Accessing the globals are quite slower than the traditional stack for locals . You can simply just do : local CurrentValue = 1
Stacks only are negligibly faster than the global environment.
function pairs(Table)
return next, Table, nil
end
That isn’t the actual source code of pairs, it’s the iterator function returned by pairs.
I also see you are commonly using table.insert() and table.remove . These are unneeded.
This is preferable and is not “unneeded”. It’s like you saying .Magnitude is unneeded when you can just do sqrt(x^2 + y^2 + z^2).
At the point when an individual uses ipairs for a “for loop”, the reader consequently realizes that the table given is an array. With pairs, this is not manifested. The performance boost is there, as seen in my benchmark. The results were astonishing, they were greater than I expected :
13:02:45.174 Pairs : 3114, Ipairs : 996886. - Server - Main:25
It was reliably quicker. There is basically no explanation, other than the explanation you brought up, to utilize pairs on an array.
As I highlighted in my post above, I misread his code and he did initialize it as a local. Your assertion is deluding. Memory in the Heap is not as tightly managed as in the stack. The stack gets gced after the scope ends. On the other hand, variables located in the heap are not as tightly managed as variables in the stack. They still get gced, don’t get me wrong, but not as quickly. I can’t help to contradicting this assertion as I don’t see it being valid, if you could provided some proof/articles backing your position that would be nice.
That is my fault, I worded it wrong.
table.insert() for appending is unneeded, it is calling a function just to achieve the equivalent as table[#table+1] = value. The analogy you made is definitely not an incredible one, it holds some similitude, but, .Magnitude is very cleanly achieving the same as the distance formula, in this case I would use .Magnitude instead of writing a whole formula down. Also, .Magnitude isn’t calling an extraneous function, it is indexing a property. For “inserting” values into tables, I fully agree with use of table.insert(), but for appending I don’t.
The GC assertion isn’t really accurate. As far as Lua’s GC is concerned, a GC-able object is the same whether it has references from the stack or elsewhere. This wouldn’t matter anyways as it doesn’t have a real impact.
The reason local is used is more about consistency and scope management than it is about miniscule performance boosts.
As for the last example, both have a built in function already provided for you to use. I don’t see the difference. Roblox could also at any point optimize table.insert() to be faster than the indexing method, too.
I don’t see anything good about declaring a variable as a global. Why recommend declaring variables as global? When accessing memory is slower in the heap, there is no guaranteed efficient use of space, memory may become fragmented over time as blocks of memory are allocated, then freed, and memory in the heap isn’t memory managed as tightly. Memory leaks can also occur. On the other hand, accessing memory in the stack is faster and the space is managed by the cpu tightly, allowing memory to not be fragmented in the stack. When writing efficient and readable code, you should always declare variables as locals, there is no reason not to. In the case you do need a variable accessible by all of the script you can simply do.
Re read my post. At no point did I recommend using globals. I corrected the reason as to why locals should be used. The performance impact is negligible in most cases.
Also, you seem to be confused. The Lua stack isn’t special or “managed by the cpu”. In fact, the Lua stack is just a normal heap allocated array. You aren’t seeing faster look up because of it being closer together, you’re seeing it because Lua globals are stored in a dictionary while stack values are indexed out of an array.
13:02:45.174 Pairs : 3114, Ipairs : 996886. - Server - Main:25
It was reliably quicker. There is basically no explanation, other than the explanation you brought up, to utilize pairs on an array.
Just because of a negligible boost over pairs doesn’t indicate you shouldn’t use pairs.
At the point when an individual uses ipairs for a “for loop”, the reader consequently realizes that the table given is an array. With pairs, this is not manifested.
This still doesn’t prove your point here. Just because you are looping through ipairs doesn’t mean the reader will realize you’re looping through an array.
As I highlighted in my post above, I misread his code and he did initialize it as a local. Your assertion is deluding. Memory in the Heap is not as tightly managed as in the stack. The stack gets gced after the scope ends. On the other hand, variables located in the heap are not as tightly managed as variables in the stack. They still get gced, don’t get me wrong, but not as quickly. I can’t help to contradicting this assertion as I don’t see it being valid, if you could provided some proof/articles backing your position that would be nice.
Your point here is also not sufficient. It is perfectly fine to use globals, the performance difference between stacks and globals in luau is negligible but yes, you should still use local but also should use globals when needed.
It significantly does. Onedar ran some tests and pairs was 0.1167416118551 seconds slower than ipairs for 100 elements amortized. Benchmark: Benchmark
You say that’s negligible but what happens if you wanted to misuse pairs 6 more times? You will end up seeing much more substantial performance drops in your code. Now you might say what if I only use pairs in some places and ipairs in some places? That only just brings inconsistency and miss-context to your code.
Just as perfectly Onedar explained, by the use of a correct iterator on a corresponding set will show the reader what your iterating through. Generally most normal people use pairs on sets containing non-numerical indices and people use ipairs on sets that do contain numeric indices. Such consistent pattern will bring correct context to your code.
The performance difference is pretty big. There is a huge performance drop when you define a global. This is because the stack is linear data structure while the heap is more complexly structured, its much more slower to access compared to a linear data structure. There is never a real reason to use globals. Putting performance aside, memory handling on the heap is very inefficient. Stack memory will never become fragmented whereas Heap memory can become fragmented as blocks of memory are first allocated and then freed. Putting efficiency aside, with the use of globals, your clogging up your main scope. You will run out of resources quickly.
My benchmarks: Benchmark
This is misleading and incorrect information.
The Stack is “special” and is managed by the CPU. The Stack is a linearly structured data structure while a Heap has a more hierarchical structure. The Stack does provide much faster access time than a Heap. The main reason for why the Heap has worse access time than a Stack is because Stack memory is allocated in a contiguous block whereas Heap memory is allocated in any random order. Not only that but access time is also justified by how the Heap is structured. The main problem Heap memory has is memory fragmentation and in some static languages you will generally have to de-allocate memory your self.
Notice how the stack is stored as a StkId which is a TValue*; a pointer to a C array.
Moreover, the difference between accesses would be so negligible that it is in fact the dictionary lookup delay that you’re observing between globals and locals. Globals are stored in a dictionary.
.
,