What can I improve in the readability of my code?

Help and Feedback Code Review
, ,
#1

I have never worked in a big studio, and never seen how other professional programmers code. Is there some untold rule about luau syntax?

I have another script which I do not wish to share, which basically has all of the main functions and handlers which need to interact to the same gui, and I figured the best way is to cram everything into one script and using coroutines to break the big script into smaller ones. Is there a better way without using extra scripts?

This script which I want to be reviewed is open source, and you’re free to check out the full game yourself and download the .rblx file. It is a raytracer, albeit a slow one.

I tried my best to clean the code and adding comments where necessary.

READ ONLY IF YOU HAVE TIME!

-- Services
local RunService = game:GetService("RunService")

-- Variables
local LocalPlayer = game.Players.LocalPlayer
local Character = LocalPlayer.Character or LocalPlayer.CharacterAdded:Wait()
local Camera = workspace.Camera

local MainGUI = LocalPlayer.PlayerGui.Main
local FramesPerSecondUI = MainGUI.FramesPerSecond; local FPSupdateSpeed = 0.2
local Display = Instance.new("Frame", MainGUI)
	Display.Size = UDim2.new(0, MainGUI.AbsoluteSize.X, 0, MainGUI.AbsoluteSize.Y)

	Display.Transparency = 1
	Display.BackgroundColor3 = Color3.fromRGB(70, 70, 70)
	Display.ZIndex = 0
	
	Display.Name = "Display"
;

-- Constants
local RayDistance = 500

local PixelScale = script.PixelScale
local Bounces = script.Bounces
local Samples = script.Samples
local Paused = script.Paused
local RenderSignal = script.RenderSignal

local Colors = {
	["Button"] = Color3.fromRGB(255, 255, 255),

	["Green"] = Color3.fromRGB(160, 255, 160),
	["Red"] = Color3.fromRGB(255, 100, 100),
	["Yellow"] = Color3.fromRGB(255, 255, 110)
}

-- Functions (Library)
local function Lerp(a, b, t)
	return a * (1 - t) + b * t
end
local function Reflect(vector, normal)
	local r = vector - 2 * (vector:Dot(normal) * normal)
	return r
end

local function RandomValueToNormalDistribution()
	local theta = 2 * math.pi * math.random()
	local rho = math.sqrt(-2 * math.log(math.random()))
	return rho * math.cos(theta)
end
local function RandomDirection()
	local x = RandomValueToNormalDistribution()
	local y = RandomValueToNormalDistribution()
	local z = RandomValueToNormalDistribution()
	
	local length = math.sqrt(x^2 + y^2 + z^2)
	return Vector3.new(x/length, y/length, z/length)
end
local function RandomHemisphereDirection(Normal)
	local Direction = RandomDirection()
	return Direction * math.sign(Normal:Dot(Direction))
end

local function MultiplyColorWithColor(ColorA, ColorB)
	return Color3.new(ColorA.R * ColorB.R, ColorA.G * ColorB.G, ColorA.B * ColorB.B)
end
local function MultiplyColorWithNumber(Color, Number)
	return Color3.new(Color.R * Number, Color.G * Number, Color.B * Number)
end
local function AddColorWithColor(ColorA, ColorB)
	return Color3.new(ColorA.R + ColorB.R, ColorA.G + ColorB.G, ColorA.B + ColorB.B)
end
local function DivideColourWithNumber(Color, Number)
	return Color3.new(Color.R / Number, Color.G / Number, Color.B / Number)
end

-- Init

