Is there any memory leak or way to optimize this script?

Help and Feedback Code Review
#1

Soo i’m working on client-side inventory part, it’s terrible, i never liked it but now i have dilemma, is this script optimized? and are there any memory leaks? i’m afraid it will lag players when they will use inventory, or that some may use it for cheating

local Render = {
	["Slots"] = {}
}

local player = game:GetService("Players").LocalPlayer
local mouse = player:GetMouse()
local System = script.Parent

-- Replication
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Inventories = ReplicatedStorage.Inventories
local PlayerInventory = Inventories:WaitForChild(player.UserId)

local Inventory = PlayerInventory:WaitForChild("Inventory",8)
local SingleItems = PlayerInventory:WaitForChild("SingleItems",8)
local Armor = PlayerInventory:WaitForChild("Armor",8)

-- Gui
local InteractionGui = player.PlayerGui:WaitForChild("InteractionGui")
local Main = InteractionGui.Main
local UseButton = Main.UseButton
local DropButton = Main.DropButton

-- Modules
local ItemData = require(game:GetService("ReplicatedStorage").ObjectsData.ItemsData)
local Interactions = require(System.Interactions)

-- Important
local debounce = true
local PickAndDrop = ReplicatedStorage.Remotes.PickAndDrop
local ItemUse = ReplicatedStorage.Remotes.ItemUse

-- Creates new frame and returns it
function Render:CreateItem(Item: ValueBase) : GuiBase
	local New = System.Item:Clone()
	New.Name = Item.Name
	New.Image = ItemData[Item.Name].Image
	
	Render["Slots"][Item] = New
	
	if Item.ClassName == "IntValue" then New.Count.Text = Item.Value else New.Count.Text = "1" end
	
	return New
end

-- Enables/Disables interaction gui
function Render:InteractionGui(Position: Vector2, Item: ValueBase) : GuiBase
	if Item.ClassName == "BoolValue" and Item.Value == true then return end
	InteractionGui.Enabled = true
	if not Position then InteractionGui.Enabled = false; InteractionGui.Inspecting.Value = nil; return end
	
	Main.Position = UDim2.new(0,Position.X,0,Position.Y)
	if ItemData[Item.Name].Class == "Item" then Main.UseButton.Visible = false else Main.UseButton.Visible = true end
	
	InteractionGui.Inspecting.Value = Item
	Main.TextLabel.Text = Item.Name
end

-- Drops item that gui currently is inspecting
local function Drop()
	local Item = InteractionGui.Inspecting.Value 	
	
	if Item and debounce then
		debounce = false
		PickAndDrop:FireServer(Item)
		if Item.ClassName == "IntValue" and Item.Value <= 1 or Item.Parent == SingleItems  then InteractionGui.Enabled = false end
		task.wait(0.1)
		debounce = true
	end
end

-- Uses inspected item
local function Use()
	local Item = InteractionGui.Inspecting.Value 	
	
	if Item and debounce then
		debounce = false
		
		local Class = ItemData[Item.Name].Class
		local Frame = Render.Slots[Item]
		local UseFunction = Interactions[Class]
		
		if UseFunction then UseFunction(Item, Frame) end

		game:GetService("ReplicatedStorage").Remotes.ItemUse:FireServer(Item, mouse.Hit.Position)
		InteractionGui.Enabled = false
		
		
		task.wait(0.1)
	
		debounce = true
	end
end

DropButton.MouseButton1Down:Connect(Drop)
UseButton.MouseButton1Down:Connect(Use)
return Render
#2

make sure to disconnect those functions

1 Like
#3

Why? They are used all the time, there is only one gui, i don’t clone it

#4

Not disconnecting events is the only thing I see wrong at a quick glance /shrug

1 Like
#5

But why i should disconnect them if this gui is nor cloned nor destroyed? they are static all the time

#6

You could probably try disconnecting them when ui is not visible and reconnect them when visible. I dont do this.

#7

But why would i do it? no one is answering this question? if this Gui isn’t cloned and it’s reused then why should i do this?

#8

They are only connecting these events once, those connections don’t need to be disconnected.

1 Like
#9

the connections still listen out, please just use a once for better memory

#10

But those connections are connected once and for all, i don’t see need to disconnect them, other than making game laggy

1 Like
#11

I think I kind of get both sides here. Disconnecting an event isn’t really laggy, don’t worry about it. You asked to look for ways to optimize memory, so they’re just giving you the best way to do it. You are technically removing a few bytes or kb that an RBLXObject takes up, so i don’t know if it’s really worth it, but it shouldn’t harm anything. Quick disclaimer, though: I haven’t tested this directly, I am writing based on my own experience.

image

When it comes to waiting for children like this, things could be a little ehh. Maybe once the player’s inventory is loaded, fire an event or set an attribute in PlayerInventory to true…Then for any script that accesses the player inventory, you could just plop this code in somewhere before the main functioning occurs.

if not PlayerInventory:GetAttribute("Loaded") then
    PlayerInventory:GetAttributeChangedSignal("Loaded"):Wait()
end
2 Likes