Sword Rewards System - How can I make this more efficient?

I have created an inventory system and a ModuleScript is required by a ServerScript to index the player’s statistics and see what tools to add to their inventory. ToolsManager:IndexPlayer() is called whenever the player opens their inventory (to update it) or when their Character is added.

-- ModuleScript
local ToolsManager = {}
local Rewards = {
	["Chocolate Milk"] = {
		["Stat"] = "HighestTime";
		["Value"] = 100
	};

	["Burger"] = {
		["Stat"] = "HighestTime";
		["Value"] = 250
	};

	["Camo Sword"] = {
		["Stat"] = "HighestTime";
		["Value"] = 500;
		["GamepassOption"] = 10804695
	};

	["Divine Sword"] = {
		["Stat"] = "HighestTime";
		["Value"] = 1000;
		["GamepassOption"] = 10804695
	};

	["Golden Sword"] = {
		["Stat"] = "HighestTime";
		["Value"] = 2000;
		["GamepassOption"] = 10804695
	};

	["Inferno Sword"] = {
		["Stat"] = "HighestTime";
		["Value"] = 5000;
		["GamepassOption"] = 10804695
	};

	["Cosmic Sword"] = {
		["Stat"] = "HighestTime";
		["Value"] = 25000
	};

	["Void Sword"] = {
		["Stat"] = "HighestTime";
		["Value"] = 10000;
		["GamepassOption"] = 12557549
	};

	["Venomshank"] = {
		["Stat"] = "Kills";
		["Value"] = 15000
	};
	["Darkheart"] = {
		["Stat"] = "Kills";
		["Value"] = 50000
	};
	["Electric Sword"] = {
		["Stat"] = "Kills";
		["Value"] = 100000
	};
	["Santa Sword"] = {
		["GamepassOption"] = 13337064;
	};
	["Sign"] = {
		["GamepassOption"] = 13077060;
	};
	["Radio"] = {
		["GamepassOption"] = 11260445;
	};
	["Turkey Sword"] = {
		["BadgeOption"] = 2124632918;
	};
	["Firework Sword"] = {
		["BadgeOption"] = 2124660253;
	};
	["Crystal Sword"] = {
		["BadgeOption"] = 2124648944;
	};
	["Ghost Sword"] = {
		["BadgeOption"] = 2124614709;
	};
	["Haunted Sword"] = {
		["BadgeOption"] = 2124620407;
	};
	["Admin Sword"] = {
		["GroupOption"] = 7373551;
	};
	["Invisible"] = {
		["GroupOption"] = 7373551;
	};
}

local MarketplaceService = game:GetService("MarketplaceService")
local BadgeService = game:GetService("BadgeService")
local PlayerItems = {}
local PassCache = {}
local BadgeCache = {}
local GroupCache = {}

local DefaultIndex = {
	__index = function()
		return {"Sword"}
	end
}
setmetatable(PlayerItems,DefaultIndex)


function ToolsManager:GetPlayerIndex(UserID)
	return PlayerItems[tostring(UserID)]
end

local function HasPass(Player,ID)
	if PassCache[Player.UserId] and PassCache[Player.UserId][ID] == true then
		return true
	else
		local Has = MarketplaceService:UserOwnsGamePassAsync(Player.UserId,ID)
		if Has then
			local Index = PassCache[Player.UserId]
			if not Index then
				PassCache[Player.UserId] = {}
			end
			Index = PassCache[Player.UserId]
			Index[ID] = true
			return true
		end
	end
	return false
end

local function HasBadge(Player,ID)
	if BadgeCache[Player.UserId] and BadgeCache[Player.UserId][ID] == true then
		return true
	else
		local Has = BadgeService:UserHasBadgeAsync(Player.UserId,ID)
		if Has then
			local Index = BadgeCache[Player.UserId]
			if not Index then
				BadgeCache[Player.UserId] = {}
			end
			Index = BadgeCache[Player.UserId]
			Index[ID] = true
			return true
		end
	end
	return false
end

