I’m still new to the advanced bits of scripting and I am wondering whether this code could possibly be modified to be cleaner and more efficient. It is currently inside a localScript in StarterPlayerScripts.
(For context the game is locked first person most of the time)
function moveCamera(part, returnTime, subject)
if returnTime == 'reset' then resetCamPosition() return end
plr.CameraMode = Enum.CameraMode.Classic
plr.CameraMinZoomDistance = 5 --Setting out of first person
for _,part in pairs(plr.Character:GetChildren()) do
if part.ClassName == 'Part' or part.ClassName == 'MeshPart' then
part.LocalTransparencyModifier = 0
elseif part.ClassName == 'Accessory' then
part:FindFirstChildWhichIsA('MeshPart').LocalTransparencyModifier = 0
end
end
--Making the player visible
inputService.MouseBehavior = Enum.MouseBehavior.Default
cam.CameraType = Enum.CameraType.Scriptable
cam.CFrame = part.CFrame --Moving the camera
if returnTime ~= nil and returnTime ~= 'reset' then --While the camera is within it's time period, keep tweening it to look at its subject
for i=0, returnTime, 0.1 do
local tween = tService:Create(cam, TweenInfo.new(0.1, Enum.EasingStyle.Linear),
{CFrame = CFrame.lookAt(cam.CFrame.Position, subject.CFrame.Position)})
tween:Play()
wait(0.1)
end
resetCamPosition()
elseif returnTime == nil then
local tween = tService:Create(cam, TweenInfo.new(0.1, Enum.EasingStyle.Linear),
{CFrame = CFrame.lookAt(cam.CFrame.Position, subject.CFrame.Position)})
tween:Play()
end
end
function resetCamPosition()
cam.CameraType = Enum.CameraType.Custom
cam.CameraSubject = plr.Character:FindFirstChild("Humanoid")
plr.CameraMode = Enum.CameraMode.LockFirstPerson
plr.CameraMinZoomDistance = 0.5
wait(0.1)
inputService.MouseBehavior = Enum.MouseBehavior.LockCenter
end
Unlike the deprecated global wait, this function does not throttle and guarantees the resumption of the thread on the first Heartbeat that occurs when it is due. This function also only returns the elapsed time and nothing else.
Though it really won’t cause game breaking lag or anything.
Rest of this reply is regarding readability, here I outline 5 different things that I believe you can do to improve the readability in your code, I’ll give some reasoning below as well.
Firstly, it does not make sense to pass "reset" into the moveCamera function. Just let other functions use the resetCameraPosition() function directly. This will improve readability in the moveCamera() function, as you will be able to remove some of the guard clauses and it will improve readability in other parts of your scripts as:
resetCameraPosition() -- is more clear than
moveCamera(nil, "reset", nil) -- less clear
I generally recommend staying away from abbreviations as they will generally cause more confusion than they’re worth. For instance tService can stand for TweenService or TeleportService. It’s ambiguous and much easier to fully write everything out. Additionally your code has inconsistencies with abbreviations, moveCamera() is fully typed out but resetCamPosition() abbreviates Camera. Regardless of using abbreviations or not, code consistency is a golden rule to save time when re-reading code down the line. I also do recommend using PascalCase for services, as most people do that to indicate services, but that’s just a suggestion.
Use part:IsA("BasePart") and character:GetDescendants(), pretty straight forward, but checking BasePart will return true for both parts and mesh parts, and GetDescendants removes the need for checking accessories. If you have some tools or other parts that you don’t want to remove transparency of, I’d recommend adding guard clauses to the for loop which will make it clear what you aren’t changing.
Copy and paste can generally be placed in it’s own function or alternatively, you could use a loop for all cases and just have the returnTime be set to 0.1 in the case of it being nil (see example code)
Something I didn’t include in the image, but you use part as an argument to the moveCamera() as well an variable in the context of the for the loop, would recommend changing one of those names for clarity sake.
This is my redo of the code, I’ve added comments on things changed, for clarity I’d recommend removing those if you incorporate parts of this into production.
local Players = game:GetService("Players") -- PascalCase services
local TweenService = game:GetService("TweenService") -- Removed abbreviations
local InputService = game:GetService("InputService")
local player = Players.LocalPlayer
local camera = workspace.CurrentCamera
function moveCamera(cameraRootPart, returnTime, targetPart) -- Changed name of arguments
local returnTimePassed = typeof(returnTime) == "number"
returnTime = returnTime or .1
player.CameraMode = Enum.CameraMode.Classic -- Moved all assignments to the same location
player.CameraMinZoomDistance = 5
camera.CameraType = Enum.CameraType.Scriptable
camera.CFrame = part.CFrame
InputService.MouseBehavior = Enum.MouseBehavior.Default
for _,part in player.Character:GetDescendants() do -- redid loop
if not part:IsA("BasePart") then continue end
part.LocalTransparencyModifier = 0
end
-- Removed if statement
for i=0, returnTime, .1 do
local info = TweenInfo.new(0.1, Enum.EasingStyle.Linear)
local targetCFrame = CFrame.lookAt(camera.CFrame.Position, targetPart.CFrame.Position)
local tween = TweenService:Create(camera, info, {CFrame = targetCFrame})
tween:Play()
if not returnTimePassed then return end
task.wait(.1) -- task.wait
end
resetCameraPositions()
end
function resetCameraPosition() -- changed the name to not use abbreviations
camera.CameraType = Enum.CameraType.Custom
camera.CameraSubject = player.Character:FindFirstChild("Humanoid")
player.CameraMode = Enum.CameraMode.LockFirstPerson
player.CameraMinZoomDistance = 0.5
task.wait(0.1)
InputService.MouseBehavior = Enum.MouseBehavior.LockCenter
end
