Need some help with saving accessories

Basically, I am trying to make a system where a custom accessory from the game saves into datastore when the player leaves the game. I had multiple attempts at creating the script but failed.
My main problem is that I can’t access player’s character when they leave to look through their character to find my custom accessories names. I tried different solutions myself and from forum but have failed (For example: local char = plr.Character or plr.CharacterAdded:Wait()).
So If you know how to check through player’s character right before they leave, please let me know.

TL;DR: I have problems with accessing player’s character accessories when they leave the game, therefore making me unable to insert accessories names into the table.

P.S. Sorry if you had a stroke while trying to read this, my English is not perfect.

The script (it’s unfinished rn due this problem), my problem is at the save function:

local Players = game:GetService("Players")
local dataService = game:GetService("DataStoreService")
local servStorage = game:GetService("ServerStorage")

local accessoryFolder = servStorage.CustomAccessories
local acsData = dataService:GetDataStore("Accessories")

--Main focus:
local function save(plr: Player)
	local char = plr.Character or plr.CharacterAdded:Wait()
	local acsTab = {}
	
	if char then
		for i, acs in pairs(char:GetChildren()) do
			table.insert(acsTab, acs.Name)
			print(acs)
		end
	end
end

local function load(plr: Player)
	local acsSaves = Instance.new("Folder")
	
	acsSaves.Name = "AccessoriesSaves"
	acsSaves.Parent = plr
	
end

Players.PlayerAdded:Connect(load)
Players.PlayerRemoving:Connect(save)

--Incase server shutdowns
local function servShutdown(plr)
	for i, v in pairs(Players:GetPlayers()) do
		task.spawn(function()
			save(plr)
		end)
	end
end

game:BindToClose(servShutdown)
1 Like

Hello! One way to approach to this is to store the character in a table before leaving such as below:

In summary: You store the player in a “characters” table during the function CharacterRemoving. You then index the player’s character when the PlayerRemoving function is called. After storing everything you need to from the character, you can remove the character from the “characters” table. Make sure to read the actual post for a more in depth and script example.

1 Like

Hello, thanks you for helping, I am gonna try doing that, if it helps I will let you know!

1 Like

Okay, I tried doing something like this but I don’t think it worked (I recived no prints). I think instead of checking player’s character I should implement some other system tho I wonder how should I do this

local Players = game:GetService("Players")
local dataService = game:GetService("DataStoreService")
local servStorage = game:GetService("ServerStorage")

local accessoryFolder = servStorage.CustomAccessories
local acsData = dataService:GetDataStore("Accessories")
local characters = {}

--Main focus:
local function save(plr: Player)
	local character = characters[plr]
	local char = plr.Character or plr.CharacterAdded:Wait()
	local acsTab = {}

	if character then
		for i, acs in pairs(char:GetChildren()) do
			print(acs.Name)
		end

		characters[plr] = nil -- could probably also just make characters a weak-keyed table but this works too
	end
end

local function load(plr: Player)
	local acsSaves = Instance.new("Folder")
	
	acsSaves.Name = "AccessoriesSaves"
	acsSaves.Parent = plr
	
	plr.CharacterRemoving:Connect(function(character)
		characters[plr] = character
	end)
	
end

Players.PlayerAdded:Connect(load)
Players.PlayerRemoving:Connect(save)

--Incase server shutdowns
local function servShutdown(plr)
	for i, v in pairs(Players:GetPlayers()) do
		task.spawn(function()
			save(plr)
		end)
	end
end

game:BindToClose(servShutdown)

Anyways, thanks for trying to help once again. The topic you send to me was useful

1 Like

I’ve figured out why it isn’t working. On the line where you say
local char = plr.Character or plr.CharacterAdded:Wait(),
you are stopping the rest of the script to run. You can fix this by deleting the line. This happens because it continues to search for a character being added but it can’t find one.
Also make sure to change

for i, acs in pairs(char:GetChildren()) do
			print(acs.Name)
end

to

for i, acs in pairs(character:GetChildren()) do -- change char to character here
			print(acs.Name)
end

It should work now, as it did when I ran it in studio.

2 Likes

Ok a few things here:

Periodically, or on event based(like on change etc), you should grab the current information about your accessories to a cached version you keep on the server. If no changes have been made, don’t save, if a change has been made, update. And you should do this, as I said, periodically and or on easily trackable events like on some change that will obvisionly change accessories on the player.

The bind on server is a great safety, to try to catch anything that may have slipped the cracks, but understand that bind should be thought of as more of an emergency just in case. And NOT what you are relying on to keep everything up2date.
There is a lot that can go wrong when the server downspins, its a more “chaotic” scenario. So still have it there as a safety check, but you should have other means that should be keeping the data synced before hand.


Two: Alright I just spent some time cleaning up your code and going over some practices to make your life easier overall part of the issue is you really need 2 kinds of saves. One save that is cache focused so saving to the server and another that performs a data store save only when necessary. I say it in the script but BEST PRACTICE IS ALWAYS Game → Saves to Cache → Cache Ref Save to DataStore

Note: once you understand the nodes just clean up the comments and it will look a lot cleaner cosmetically. Your welcome

local AccessoryStorageManager = {
--[[ I AM TREATING THIS LIKE A MODULE NOT A SCRIPT
Accessory syncronizing and managment Code 
This will manager all things in regards to ensuring data store has an update stored 
data in relation to the players accessories 
--]]
--[[NOTES: The following code will have comments where I will critic your code as as well be aware
I also lean to readability as a key part of coding and debugging because I never want to get confused and or lose 
time to trying to figure it out or make my brain spend any resources on reading or interpreting my code vs thinking about 
whatever problem im solving
]]
}
-- Last note, you will notice a --[[]] block above each function or function hook I do this out of 
-- buit in habit to make it super clear where any function begins at a glance 

-- Notice I context split services/requires with initialized Variables DO it for your santiy and because you are a wonderful 
-- thoughtful person 
local Players = game:GetService("Players")
local DataService = game:GetService("DataStoreService") -- Be consistent with your Naming conventions of requires CAPTIALS every time
local RunService= game:GetService("RunService")
---------------------------------------------------------------------------------------------------------------
---------------------------------------------------------------------------------------------------------------
-- Dont mind my 2 groupin g this is a habit, I find pairing in 2s and thress feels easier to read then blocking any more
local ServStorage = game:GetService("ServerStorage")	
local AccessoryFolder = ServStorage.CustomAccessories

local AccessoryDataStore = DataService:GetDataStore("Accessories") -- DO not abbreviate your Variable names! Clear To what they are 
AccessoryStorageManager.PlayerAccessoryStorage = {}
---------------------------------------------------------------------------------------------------------------
---------------------------------------------------------------------------------------------------------------
-- Helpers
--[[
runs a loop over all current players and passes the Player obect through a function paramater provided
]]
-- Yes I shortaned function here, fn is pretty universal dont get nitpicky with me I am older and wiser by far and I dont care about your 
-- judgments 
local function processCurrentPlayers(fn)
	for Index, PlayerInstance in pairs(Players:GetPlayers()) do
		fn(PlayerInstance)
	end
end

-- Technically speaking Accessor filter belongs here but i have more notes for you so i put it below it
---------------------------------------------------------------------------------------------------------------
---------------------------------------------------------------------------------------------------------------
--[[NOTES
I suggest do not use typing at the start, especially with things that your hooking up 
to systems that you can 100% know will pass you that value.   And you did not dictate this 
script as being strict so you are losing the benefits of being strict typed and just making more work for 
yourself.   
]]

--[[
Gets us a fresh table that filters out all things accept for accessories
-- I have my vary own more generic variant of a filter function but this works for this
-- you may be asking yourself, why is he encuring an extra processing loop here 
1: In th ecase of this you dont have 1000s of players and its relativistically not costing you anything
you need to worry about 
2: It makes it easier to read and more safe to work with
]]
local function _accessoryFilter(CharacterChildrenTable)
	local returnTable = {}
	
	for i = 1, #CharacterChildrenTable do
		-- PredicateSafties:
		-- If we are nil for any reason then we skip
		-- If we are not an accessory we skip
		if CharacterChildrenTable[i] == nil then continue end
		if CharacterChildrenTable[i]:IsA("Accessory") == false then continue end		

		if CharacterChildrenTable[i]:IsA("Accessory") then 
			table.insert(returnTable, CharacterChildrenTable[i])
		end
	end 
	
	return returnTable