local PixelGrid = {}
local PixelInit = true -- By default, should be false because pixels haven't initialized. Set to true because the initializepixels function will automatically set it to false.
local function InitializePixels()
	if PixelInit == true then
		PixelInit = false
		
		Display:ClearAllChildren()
		for x = 1, math.floor(Display.Size.X.Offset/PixelScale.Value) do
			PixelGrid[x] = {}
			for y = 1, math.floor(Display.Size.Y.Offset/PixelScale.Value) do
				local PixelFrame = Instance.new("Frame", Display)
				PixelFrame.Size = UDim2.new(0, PixelScale.Value, 0, PixelScale.Value)
				PixelFrame.Position = UDim2.new(0, x * PixelScale.Value - PixelScale.Value, 0, y * PixelScale.Value - PixelScale.Value) -- Since x and y start from 1, subtraction by 1 (* PixelSize) is needed to set the first pixel position to 0 as an offset.

				PixelFrame.BackgroundColor3 = Colors.Red
				PixelFrame.BorderSizePixel = 0.1

				PixelGrid[x][y] = PixelFrame -- Store the pixel frame and coordinates in the grid
			end
			-- Insert a wait() so potato phone doesn't explode
			if x % 2 == 0 then task.wait() end
		end

		for i, v in Display:GetChildren() do
			v.BackgroundColor3 = Colors.Green
		end

		PixelInit = true
	end
end
InitializePixels()

PixelScale:GetPropertyChangedSignal("Value"):Connect(function()
	InitializePixels()
end)
MainGUI:GetPropertyChangedSignal("AbsoluteSize"):Connect(function()
	Display.Size = UDim2.new(0, MainGUI.AbsoluteSize.X, 0, MainGUI.AbsoluteSize.Y)
	InitializePixels()
end)

-- Functions
local function RenderScene(Slow : boolean?)
	if Slow then
		Camera.CameraType = Enum.CameraType.Scriptable
		Character.HumanoidRootPart.Anchored = true
	end
	
	--Run for every pixel in the display (the for loop(s))
	for x = 1, math.floor(Display.Size.X.Offset/PixelScale.Value) do
		for y = 1, math.floor(Display.Size.Y.Offset/PixelScale.Value) do
			local PixelFrame = PixelGrid[x][y]
			-- Get the pixel coordinates
			local PixelX = PixelFrame.Position.X.Offset
			local PixelY = PixelFrame.Position.Y.Offset
			local UnitRay = Camera:ScreenPointToRay(PixelX, PixelY) -- Where all the magic happens

			-- Ray Calculation
			local function Trace()
				-- Convert origin and direction to vector3s so we can edit them (UnitRay.Origin is immutable. UnitRayOrigin is mutable)
				local UnitRayOrigin = Vector3.new(UnitRay.Origin.X, UnitRay.Origin.Y, UnitRay.Origin.Z)
				local UnitRayDirection = Vector3.new(UnitRay.Direction.X, UnitRay.Direction.Y, UnitRay.Direction.Z)

				local IncomingLight = 0.5 -- Default is 0 and light the scene by default with objects. That would take too long though
				local RayColor = Color3.new(1, 1, 1) -- Default skybox colour
				
				for i = 0, Bounces.Value do
					local RayResult = workspace:Raycast(UnitRayOrigin, UnitRayDirection * RayDistance)
					
					if RayResult then
						local DiffusedDirection = (RayResult.Normal + RandomDirection()).Unit
						local SpecularDirection = Reflect(UnitRayDirection, RayResult.Normal)
						UnitRayDirection = Lerp(DiffusedDirection, SpecularDirection, RayResult.Instance.Reflectance)
						UnitRayOrigin = RayResult.Position

						if RayResult.Instance:FindFirstChild("Emission") then
							IncomingLight += RayResult.Instance.Emission.Value
						end

						RayColor = MultiplyColorWithColor(RayResult.Instance.Color, RayColor)
					else
						break
					end
				end
				return MultiplyColorWithNumber(RayColor, IncomingLight)
			end

			local TotalColours = Color3.new(0, 0, 0)
			local AmountOfSamplesDone = 0
			for i = 1, Samples.Value do
				AmountOfSamplesDone += 1
				
				local TracedColour = Trace()
				TotalColours = AddColorWithColor(TotalColours, TracedColour)
			end

			PixelFrame.BackgroundColor3 = DivideColourWithNumber(TotalColours, AmountOfSamplesDone)
		end
		if Slow then task.wait() end -- Halt every coloumn. So roblox doesn't try rendering everything in a single frame and take too long and crash
	end
	
	Camera.CameraType = Enum.CameraType.Custom
	Character.HumanoidRootPart.Anchored = false
