FPS script - what to improve?

Help and Feedback Code Review
#1

Alright so, the code works 100% fine, no bugs whatsoever but I am simply not happy with it.

Code 
-- Left out some variables, because they aren't necessary

local aim_count = 0
local AIM_OFFSET = weapon:GetPrimaryPartCFrame():Inverse()*weapon.Aim.CFrame

local function aim_down_sights(aiming)
	local start = joint.C1
	local goal = aiming and joint.C0*AIM_OFFSET or CFrame.new()

	aim_count += 1

	local current = aim_count

	for i = 0, 101, 5 do
		if current ~= aim_count then
			break
		end
		RunService.RenderStepped:Wait()
		joint.C1 = start:Lerp(goal, i/100)
	end
end

local function update_arm(side)
	local glove = view_model[side .. "Glove"]
	local arm = view_model[side .. " Arm"]
	local shoulder = view_model.PrimaryPart[side .. "Shoulder"]
	local z = arm.Size.Z/2 + glove.Size.Z*2 + glove.Size.Z/2
	local cf = weapon[side].CFrame*CFrame.new(0, 0, z)*CFrame.Angles(math.pi/2, 0, 0)
	
	shoulder.C1 = cf:Inverse()*shoulder.Part0.CFrame*shoulder.C0
end

local function get_bobbing(addition, speed, modifier)
	return math.sin(os.clock()*addition*speed)*modifier
end

local walk_cycle = Spring.create()
local sway = Spring.create()

local SWAY_SPEED = 1
local SWAY_MODIFIER = 0.1

RunService.RenderStepped:Connect(function(dt)
	view_model:SetPrimaryPartCFrame(camera.CFrame)
	update_arm("Left")
	update_arm("Right")

	local character = client.Character

	if not character or not character.PrimaryPart or not character:FindFirstChild("Humanoid") then
		return
	end

	local velocity = character.PrimaryPart.Velocity

	local is_rmb_down = UserInputService:IsMouseButtonPressed(Enum.UserInputType.MouseButton2)
	local is_rmb_up = not is_rmb_down

	local delta = UserInputService:GetMouseDelta()
	sway:shove(Vector3.new(delta.X/(800*(is_rmb_up and 0.5 or 1)), delta.Y/(800*(is_rmb_up and 0.5 or 1)), 0))

	local movement_sway = Vector3.new(
		get_bobbing(-10, SWAY_SPEED, SWAY_MODIFIER),
		get_bobbing(5, SWAY_SPEED, SWAY_MODIFIER),
		get_bobbing(5, SWAY_SPEED, SWAY_MODIFIER)
	)

	walk_cycle:shove((movement_sway/100)*dt*60*velocity.Magnitude*(character.Humanoid.WalkSpeed/16))
	local _sway = sway:update(dt)
	local _walk_cycle = walk_cycle:update(dt)

	view_model:SetPrimaryPartCFrame(camera.CFrame)
	view_model:SetPrimaryPartCFrame(
		view_model:GetPrimaryPartCFrame():ToWorldSpace(
			CFrame.new(_walk_cycle.X/(2*(is_rmb_down and 1 or 0.1)), _walk_cycle.Y/(2*(is_rmb_down and 1 or 0.1)), 0)
		)
	)
	view_model:SetPrimaryPartCFrame(view_model:GetPrimaryPartCFrame()*CFrame.Angles(0, -_sway.X, _sway.Y))
	view_model:SetPrimaryPartCFrame(view_model:GetPrimaryPartCFrame()*CFrame.Angles(0, _walk_cycle.Y, _walk_cycle.X))
end)

UserInputService.InputBegan:Connect(function(input, engine_processed)
	if engine_processed then
		return
	end

	if input.UserInputType == Enum.UserInputType.MouseButton2 then
		UserInputService.MouseIconEnabled = false
		aim_down_sights(true)
		client.Character.Humanoid.WalkSpeed /= 2
	end
end)

UserInputService.InputEnded:Connect(function(input, engine_processed)
	if engine_processed then
		return
	end

	if input.UserInputType == Enum.UserInputType.MouseButton2 then
		UserInputService.MouseIconEnabled = true
		aim_down_sights(false)
		client.Character.Humanoid.WalkSpeed *= 2
	end
end)

What I don’t like is this.

local velocity = character.PrimaryPart.Velocity

	local is_rmb_down = UserInputService:IsMouseButtonPressed(Enum.UserInputType.MouseButton2)
	local is_rmb_up = not is_rmb_down

	local delta = UserInputService:GetMouseDelta()
	sway:shove(Vector3.new(delta.X/(800*(is_rmb_up and 0.5 or 1)), delta.Y/(800*(is_rmb_up and 0.5 or 1)), 0))

I am sure I could use more variables somewhere, but i cant think right now.

This disgusts me even more.

walk_cycle:shove((movement_sway/100)*dt*60*velocity.Magnitude*(character.Humanoid.WalkSpeed/16))
	local _sway = sway:update(dt)
	local _walk_cycle = walk_cycle:update(dt)

	view_model:SetPrimaryPartCFrame(camera.CFrame)
	view_model:SetPrimaryPartCFrame(
		view_model:GetPrimaryPartCFrame():ToWorldSpace(
			CFrame.new(_walk_cycle.X/(2*(is_rmb_down and 1 or 0.1)), _walk_cycle.Y/(2*(is_rmb_down and 1 or 0.1)), 0) -- yes
		)
	)
	view_model:SetPrimaryPartCFrame(view_model:GetPrimaryPartCFrame()*CFrame.Angles(0, -_sway.X, _sway.Y))
	view_model:SetPrimaryPartCFrame(view_model:GetPrimaryPartCFrame()*CFrame.Angles(0, _walk_cycle.Y, _walk_cycle.X))

Any way for me to get this possibly into a single call? And again where should I use variables for the line with the comment? What should I name them as well?

As for the functions, I feel they clutter the code. Should I consider putting them in module scripts and requiring them from the main script?

General feedback outside of what i specifically want improved is appreciated as well please, thanks.

3 Likes
#2

Well with the first example you can cut down lines in that some things are already implied (so you don’t need to include it ) such as:

  • is_rmb_up (because the mouse can’t be both up and down at the same time)

But its important we keep delta as its own variable name as we need the delta values to be from the same fetch (in that it could be a .00001 value difference which can make a difference in scaling) in the calculation - as far as I am concerned that is all you can really cut down.

I mean its hard to fully understand from a glance as to what the second one does, but sometimes its more preferable to do these things individually for simplicity instead of just doing a single line of call. You could do it in one line but that would mean you’d need to do is solve the maths beforehand - aka create a single line formula which handles all of this.

3 Likes
#3

For all the CFrame setting, it’s a much better option to do it in one move to prevent firing property changed events with every move, and also to clean it up / make it readable.

local rmbFactor = 2*(is_rmb_down and 1 or 0.1)

local walkCframe = CFrame.new(_walk_cycle.X/rmbFactor, _walk_cycle.Y/rmbFactor, 0)

local viewCframe = camera.CFrame:ToWorldSpace(walkCframe)
viewCframe = viewCframe*CFrame.Angles(0, -_sway.X, _sway.Y)*CFrame.Angles(0, _walk_cycle.Y, _walk_cycle.X)

view_model:SetPrimaryPartCFrame( viewCframe )

You can do all of the viewCframe bit on one line but best to avoid having incredibly long lines you need to scroll for.

You’ll see I split out the factor - you want to avoid repeating any identical code, as any changes require changes in multiple places, e.g. to change from 2* to 3*. Anything where you copy-paste should be in a variable or a function.

4 Likes