local function InGroup(Player,ID)
	if GroupCache[Player.UserId] and GroupCache[Player.UserId][ID] == false then
		return false
	else
		local In = Player:IsInGroup(ID)
		if In then
			return true
		else
			local Index = GroupCache[Player.UserId]
			if not Index then
				GroupCache[Player.UserId] = {}
			end
			Index = GroupCache[Player.UserId]
			Index[ID] = false
			return false
		end
	end
end

function ToolsManager:IndexPlayer(player) -- PlayerObject
	-- print("Received request to index " .. player.Name .. " in module")
	local Leaderstats = player:WaitForChild("leaderstats")
	-- print("Found " .. player.Name .. "'s leaderstats")
	if Leaderstats then
		-- print("Confirmed " .. player.Name .. "'s leaderstats existed")
		local LeaderstatVals = {}
		LeaderstatVals.HighestTime = Leaderstats.HighestTime.Value
		LeaderstatVals.CurrentTime = Leaderstats.Time.Value
		LeaderstatVals.Kills = Leaderstats.Kills.Value
		local ToolsPlayerHas = PlayerItems[tostring(player.UserId)] or {}
		for ToolName,ToolInfo in pairs (Rewards) do
			if ToolInfo.GamepassOption and not table.find(ToolsPlayerHas,ToolName) then
				--// Item can be obtained via gamepass
				if HasPass(player,ToolInfo.GamepassOption) then
					table.insert(ToolsPlayerHas,ToolName)
					-- print("Added " .. ToolName .. " to " .. player.Name .. "'s index because they own the gamepass for it")
				else
					--// Item cannot be obtained via gamepass
				end
			end
			if ToolInfo.BadgeOption and not table.find(ToolsPlayerHas,ToolName) then
				--// Item can be obtained via badge
				if HasBadge(player,ToolInfo.BadgeOption) then
					table.insert(ToolsPlayerHas,ToolName)
				--	 print("Added " .. ToolName .. " to " .. player.Name .. "'s index because they have the badge for it")
				end
			end
			if ToolInfo.GroupOption and not table.find(ToolsPlayerHas,ToolName) then
				--// Item can be obtained via group
				if InGroup(player, ToolInfo.GroupOption) then
					table.insert(ToolsPlayerHas,ToolName)
					-- print("Added " .. ToolName .. " to " .. player.Name .. "'s index because they have the badge for it")
				end
			end
			local StatRequired = ToolInfo.Stat
			if StatRequired then
				if LeaderstatVals[StatRequired] and not table.find(ToolsPlayerHas,ToolName) then
					if LeaderstatVals[StatRequired] >= ToolInfo.Value then
						table.insert(ToolsPlayerHas,ToolName)
						-- print("Added " .. ToolName .. " to " .. player.Name .. "'s index because they have the correct for stats for it")

					end
				else
					-- print(player.Name .. " does not have " .. StatRequired .. " in their leaderstats")
				end
			else
				-- print("No stat set for " .. ToolName)
			end

		end
		PlayerItems[tostring(player.UserId)] = ToolsPlayerHas
		return ToolsPlayerHas
	end
end

spawn(function()
	while true do
		wait(60)
		PassCache = {}
		BadgeCache = {}
		GroupCache = {}
	end
end)

return ToolsManager

Any suggestions on how to put less stress on the server while still maintaining security are welcome!

3 Likes

Are you running into performance issues currently? Seems like this should perform okay to me.

One thing I noticed is a few service methods being called like userHasBadge, userHasGamepass but not within a pcall which could be changed to give some protection against these functions erroring for whatever reason.

1 Like

There aren’t any pressing performance issues, I was just interested in making it as efficient as possible.
I forgot to wrap the service functions with a pcall—thanks for the reminder.

1 Like
  • spawn shouldn’t be used but coroutine.wrap, give this a good read: Why spawn shouldn’t be used. You may as well want to replace wait with a custom function since it’s inconsistent, the link I sent above explains why.
coroutine.wrap(function()
	while true do
		wait(60) --> replace with custom wait function
		PassCache = {}
		BadgeCache = {}
		GroupCache = {}
	end
end)()

  • Not using self

You don’t seem to be using the self keyword, so having : is bad practice and redundant, instead use .. Even though it just adds an extra “argument”, you are still not using it.

function ToolsManager:IndexPlayer(player) -- change : to .


