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