Help with Simplifying Camera Module

Help and Feedback Code Review
#1

Provide an overview of:

  • What does the code do and what are you not satisfied with?
    • The module controls a camera that orbits a golf ball object.
  • What potential improvements have you considered?
    • I have been considering making this module not object oriented, since I doubt I will ever have a use for having multiple camera objects.
  • How (specifically) do you want to improve the code?
    • The code is long, and certain parts feel repetitive/unintuitive. I am still relatively new to scripting, and would like to start getting into the habit of making readable and expandable code.

For context, the GolfCam.new(ball) function takes a ball object with an instance

note: for some odd reason, this only works with events deffered

local GolfCam = {}
GolfCam.__index = GolfCam

local RunService = game:GetService("RunService")
local UserInputService = game:GetService("UserInputService")
local ContextActionService = game:GetService("ContextActionService")
local Players = game:GetService("Players")
local CollectionService = game:GetService("CollectionService")
local ReplicatedStorage = game:GetService("ReplicatedStorage")

local Maid = require(ReplicatedStorage:WaitForChild("Maid"))

function GolfCam.new(ball)
	local self = setmetatable({}, GolfCam)

	self.camera 				= workspace.CurrentCamera
	self.yAngle 				= 90
	self.xAngle 				= -30
	self.yOffset 				= 2
	self.xOffset 				= 0
	self.distance 				= 15
	self.desiredDistance		= 15
	self.distanceLerp			= 12
	self.controllerSpeed 		= 2
	self.controllerSpeedSlow 	= 0.15
	self.mouseSpeed 			= 1
	self.clampMax 				= 0
	self.clampMin 				= -90
	self.minDistance 			= 5
	self.maxDistance 			= 50
	self.zoomSpeed 				= 5
	self._controllerCamRotY 	= 0
	self._controllerCamRotX 	= 0
	self._maid 					= Maid.new()

	self.focus = ball.instance
	
	--set camera raycast filter
	self.filter = {}
	table.insert(self.filter, self.focus)
	--loops through players and adds their character to the filter
	for _, plr in ipairs(Players:GetChildren()) do
		if plr.Character then
			table.insert(self.filter, plr.Character)
		end
	end
	--filters golf balls
	for _, v in ipairs(CollectionService:GetTagged("golfball")) do
		table.insert(self.filter, v)
	end
	for _, v in ipairs(CollectionService:GetTagged("pointer")) do
		table.insert(self.filter, v)
	end

	workspace.ChildAdded:Connect(function(child)
		self:_updateRaycastFilter(child)
	end)
	
	self:enable()
	
	return self
end

function GolfCam:_clampXAngle()
	if self.xAngle > self.clampMax then
		self.xAngle = self.clampMax
	elseif self.xAngle < self.clampMin then
		self.xAngle = self.clampMin
	end
end