function ToolsManager:GetPlayerIndex(UserID) -- change : to .
  • Unnecessary and expensive function calls

There is also no pointing in converting UserID to a string (luau does that automatically) but just calling an extraneous function which is expensive since string creations are expensive because luau checks if there is a copy of the string, if so it will use that copy and if there is no copy, it will create a new one. This process of checking is what makes string creation slow. Even though luau does this automatically, it is needed and you converting it to a string again is redundant and unnecessarily making it more expensive.

function ToolsManager.GetPlayerIndex(UserID)  
	return PlayerItems[tostring(UserID)] --> remove to string
end

local ToolsPlayerHas = PlayerItems[tostring(player.UserId)] or {} --remove to string
  • Incorrect use of WaitForChild and unnecessary checking.

You do realize that the server is authoritative and waiting for something is redundant. Only should you wait for an instance to load is on the client when the server has to replicate all the necessary instances to the client.

local Leaderstats = player:WaitForChild("leaderstats") --> remove wait for child

Also, leaderstats will always be a truthy value, even though the WaitForChild call you have on your leaderstats is redundant, it will go in a infinite yield warning you if leaderstats wasn’t found in under 10 seconds.

if Leaderstats then -- remove this line, redundant
  • Unnecessary table function calls

table.find is inefficient and expensive, because it loops through the given table finding the argument, (only proves useful when working with arrays because you don’t know at which index a value is located at). Assuming ToolsPlayerHas is a dictionary, calling table.find is expensive and unnecessary when you can just index the dictionary and check if it is nil or not.

if ToolInfo.GamepassOption and not table.find(ToolsPlayerHas,ToolName) then

--[[

if ToolInfo.GamepassOption and not ToolsPlayerHas.ToolName then

]]

Calling table.insert just to append a value is redundant when you can just add it your self. Also it adds an element numerically at the end of a table (numerically).

table.insert(ToolsPlayerHas,ToolName) --> ToolsPlayerHas[ToolName] = ToolName

Note: If ToolsPlayerHas is an array, it should be an dictionary for easier access because every time you would have to find an element in an array, you would either have to use table.find which is expensive or guess the key at which value is located which is also inefficient.

  • Not pcalling asynchronous functions

Functions with async at the end are known as asynchronous functions which when called, send a HTTP request to a specific API and when the API’s server responds back to that request, that respond may be an error (usually invalid arguments or general some API error).

local Has = MarketplaceService:UserOwnsGamePassAsync(Player.UserId,ID)

--[[

local success, Has = pcall(MarketplaceService.UserOwnsGamePassAsync, MarketplaceService, Player.UserId, ID)


local success, Has = pcall(BadgeService.UserHasBadgeAsync, BadgeService, Player.UserId, ID)

local success, inGroup = pcall(Player.IsInGroup, Player, Player.UserId)

]]

Additionally, you may also want to check if the pcall was a success or not and then handle the errors.

  • Syntax Sugars

table.keyWithNoSpaces is syntax sugar for table["keyWithNoSpaces"]. The luau compiler doesn’t optimize indexing dictionaries with ["key"] so you should instead index a dictionary directly via dictionary.key. Exceptions include if the key has spaces in between where you would have to index it via the native method, ["key"].

local dict = { a = 5 , ["keyWith Space"] = 123 }

local a = dict["a"] -- bad, not optimized
local a = dict.a -- good readability, optimized

-- exceptions:
local spaceKey = dict["KeyWith Space"] -- fine, necessary 
2 Likes

To add onto what silent said on the custom wait function what you are basically doing is getting a value you know is as accurate as possible in terms of incrementing (that being os.time()) and yielding the thread until then. Something like this

  local startTime = os.time() -- unix epoch
  local cycle = game:GetService("RunService"):Connect(function()
    if os.time - startTime > 60 then
       PassCache = {}
		BadgeCache = {}
		GroupCache = {}
        startTime = os.time()
    end
  end)
1 Like

Although I’m not using the self function, I prefer to use : for methods to help myself remember that it’s a method and not a property. I’ve looked into this and it is not redundant.

Also, I think you’re under the impression that ToolsPlayerHas is a dictionary. To clarify, it’s an array.

