Any ideas on how to improve my inventory system?

Help and Feedback Code Review
#1

Right now its not too much, but i dont really know if it will work alright in a decent made game

local script code in the gui

local player = game.Players.LocalPlayer
local mainFolder = game.ReplicatedStorage.PlayerInvetorys:WaitForChild(player.Name .. "Folder")
local inv = mainFolder:WaitForChild("Inventory")
local hb = mainFolder:WaitForChild("Hotbar")
local CurrentInv = inv:GetChildren()
local C = game.ReplicatedStorage.ItemFrame
local hoverinv = script.Parent.Parent.HoverInfo
local mouse = player:GetMouse()
local InventoryFrames = {}
print(CurrentInv)


function equipitem(item, frame)
	if not item then warn("Wheres My Gun At?") return end
	hoverinv.Visible = false
	game.ReplicatedStorage.TotallyCoolEvents.EquipGun:FireServer(item)
end

local function Hover(item)
	hoverinv.Visible = true
	if item:FindFirstChild("strings") then
		hoverinv.GunName.Text = item.Name
		hoverinv.ExpNumber.Text = item.strings.CurrentExp.Value .. " / " .. item.strings.ReqExp.Value
		hoverinv.LvlNumber.Text = item.strings.GunLevel.Value
		hoverinv.GunDmg.Text = item.strings.Damage.Value
		hoverinv.GunRPM.Text = item.strings.FireRate.Value
		hoverinv.GunAmmo.Text = item.strings.Ammo.Value .. " / " .. item.strings.StoredAmmo.Value
		hoverinv.Tal1.Text = item.strings.T1.Value .. " | Unlocks @ Level " .. item.strings.ReqT1.Value
		hoverinv.Tal2.Text = item.strings.T2.Value .. " | Unlocks @ Level " .. item.strings.ReqT2.Value
		hoverinv.Tal3.Text = item.strings.T3.Value .. " | Unlocks @ Level " .. item.strings.ReqT3.Value
		hoverinv.Tal4.Text = item.strings.T4.Value .. " | Unlocks @ Level " .. item.strings.ReqT4.Value
	end
end

function Creation(item)
	local K = C:Clone()
	InventoryFrames[item] = K
	K.Text = item.Name
	K.Visible = true
	K.Parent = script.Parent.ISF
	
	K.MouseButton1Click:Connect(function()
		print(InventoryFrames)
		equipitem(item, K)
	end)
	
	K.MouseEnter:Connect(function()
		Hover(item)
	end)
	
	K.MouseLeave:Connect(function()
		hoverinv.Visible = false
	end)
end

function Deletion(item)
	local frame = InventoryFrames[item]
	frame:Destroy()
	InventoryFrames[item] = nil
end


for i, v in ipairs(CurrentInv) do
	Creation(v)
end

inv.ChildAdded:Connect(function(Child)
	Creation(Child)
end)

inv.ChildRemoved:Connect(function(Child)
	Deletion(Child)
end)

mouse.Move:Connect(function()
	if script.Parent.Visible == true then
		hoverinv.Position = UDim2.new(0,mouse.X,0,mouse.Y)
	end
end)

Script where remote event is fired to

game.ReplicatedStorage.TotallyCoolEvents.EquipGun.OnServerEvent:Connect(function(plr, d)
	local Char = plr.Character
	if not d then return end
	if not Char then return end
	if not d:IsA("Tool") then return end
	if Char:FindFirstChildWhichIsA("Tool") then
		d.Parent = plr.Backpack
	else
		d.Parent = Char
	end
end)

And the script that creates a folder on join

game.Players.PlayerAdded:Connect(function(plr)
	local c = Instance.new("Folder", game.ReplicatedStorage.PlayerInvetorys)
	c.Name = plr.Name .. "Folder"

	local d = Instance.new("Folder", c)
	d.Name = "Inventory"
	
	local e = Instance.new("Folder", c)
	e.Name = "Hotbar"
end)

Mainly looking on stuff i could add to improve it / “make the code better”

(Edit)

Heres a video on how it currently works

(Edit 2)

A little bit more shown in this video how it works

(Edit 3 / 4)
Updated code with some help i got from

2 Likes
#2

I won’t list all the improvements that you could do, but here are a few tips:

  • Whenever you have duplicated code, make a unique function and call it from the multiple places where you need that code to run. In your case:
local function makeTool(Tool)
    -- setup your gun here, using tool as an argument
end

for _, Child in ipairs(inv:GetChildren()) do
    makeTool(Child)
end

inv.ChildAdded:Connect(makeTool)
  • You are not disconnecting event connections when they become useless, this is a problem that will lead to memory leaks on the client
  • Maybe it’s intended, but you are not handling the case where a tool is removed from the inventory
#3

This is great i will do this

Not entirely sure what this is but it might be important so ill look into it

ive thought about this, not sure how i would find the exact frame that was created by the tool and delete that frame

Otherwise thanks for the information, ill get to it!

#4

Simply store your frame in a table with the tool instance as the key, and listen for the removal of tools from the inventory. However be careful, equipping a tool will remove it from the Backpack, it will be moved into the player’s Character.

#5

Alright cleaned up some of the code using functions,

Ill try to do this, never been really been good with tables

#6

Found out how to do this, and it works pretty well :+1:

1 Like
#7

Can’t help you much with the actual code, but I recommend you use this:

https://roblox.github.io/lua-style-guide/