WeaponSystem Feedback

[AkaiHellerium (Clannerism)]

G’day, I’m looking for feedback on what I can do to improve this script, it will bug out occasionally but it is functional. Improving speed/ reducing lag would also be useful. It is the local core for a weapon system.

local PlayerService = game:GetService("Players")
local RStorage = game:GetService("ReplicatedStorage")
local UserInputS = game:GetService("UserInputService")
local RunService = game:GetService("RunService")
local MarketPlace = game:GetService("MarketplaceService")
local animationData = RStorage:WaitForChild("Animations")
local Events = RStorage:WaitForChild("Events")
local RenderReq = RStorage:WaitForChild("Render")
local gunDataModule = require(RStorage:WaitForChild("GunData"))
local gunData = gunDataModule.Weaponry 
local gunDataFunctions = gunDataModule.Functions 
local LocalPlayer = PlayerService.LocalPlayer
local Character = nil
local Humanoid = nil
local loadedAnimations = {}
local registeredWeapons = {}
local equippedWeapon = nil
local Equipped = false
local equippedWeaponInfo = {}
local equippedWeaponAnimations = {}
local equippedWeaponRegister = {}
local mouseDown = false
local canFire = true
local isFiring = false
local Reloading = false
local isHolstering = false
local weaponGUI = nil
local processedFirstChar = false
local redCursor = "rbxassetid://131718495"
local whiteCursor = "rbxassetid://131581677"
local greenCursor = "rbxassetid://131718487"
local pool = require(RStorage:WaitForChild("ClientUtil", 30):WaitForChild("ObjectPool", 30)).new(250)
local GunClient={
	['trailPool'] = pool;
	['timeout'] = {};
	['bullets'] = {};
	['returnPool'] = {};
}

function loadAnimation(AnimationObj)
	if not Character then return end
	if loadedAnimations[AnimationObj] then return loadedAnimations[AnimationObj] end
	
	local loadedAnimation = Humanoid:LoadAnimation(AnimationObj)
	loadedAnimations[AnimationObj] = loadedAnimation
end


function newRayCast(TargetCFrame)
	local ray = Ray.new(Character.Torso.CFrame.p, (TargetCFrame.p - Character.Torso.CFrame.p).unit * 300)
	return ray
end

function createRay(targetPosition, overrideInfo,hitPart)
	overrideInfo = overrideInfo or {}
	local rayDistance = ((overrideInfo.Origin or equippedWeapon.Barrel.CFrame.p) - targetPosition).magnitude
	if rayDistance > 5000 then
		rayDistance = 500
	end
	
	local a1 = Instance.new("Attachment")
	local a2 = Instance.new("Attachment")
	local trail = GunClient.trailPool:GetInstance("Trail", false, true)
	trail.Enabled = true
	local color = nil
	if overrideInfo.Color then
		color = ColorSequence.new(overrideInfo.Color.Color)
	else
		color = ColorSequence.new(equippedWeaponInfo.BulletColor.Color)
	end
	trail.Color = color
	trail.Attachment0 = a1
	trail.Attachment1 = a2
	a1.Position = targetPosition
	a2.Position = targetPosition
	a1.Parent = workspace.Terrain
	a2.Parent = workspace.Terrain
	trail.Parent = workspace.Terrain
	table.insert(GunClient.bullets, {
		t = trail,
		a1 = a1,
		a2 = a2,
		l = ((overrideInfo.Origin or equippedWeapon.Barrel.CFrame.p) - targetPosition).magnitude,
		dir = CFrame.new((overrideInfo.Origin or equippedWeapon.Barrel.CFrame.p), targetPosition),
		s = 0
	})
end

function getPartOnRay(ray)
	if ray then
		return workspace:FindPartOnRay(ray, LocalPlayer.Character, false, true)
	end
end

function updateCursor(mouse) 
	if displayHit then 
		weaponGUI.Hit.Position = UDim2.new(0,mouse.X-16,0,mouse.Y-16)
	end
	
	if mouse.Target then
		local hum = getHumanoid(mouse.Target)
		
		if hum then
			local Player = PlayerService:GetPlayerFromCharacter(hum.Parent)
			if Player then
				if Player.Team == LocalPlayer.Team then
					mouse.Icon = greenCursor
				else
					mouse.Icon = redCursor
				end
			else	
				mouse.Icon = redCursor
			end
		else
			mouse.Icon = whiteCursor
		end
	end
end

function getHumanoid(Object)
	if not Object then return end
	
	if Object.Parent:FindFirstChild("Humanoid") then
		return Object.Parent.Humanoid
	elseif Object.Parent.Parent:FindFirstChild("Humanoid") then
		return Object.Parent.Parent.Humanoid
	end
end

function animateCharacter(animationname)
	if equippedWeaponRegister and equippedWeaponRegister.Animations[animationname] then
		equippedWeaponRegister.Animations[animationname]:Play()
	end
end

function StopSpecAni(animationName)
	if equippedWeaponRegister and equippedWeaponRegister.Animations[animationName] then
		equippedWeaponRegister.Animations[animationName]:Stop()
	end
end

function StopWepAni()
	if not equippedWeaponRegister or not equippedWeaponRegister.Animations then return end
	
	for i,v in pairs(equippedWeaponRegister.Animations) do
		v:Stop()
	end
end

function playSound(soundname)
	if equippedWeaponRegister and equippedWeaponRegister.Sounds[soundname] then
		equippedWeaponRegister.Sounds[soundname]:Play()
	end
end

function runWeapon(newRunVal)
	if newRunVal then
		if Reloading or isFiring then return end
		isHolstering = true
		StopSpecAni("Idle")
		animateCharacter("Run")
		Humanoid.WalkSpeed = 24
	else
		isHolstering = false
		animateCharacter("Idle")
		StopSpecAni("Run")
		Humanoid.WalkSpeed = 16
	end
end

function UpdateWepGUI()
	if equippedWeaponRegister and equippedWeaponRegister.Ammo then
		weaponGUI.Main.Ammo.Text = equippedWeaponRegister.Ammo
		weaponGUI.Main.FiringMode.Text = "Firing mode: " .. equippedWeaponRegister.weaponType
	end
end

function shoot(mouse)
	canFire = false
	
	if equippedWeapon and Equipped then
		local targetPos = mouse.Hit.p
		local rayDistance = (equippedWeapon.Handle.Position - targetPos).magnitude
		local aimSpread = Vector3.new((targetPos.x)+(math.random(-(equippedWeaponInfo.BulletSpread/15)*rayDistance,(equippedWeaponInfo.BulletSpread/15)*rayDistance)),(targetPos.y)+(math.random(-(equippedWeaponInfo.BulletSpread/15)*rayDistance,(equippedWeaponInfo.BulletSpread/15)*rayDistance)),(targetPos.z)+(math.random(-(equippedWeaponInfo.BulletSpread/15)*rayDistance,(equippedWeaponInfo.BulletSpread/15)*rayDistance)))	
		local ray = newRayCast(CFrame.new(aimSpread))
		local hitPart,hitPosition = getPartOnRay(ray)
		local visualRay = createRay(hitPosition)
		
		if hitPart then
			local Humanoid = getHumanoid(hitPart)
			
			if Humanoid then
				weaponGUI.Hit.Visible = true
				displayHit = true
				weaponGUI.hitSound:Play()
				weaponGUI.Hit.Position = UDim2.new(0,mouse.X-16,0,mouse.Y-16)
				delay(0.1, function() displayHit = false weaponGUI.Hit.Visible = false end)
			end
		end
		
		Events.GunFire:FireServer(equippedWeapon, hitPart, equippedWeapon.Barrel.Position, targetPos, equippedWeaponInfo.BulletColor)
	end
	
	spawn(function()
		equippedWeapon.Barrel.Flash.Enabled = true
		wait(0.1)
		equippedWeapon.Barrel.Flash.Enabled = false
	end)
	
	equippedWeaponRegister.Ammo = equippedWeaponRegister.Ammo - 1
	UpdateWepGUI()
	wait(equippedWeaponInfo.FireRate)
	canFire = true
end

function reload(Weapon)
	if Reloading then return end
	if isHolstering then
		runWeapon(false)
	end
	
	canFire = false
	Reloading = true
	
	animateCharacter("Reload")
	playSound("Reload")
	
	spawn(function()
		for i=1,3,1 do
			weaponGUI.Main.Ammo.Text = string.rep(".", i)
			if equippedWeaponRegister.Animations then
				wait(equippedWeaponRegister.Animations.Reload.Length/3)
			end
		end
	end)
	
	wait(equippedWeaponRegister.Animations.Reload.Length)
	
	if not equippedWeapon == Weapon then return end
	
	equippedWeaponRegister.Ammo = equippedWeaponInfo.MaxAmmo
	
	UpdateWepGUI()
	
	Reloading = false
	canFire = true
end

function fireloop(mouse, Weapon)
	if not canFire or not Equipped or not equippedWeapon == Weapon or Reloading or Humanoid.Health <= 0 then return end
	
	if mouseDown then
		if equippedWeaponInfo.Wait then
			warn("Waiting for weapon,", equippedWeaponInfo.Wait)
			playSound("Wind")
			wait(equippedWeaponInfo.Wait)
		end
		if isHolstering then
			runWeapon(false)
		end
		
		if equippedWeaponRegister.weaponType == "Auto" then
			while mouseDown and Equipped and equippedWeapon == Weapon and equippedWeaponRegister.Ammo > 0 and canFire and not Reloading and Humanoid.Health > 0 do
				isFiring = true
				shoot(mouse)
				isFiring = false
			end
		elseif equippedWeaponRegister.weaponType == "Semi" and equippedWeaponRegister.Ammo > 0 then
			isFiring = true
			shoot(mouse)
			isFiring = false
		end
		
		if equippedWeaponRegister.Ammo <= 0 then
			playSound("EmptyMag")
			reload(Weapon)
		end
	end
end

function equip(mouse, Weapon)
	if not registeredWeapons[Weapon] then warn("Akai: This weapon is Unregistered") return end
	if Humanoid.Health <= 0 then return end
	
	equippedWeapon = Weapon
	equippedWeaponInfo = registeredWeapons[Weapon].weaponInfo
	equippedWeaponRegister = registeredWeapons[Weapon]
	Equipped = true
	
	weaponGUI.Main._.Border.BackgroundColor3 = equippedWeaponInfo.BulletColor.Color
	weaponGUI.Main._.Border2.BackgroundColor3 = equippedWeaponInfo.BulletColor.Color
	weaponGUI.Main.WeaponName.Text = equippedWeapon.Name
	weaponGUI.Main.Visible = true
	
	UpdateWepGUI()

	if not equippedWeaponRegister.Animations["Idle"] then
		local specificAnimations = gunDataFunctions.GetSpecificAnimations(equippedWeaponInfo.SpecificType)

		for i,v in pairs(specificAnimations) do
			local loadedAnimation = loadAnimation(v)
			equippedWeaponRegister.Animations[v.Name] = loadedAnimation
		end
	end
	
	if not equippedWeaponRegister.Sounds["Fire"] then
		for i,v in pairs(Weapon.Handle:GetChildren()) do
			if v:IsA("Sound") then
				equippedWeaponRegister.Sounds[v.Name] = v
			end
		end
	end
	
	animateCharacter("Idle")

	mouse.Button1Down:Connect(function()
		mouseDown = true
		if equippedWeaponRegister.Warmup then
			wait(equippedWeaponRegister.Warmup)
		end
		fireloop(mouse, Weapon)
	end)
	
	mouse.Button1Up:Connect(function()
		mouseDown = false
	end)
	
	mouse.Move:Connect(function()
		updateCursor(mouse)
	end)
end

function unEquip()
	if Humanoid.Health <= 0 then return end
	
	Humanoid.WalkSpeed = 16
	StopWepAni()
	Equipped = false
	equippedWeapon = nil
	equippedWeaponInfo = {}
	equippedWeaponRegister = {}
	weaponGUI.Main.Visible = false
end

function register(Weapon)
	if registeredWeapons[Weapon] then return end
	
	local localWeaponInfo = gunData[Weapon.Name]
	
	if localWeaponInfo then
		Weapon:WaitForChild("Barrel"):WaitForChild("Flash").Color = ColorSequence.new(localWeaponInfo.BulletColor.Color)
		
		local equippedConnection = Weapon.Equipped:Connect(function(mouse)
			equip(mouse, Weapon)
		end)
		
		local unequipConnection = Weapon.Unequipped:Connect(unEquip)
		
		registeredWeapons[Weapon] = {
			["equippedConnection"] = equippedConnection;
			["unequippedConnection"] = unequipConnection;
			["Ammo"] = localWeaponInfo.MaxAmmo;
			["weaponInfo"] = localWeaponInfo;
			["weaponType"] = localWeaponInfo.WeaponType;
			["Animations"] = {};
			["Sounds"] = {};
		}
	else
		warn("Akai: Weapon unable to register: " .. Weapon.Name .. ", no reference in GunData.")
	end
end

local ChatEnabled = true

function keyDown(Key)
	if Key == Enum.KeyCode["Z"] and not Reloading then
		ChatEnabled = not ChatEnabled
		game:GetService("StarterGui"):SetCoreGuiEnabled(Enum.CoreGuiType.Chat, ChatEnabled)
	elseif Key == Enum.KeyCode["R"] and not Reloading then
		if Equipped and equippedWeaponRegister.Ammo < equippedWeaponInfo.MaxAmmo then
			reload(equippedWeapon)
		end
	elseif Key == Enum.KeyCode["V"] and not Reloading and not isFiring then
		if Equipped and equippedWeaponInfo.SpecificType:lower():match("auto") then
			if equippedWeaponRegister.weaponType == "Auto" then
				animateCharacter("Switch")
				equippedWeaponRegister.weaponType = "Semi"
			elseif equippedWeaponRegister.weaponTypr == "Semi" then
				animateCharacter("Switch")
				equippedWeaponRegister.weaponType = "Auto"
			end
			
			UpdateWepGUI()
		end
	elseif Key == Enum.KeyCode["F"] and not Reloading and not isFiring then
		if Equipped then
			if not isHolstering then
				runWeapon(true)
			else
				runWeapon(false)
			end
		end
	end
end

function userInput(inputObject, GPE)
	if GPE then return end
	
	if inputObject.UserInputType == Enum.UserInputType.Keyboard then
		keyDown(inputObject.KeyCode)
	end
end

function newchar(newCharacter)
	loadedAnimations = {}
	registeredWeapons = {}
	
	local Backpack = LocalPlayer:WaitForChild("Backpack")
	local PlayerGui = LocalPlayer:WaitForChild("PlayerGui")
	
	Character = newCharacter
	Humanoid = newCharacter:WaitForChild("Humanoid")
	
	weaponGUI = PlayerGui:WaitForChild("GunGUI")
	
	Backpack.ChildAdded:Connect(function(child)
		if child:IsA("Tool") then
			register(child)
		end
	end)
	
	Backpack.ChildRemoved:Connect(function(Child)
		if Child:IsA("Tool") then
			if Child.Parent == Character then return end

			if registeredWeapons[Child] then
				registeredWeapons[Child]["equippedConnection"]:Disconnect()
				registeredWeapons[Child]["unequippedConnection"]:Disconnect()
				registeredWeapons[Child] = nil
			end
		end
	end)
	
	Humanoid.Died:Connect(function()
		Humanoid:UnequipTools()
		
		for i,v in pairs(Backpack:GetChildren()) do
			if registeredWeapons[v] then
				registeredWeapons[v]["equippedConnection"]:Disconnect()
				registeredWeapons[v]["unequippedConnection"]:Disconnect()
			end
		end
		
		registeredWeapons = {}
		equippedWeapon = nil
		equippedWeaponInfo = {}
		equippedWeaponRegister = {}
		Equipped = false
	end)
	
	for i,v in pairs(Backpack:GetChildren()) do
		if v:IsA("Tool") then
			if not registeredWeapons[v] then
				register(v)
			end
		end
	end
end

function hRender(Target, Origin, Color)
	createRay(Target, {Origin = Origin, Color = Color})
end


RenderReq.OnClientEvent:Connect(hRender)
LocalPlayer.CharacterAdded:Connect(newchar)
UserInputS.InputBegan:Connect(userInput)

repeat wait() until LocalPlayer.Character

if not processedFirstChar then
	newchar(LocalPlayer.Character)
end

game:GetService("RunService"):BindToRenderStep("GunClientThread", Enum.RenderPriority.Camera.Value, function(delta)
	for i, v in next, GunClient.bullets, nil do
		local new = 850 * v.s
		if v.s >= 0 then
			if v.ready then
				v.t.Enabled = false
				game.Debris:AddItem(v.a1, v.t.Lifetime + 1)
				game.Debris:AddItem(v.a2, v.t.Lifetime + 1)
				table.insert(GunClient.returnPool, {
					tick = tick(),
					lifetime = v.t.Lifetime + 1,
					pool = pool,
					obj = v.t
				})
				table.remove(GunClient.bullets, i)
			else
				local fd = math.min(new, v.l)
				v.a1.Position = v.dir * Vector3.new(0.1, 0, -fd)
				v.a2.Position = v.dir * Vector3.new(-0.1, 0, -fd)
				v.ready = fd >= v.l
			end
		end
		v.s = v.s + delta
	end
	for i, v in next, GunClient.timeout, nil do
		if tick() - v.tick > v.lifetime then
			v.func()
			table.remove(GunClient.timeout, i)
		end
	end
	for i, v in next, GunClient.returnPool, nil do
		if tick() - v.tick > v.lifetime then
			v.pool:ReturnInstance(v.obj)
			table.remove(GunClient.returnPool, i)
		end
	end
end)

Thanks!

[unix_system]

In terms of code organisation, have you considered using an object-oriented approach?

This would mean you would be able to save state outside of variables, and this might make it easier to handle the different parts (networking, animating, shooting, etc) in an easier manner. You have a lot of variables at the top of your script which I’d recommend trying to organise into distinct “modules”.

For example:

local registeredWeapons = {}
local equippedWeapon = nil
local Equipped = false
local equippedWeaponInfo = {}
local equippedWeaponAnimations = {}
local equippedWeaponRegister = {}
local mouseDown = false
local canFire = true

can turn into

local Weapon = {
  currentWeapon = nil,
  equipped = false,
  weaponInfo = {
    animations = {},
    register = {},
  },
  canFire = false
}

This then means you can do functions that are built into that, and that limits the scope of what you’re doing - for example:

function Weapon:Fire(Position)
  print(self.equipped)
  ...
end

Hope that helps!

[kingerman88]

Regarding general code cleanup, you can use lua’s version of the ternary operator to reduce some clutter

local condition = true
local var = 0

if condition then 
    var = 5
else 
    var = 10
end

-- can turn into
local condition = true
-- variable = condition and value1 or value2
local var = (condition and 5) or 10 -- technically you don't need the ()

Other thing regarding performance in the RunService event
Since you are using global functions a lot
(table.insert, Vector3.new, etc)
You may want to consider caching functions

sample.pdf

local Vector3_new = Vector3.new
local math_min = math.min

[DarthFS]

@kingerman88

Since you are using global functions a lot
(table.insert, Vector3.new, etc)
You may want to consider caching functions

Localizing globals gives a very negligible speed boost. It’s not really worth considering.

[kingerman88]

That is true, however since you are using runService which fires around 60 times per second it’s just something to consider

[Brickman808 (NonFunctionalProgram)]

I think that the biggest issue with your current code is that it is all located in one script. This makes it very hard to read, understand and maintain. A solution to this would be to make your code object-oriented as @unix_system mentioned. This allows you to divide your code into separate classes for every object that is present in your code (a weapon, a bullet, input, et cetera). The biggest advantage of this approach is that you can place every class in a ModuleScript. This way, you can ensure that your code stays under about 100 lines of in every script, which improves maintainability and readability.

You have already done a great job by dividing functionality into functions. Classes are essentially another layer on top of that. Here is a decent tutorial on OOP for ROBLOX: All about Object Oriented Programming.

[DarthFS]

The differences are completely unnoticeable.

local Insert = table.insert

for i = 1, 10 do
	local ArrayOne = {}
	
	local Global = os.clock()
	table.insert(ArrayOne, 1)
	Global = os.clock() - Global
	
	local ArrayTwo = {}

	local Local = os.clock()
	Insert(ArrayTwo, 1)
	Local = os.clock() - Local
	
	print('GLOBAL: '..Global)
	print('LOCAL: '..Local)
end

The speed difference between a localized vs global insert is around 0.0000001. The average speed for a localized table.insert to execute is 0.00000001. Even in a RunService loop, it would make no noticeable differences. 0.0166 (RunService) is on a completely different scale than 0.00000001.

[kingerman88]

Well the speed that you are comparing is the time between one execution in which cause it definitely will be unnoticeable . However, over time it will compound hence why I brought up RunService.

---

If you are to do that test 10000 times, which is not unreasonable.
Since RunService fires 60 times a second. (60x60x3 = 10800 the executions in 3 minutes)

You actually see some performance difference.

HOWEVER: table.insert is pretty efficient and would not be what I’m looking at.
Vector3.new is a more complex function which is going to show greater performance drops.

I do agree that these are minor, however in the OP you have multiple global functions being used:

string.rep
math.random
math.min
table.insert
table.remove
Vector3.new
CFrame.new
ColorSequence.new

These will slowly add up to give a minor performance boost. (minor at best)

Also, it has been shown to be able to increase script performance as seen here.

Grass System: looking for advice on how to lower script activity

I was able to knock activity down from 15% to 13% so thanks for the advice! Will remember to do that in the future.

Code used for testing table.insert/Vector3.new

local Vector3_new = Vector3.new;
local table_insert = table.insert;

local part = script.Parent

local Global = os.clock()

local array1 = {};
for i=1, 10000 do
	table.insert(array1, 1);
	--part.Position +=  Vector3.new(0,.1,0)
end

Global = os.clock() - Global
print('GLOBAL: '..Global)


local Local = os.clock()

local array2 = {};
for i=1, 10000 do
	table_insert(array2, 1);
	--part.Position += Vector3_new(0,.1,0)
end

Local = os.clock() - Local
print('LOCAL: '..Local)

[iGottic]

Don’t use spawn or wait, they are bad practice and inaccurate, which is especially bad for a gun system, where accuracy by frame means everything.

[AkaiHellerium (Clannerism)]

Do you have any recommendations on alternate options?

[iGottic]

Try using Clonetrooper’s thread handler

[AkaiHellerium (Clannerism)]

Do you have a link for it? I can’t seem to find it.

[iGottic]

https://gist.github.com/CloneTrooper1019/538f0ab2541ef98912a2694dd8d274e7

Hope this helps.

[angrybino]

Such extremely small micro optimizations wouldn’t compound up over time.

Vector3.new is a more complex function which is going to show greater performance drops.

Please don’t make false subjective statements when you clearly don’t know what you really seem to be talking about. Here is the benchmark down below conducted a million times. This also proves my above point valid.

local vec = Vector3.new

local e = os.clock()

for i = 1, 1_000_000 do
	vec(i, i, i)
end

print(os.clock() - e)

Result: 0.095186300000023

Code used for testing table.insert/Vector3.new

You are literally testing different things than what you claim.

Sorry for the accident ping gottic

[kingerman88]

Interesting thanks for some of the clarification. I’ll edit my posts to reflect that.

Regarding optimizations, what other strategies would you use! I’m very interested to hear about them