Also, I think you’re under the impression that ToolsPlayerHas is a dictionary. To clarify, it’s an array.

Please read my post a bit more clearly.

“Note: If ToolsPlayerHas is an array, it should be an dictionary for easier access because every time you would have to find an element in an array, you would either have to use table.find which is expensive or guess the key at which value is located which is also inefficient.”

Also, you’re wrong about using :. Whenever you do part:Destroy(), you are basically passing the part as self so the function knows which instance to destroy. All instances have 1 shared metatable. Using : just to make your self remember that it’s a method is not a good point to use it.

Your function calls are not methods because they don’t utilize the self argument properly.

Right, but how would this become a dictionary? What would the key/value relationship be? ToolsPlayerHas is a value of the player’s UserId in the PlayerItems dictionary. I’m not sure what the key to scope relationship would be if I turned an array of the items they have into a dictionary.

As for the use of the colon, I appreciate your input but it helps me greatly to remember functions (since it’s not technically a method) in my modules over variables.

A custom wait function should be made inside a module in ReplicatedStorage so both the client and server can use it, not just 1. Also, your method causes a memory leak and there is a much better alternative to your custom wait function.

-- Custom wait function by SilentsReplacement

local RunService = game:GetService("RunService")

local bindable = Instance.new("BindableEvent")

return function(yield : number)
    local timePassed = 0
    local connection
    
    if (yield <= 0) then return end

    connection = RunService.Heartbeat:Connect(function(deltaTime) 
        if (timePassed >= yield) then
            bindable:Fire()
            connection:Disconnect() --disconnect the connection to prevent memory leaks
        end

        timePassed += deltaTime
    end)
    
    bindable.Event:Wait() -- yield the current thread until the event has been fired
end

memory leaks can happen for many reasons, such as too many connections (such as spawning like 10000 parts for physics checks lol) or by allocating too much memory than the client or server can handle (like looping through 1 * 10 ^ math.huge). I understand what you mean with too many connections however like you suggest if you are having multiply stuff thats being wait well sorry but theres no way to tell the scheduler that without sending more events. and like he suggested in his original thing right its a infinite loop so unbinding and binding it constantly over again might stress the CPU (not cause a memory leak but still lag your method)

ok a few things I would like to mention about that wait function

  1. This is not your wait function. I’m the one that showed you this as a better alternative for that custom wait that polls. (Don’t take this as an attack I’m just saying).

  2. I wouldn’t recommend using this function in most scenarios as a complete substitute to yield your code instead of the default rblx “wait(n)” for the following reasons.

2A. There still comes the creation costs of implementing and destroying bindables which ruins the purpose of a customWait. A customWait is supposed to be accurate. Lets say you have to constantly call your customWait every 0.08 seconds. Imagine creating a bindable and connection every 0.08 seconds. The customWait that polls is technically more accurate than this anyways.

-- I did not make this function :/\
local RunService = game:GetService("RunService")

local function customWait(yield : number) --> probably place it in rep storage in a module so both the server and client can use this function
      local timePassed : number = 0

      while true do
          timePassed += RunService.Heartbeat:Wait()
          if (timePassed >= yield) then break end 
      end
end

customWait(5)

This wait function is infact more precise than the customWait made with bindables which gives this function that doesn’t use bindables more purpose.

Disclaimer: Do not use this function that doesn’t use bindables either. It ends up flooding the task scheduler with instructions each time you call RunService.Heartbeat:Wait() since this function supports polling. And polling is not good practice.

Polling is bad practice XD – by Kampfkarren
^^ Here is an article that explains why polling is not recommended in production code.

2B. “Whats wrong with flooding the task scheduler :/”

In this scenario, when you keep calling your customWait function that polls, instructions appended into the task scheduler’s primary queue, end up not being ran on time (delayed). You are using heartbeat which is good since it has the lowest priority in the task scheduler but more important functions could be using Heartbeat as well, which makes this custom wait impractical to use in game dev.

Proof that yielding adds instructions into the task scheduler:
Proof yielding adds instructions into the task scheduler XD – by Thedagz

