Afternoon! Amateur coder here, looking for feedback on my beefiest script to date and some optimization tips.
I’m positive there are better ways to accomplish the actual function of the script (animated menu that launches into a cutscene), but the method I use is a little more tailored to how I design and better for a handful of things in my specific use case. This post isn’t “help find the best way ever to do this”, this post is intended for me to learn how improve the method I use.
That being said, here’s the code and some notes.
local Player = game.Players.LocalPlayer
local Mouse = game.Players.LocalPlayer:GetMouse()
local Cam = game.Workspace.CurrentCamera
Cam.CameraType = Enum.CameraType.Scriptable
local MaxTilt = 10
local OnMenu = true
local MenuNowTrue = Instance.new("BindableEvent")
local FirstTimeCut = true -- naturally you wouldn't want to watch the cutscene every time you opened the game. This variable is a placeholder and I'll get to this later.
local Intro = workspace:WaitForChild("INTRO")
local MenuCam = Intro:WaitForChild("Menu"):WaitForChild("CameraPart")
local CamParts = { -- yes, it seems a little wrong to do multiple cameras like this, but having multiple is more useful for a feature I have planned in the future
{part = workspace.CamPart1, duration = 2},
{part = workspace.CamPart2, duration = 2},
{part = workspace.CamPart3, duration = 2}
}
local function setCamToPart(part, duration)
-- This gave me some trouble but I would like to think this can't get more optimized than it currently is. A previous version had it keep waiting even after the cutscene was long done
local startTime = tick()
while tick() - startTime < duration do
Cam.CameraType = Enum.CameraType.Scriptable
Cam.CFrame = part.CFrame
task.wait()
end
end
local function runCamSequence()
for _, camData in ipairs(CamParts) do
setCamToPart(camData.part, camData.duration)
end
-- For some reason this reset is needed. Not sure why it doesn't work without it
Cam.CameraType = Enum.CameraType.Custom
end
game:GetService("RunService").RenderStepped:Connect(function()
-- This makes the camera wobble in relation to the mouse during the menu part.
if OnMenu == true then
Cam.CFrame = MenuCam.CFrame * CFrame.Angles(
math.rad(((Mouse.Y - Mouse.ViewSizeY / 2) / Mouse.ViewSizeY) * -MaxTilt),
math.rad(((Mouse.X - Mouse.ViewSizeX / 2) / Mouse.ViewSizeX) * -MaxTilt),
0)
end -- After the menu is exited does this keep running in the background? I think I avoided that everywhere else but here.
end)
local StartButton = Intro:WaitForChild("Menu"):WaitForChild("START"):WaitForChild("ClickDetector")
StartButton.MouseClick:Connect(function(Player)
OnMenu = false
MenuNowTrue:Fire()
Player.CameraMinZoomDistance = .5
Player.CameraMaxZoomDistance = .5
end)
MenuNowTrue.Event:Wait() -- I'm pretty sure I did this right so it prevents lag. In an ideal world I would this inside the above function but that caused some issues for me earlier.
if FirstTimeCut == true then
runCamSequence()
end
It be great if you could rank the readability from 1 to 10 and the efficiency from 1 to 10. Any notes you have are appreciated.
I think this is preference, but i would call it
isMenuEnabled instead of OnMenu
because booleans should start with is (according to me)
Instead of
if OnMenu == true then
I’d write
if not OnMenu then return end
This helps readability.
if FirstTimeCut == true then
Could do the same for FirstTimeCut too (Which should be isFirstTimeCut):
if not isFirstTimeCut then return end
For efficiency, since this is run every renderframe, consider doing
local rad = math.rad
local CFA = CFrame.Angles
game:GetService("RunService").RenderStepped:Connect(function()
-- This makes the camera wobble in relation to the mouse during the menu part.
if OnMenu == true then
Cam.CFrame = MenuCam.CFrame * CFA(
rad(((Mouse.Y - Mouse.ViewSizeY / 2) / Mouse.ViewSizeY) * -MaxTilt),
rad(((Mouse.X - Mouse.ViewSizeX / 2) / Mouse.ViewSizeX) * -MaxTilt),
0)
All in all I think your code is very clean as it is!
Here are some objective improvements that could be made ordered most to least important/impactful.
Your code is all over the place, and not very readable. My first step would be to make ONE function that handles this cutscene. That function, of course, can call other helper functions. But the readability here is pretty rough. The function would also allow you to only run this menu code IF the menu is going to display. Right now, you still make a bindable event if there was to be no menu showing. This is unnecessary overhead.
The whole “MenuNowTrue” thing is redundant. You can just directly call the function from where you fire this event. Also it would be more optimal to just implement a light weight, code based version of an event if ever needed instead of creating the instance. This is cleaner, and will help you avoid memory leaks down the road. (memory leak not made here because event stays parented to nil, and all references to event are collected when char is reset)
The renderstepped loop. This does continue to run in the background. While not a memory leak (even if in character scripts, roblox cleans up connections for you now), this should still be disconnected for “optimal performance”. I put that in “” because honestly, the cost of running the code to disconnect the loop probably equals the cost of the loop on your cpu regardless. A small, and unnoticeable optimization but disconnecting is technically correct.
setcamtopart function is overcomplicated/overengineered. Roblox has built in functions that do a lot of this. See updated code below.
Consistency. Some of your variables are camel cased, some aren’t. I suggest camel casing everything because ive wrote so much code my eyes are kinda just trained to be able to read it sorta?? I dont know how else to describe it. Very small problem, if even a problem at all- just caught my eye.
I have reorganized this to be more readable, as well as disconnect the loop below. Otherwise, the script seems pretty well put together.
local Player = game.Players.LocalPlayer
local Mouse = game.Players.LocalPlayer:GetMouse()
local Cam = game.Workspace.CurrentCamera
local MaxTilt = 10
local FirstTimeCut = true
local Intro = workspace:WaitForChild("INTRO")
local MenuCam = Intro:WaitForChild("Menu"):WaitForChild("CameraPart")
local StartButton = Intro:WaitForChild("Menu"):WaitForChild("START"):WaitForChild("ClickDetector") --moved this up top, always keep definitions at the top of a script. avoid defining global variables down below
local CamParts = { --perfectly reasoable way to store camera data
{part = workspace.CamPart1, duration = 2},
{part = workspace.CamPart2, duration = 2},
{part = workspace.CamPart3, duration = 2}
}
--//This code will ALWAYS have to run with or without cutscene, so it makes sense to put it up top
local connection = game:GetService("RunService").RenderStepped:Connect(function()
Cam.CFrame = MenuCam.CFrame * CFrame.Angles(
math.rad(((Mouse.Y - Mouse.ViewSizeY / 2) / Mouse.ViewSizeY) * -MaxTilt),
math.rad(((Mouse.X - Mouse.ViewSizeX / 2) / Mouse.ViewSizeX) * -MaxTilt),
0)
end)
local function setCamToPart(part, duration)
Cam.CFrame = part.CFrame
task.wait(duration)
end
local function cutscene()
Cam.CameraType = Enum.CameraType.Scriptable --moved this line out of setcamtopart because it does not need to be set back to scriptable for every part
for _, camData in ipairs(CamParts) do
setCamToPart(camData.part, camData.duration)
end
-- this reset is needed because the camera was set to scriptable in the setcamtopart function, and needs to be set to custom for roblox core scripts
Cam.CameraType = Enum.CameraType.Custom
end
StartButton.MouseClick:Connect(function(Player)
connection:Disconnect() --replaced onMenu variable with this. Now, we just disconnect the entire run service loop causing the camera to stop shaking when play is clicked
Player.CameraMinZoomDistance = .5
Player.CameraMaxZoomDistance = .5
if FirstTimeCut == true then
cutscene()
end
end)