--switches to the golfcam and creates connections
function GolfCam:enable()

	self.camera.CameraType = Enum.CameraType.Scriptable
	local raycastParams = RaycastParams.new()
	raycastParams.FilterType = Enum.RaycastFilterType.Blacklist
	--Sets the camera's final rotation
	local function stepped(delta)
		if not self.focus or not self.focus:IsDescendantOf(game) then
			self.focus = nil
			return
		end
		self:_processInput()
		
		--makes the camera smoothly zoom when zooming with scroll wheel 
		self.distance += (self.desiredDistance - self.distance) * self.distanceLerp * math.min(delta, 1) 

		local controllerSens = self.controllerSpeed
		if UserInputService:IsKeyDown(Enum.KeyCode.LeftShift) then
			controllerSens = self.controllerSpeed * self.controllerSpeedSlow
		end
		self.yAngle += self._controllerCamRotY * controllerSens
		self.xAngle += self._controllerCamRotX * controllerSens
		self:_clampXAngle()
		local focuspos = self.focus.Position + Vector3.new(self.xOffset, self.yOffset, 0)
		self.camera.CFrame = CFrame.new(focuspos) 
			* CFrame.Angles(0, math.rad(self.yAngle), 0) 
			* CFrame.Angles(math.rad(self.xAngle), 0, 0) 
			* CFrame.new(self.xOffset, 0, self.distance)
		--Moves the camera to a new location if it's obstructed
		--raycastParams.FilterDescendantsInstances = self.filter
		--local raycastResult = workspace:Raycast(
		--	focuspos,
		--	-(focuspos - self.camera.CFrame.Position).Unit * self.distance,
		--	raycastParams
		--)
		--if raycastResult then
		--	self.camera.CFrame = CFrame.LookAt(raycastResult.Position, focuspos)
		--end
	end

	RunService:BindToRenderStep("cam", Enum.RenderPriority.Camera.Value, stepped)

	--Camera rotation
	self._maid:GiveTask(UserInputService.InputChanged:Connect(function(input)
		if input.UserInputType == Enum.UserInputType.MouseMovement then
			local delta = UserInputService:GetMouseDelta()
			self.yAngle = self.yAngle - delta.x * self.mouseSpeed
			self.xAngle = self.xAngle - delta.y * self.mouseSpeed
			self:_clampXAngle()
		end
		if input.UserInputType == Enum.UserInputType.MouseWheel then
			if (self.desiredDistance >= self.minDistance and input.Position.Z > 0) 
				or (input.Position.Z < 0 and self.desiredDistance <= self.maxDistance) then
				self.desiredDistance -= input.Position.Z * self.zoomSpeed
			end
		end
	end))

	self._maid:GiveTask(UserInputService.InputBegan:Connect(function(input)
		if input.UserInputType == Enum.UserInputType.MouseButton2 then
			UserInputService.MouseBehavior = Enum.MouseBehavior.LockCurrentPosition
		end
	end))

	self._maid:GiveTask(UserInputService.InputEnded:Connect(function(input)
		if input.UserInputType == Enum.UserInputType.MouseButton2 then
			UserInputService.MouseBehavior = Enum.MouseBehavior.Default
		end
	end))
end

function GolfCam:disable()
	self._maid:DoCleaning()
	RunService:UnbindFromRenderStep("cam")
	UserInputService.MouseBehavior = Enum.MouseBehavior.Default
end

function GolfCam:destroy()
	self:disable()
	self = nil
end

function GolfCam:_processInput()
	self._controllerCamRotX = 0
	self._controllerCamRotY = 0
	local function key(...)
		return UserInputService:IsKeyDown(...)
	end
	if key(Enum.KeyCode.Left) or key(Enum.KeyCode.A) then
		self._controllerCamRotY += 1
	end
	if key(Enum.KeyCode.Right) or key(Enum.KeyCode.D) then
		self._controllerCamRotY -= 1
	end
	if key(Enum.KeyCode.Up) or key(Enum.KeyCode.W) then
		self._controllerCamRotX += 1
	end
	if key(Enum.KeyCode.Down) or key(Enum.KeyCode.S) then
		self._controllerCamRotX -= 1
	end
end

function GolfCam:_updateRaycastFilter(child)
	if not child then
		return
	end
	if Players:GetPlayerFromCharacter(child) then
		table.insert(self.filter, child)
	elseif CollectionService:HasTag(child, "pointer") then
		table.insert(self.filter, child)
	elseif CollectionService:HasTag(child, "golfball") then
		table.insert(self.filter, child)
	end
end

return GolfCam

Currently it’s around 190 lines long and I feel like a lot of what’s happening is inefficient.
cam.rbxl (31.7 KB)

1 Like
#2

I would never say it is too long or inefficient. I am in a similar situation to you too: quite new to the idea of OOP in Roblox and the use of module scripts, so I am also learning from your code. However it doesn’t seem much different from how I would make a module.

Correct me if I am wrong please lol

#3

Thanks, I’m mostly just trying to get fresh eyes on it since I don’t yet know what’s the most “correct” way to things

1 Like
#4

can be simplified to

function GolfCam:_updateRaycastFilter(child)
	if not child then
		return
	end
	if Players:GetPlayerFromCharacter(child) or CollectionService:HasTag(child, "pointer") or CollectionService:HasTag(child, "golfball") then
		table.insert(self.filter, child)
	end
end

If your scripting in studio, I recommend turning on word wrapping in setttings.

3 Likes
#5

This function can be greatly simplified via the math.clamp function.

function GolfCam:ClampXAngle()
	self.XAngle = math.clamp(self.XAngle, self.Min, self.Max)
end

