Inventory System Issues (How can I fix it?)

Zombiefin · April 10, 2019, 8:25pm

So I am trying to make a system where the game saves the weapons skins you earn from crates and it seemed to work at first but when I tested it out with a friend he had all the items I had in his inventory as well. Could maybe someone else tell me where I might have messed up in my code.

Code is below:


local skinfolder = nil

game.Players.PlayerAdded:connect(function(plr)
	local key = "id-"..plr.userId
	pcall(function()
		skinfolder = Instance.new("Folder", plr)
		skinfolder.Name = "skinfolder"
		
		local skins = ds:GetAsync(key)
		if skins then
			for i,v in pairs(skins) do
				local newval = Instance.new("BoolValue", skinfolder)
				newval.Name = v
			end
		end
	end)
end)



game.ReplicatedStorage.SkinWon.OnServerEvent:Connect(function(plr)
	local key = "id-"..plr.userId
	pcall(function()
		local skinsToSave = {}
		if skinfolder ~= nil then
			for i,v in pairs(skinfolder:GetChildren()) do
				if v then
					table.insert(skinsToSave,v.Name)
				end
			end
			ds:SetAsync(key,skinsToSave)
		end
	end)
	pcall(function()
		local skinsToSave = {}
		if skinfolder ~= nil then
			for i,v in pairs(skinfolder:GetChildren()) do
				if v then
					table.insert(skinsToSave,v.Name)
				end
			end
			ds:SetAsync(key,skinsToSave)
		end
	end)
end)

colbert2677 · April 10, 2019, 8:40pm

You’ve provided your code but you don’t exactly specify what the issue is. Is there any errors in your console or anything?

ThatTimothy · April 10, 2019, 8:40pm

Make skinfolder a local variable. If one player joins, then a second player does, all of player one’s item saves will go to player two. skinfolder needs to be defined / stored locally. Put it in server storage, then pull it from there when saving.

Zombiefin · April 10, 2019, 8:41pm

No Errors, It seems like it saves the data globally and everyone can access it.

ThatTimothy · April 10, 2019, 8:41pm

I believe this is his problem.

ThatTimothy · April 10, 2019, 8:43pm

You define skin folder globally. Every time a player joins, this value is overidden.

All players use this value, so every player’s data we be saved to the newest player.

You can fix your code with the following:

local skinfolders = game.ServerStorage

game.Players.PlayerAdded:connect(function(plr)
	local key = "id-"..plr.userId
	pcall(function()
		local skinfolder = Instance.new("Folder")--Shouldn't use parent of instance.new, it's depreciated
                skinfolder.Parent = skinfolders--Use this instead
		skinfolder.Name = plr.Name
		local skins = ds:GetAsync(key)
		if skins then
			for i,v in pairs(skins) do
				local newval = Instance.new("BoolValue", skinfolder)
				newval.Name = v
			end
		end
	end)
end)



game.ReplicatedStorage.SkinWon.OnServerEvent:Connect(function(plr)
	local key = "id-"..plr.userId
	pcall(function()
		local skinsToSave = {}
                local skinfolder = skinfolders:FindFirstChild(plr.Name)
		if skinfolder ~= nil then
			for i,v in pairs(skinfolder:GetChildren()) do
				if v then
					table.insert(skinsToSave,v.Name)
				end
			end
			ds:SetAsync(key,skinsToSave)
		end
	end)
	pcall(function()
		local skinsToSave = {}
		if skinfolder ~= nil then
			for i,v in pairs(skinfolder:GetChildren()) do
				if v then
					table.insert(skinsToSave,v.Name)
				end
			end
			ds:SetAsync(key,skinsToSave)
		end
	end)
end)

Explanation:
When you save skinfolder globally, every new player overrides it. In this manner, the newest joined player will get everyone else’s data saves. You can fix this by storing their folders in a array. Now, the data can be easily pulled from the array, and every player has their own place for their data, preventing them from being overwritten.

~~More Questions? Feel free to ask.~~(Solved)

Zombiefin · April 10, 2019, 8:47pm

Thanks a lot you fixed the code for my game!

colbert2677 · April 10, 2019, 8:53pm

Implementation then. There’s a couple of issues with your code, so let’s walk through them.

First thing is that skinfolder is an unnecessary upvalue. Move that down into the scope of PlayerAdded.

-- Good practice to index services first, plus this is the canonical way to get a service
local Players = game:GetService("Players")

Players.PlayerAdded:Connect(function (Player)
    local skinfolder = Instance.new("Folder") -- Do NOT use the Parent argument!
    skinfolder.Name = "skinfolder"
    skinfolder.Parent = Player
end)

That being said, this implementation could potentially be a little flawed.

Your original code used the parent argument of Instance.new - don’t do that. It’s also worth noticing that you’re putting this folder under the player which is, well, undesirable. You’ll probably understand later as you code but this typically isn’t a comfortable way of storing data. You should typically keep it in ServerStorage under some kind of session data container.

Your code isn’t making full use of the benefits of protected call (pcall). Rather than wrapping your entire code in a pcall, you should only wrap methods that have the potential to fail in a pcall.

You can change that whole line below PlayerAdded:

-- I'm not using the new code I posted before, I'm using your direct code

game.Players.PlayerAdded:connect(function(plr)
    local skinfolder = Instance.new("Folder")
    skinfolder.Name = "skinfolder"
    skinfolder.Parent = plr

    -- Wraps GetAsync in protected mode in case it fails
    local success, result = pcall(ds.GetAsync, ds, "id-"..plr.UserId) -- userId is deprecated
    if success and result then
        for _, skin in pairs(skins) do
            local newval = Instance.new("BoolValue")
            newval.Name = skin
            newval.Parent = skinfolder
        end
    else
        -- handle errors (success returns false from the pcall)
    end
end)

As far as separating data and not data being saved to wrong players, this should be fine as it is. The next major thing to address is the remote, which has its own respective issues (and even security flaws). At this point, it’s up to you to figure out how to fix it, though I’ll give you a few pointers.

You shouldn’t automatically handle events when FireServer is called from the client. In this case, the server should have some kind of debouncing of some sort to prevent throttling on your DataStores or pointless waste of the request budget.
Just like in the code above, try to make full use of pcalls. They aren’t really meant for you to throw anonymous functions where all your code is in a pcall - they’re really meant for wrapping functions that have the potential to fail.
For some reason, you’re having the remote save twice when it’s called?

The rest should be good after these fixes are made.

colbert2677 · April 10, 2019, 8:56pm

Just saying, that code’s probably dangerous to run. You’ve got an unsolved memory leak there. You can clean that up really quickly by handling the removal from a PlayerRemoving event, though that might cause conflictions with the rest of the code.

local Players = game:GetService("Players")

local skinfolders = {}

Players.PlayerRemoving:Connect(function (plr)
    skinfolders[plr] = nil -- Memory leak, begone!
end)

References to the players prevent the player from being garbage collected, so they hang around in memory forever. That’s going to be real hard hitting later because now you have memory being taken up unnecessarily.

On the other hand, why this isn’t necessarily a good idea is because you’re going to want to save the data. Passing it off to the ServerStorage means you can handle it explicitly yourself, even after the player disappears, rather than having all the data disappear along with the Player object.

ThatTimothy · April 10, 2019, 9:00pm

Fair point. Memory leaks can cause huge problems. It would be much better to send the data to ServerStorage. I’ve edited by original reply, thanks.