end


--Main focus:
--[[
We get the players Character and we want to get all of the accessories  to save by name

Since as it stands you are doing this when a player is loading, leaving, and when the server dies

Cache Saving is where we are setting up our intiial ref in our data table and we may update this 
independantly of when we want to push our cacheSaved Data into our Data Store

READ IMPORTANT!!!!!!!!!!!!!!!!!!!!!!!!!!!!!: 
-- ALWAYS SAVE TO CACHED data, THEN WHEN SAVING TO DATA STORE PUSH FROM THE CACHE
---- You can push updates to your local cached server data as often as you want with ZERO rate limiting where DataStore has rate limits 
---- can fail blah blah its BEST practice to be active saving to your CACHE, then when you push to DataStore, Clone or reconstyruct/searlize 
into the table you want to push to datastore

Due to that we are going to have 2 different kind of saves so I broke your save into 2 kinds
cacheSave 
dataStoreSave
]]
-- cacheSave will occur as a result of the initial load and on a periodic check hooked below
local function cacheSave(Player)
	
	-- Since we HAVE to have the character to move forward its ok to use a repeat until here
	-- we have a catch so if they leave and player nils out we can abandon the wait 
	if Player.Character == nil then 
		repeat 			
			task.wait()
			if Player == nil then return end
		until Player.Character ~= nil
	end
	
	-- I am jsut mention STOP abreviating your variables, you are making your life harder
	-- don't argue, i know this is harsh I am not trying to be mean but any justification of you 
	-- shorting your variable names through abbreviations is 1000% wrong don't do it it doesnt make you cool or more l33t
	-- It makes you look l ike a cheap outsorced coding peon
	local Character = Player.Character 
	-- I am leaving this here for now to demonstate DONT abbriveate but since we are moving away 
	-- from using an instnaced folder and instead storing our references via a table 
	-- WE don't need it
	local AccessoryTab = {}
	
	local AccessoryListToProcess = _accessoryFilter(Character:GetChildren())
	
	-- I removed your safty for character here because we already went through great lengths to secure
	-- that you have a character so checking it again is just unnecessary  and adds visual clutter

	-- I am not a fan of the "_" for indicating unused part of pairs but I am fine with it generally speaking
	-- I think its better policy to just call it "Index"
	-- Now we can do a straight index table loop or an pairs here it doesnt really matter 
	-- I am a bit old school and based on my experience if you want speed always do ordered index loop
	-- They are still faster but its a micro improvement so if this is more readable to you and more comfy you can do it
	
	-- We need to scrub the table before we re insert to it so we dont get ever expanding
	table.clear(AccessoryStorageManager.PlayerAccessoryStorage[Player.UserId])
	for _, accessoryInstance in pairs(AccessoryListToProcess) do
		-- Note we are saving a direct reference to the accessory Instance into our data table 
		-- I am doing this because I plan to 
		table.insert(AccessoryStorageManager.PlayerAccessoryStorage[Player.UserId] , accessoryInstance)
		print(accessoryInstance.Name)
	end	
	-- We have completed the cache preperation of our playercharacter
end

--[[
This is where you perform all your searlization and preperation for saving into data store your accessories
I leave this blank cuz there are a ton of ways you can store things to data store and thats up to you and 
how your game works
I suggest if you have your own undersatnding of accessories that are game specific 
you should make an enum of Types, and any accessory created by your system have a UUID attatched to it as an attribute
-- Use the UUID for storage Key and Type as your Value this way you can have same named attachments if you want.  
-- But it all depends how you construct and handle your accessories and thats game specific
]]
local function dataStoreSave(Player) end 

--I Moved load above save, because in order the player LOADS in, and saves occur as a result of leaving
-- or server going down
--[[
Here is us hypothetically creating our accessories
-- Alright I am going to SAVE you a ton of trouble here and I changing your loading up to be 
easier to manage
]]
local function load(Player)
	-- Instead of making a physical folder that requires us to manage an in world instance 
	-- we instead are going to store and manage or checks and refernces in a table refernence
	-- Think of it like a folder, with less chances of issues like destroying and such happening and easier to debug
	-- Plus we Don't need to name it and eat the cost of parenting  
	
	-- We can safrly assume since load only happens when Player is add not character that 
	-- we do not have anything stored for them, if we do we may as well just refresh it at this point anyway
	
	AccessoryStorageManager.PlayerAccessoryStorage[Player.UserId] = {}
	-- At this point we likely are going to do some call to data store to retrieve the Accessory list 
	-- Which we then cache into our table 
	-- Whcih we can then use to do any processes involved with placing them here
	
	-- once we have done with all the data store pull down loading stuff we go ahead and cache that into our server local storage 
	cacheSave(Player)	
