Updated with comments and how it works, sorry!
If your self.Arguments table is an array you can use ipairs instead of pairs for slightly better performance.
I’ve never seen someone remove something from a table with remove instead of table.remove but I’d add in the table. to make it clear. I realise that’s a custom function. Try to avoid keywords and this is one of the problems with asking people to review a smaller portion of a larger bit of code where referenced functions and variables are not given. Apologies.
Where you “clone” the whitelist, you’ve not cloned it and you will be editing the original whitelist table when you remove things from it. See many other posts and devhub article on how to clone a table properly, not just reference the existing one.
1 Like
Alright, thanks for your help. The last bit was really helpful, I completely forgot that it references, not clones.
Edit:
Also, remove is just local remove = table.remove, decided to shortern it.
No worries. In general it seems like a pretty lean function.
Just watch out for the connections if you expect to call this method more than once - you don’t want multiple heartbeat connections all doing the same code as one another on the same instances.
Alright. I will take that in mind.
From professional’s prespective it isn’t considered goot-at all. Heartbeat:Connect… creates a new thread ( every event does ), u should replace that with a loop which has Heartbeat:Wait() (doesn’t create a new thread but still waits , some people think it nulifies it , it does not ). Pairs is relatively slow, should be replaced with a numeric loop , I bench marked it a month or 2 ago and Numeric loop is about 3 times faster meaning its not only cheaper (cus it doesnt get the index) but also faster , a lot faster. I recommend cache-ing what it Hit, Last points and Ray points seperately (in different tables). Swordphin’s Raycast hitbox is pretty bad and inefficient too so I recommend avoiding learning or relying on his code.
1 Like
This module must be capable of handling multiple weapons’ hit detection, not just one.
Doesn’t give you the ability to receive the index and value of a dictionary.
How is this going to be useful at all? There’s no tasks I will be dealing with later in which I would need to cache the information for.
Now you would have to search both tables meaning more code.
Never said I was relying on his code, it was implied that I was using his raycasting method adapted from his references.
Please do not slander other developers without any evidence that backs up your claim.
Make sure to examine and analyse the code, this is not a review at all. You have either made claims without any further evidence or gave advice that weren’t about improving the code, just altering the functionality.
Either way, I appreciate your feedback.
1 Like
I’m thinking of posting my own Raycast hitbox since I don’t expect many people to know how to handle stuff in a single thread , it handles all multiple hit detections and is very efficient (doesn’t spawna new thread) it also has a cool feature Hitbox.Changed:Connect(function(Hit) – end) where im using shadow tables to fire when changed (doesn’t use networking). For your replies:
Pairs is about 3 times slower than numeric loop and more expensive since it gets both index and the value thats why i recommended caching Last points, Ray points and what it Hit in seperate arrays (so it can be looped through with numeric loop). As for swordphin I’m recommending to not rely/ learn from it since he himself isn’t too good of a scripter (based on his code i’ve seen) so it’s not slendering it’s really just the truth.
Numerical loop is much slower than pairs, and pairs is slower than ipairs.
Using two tables as opposed to a dictionary is also just premature optimization. This is confirmed to be bad practice by many programmers.
What it hit does not matter, I don’t know why I would need to cache it. If you’re talking about hit positions from the ray, it still doesn’t matter, because you will most likely end up with a large float number that will 99.9% of the times not match with anything in the cached positions.
Again, I still appreciate your feedback. I will receive any advice given as long as I agree with it.
2 Likes
First of all, think about what u just said pairs is a custom function which calls another function (functions calls are slow). Pairs also gets the index and the value so it can’t logically be faster than numeric loop which only gets the value it’s just not possible. Using 2 arrays instead of a dictionary is also a premature optimization? Confirmed ? I don’t think so. You are asking to optimize your code as much as possible thats why I recommended u having 1 array for ray points so u’d loop through that and cast the rays , last points in another and for hits it doesnt really matter if its an array or not really depends on what type of a hitbox u are aiming for (multiple hit/ single hit, etc…)
Numeric for loop is the fastest by quite a margin, also what prexxo said earlier about using the Heartbeat:Wait() thing im assuming his talking about creating a table of objects that need to be raycasting and then you just loop through that table with a for loop which will handle all the objects in 1 thread sort of like a main loop
1 Like
3 times faster isn’t something to be called a micro optimization. And yeah you are correct he does need the index thats why i recommended storing each data in its own array and not a dictionary to avoid pairs 
Can you link exactly the source from which you inferred pairs is 3x faster?
The new interpreter changed things significantly, also by index I meant ‘string’ index, as seen in other places in this thread.
Pairs while being used for its specific case and running similar code, takes approximately how long a numeric loop takes (the differences in speed are comparable).
Why would a numeric loop have to be used on a numerically indexed table then? Ipairs is blazingly fast in Luau and becomes the faster choice when traversing a table with the loop’s counter index.
Again, all loops are comparable in speed and differences are minuscule.
Here just bench marked it (rn so results are reliable)
> Test 1:
> Numeric Loop: 0.00041007995605469
> Pairs Loop: 0.00077247619628906
>
> Test 2:
> Numeric Loop: 0.00041007995605469
> Pairs Loop: 0.0007636547088623
>
> Test 3:
> Numeric Loop: 0.00043630599975586
> Pairs Loop: 0.00077438354492188
>
It seems that pairs was optimized (which means next is also faster since pairs is a custom function using next which looks like this :
local function pairs(Table)
return next, Table
end
)
Pairs should be avoided when possible. U may see it as a micro optimization but you must think of it as being ran hundreds of times (each time player swings with a sword as an example) which can result in noticeable results if not using numeric.
1 Like
Yeah I thought I posted the code my bad here :
local Array = {}
for i = 1, 1e5 do
Array[i] = i
end
local Start = tick()
for i = 1, #Array do
end
print(tick()-Start)
local Start = tick()
for i,v in pairs(Array) do
end
print(tick()-Start)
Also people should not avoid extra code ( very little in this case ) to optimize stuff like this. Pairs is also lua only so u could consider using numeric loop over pairs (when pairs ofc not needed ) good practice.
1 Like
Unless you’re a micro-optimization freak, go for pairs to simplify things when needed, the difference is terribly small.
Yes, but this isn’t vanilla Lua, there’s an interpreter which sped up loops a lot for different cases.
And running two loops just to avoid using pairs would prove to take longer then iterating once and getting over with it.
Please keep in mind that #help-and-feedback:code-review should not be used to discuss such negligible optimizations and that the review should be relevant to the topic at hand.
For the given code, pairs should be used, as premature optimizations are “the root of all evil”. This falls under said case.
Using next, table is by no means a representation of “good” production code.
Something many gloss over is that code must be maintained, reviewed, and augmented at a later date. “Good” code is something that can be universally understood, readable, and optimized for developer ergonomics with performance in mind, but not at the forefront of the designing process.
Premature optimizations are an anti-pattern, and should be avoided.
This is a universal pattern in programming at any layer of abstraction. Most language references, cookbooks, and handbooks–of any reputability–will advise against such a negligible optimization at the expense of the developer. This is no different for Lua, as the creator of the Lua language advises against these types of optimizations.
Tangent on for loop optimization
As a matter of fact, next, table is less performant than pairs or ipairs. This performance issue only existed for Lua JIT and, for some reason, most believe it is an issue global to all Lua code.
Also, most people don’t realize the benefit that ipairs gives you the index and value. When you incorporate a generic for loop that indexes the value, to be the same as ipairs, it is the least performant.
local array = {};
for index = 1, 1e5 do
array[index] = true;
end
local startFor = tick();
for index = 1, #array do
local value = array[index];
end
print("Generic for loop: ", tick() - startFor);
local startIpairs = tick();
for index, value in ipairs(array) do
end
print("for in ipairs loop", tick() - startIpairs);
local startPairs = tick();
for index, value in pairs(array) do
end
print("for in pairs loop", tick() - startPairs);
local startNext = tick();
for index, value in next, array do
end
print("for in next loop", tick() - startNext);
This, on average, shows in Luau (Roblox’s implementation of Lua):
Generic for loop: ~0.00072
For in ipairs loop: ~0.00044
For in pairs loop: ~0.00057
For in next, table loop: ~0.00069
It’s often a good practice to benchmark proposed optimizations in the rare chance you must perform them before implementation.
Conclusion
Do not optimize your code at such a negligible level to where it hurts your codebase’s maintainability and readability, as these are more important than whatever ‘optimizations’ you can squeeze in. This is the “professional” standpoint of things in any layer of abstraction in computer science as a study. Therefore, in this scope, disagreements about these optimizations are trivial and are a vacuum of time given that many, many people have already discussed this and come to the conclusion above.
@Procedurall’s code, under @BanTech’s advice, is performant and readable enough for production code. Until further notice, it should not be optimized unless realistic issues come of this, in which case it is not the design of this function, but the entirety of the project’s design that should be put into question.
Ergo:
“Premature optimizations are the root of all evil.”
2 Likes
Sorry for bumping but events actually do reuse the same thread if the thread isn’t yielding.
local RunService = game:GetService("RunService")
RunService.Heartbeat:Connect(function()
print(coroutine.running())
end)
Try this it always prints the same thread reference because the thread is being reused.
Every unique event creates a new thread.
Example:
local RunService = game:GetService("RunService")
local HB = RunService.Heartbeat
local Thread1
local Thread2
local Conn
Conn = HB:Connect(function()
Thread1 = coroutine.running()
Conn:Disconnect()
end)
Conn = HB:Connect(function()
Thread2 = coroutine.running()
Conn:Disconnect()
end)
wait()
print(Thread1 == Thread2) --> false
Oh I thought he meant it for the same event connection in that case yah then roblox doesn’t reuse threads. By that I mean a ( RBXScriptConnection)