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 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
return Vector3.new(c*S,-C,s*S)
end
return Vector3.new(c*S,C,s*S)
end
local X,Y,Z = z/m,y*c,x/m
return Vector3.new((X*s-Y*Z)*S+x*C,(z*X+x*Z)*c*S+y*C,z*C-(Y*X+Z*s)*S)
end
``````

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()
end
``````

This is one of my previous implementations that I posted, however I don’t like it that much since it creates many temporary CFrame objects to get the result.

``````function RNG(Min, Max)
return Random.new():NextNumber(Min, Max)
end

function SinAndCos(Number)
return math.sin(Number), math.cos(Number)
end

local NewAxis = {}
local OldAxis = {
["X"] = Vector.X,
["Y"] =  Vector.Y,
["Z"] =  Vector.Z
}

local Root = math.sqrt(OldAxis["X"]^2 + OldAxis["Z"]^2)
local Sin1, Cos1 = SinAndCos(RNG(-math.pi, math.pi))
local Sin2, Cos2 = SinAndCos(RNG(0, Radius))

for _, Axis in ipairs({"X", "Y", "Z"}) do
NewAxis[Axis] = Vector[Axis] / Root
end
NewAxis["Y"] = Vector["Y"] * Cos1

if Root <= 1E-5 then
return Vector3.new(Cos1 * Sin2, math.sign(OldAxis["Y"]) * Cos2, Sin1 * Sin2)
else
local X = (NewAxis["X"] * Sin1 - NewAxis["Y"] * NewAxis["Z"]) * Sin2 + OldAxis["X"] * Cos2
local Y = (OldAxis["Z"] * NewAxis["X"] + OldAxis["X"] * NewAxis["Z"]) * Cos1 * Sin2 + OldAxis["Y"] * Cos2
local Z = OldAxis["Z"] * Cos2 - (NewAxis["Y"] * NewAxis["X"] + NewAxis["Z"] * Sin1) * Sin2

return Vector3.new(X, Y, Z)
end
end
``````

I wanted to rewrite your code and use actual words, might be longer but I like it better imo

``````function RNG(Min, Max)
return Random.new():NextNumber(Min, Max)
end

local CFrames = {
CFrame.lookAt(Vector3.new(), Direction),
CFrame.Angles(0, 0, RNG(-math.pi, math.pi)),
CFrame.Angles(RNG(0, Amount), 0, 0)
}

return (CFrames * CFrames * CFrames).LookVector
end
``````

this is how I would rewrite V_ntex’s code

hope this helps

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 `v` `AnUnnecessarilyLongVariableNameForTheLetterV` 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.

I didn’t even know what they were, so I named them that at the time

``````function RNG(Min, Max)
return Random.new():NextNumber(Min, Max)
end

function SinAndCos(Number)
return math.sin(Number), math.cos(Number)
end

local AxisTable = {}

local Magnitude = math.sqrt(Vector.X^2 + Vector.Z^2)
local PiSin, PiCos = SinAndCos(RNG(-math.pi, math.pi))

for _, Axis in ipairs({"X", "Y", "Z"}) do
AxisTable[Axis] = Vector[Axis] / Magnitude
end
AxisTable.Y = Vector.Y * PiCos

if Magnitude <= 1E-5 then
return Vector3.new(
)
else
return Vector3.new(
(AxisTable.X * PiSin - AxisTable.Y * AxisTable.Z) * RadiusSin + Vector.X * RadiusCos,
(Vector.Z * AxisTable.X + Vector.X * AxisTable.Z) * PiCos * RadiusSin + Vector.Y * RadiusCos,
Vector.Z * RadiusCos - (AxisTable.Y * AxisTable.X + AxisTable.Z * PiSin) * RadiusSin
)
end
end
``````

there, I made it even better(then my previous code)

actually that line isn’t wrong and the square brackets was preference, because I like how it looks
I changed it to “.”, because you apparently care about that small detail

I’m sorry, but this is just bad practice

it’s called making the code readable, putting everything on one line is just dumb

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.

oh actually that’s my fault

``````Axes.Y = Vector.Y * PiCos
``````

that’s the right line, I edited my replies

EDIT: I never defined axis and forgot to edit it into Y

first part: what do you want me to name them…, and no they are not `sin(pi)` and `cos(pi)`
they are `sin(random(-pi, pi))` and `cos(random(-pi, pi))`
you can see that by reading the code

second part: I edited the post to have that table name changed, I forgot that the plural of axis was a global variable