How can I improve this first-person script?

Help and Feedback Code Review
#1

Hi, I’m wondering how I can improve my first-person script. I think some optimizations can come in handy. If you can help me, I will greatly appreciate it.

Localscript in StarterCharacterScripts:

local Players = game:GetService("Players")
local RunService = game:GetService("RunService")
local TweenService = game:GetService("TweenService")
local UserInputService = game:GetService("UserInputService")

local player = Players.LocalPlayer

local tweenInfo = TweenInfo.new(
	0.1,
	Enum.EasingStyle.Linear,
	Enum.EasingDirection.Out
)

local character = script.Parent
local humanoid = character:WaitForChild("Humanoid")

local function onInputBegan(input, gameProcessedEvent)
	if not gameProcessedEvent and input.KeyCode == Enum.KeyCode.V then
		if player.CameraMode == Enum.CameraMode.Classic then
			player.CameraMode = Enum.CameraMode.LockFirstPerson
		else
			player.CameraMode = Enum.CameraMode.Classic
		end
	end
end

local function onRenderStepped()
	local moveDirection = humanoid.MoveDirection
	
	if player.CameraMode == Enum.CameraMode.Classic then
		local tween = TweenService:Create(humanoid, tweenInfo, {CameraOffset = Vector3.new(0, 0, 0)}):Play()
	else
		if moveDirection.Magnitude == 0 then
			local tween = TweenService:Create(humanoid, tweenInfo, {CameraOffset = Vector3.new(0, 0, -1)}):Play()
			
		else
			local tween = TweenService:Create(humanoid, tweenInfo, {CameraOffset = Vector3.new(0, 0, -2)}):Play()
			
			if player.CameraMode == Enum.CameraMode.Classic then
				tween:Stop()
			end
		end
	end

	if UserInputService.MouseEnabled then
		local mouse = player:GetMouse()		

		if player.CameraMode == Enum.CameraMode.Classic then
			mouse.Icon = ""
		else
			mouse.Icon = "rbxassetid://10973992923"			
		end
	end
end

RunService.RenderStepped:Connect(onRenderStepped)
UserInputService.InputBegan:Connect(onInputBegan)
2 Likes
#2

Ok, here’s what you can change:

  • Use Vector3:Lerp instead of Tweening

  • Keep Constants of your Vectors so you don’t have to generate them every frame

  • Use Guard-Clauses instead of nesting If-Statements

  • Use Ternary Expressions

  • Use Player:GetPropertyChangedSignal to get changes in the CameraMode to set Mouse.Icon

  • Use RunService:BindToRenderStepped with Camera Priority instead of RunService.RenderStepped

  • Use DeltaTime to update the Lerp

With that said, here’s a rewrite I did that doesn’t require you putting it in PlayerCharacterScripts and only needs to be ran once!

local Players: Players = game:GetService("Players")
local RunService: RunService = game:GetService("RunService")
local UserInputService: UserInputService = game:GetService("UserInputService")

local Player: Player = Players.LocalPlayer or Players:GetPropertyChangedSignal("LocalPlayer"):Wait()

local Mouse: Mouse = Player:GetMouse()

local VectorZero: Vector3 = Vector3.zero
local VectorA: Vector3 = -Vector3.zAxis
local VectorB: Vector3 = Vector3.zAxis * -2

local function ChangeMouseIcon()
	if not UserInputService.MouseEnabled then return end
	Mouse.Icon = if Player.CameraMode == Enum.CameraMode.LockFirstPerson then "rbxassetid://10973992923" else ""
end

local function OnInputBegan(Input: InputObject, GameProcessedEvent: boolean)
	if Input.KeyCode ~= Enum.KeyCode.V or GameProcessedEvent then return end
	
	local CurrentMode: Enum.CameraMode = Player.CameraMode
	local IsClassic: boolean = CurrentMode == Enum.CameraMode.Classic
	
	Player.CameraMode = if IsClassic then Enum.CameraMode.LockFirstPerson else Enum.CameraMode.Classic
end