Setting self to nil doesn’t actually do anything here. Maybe you simply just wanted to remove the object’s access to any methods?

function GolfCam:Destroy()
	self:Disable()
	setmetatable(self, nil)
end

Also, I highly recommend using :GetPlayers instead of :GetChildren, since it’s more secure. Unwanted children can be parented under Players and can error your code as a result since you are attempting to index the Character each iteration.

2 Likes
#6

Anyhow, I don’t see the point of using OOP In this scenario since the player will only have one camera locally ofcourse.

#7

The camera does get destroyed and replaced with a different camera from time to time, but since there will only ever be one instance of it active at a time I’m probably just going to make it a “normal” module. Thanks, by the way!

#8

ContextActionService is unused, so get rid of it.

Rename distanceLerp to distanceLerpSpeed to communicate that it’s a speed and be consistent with zoomSpeed and controllerSpeed.

Rename clampMax and clampMin to maxAngle and minAngle to communicate that it’s the limits of the angle clamped and be consistent with the maxDistance´ and minDistance`.

Rename single-letter iterator variables that refer to a known type of thing to communicate that. E.g. for _, v in ipairs(CollectionService:GetTagged("golfball")) becomes for _, golfball in ipairs(CollectionService:GetTagged("golfball")).

I like adding whitespace to visually separate logically separate pieces of the code, e.g. change

--set camera raycast filter
self.filter = {}
table.insert(self.filter, self.focus)
--loops through players and adds their character to the filter
for _, plr in ipairs(Players:GetChildren()) do
...
end

to

--set camera raycast filter
self.filter = {}
table.insert(self.filter, self.focus)

--loops through players and adds their character to the filter
for _, plr in ipairs(Players:GetChildren()) do
...
end

The comment “–loops through players and adds their character to the filter” can be made a lot shorter: “–Add all existing characters to the filter”.

Change _updateRaycastFilter to shouldBeFiltered, and make it return true/false, then have the filter-adding logic in the single place where this function is called. Also make it a static function because it no longer operates on a specific instance.

function shouldBeFiltered(instance)
	return (Players:GetPlayerFromCharacter(instance) ~= nil)
		or CollectionService:HasTag(instance, "pointer")
		or CollectionService:HasTag(instance, "golfball")
end

…

--Conditionally add all subsequent instances to the filter
workspace.ChildAdded:Connect(function(child)
	if shouldBeFiltered(child) then
		table.insert(self.filter, child)
	end
end)

In enable: stepped never gets called by its name, it only gets passed to BindToRenderStepped. You can make it a bit more readable by passing it to BindToRenderStepped as an anon function instead:

RunService:BindToRenderStep("cam", Enum.RenderPriority.Camera.Value, function(delta)
   ...
end)

In disable: your use if Maids throughout is great, but it can be even better if you put all the teardown logic inside the maid, so change enable to have these lines as well:

self.maid:GiveTask(function()
	RunService:UnbindFromRenderStep("cam")
	UserInputService.MouseBehavior = Enum.MouseBehavior.Default
end)

Then change disable to

function GolfCam:disable()
	self._maid:DoCleaning()
end

In destroy: doing self = nil doesn’t really do anything. self is just a local variable in the scope of the destroy function, so self = nil only affects the local variable. Since it’s the last line anyway and the variable immediately goes out of scope, it’s exactly the same as not doing anything. The only way you can prevent dangling references to the object is to be careful in the using code.

I doubt I will ever have a use for having multiple camera objects.

Prevent wrong usage of the class by making it a singleton:

if GolfCam.Instance then
	error()
end

local self = setmetatable({}, GolfCam)
GolfCam.Instance = self

OR

make it into a static class by not having a constructor that returns a new self object every call, and have all methods operate directly on the GolfCam object. I.e. replace GolfCam.new with this:


function GolfCam.Initialize(ball)
	GolfCam.camera 					= workspace.CurrentCamera
	GolfCam.yAngle 					= 90
	GolfCam.xAngle 					= -30
    ...

You can quickly replace all occurrences of self with GolfCam by using find-and-replace. I would also make all the methods use . instead of : to indicate that they’re static / class methods and not instance methods.

The main things you get from OOP are inheritance and encapsulation, and since you aren’t using those things then you don’t really need an OOP approach.

1 Like