I’ve been working on making a random spread from a direction (unit vector) and amount (radians). A random direction and amount is chosen to rotate by. This means that it isn’t completely random over the whole area, and that 50% of the time the result will be in the area with half the radius specified. This is intentional.
Here is my current implementation (v=vector,d=radius):
local g = Random.new()
local function spread(v,d)
local x,y,z = v.X,v.Y,v.Z
local m = (x*x+z*z)^.5
local r = g:NextNumber(-math.pi,math.pi)
local s,c = math.sin(r),math.cos(r)
local a = g:NextNumber(0,d)
local S,C = math.sin(a),math.cos(a)
if m <= 1E-5 then
if y < 0 then
local X,Y,Z = z/m,y*c,x/m
This code works well quite well, and handles the edge cases of directly up and directly down fine. This code seems pretty efficient, but I haven’t tested it. To me this code is reasonably readable, but I’m not sure how readable it is to other people. I’m mainly concerned about performance and readability, so I avoided creating temporary objects (like CFrames) to make the function shorter. What can be done to make the code more readable or efficient?
local SpreadRandom = Random.new()
local function Spread(Direction,Amount)
This has the problem that I mentioned in my reply to V_ntex. It creates many temporary objects, 2 tables for NewAxis and OldAxis, 2 Random objects for the calls to RNG, and a temporary table to iterate over (zero lines were saved?). What is the purpose of OldAxis anyways, it is just the vector stored in a table?
The NewAxis["Y"] = Vector[Axis] * Cos1 line should be removed?
Why is square brackets used for indexing the tables with constant strings which are valid identifiers? There is . for a reason.
The SinAndCos function is really unnecessary, and the name is somewhat ambiguous (sin(cos(x))?).
You weren’t very careful with the X, Y, and Z variables. X has nothing to do with x, Z has nothing to do with z, and Y has nothing to do with m. Because of this the function doesn’t really make sense, did you even test it?
Just putting the names in words doesn’t inherently provide any extra value, I could have named vAnUnnecessarilyLongVariableNameForTheLetterV and it wouldn’t matter beyond making the code horribly verbose. Names like NewAxis and OldAxis don’t really provide any meaning when they’re vectors, and not axes. Sin1, Sin2, Cos1, Cos2 obviously don’t provide any meaning beyond that they’re the result of sin and cos. Root doesn’t provide any extra meaning beyond that it is the result of sqrt, which is why in the original code I named it m (for magnitude). It doesn’t matter if you write vector as vector or v, they convey the same meaning. In this context, I think single letter abbreviations are fine, and get the convey the meaning reasonably well.
Same problem with the temporary objects, and the CFrames table is really useless.
The line that I specifically mentioned unconditionally errors, which is why I questioned its existence.
PiSin and PiCos don’t really make sense, are they supposed to be sin(pi) and cos(pi)?
Axes is a built in global variable, I would avoid creating a variable with the same name. Axes becomes a vector anyways, which makes it still confusing.
The function still creates many temporary objects, many that can be easily removed (like the Random objects that are wastefully created on every call to RNG, and the temporary table to iterate over the axes X,Y,Z).
This function still works incorrectly, because it didn’t substitute the X, Y, and Z variables correctly. Again, do you even test it? I tested it for the basic case of (0,0,1) and it worked incorrectly.
Why is it bad practice? Using one letter names is very common in mathematics. Time → t, Velocity → v, and Position → p are good examples of this.
If you wanted to store the CFrames to use in the return expression to make it shorter, it doesn’t have to be done by constructing an array of the CFrames just to obtain the CFrames from the array once. A much more sensible way of doing it would be using 3 variables to store the CFrames.