local function OnRenderStepped(DeltaTime: number)
	local Character: Model? = Player.Character
	local Humanoid: Humanoid? = if Character then Player.Character:FindFirstChildWhichIsA("Humanoid") else nil
	
	if not Character or not Humanoid then return end
	
	local MoveDirection: Vector3 = Humanoid.MoveDirection
	local CurrentMode: Enum.CameraMode = Player.CameraMode
	
	local IsClassic: boolean = CurrentMode == Enum.CameraMode.Classic
	local IsMagnitudeZero: boolean = MoveDirection.Magnitude == 0
	
	local Vector: Vector3 = if IsClassic and IsMagnitudeZero then VectorZero elseif IsMagnitudeZero then VectorA else VectorB
	
	Humanoid.CameraOffset = Humanoid.CameraOffset:Lerp(Vector, DeltaTime)
end


ChangeMouseIcon()
Player:GetPropertyChangedSignal("CameraMode"):Connect(ChangeMouseIcon)

UserInputService.InputBegan:Connect(OnInputBegan)

RunService:BindToRenderStep("Camera", Enum.RenderPriority.Camera.Value, OnRenderStepped)
4 Likes
#3

I think you have a great answer to my question, but I don’t get this part:

  • Use Guard-Clauses instead of nesting If-Statements

Is there really an advantage in this?

1 Like
#4

Yes, definitely.
Your code becomes shorter and easier to read, especially in long scripts.

2 Likes
#5

Your answer helped enormously, but there was still an error with walking with the camera offset.

How do I convert this:

	if isClassic and isMagnitudeZero then
		humanoid.CameraOffset = humanoid.CameraOffset:Lerp(vectorZero, deltaTime * 10)			
	elseif not isClassic and isMagnitudeZero then
		humanoid.CameraOffset = humanoid.CameraOffset:Lerp(vectorA, deltaTime * 10)		
	elseif isClassic and not isMagnitudeZero then
		humanoid.CameraOffset = humanoid.CameraOffset:Lerp(vectorZero, deltaTime * 10)		
	elseif not isClassic and not isMagnitudeZero then
		humanoid.CameraOffset = humanoid.CameraOffset:Lerp(vectorB, deltaTime * 10)				
	end

to this:

	local vector: Vector3 = if isClassic and isMagnitudeZero then vectorZero elseif isMagnitudeZero then vectorA else vectorB
1 Like
#6

What I usually do when my if-blocks get too big is split up the conditions into their own variables like so:

local IsClassicAndZero: boolean = if IsClassic and IsMagnitudeZero then true else false
local IsClassicOrZero: boolean = if IsClassic or IsMagnitudeZero then true else false

local IsClassicAndNotZero: boolean = if IsClassic and not IsMagnitudeZero then true else false
local IsNotClassAndZero: boolean = if IsClassic and not IsMagnitudeZero then true else false

-- Option 1 (The do-block is Syntax Sugar)
local Vector: Vector3 do
	Vector = if IsClassicAndZero or IsClassicAndNotZero then VectorZero elseif IsNotClassAndZero then VectorA else VectorB
end

-- Option 2 (Only do this if the expression gets reeeeally long)
local Vector: Vector3
if IsNotClassAndZero or IsClassicAndNotZero then Vector = VectorZero end
if IsNotClassAndZero then Vector = VectorA else Vector = VectorB end
2 Likes
#7

Check, thanks a lot for the coding tips, but I have one more issue.

local Players: Players = game:GetService("Players")

local player: Player = Players.LocalPlayer or Players:GetPropertyChangedSignal("LocalPlayer"):Wait()
local playerGui: PlayerGui = player.PlayerGui

local screenGui = script.ScreenGui
screenGui.Parent = playerGui

Random errors appear in my script: Key ‘‘x’’ not found in class ‘‘x’’? Should these errors be ignored? Is there any workaround?

1 Like
#8

Possibly because it’s trying to index PlayerGui as a property instead of a child because of the period. I think in this case you could just use WaitForChild with an assertion operator

local PlayerGui: PlayerGui = Player:WaitForChild("PlayerGui") :: PlayerGui
1 Like
#9

I think I’ll just stick with :FindFirstChild, because overusing :WaitForChild could be bad, correct me if im wrong!

You helped me a lot with not only this code, but I’m gonna use this technique every time now!

1 Like