end


--[[
We process our run and push to DataStore
]]
--[[ Notes baout your old set up here
		-- Got rid of your spawn, it doesnt make it more safe here to do so 
		-- just run it here, once the server is shutting down the time limite for bind to close process 
		-- is global so it wont matter
		-- Also your technically shunting all those qued up processes to the end of the frame which is even more 
		-- technically dangerous
]]
-- You might as well just inline this bind in this case
game:BindToClose(function()
	-- Technically we could pass dataStore save as just an argument but I find its just better practice
	-- To encapsulate and it opens opertunity for if you need to break it out to do other things or add to easier 
	-- preset for modularity adjustments	
	processCurrentPlayers(
		function(PlayerInstance)
			dataStoreSave(PlayerInstance)
		end
	)
end)

-- As a force of habit i generally do all my connections at the bottom of a script or inline 
-- there is nothing wrong with inlining it if you dont intend to reuse the save and load functions elsewhere
Players.PlayerAdded:Connect(load)
Players.PlayerRemoving:Connect(function(PlayerInstance)
	local PlayerId = PlayerInstance.UserId
	dataStoreSave(PlayerInstance)
	
	--PassReference
	-- We do this because if you REMEMBER we saved the Instance references here and if we dont clear 
	-- them out there is a chance that it will get into a weird psudo state where the table won't clean up
	local cachedTable = AccessoryStorageManager.PlayerAccessoryStorage[PlayerId]
	
	-- Remove from the universal storage, this is also just in case you do any active iterations over t his
	-- or may have otyher process use this for anything else elsehwere this is the safest thing to do to get it out asap
	AccessoryStorageManager.PlayerAccessoryStorage[PlayerId] = nil
	
	-- This will ensure that the assecesoory is properly killed and cleaned from references 
	-- YES in THEORY as the player is removed they may clean up but this is where weirdness CAN happen 
	-- Its best to just take care of it and ensure, if they happen to already be destroyed then no problem 
	-- it will just speed run over the table to ensure it was cleaned 
	-- and then release the table for garbage collection since it has no other pointer 
	for i = 1, #cachedTable do 
		-- If for some reaosn one is gone keep going
		if cachedTable[i] == nil then continue end
		cachedTable[i]:Destroy()
		cachedTable[i] = nil 
	end	
end)

--[[
Here we are going to periodically run over all the players and save their accessories
-- DONT task spawn here, relativilisticly speaking this shouldnt cost anything on teh server to do 
and we dont need to optimize this unless you had 1000s of players or your doing some weird intensive stuff
-- But there are lots of solutions for that kind of optimizations and saving things over time
]]
local UpdateRate = 1/15 -- 15fps aka every 0.06 every 4 process frames
local TotalTimePassed = 0
RunService.Heartbeat:Connect(function(deltaTime)
	
	TotalTimePassed += deltaTime
	
	-- If we are not greater then our update rate then we wanna punch out now 
	--- I know you may feel a way that there will be small opertunities for micro seconds off 
	---  but dont be not worth the trouble in this case
	if TotalTimePassed < UpdateRate then return end
	-- Instead of zeroing it out we are going to subtract the update rate, so that any left over 
	-- is left within the time passed,  this is a touch more honest with update rating but it does mean 
	-- there may be a few times its on the next 3rd frame instead of the 4th  but thats because delta time 
	-- is inconsistent over the grand scheme of things it will average out correctly
	TotalTimePassed -= UpdateRate
	
	processCurrentPlayers(
		function(PlayerInstance)
			cacheSave(PlayerInstance)
		end
	)
	
	
end)

return AccessoryStorageManager
3 Likes

