Review of a local preload script

Help and Feedback Code Review
#1

I’d like to request a review of the following code. The objective of this code, sourced in a LocalScript, is to preload assets located in workspace. I currently feel that it may not be as efficient or grab all objects located in workspace upon the player entering the server. Any feedback / review would be most appreciated.

LocalScript, located in ReplicatedFirst

local repfirst = game:GetService('ReplicatedFirst')
local lplr = game:GetService('Players').LocalPlayer
local preloadGui = script.Parent
local ContentProviderService = game:GetService('ContentProvider')
local SoundToPlay = preloadGui:WaitForChild('sound')

local BackgroundFrame = preloadGui:WaitForChild('BackgroundFrame')
local USMCLogoImage, IntroTextLabel, LoadingTextLabel = BackgroundFrame:WaitForChild('USMCLogoImage'), BackgroundFrame:WaitForChild('IntroTextLabel'), BackgroundFrame:WaitForChild('LoadingTextLabel')
repeat wait() until #workspace:GetChildren() ~= 1
local assets = {}
local instancesLoaded = 0
preloadGui.Parent = lplr.PlayerGui
repfirst:RemoveDefaultLoadingScreen()
wait()
for _, thing in pairs(workspace:GetChildren()) do
	table.insert(assets, thing)
end
function callBackForLoadingEachInstance()
	print(ContentProviderService.RequestQueueSize)
	LoadingTextLabel.Text = "Loading "..ContentProviderService.RequestQueueSize.." assets"
end
SoundToPlay.Playing = true
ContentProviderService:PreloadAsync(assets, callBackForLoadingEachInstance)
LoadingTextLabel.Text = 'Done loading! Please enjoy your stay while in Parris Island. Remember to adhere to applicable law and regulations.'
wait(2)
script.Parent:Destroy()

Ascending and Descending Hierarchy of the LocalScript:

This is my first official post on DevForum by the way. Let me know if I used tags/title/etc wrong. :slightly_smiling_face:

2 Likes
#2

Don’t use pairs() on getchildren(), getchildren() is an array so u must use ipairs.
First you preload the gui part like the main menu after that you go to the workspace part
then you only pre load decals, textures, animations, specialmesh, mesh, shirts, pants.And don’t
use wait(), instead of using wait get a custom wait module like RBXWait(), because wait waits more time than you want.

#3

wait() is fine to use for everyone.

1 Like
#4

Only when you do not care for the delay or lag.

#5

Personally I find that wait can be unreliable and tend to avoid using it whenever. In place of wait I use events because events tend to be more reliable, and can fire exactly on-time.

In my own preload script, I would use the function() that can be passed to the :PreloadAsync() function like this:

ContentProvider:PreloadAsync({asset}, function()
    -- Fire the "promise" event.
end)

The reason I choose events over wait stems from this thread. And I have incorporated most of the advice of that thread in my own work.

Here is a snippet from my own work (it is only intended as an example)

local ContentProvider = game:GetService("ContentProvider")
local LoadedEvent = Instance.new("BindableEvent") -- a temporary event

-- loading assets from a table named "assets"
-- I typically load assets individually so I can keep a counter of them
local assetsLoaded = 0
for _, asset in ipairs(assets) do
    coroutine.wrap(function()
        ContentProvider:PreloadAsync({asset}, function()
              assetsLoaded += 1 -- increment the "loaded" variable, to keep track.
              if assetsLoaded >= #asset then  -- checking to see if this is the final asset to load.
                    LoadedEvent:Fire() -- the last asset was loaded, we can continue the script
              end
        end)
    end)() -- I also do it asynchronously so 1 asset doesn't hold it all up
end

-- yielding the script until all assets are loaded
if LoadedEvent ~= true then LoadedEvent.Event:Wait() end 

print("Everything is loaded!")

I hope my script above gives you some insight on how useful events can be.
Thank you! :grinning_face_with_smiling_eyes:

Final Note: Loading screens are intended to load something in the background, and inform the user what it’s doing. So please try to avoid unncessary waits so you can keep user-attention and only load things you find absolutely necessary (like ui elements that need to be seen immediately)

2 Likes
#6

@ErickShinjitsu Noted. I have changed code snippets iterating through arrays to ipairs instead of pairs. I should’ve done that in the first place and it was an oversight on my part (an oversight I’m afraid I’ve overlooked too many times; going to try and work on that personally!).

@achdef While not addressed to me, noted.

@QuantixDev I’ve never thought to asynchronously load assets or use coroutines in this instance, and I forgot you could use BindableEvents in such a fashion to replace Wait() (in the instance provided here, anyway). I’ve incorporated some of your code into my script, and it’s worked for the most part.

However, I have ran into a slight issue. In the following snippet, the number of GUI objects being loaded (being 3) eventually are equivalent to the amount of objects indicated in Guiassets. Although for some reason, this is not the case in Physicalassets. Out of the 6 objects in Physicalassets, in output it only says 4 were loaded.

It’s quite late at the moment (3:20 PM CST) and I think I’ve made an oversight somewhere that would cause the number of assets found in Physicalassets to not equal workspaceinstancesinstancesLoaded. I’m sure the assets are being loaded, though. Any insight into this bug would be appreciated. I’m going to attempt to go over the code again when I next wake up with a fresh mind and hopefully find it.

Code snippet:

-- BindableEvents to yield until each part is complete of the script
local LoadedGuiAssetsBE = Instance.new('BindableEvent')
local LoadedPhysicalAssetsBE = Instance.new('BindableEvent')
-- local plr, replicatedfirst service, preloadgui
local repfirst = game:GetService('ReplicatedFirst')
local lplr = game:GetService('Players').LocalPlayer
local preloadGui = script.Parent
-- service and audio
local ContentProviderService = game:GetService('ContentProvider')
local SoundToPlay = preloadGui:WaitForChild('sound')
-- gui elements
local BackgroundFrame = preloadGui:WaitForChild('BackgroundFrame')
local USMCLogoImage, IntroTextLabel, LoadingTextLabel = BackgroundFrame:WaitForChild('USMCLogoImage'), BackgroundFrame:WaitForChild('IntroTextLabel'), BackgroundFrame:WaitForChild('LoadingTextLabel')
-- workspace assets & instances loaded counter
local Physicalassets = {}
local Guiassets = {}
local workspaceinstancesinstancesLoaded = 0
local preloadguiinstancesloaded = 0
-- parenting to player and disabling default load screen
preloadGui.Parent = lplr.PlayerGui
repfirst:RemoveDefaultLoadingScreen()
-- loading GUI descendents first
local guidescendents = script.Parent:GetChildren()
for _, preGuiElement in ipairs(guidescendents) do
	table.insert(Guiassets, preGuiElement)
end
--
for _, thingy in ipairs(Guiassets) do
	print(tostring(thingy))
end
--
for _, addedGuiElement in ipairs(Guiassets) do
	coroutine.wrap(
		function()
			ContentProviderService:PreloadAsync({addedGuiElement}, function()
				preloadguiinstancesloaded = preloadguiinstancesloaded + 1
				print("Gui Loading: ".. preloadguiinstancesloaded .. " / " .. #Guiassets )
				if preloadguiinstancesloaded >= #Guiassets then LoadedGuiAssetsBE:Fire() end
			end)
		end
	)()
end
if LoadedGuiAssetsBE ~= true then LoadedGuiAssetsBE.Event:Wait() end
print('Finished loading preload GUI! Moving to physical workspace assets!')
-- loading physical game objects
local workspaceDescendents = workspace:GetChildren()
for _, workspaceObj in ipairs(workspaceDescendents) do
	if workspaceObj.ClassName == 'Part' or workspaceObj.ClassName == 'Model' or workspaceObj.ClassName == 'UnionOperation' then
		table.insert(Physicalassets, workspaceObj)
	end
end
for _, addedPhysicalObject in ipairs(Physicalassets) do
	coroutine.wrap(
		function()
			ContentProviderService:PreloadAsync({addedPhysicalObject}, function()
				workspaceinstancesinstancesLoaded = workspaceinstancesinstancesLoaded + 1
				print("Workspace Loading: ".. workspaceinstancesinstancesLoaded .. " / " .. #Physicalassets )
				if workspaceinstancesinstancesLoaded >= #Physicalassets then LoadedPhysicalAssetsBE:Fire() end
			end)
		end
	)()
end
if LoadedPhysicalAssetsBE ~= true then LoadedPhysicalAssetsBE.Event:Wait() end
print('Finished loading preload GUI! Moving to physical workspace assets!')
LoadingTextLabel.Text = 'Done loading! Please enjoy your stay while in Parris Island. Remember to adhere to applicable law and regulations.'
wait(2)
script.Parent:Destroy()

Attached repro containing scripts & physical assets:
repro.rbxl (37.4 KB)

By the way, I recognize that this post delves more into another category within the Help and Feedback section, but I’m not sure if I should make a new post with this bug/issue or just leave it here. Let me know if I need to correct anything regarding this.

Thanks! :slight_smile:

#7

ipairs or pairs work, however ipairs is considerably faster. Pairs can itterate over non-sequential tables without issue however ipairs cannot, which is where alot of it’s speed comes from (the fact it tterates over sequential keys).

#8

ipairs is only negligibly faster, should be used on arrays while pairs should be used on dictionaries.

#9

It’s not negligibly faster, Check: Speed comparison : ipairs vs pairs vs numerical loop [benchmark]

#10

It is negligibly faster, the difference was hardly 10ms, and recently, iterator functions were optimised.

Ipairs should be used on arrays despite of its speed since it was made to iterate through numeric indexes sequential, while pairs should be used on dictionaries with non sequential indexes, instances or strings and etc.

#11

That was my original point. Or, at least I hoped it could be extrapolated from what I had said about sequential and non-sequential tables.

#12

It still is invalid; conduct a benchmark your self to see it your self. The post was made 11 months ago and and I said, iterator functions were recently optimised.

#13

What? No it’s not, my point is valid. You, yourself, agreed with me.

Benchmark:

local tbl = {}
for i = 1,100000000 do
    tbl[i] = true
end
local time = os.clock()
for k,v in ipairs(tbl) do
end
print("IPAIRS Diff:", os.clock()-time) -- IPAIRS Diff: 0.00035090005258098
time = os.clock()
for k,v in pairs(tbl) do
end
print("PAIRS Diff:", os.clock()-time) --PAIRS Diff: 0.00039059994742274

I ran this server-side in Roblox studio. Note, this is os.clock which is highly accurate.
Bearing in mind there are no operations going on within the loop and thus should be as fair a test as any.
There is still a ~0.00002 difference between the two loops. Although negligible in this context it could provide a decent amount of improvement in other applications/situations.

#14

My point is that there is a negligible boost, while your point was that there is a considerable amount of boost which was invalid.

#15

This is completely a subjective statement, such premature optimisation will hurt your code readability and will serve no purpose.

#16

It very much depends on the context at which the loop has been created, either way it’s best practice to use ipairs where possible over pairs.

#17

No, it isn’t “best practice”, use ipairs in arrays and pairs on dictionaries, that’s it.

Ipairs was made to iterate through sequential numeric indices.

#18

That is not our problem but the Roblox Engine one. The Roblox Engine somes times mess up, and you have to pardon it as it’s what make with example Jailbreak live.

#19

Any optimisation is optimisation my friend, this conversation is going nowhere so I’ll leave it here.
Have a good day!

#20

That isn’t optimisation, that is premature optimisation which will never be needed and will only hurt readability, stop arguing pointlessly.