end

local deltaTimeSamples = {}
RunService.RenderStepped:Connect(function(deltaTime)
	-- FPS stuff
	table.insert(deltaTimeSamples, math.floor(1 / deltaTime))
	if #deltaTimeSamples > 20 then
		table.remove(deltaTimeSamples, 1)
	end
	
	local TotalValues = 0
	for i, v in deltaTimeSamples do
		TotalValues += v
	end

	local FPS = math.floor(TotalValues / #deltaTimeSamples)
	FramesPerSecondUI.Text = FPS

	if (FPS >= 0) and (FPS < 15) then
		FramesPerSecondUI.TextColor3 = Colors.Red
	elseif (FPS >= 15) and (FPS < 30) then
		FramesPerSecondUI.TextColor3 = Colors.Yellow
	elseif (FPS >= 30) then
		FramesPerSecondUI.TextColor3 = Colors.Green
	end
	
	-- Render!
	if not Paused.Value then
		RenderScene()
	end
end)

RenderSignal:GetPropertyChangedSignal("Value"):Connect(function()
	if Paused.Value == true then
		RenderScene(true)
	end
end)

The game link and the rblx file:

RobloxRaytracing [13].rbxl (88.5 KB)

#2

some general improvements you can make yourself:

print(5 // 2) --output: 2

You can also test your luck with AI to simplify, but it can spew nonsense most of the time.

Also don’t put everything into one script lol

#3

Don’t throw everything in one script! D:

Think about it like a book. You wouldn’t want to create a series of books, collapse them into one text, and remove the chapter breaks!

In terms of formatting code, the biggest tip I have is to code like you write. Good, readable code should read like a sentence. Anyone should be able to pick up your code and understand immediately what is going on. They shouldn’t have to re-read, squint, double check, etc. So, your comments should be detailed and placed often.
Your variable names should be descriptive (they are as much for you as for any random person who might suddenly have to look at it; remember, any solo project can become a team project, you never know).

Other good tips are to avoid if .... then elseif ... then elseif ... blackholes. I prefer to check for things in the negative and separate each check out.

local PlayerIsAlive = ...
local PlayerIsTheChampion = ...

--// Rather than
if PlayerIsAlive then
    if PlayerIsTheChampion then
        DoThingForSpecialPlayer()
    else
        DoThingForNormalPlayer()
    end
else
    SpectatePlayer()
end

--// Do something cleaner like
if not PlayerIsAlive then
    SpectatePlayer()

    return
end

--// Handle all logic related to the player being alive below
if not PlayerIsTheChampion then
    DoThingForNormalPlayer()

    return
end

--// Handle all logic for the champion
DoThingForSpecialPlayer()

Yes, it’s a bit longer, but you aren’t being charged by the line. It’s also way easier to maintain, since if you want to make modifications to the behavior or tweak the logic, you don’t have to worry about accidentally messing up a crazy conditional tree. This way also reads much more logically. Conditional trees read like a run-on sentence.

One other note, when doing math - separate out your steps and show your work! It’s incredibly challenging (for you or anyone reviewing your code to reverse engineer your math, just because you wanted to flex and through it one line ex: (6^2 - (7 * (13%5))). You quickly will run into issues of “how did I get that number” and “why did I do add [x] to it?”.

#4

So in this case, what should I separate? I guess the FPS counter, but what else?

The basic flow chart of the script would be
Variables → Create display → Color display

  • Check for changes in resolution of display
  • Check for changes in variables

What my script is doing in the render scene function is applying the colors immediately
It renders the pixel and applies the color in the same pass, instead of two passes. What I mean about pass is using a for loop to loop over the entire grid. And two passes will do

  1. Loop over the grid to calculate each pixel’s color
  2. When done, loop over the grid again to update the display with the pixel color
    Less efficient but readable.

I also do not mean to flex when making mathematical functions all in one line, I validated these functions and I just plugged in formulas that’s all.

Thanks so much for your help too! I probably would continue with my horrible habits of writing everything in one script instead of splitting them thanks to you guys :slight_smile: