Feedback on spread function

Help and Feedback Code Review
#1

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
			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?

#2 
local SpreadRandom = Random.new()
local function Spread(Direction,Amount)
    return (CFrame.lookAt(Vector3.new(),Direction)*CFrame.Angles(0,0,SpreadRandom:NextNumber(-math.pi,math.pi))*CFrame.Angles(SpreadRandom:NextNumber(0,Amount),0,0)).LookVector
end
#3

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.

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

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

function spread(Vector, Radius)
    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 function Spread(Direction,Amount)
    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[1] * CFrames[2] * CFrames[3]).LookVector
end

this is how I would rewrite V_ntex’s code

hope this helps

#5

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.

#6

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

function spread(Vector, Radius)
    local AxisTable = {}

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

    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(
            PiCos * RadiusSin,
            math.sign(Vector.Y) * RadiusCos,
            PiSin * RadiusSin
        )
    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

#7

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.

#8

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

#9

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
that one is my bad

that would have been better, but then I wouldn’t know what to name the variables(and we know you would tell me about how bad the names are)