2C. Creating a heartbeat connection each time you want to yield with your bindable customWait function doesn’t seem very practical either. Each time you create a connection, that connection runs on it’s own thread. Threads cost resources.
3. In conclusion, using the default rblx “wait(n)” would more practical to use (and if you want to yield for a frame use “runserviceHeartbeat:Wait()” to yield instead of “wait()”). An accurate custom wait would be useful in situations that require more precise accuracy. I’ve already mentioned a few reasons why that it shouldn’t be spammed.

Proof of a polling wait actually affecting in an actual game with 8K + players:
Proof of a polling wait function having negative effects XD – From one of the programmers of Tower Defense Simulator

1 Like

it constantly over again might stress the CPU

Luau doesn’t run on your CPU, it isn’t compiled to machine code, only to byte code instructions.

I don’t really understand then how it is ran then. Is it like an object file? It is meant to be the product of compilation, almost like a transcript of what is happening? Otherwise I don’t really understand how an interpreter language isn’t accessing the CPU. Take python for example, it compiles into byte code and is partially ran from its own VM but none the less it still has to be interpreted by the OS, that being the CPU. I am really interested though how it works on a deep level. Could you expand on what it means by byte code instructions? How is the code ran then?

This is not your wait function. I’m the one that showed you this as a better alternative for that custom wait that polls. (Don’t take this as an attack I’m just saying).

I accidentally posted an unedited part of my code, your function and my function are a little different. Mine doesn’t create a bindable every time the function is called like yours. The function I posted is my function, I took the same approach as you suggested via using bindables and a new thread (not poll constantly).

-- Custom wait function by SilentsReplacement

local RunService = game:GetService("RunService")

local bindable = Instance.new("BindableEvent")

return function(yield : number)
    local timePassed = 0
    local connection
    
    if (yield <= 0) then return end

    connection = RunService.Heartbeat:Connect(function(deltaTime) 
        if (timePassed >= yield) then
            bindable:Fire()
            connection:Disconnect() --disconnect the connection to prevent memory leaks
        end

        timePassed += deltaTime
    end)
    
    bindable.Event:Wait() -- yield the current thread until the event has been fired
end

Imagine creating a bindable and connection every 0.08 seconds

The thread is destroyed when the connection is disconnected, doesn’t matter. My function also doesn’t create or destroy a bindable every time it is called.

he customWait that polls is technically more accurate than this anyways.

No, both have the same accuracy.

Proof that yielding adds instructions into the task scheduler:

The terminology here is wrong, there is a difference between a task and a instruction.

  1. In conclusion, using the default rblx “wait(n)” would more practical to use (and if you want to yield for a frame use “runserviceHeartbeat:Wait()” to yield instead of “wait()”). An accurate custom wait would be useful in situations that require more precise accuracy. I’ve already mentioned a few reasons why that it shouldn’t be spammed.

wait() belongs to the secondary queue while Heartbeat:Wait() to the primary queue. Heartbeat:Wait() is much superior when comparing it towards wait() which is limited to only 30hz, not only that, it has a chance of yielding for more since all the threads in the primary queue are resumed before the secondary queue, not to mention the fact that wait() is also inconsistent in general. It’s best to stay with what is better than the other.

This is a good read: https://eryn.io/gist/3db84579866c099cdd5bb2ff37947cec

In this scenario, when you keep calling your customWait function that polls, instructions appended into the task scheduler’s primary queue, end up not being ran on time (delayed). You are using heartbeat which is good since it has the lowest priority in the task scheduler but more important functions could be using Heartbeat as well, which makes this custom wait impractical to use in game dev.

How is this again relevant to my function?

1 Like

yeah… thats why its flawed. All scripts requiring the customWait function at the same time will use that single bindable meaning that that the customWait will terminate much earlier than expected. If you have 20 scripts in your game imagine how many times bindable:Fire() is going to be called.

thats cuz of the flaw lol. Without the flaw you would have to be doing Instance.new(‘BindableEvent’). Creating objects aren’t instantaneous and so is the creation of a connection.

Ik she’s famous. But whatever she said had 0 proof. There is no mentioning of the “primary queue” or “secondary queue” on task scheduler docs? Spawn should be used only in moderation to prevent those delays she’s getting. Spawn uses wait() which comes from 30HZ pipeline. That is correct, but threads in general are expensive in the first place and should not be spammed.