I tested all of this rudementery, it all works, mins I’ve not added any of the data store loading parts cuz I’m lazy you will have to do that portion I just had it say " I saved to data store" lol but I did a cursery test. Yes you will need to put it in a module and require the module for it to get booted up.

2 Likes

While yes, this does fix the most imminate issue, there is a lot of other problems with this code that needs addressing. But I must give credit to Green Bit for addressing the most direct issue.

Due to some of my more eccentric natures, I just provided you a comprehensive solution…

2 Likes

Yes, I agree that the code I provided is not very secure, as stated in the post I linked. The code you have written is much cleaner, and a better option to tackle this issue. The amount of effort put into your replies is phenomenal.

2 Likes

I made a boo boo, but I fixed it, I forgot that I was inserting instead of doing some things I normally do, on a cache save it now ensures that we clean out the prior table to save our most update accessory references of the players stuff

1 Like

Well, if I can inspire just one person every once in awhile to code more readably then I feel great.

Plus its a great warm up to my real work. I stream from time to time when I am coding.

But yea I maybe a bit of a … stickler when it comes to code practices. For better or worse,
For example people confuse clean code vs Readable code vs Maintanable code.

Clean and Maintainable are generally interchangeable → This is following good patterns, organization of processes and even taking into consideration that your formating some how reflects your logic.

Where as Readable code, while it inherently makes your code more maintainable is not the same thing. Readable is like reading comprehension
→ Good naming conventions
→ Good consistent formating
→ Organization
→ Creating spacing for brain comprehension and reading
→ Following 80/120 target 80 char per line max 120 for again ease of brain reading
→ overall making choices in your code that promote making it more comprehensible

I follow
Readable > Clean > Performance
—> Sometimes you need performance and that will come at the cost of something. But in my experience its best practice to only do simple non intrusive performance adjustments all the time, and only consider doing higher performance implementations as you need them. Which generally speaking if it is event based, and not something in your update loop likely doesn’t need to be ultra performant. And you will love yourself for letting a little performance go in exchange for code you can look at that you haven’t seen in what feels like 5 years and it takes you 10 seconds to know whats going on

2 Likes

You’ve definitely inspired me! I’m feeling very optimistic to apply readability to all the languages I use including lua. I will admit I don’t usually create readable code. I definitely agree with the points you brought up, I have previously struggled to read code I have written months or years in advance, and it can get pretty confusing. This will definitely be life changing.

1 Like

Oh god, thank you so much! I appreciate the dedication for helping me, tho I have a question this a module script right? If so I have really bad memories with them lol, I was tortued by them. Tho I will try to look over the script to understand but I don’t promise anything since I am not this good at scripting

Damn, scripting is so hard at the beginning. I was wondering about how to make the script “perfect” to save accessories but I got overwhelmed by other problems (the script I sent was just a placeholder for me to test how to get character’s accessories), but looking at your code I feel like my brain is gonna explode from amount of information, I really appreciate yall helping me.
Also could I ask where do you stream your coding? I might wanna check it out

Okay, so I went over all of the script and I must say I am impressed. This is probably the most advanced stuff I’ve read and understood. Though I have to say that some things like cache, deltatime, heartbeat were confusing for me. (Two of the reasons could be my lack of experience and language barrier.) I am really thankful to both of you for helping.

If you have any advices to help me understand some of the parts of this script better, I would gladly accept them. Are there any tutorials/books that I should take a look at? Anyways thanks for the help

And yeah once again I am really greatful for having both of you to help me, thank you!
P.S. I am gonna mark Crane’s script as the solutions since it goes more into detail, no offense Green Bit, love you <3

1 Like

Sure, if you have questions just ask or post another and link me to it or whatever.

So Here is the BEST ADVICE.

Start small, make atomic pieces. Then play around with how to use them different ways. Then systemize it. So after you learn something, try to create a module script as a library that now applies that in some specific way.

Getting into the head space of “Create a thing to do the work for you” Kind of mentality. I apologize if I made your head hurt I didn’t mean too I tried to keep it concise and explain what I was doing.

Even if you make a function to do something just once, its good habit gets you in the mindset of thinking generic or if you could make it generic to reuse but if not no big deal.

I stream some and you can ask some questions there when I do =)
TheCraneStyle On twitch

1 Like

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.