Review on my Weapon pause system

Alright… I probably have the most complicated weapon pause system on this universe. It took me an entire whole day to think, construct and try and try and cry until I finally got it working, however I think that the code’s structure and design can be further improved.

I’m working on a shooter game and I’m thinking of the best way to pause a weapon, it sounds simple right? Weapon.CanShoot = false, then wait(second), then Weapon.CanShoot = true? No, things
get complicated. Let me break it down:

There are currently three main scenarios where the weapon will be paused and not being able to be fired:

  • Weapon firing (fire-rate cooldown)
  • Reloading
  • Equipping

The problem is how these 3 events and clash together, what if a player reloads when he is equipping the gun? (I don’t want to disallow player to reload while equipping), let’s say the equip time is 0.5 seconds, while the reload time is 2 seconds. If the player reloads while waiting for the 0.5 seconds and the CanShoot signal becomes true after that, the player will be able to fire his/her weapon while reloading!

Moreover, I don’t want player to spam re-equipping their weapon to gain faster fire-rate. For an example, a pump action shotgun having a fire rate of 1 second per shot and a 0.5 second of equipping time. The player can simply re-equip the weapon after firing to have 50% faster fire-rate.

So here I propose you my weapon pause system, which I’d like it to be reviewed and see whether any parts of it can be improved.

The following has been slightly modified for easier readability.

local WeaponPauseDebounce = {}
local function PauseWeapon(PauseTime, ToolInstance, priority)
	-- Pause time: In seconds.
    -- ToolInstance: The tool for table index.
    -- Priority: LOWER priority will overlay the higher priority. For example: Firstly pause a weapon 
    -- with priority 1 with 1 second, then pause the weapon again with priority 2 with 0.5 second. The 
    -- weapon will not be able to fire again until priority 1 / any lower priority pauses are finished.

	local conn
	local tag = tick()
	CanShoot = false
	
   -- Start storing the weapon in table with tool instance as an index.
	if WeaponPauseDebounce[ToolInstance] == nil then
		WeaponPauseDebounce[ToolInstance] = {}
	end
	
   -- If the table is empty OR the already pausing session has a LOWER (note: smaller number 
   -- means higher priority) priority than the current one (means the NEW pausing session has a 
   -- higher priority, then put in the FIRST index of the table. Otherwise, queue it. (#table + 1))

	local index = (#WeaponPauseDebounce[ToolInstance] <= 0 or WeaponPauseDebounce[ToolInstance][1].Priority >= priority) and 1 or #WeaponPauseDebounce[ToolInstance] + 1
	print(index)
    -- Insert the data to the table with a tick tag.
	table.insert(WeaponPauseDebounce[ToolInstance], index, {Priority = priority, Tag = tag})
	print(WeaponPauseDebounce)
   
    -- Pausing the weapon.
	local TimeToUnpause = tag + PauseTime	
	conn = RunService.Stepped:Connect(function()
		if tick() > TimeToUnpause or 
			ToolInstance == nil or
			ToolInstance.Parent == nil then

			conn:Disconnect()
			--print(WeaponPauseDebounce[ToolInstance])

                    -- Since higher priority pause sessions are inserted into the first index of the table,
                    -- if there is any NEW pausing sessions that were added during the pause and it has a 
                    -- higher priority, DO NOT resume the weapon.
                    -- Plus, the tag has to be matched just in-case if they are having the same priority. 
                    -- (Spam equipping the weapon which keep pausing the weapon in the same priority.)

			if WeaponPauseDebounce[ToolInstance][1].Priority <= priority and WeaponPauseDebounce[ToolInstance][1].Tag == tag then
				warn("end")
				temptoolStates.CanShoot = true
			end
			
                   -- Remove the data from table to avoid memory leak.
			for i,v in pairs (WeaponPauseDebounce[ToolInstance]) do
				if v.Tag == tag then
					table.remove(WeaponPauseDebounce[ToolInstance], i)
					break
				end
			end
			if #WeaponPauseDebounce[ToolInstance] <= 0 then
				WeaponPauseDebounce[ToolInstance] = nil
			end

			temptoolStates = nil
			conn = nil
			TimeToUnpause = nil
			tag = nil

                    -- To see whether the table is cleaned (Memory leak), so far from my testing, no. It 
                    -- always prints 0 at the end.
			warn(#WeaponPauseDebounce)
		end
	end)
	
end

Example:

PauseWeapon(FireRate, Tool, 1)
PauseWeapon(EquipTime, Tool, 2)
PauseWeapon(ReloadTotalTime, Tool, 3)

Result:

Example1: Spam equipping: The weapon will not fire due to the last equip.
https://gyazo.com/4e121a28ee72a5f328a1197ac1d676d8

Example2: Reload DURING equipping: The weapon will not fire when reloading due to the equip.
https://gyazo.com/153a1013fead459bee7a19c486107b1d

Example3: No re-equip to gain faster fire-rate: I’ve set the equip time of this weapon to 0 and fire-rate to 1 second, as you can see how the weapon can not be fired right after re-equipping because firing has a higher priority (1) while equip has lower (2). The first shot can be fired immediately right after equip because it wasn’t firing before equipping.
https://gyazo.com/dfc060b56f72f110709388ed7608c9cd

I have documented it as detail and as confusing as possible, if there’s something that you don’t understand about, feel free to raise your quires!

Anyways, something I dislike about this code is it’s somewhat a bit too complicated. I’m not sure is everything here necessary, and this code is also supposed to be called in a pretty frequent rate such as when firing a weapon. It may be a bit expensive especially when the weapon has a really fast fire-rate (like 0.07 shots per second)? I kinda dislike the for-loop at the end of the code, which loops through the table just to get the index and remove the data, since new quires can be added to the table during the pause. So I’m wondering: Is there anything in this code that can be improved? I’d like some improvements regarding the code’s structure and design, and I’m looking forward to receive some feedback on it.

But I bet no-one is going to reply such long, boring and complicated thread lol, let’s see.

3 Likes

Code review is one of the categories who’s topics don’t get shown on the home page usually. Which is a good or bad thing depending on what you are looking for. Personally #help-and-feedback:scripting-support gets filled with like 20 spam comments of people who don’t know what people are actually asking :rofl:


Anyways lets get to the code, it’s not bad. You definitely took memory leaks into consideration which is good.

However…
If your game has 30 guns (for each player), that’s a lot of Stepped Connections which probably has more of a performance impact than having one that loops through everything.

What I would suggest is something like this

-- example
local RunService = game:GetService("RunService");

local toolData = {}
local function pause(tool, number)
	toolData[tool] = {timeLeft = number} 
end

RunService.Stepped:Connect(function(step)
	for tool, data in pairs(toolData) do
		data["timeLeft"] -= step
		if data["timeLeft"] < 0 then
			toolData[tool] = nil
		end
	end
end)

You will have to adjust this code to fit your own needs

This removes the need for you to create new connections and stop them every time a tool gets added.


As shown in the example above, you can use step instead and just reduce the value. It’s probably easier than doing tick() every time. Step is the delay between the function calls.


I wouldn’t really worry about the loop too much.


If you have any questions feel free to ask about anything. Those are just my thoughts on the code :stuck_out_tongue:

Instead of doing this in every gun, you can do it in one module/script. The module for the gun would just contain data for that gun only, everything else is done in that module script I’m going to talk about:

--// Gun Module

return {
    Reload = 4, -- Time in seconds to reload
    FireRate = 2, -- How many bullets are fired every second
    Equip = 2, -- Time in seconds to equip
    Damage = 5,
    etc...
}
--// Script / Module Script Controller

local playerCooldown = os.clock() -- Setting the timer

local gunData = require("Path to gun module")

--//

PlayerReloading:Connect(function() -- Undefined function for clarity
    if os.clock() < playerCooldown then return end -- If any action is active this line will stop the enxt liens from running
    playerCooldown = os.clock() + Reload -- Setting the playerCooldown + the time it takes to reload
    -- Do stuff
end)

PlayerFiring:Connect(function() -- Undefined function for clarity
    if os.clock() < playerCooldown then return end -- If any action is active this line will stop the enxt liens from running
    playerCooldown = os.clock() + (1 / gunData.FireRate) -- Setting the playerCooldown + the time it takes to fire
    --[[
        [1 / gunData.FireRate] calculates the seconds to wait before every shot based
        on fire rate. In this case, the fire rate is 2, so [1 / 2 = 0.5] which means
        there is a 0.5 second delay between fires, hence 2 bullets per second fire rate
    --]]
    -- Do stuff
end)

PlayerEquipping:Connect(function(newGun) -- Undefined function for clarity
    gunData = require("Path to new gun")
    playerCooldown = os.clock() + gunData.Equip
end)

This strategy ensures that you only have to do this all in one script, not in each individual tool.