Spamming 100 - 1,000,000 spawn in a “benchmark” would not even be practical in an actual game development standpoint.

Like I said, there is no mentioning on task scheduler docs on this “primary or secondary” queue. wait(5) inserts 1 instruction into the scheduler which is absolutely harmless. It’s not recommended to constantly call wait() or runService.Heartbeat:Wait() into the scheduler in the first place which would technically be polling.

I advocate the use of runServiceHeartbeat:Wait() over wait(–no arguments) when it comes to yielding for a frame. I never stated that wait(-- no arguments) should be used over runServiceHeartbeat:Wait().

oops got off-topic again lmao.

Ik she’s famous. But whatever she said had 0 proof. There is no mentioning of the “primary queue” or “secondary queue” on task scheduler docs? Spawn should be used only in moderation to prevent those delays she’s getting. Spawn uses wait() which comes from 30HZ pipeline. That is correct, but threads in general are expensive in the first place and should not be spammed.

The documentation on the task scheduler in no way is all the information regarding on how the scheduler works. What eyrn is correct, search up before making a claim that isn’t even true. Have you ever searched up “variables tutorial roblox”, does the API tell you that a local variable is allocated to the stack and etc? The answer to that is no, you would have to deeply search up for information regarding how they are stored. Same goes for learning know the task scheduler works and manages it’s tasks’. :expressionless:

Like I said, there is no mentioning on task scheduler docs on this “primary or secondary” queue. wait(5) inserts 1 instruction into the scheduler which is absolutely harmless. It’s not recommended to constantly call wait() or runService.Heartbeat:Wait() into the scheduler in the first place which would technically be polling.

Why would you NOT use something that is better than using something which isn’t good for production and can cause problems in the later time. Using spawn is just the same as putting your hand in a partially trained snake for it to bite you again, don’t set up your self for nightmares using spawn. Using coroutines’ doesn’t guarantee that it will always be resumed in 1 frame, it can also throttle but the chances of it are incredibly low when comparing it with spawn.

Also, please use the correct wording. wait(5) inserts 1 task, not instruction. It confuses the reader.

Spawn uses wait() which comes from 30HZ pipeline.

That’s just incorrect. Spawn doesn’t use wait(), it just calls the callback after 2 frames if there is no throttling in-between but yes, it does belong to the 30HZ pipline of course.

alright. let me know when you have a valid source on the task scheduler mentioning about this “primary” or “secondary” queue.

That’s why you use threads in moderation. Many other devs seen conducted “benchmarks” spamming spawn 100 x - 1,000,000 times resulting in unfair benchmarks. Threads are expensive in the first place and should not be spammed from a practical game development stand point no matter if its spawns / coroutines or a customOne.

What? Spawn has the same delay as wait(). Even stated in Task Scheduler docs.
" Similarly, avoid using spawn() or delay() as they use the same internal mechanics as wait() ."

Spamming spawn floods tasks into the scheduler because it uses wait() which yields for around 0.03 seconds or more depending on how much you spam spawn which I just stated, is not a good idea.

If you are still not convinced. Try adding wait() inside a coroutine thread. It will function the exact same way as spawn(). (The code in the coroutine will not silently error).

Spawn / Coroutines are ultimately better than custom bindable or heartbeat spawn. I would rather not create a bindable event and connection each time I want to create a new thread. In other words, spawns / coroutines are less expensive.

didn’t you recommend globals over locals and pairs to iterate through an array over ipairs like a week ago??

2 Likes

didn’t you recommend globals over locals and pairs to iterate through an array over ipairs like a week ago??

You’re just going off topic here, I never recommended globals over locals or ipairs over arrays, I just defended the use cases for them.

What? Spawn has the same delay as wait(). Even stated in Task Scheduler docs.
" Similarly, avoid using spawn() or delay() as they use the same internal mechanics as wait() . If you are still not convinced. Try adding wait() inside a coroutine thread. It will function the exact same way as spawn(). (The code in the coroutine will not silently error)."

It has the same delay as wait but it doesn’t actually use wait which was my point.

1 Like

what source did you use to know that spawn doesnt use wait()?