Rainbow Items Script From client

ok so I made a script that uses CollectionService to make it so I can make items have a rainbow effect in a game I’m just wondering if there is some big problem that I may have put in it to cause memory leaks or somthing I did my best to avoid that and I did have a pretty good scripter help me make it Haydz6 he helped do the part that actually changes the color of parts

local CollectionService = game:GetService("CollectionService")
local RainbowWood = {}



for _, object in pairs(CollectionService:GetTagged("RainbowWood")) do
	table.insert(RainbowWood,object)
end


local speed = 3
coroutine.resume(coroutine.create(function()
	while true do
		for i = 0,1,0.001*speed do
			local ComputedColor = Color3.fromHSV(i,1,1)
			for A = 1,#RainbowWood do
				if RainbowWood[A] then
					RainbowWood[A].Color = ComputedColor
				else
					table.remove(RainbowWood,A)
				end
			end
			wait()
		end
	end
end))



CollectionService:GetInstanceAddedSignal("RainbowWood"):Connect(function(Object)
	table.insert(RainbowWood,Object)
	
end)

CollectionService:GetInstanceRemovedSignal("RainbowWood"):Connect(function(Object)
	table.remove(RainbowWood,Object)
end)

CollectionService:GetInstanceAddedSignal("RainbowWood") 
CollectionService:GetInstanceRemovedSignal("RainbowWood")

this is my first post on the developer forums so feedback on how bad the post is would also be nice

It’s hard to give feedback on such a small script, but here’s some tips.

  1. Directly appending a new key like RainbowWood[#RainbowWood + 1] runs faster than calling table.insert. Same concept applies for removing a table key. Instead of using table.remove, RainbowWood[key] = nil is more efficient.

  2. Instead of having to create a new thread with coroutines costing resources, you can just put that while loop at the end of the script, so it doesn’t yield the rest of the code that needs to run.

  3. ‘wait()’ is a big code smell. Instead of calling that to yield your code, it’s better to use ‘runservice.Heartbeat:Wait()’. If you want faster checks it’s better to use ‘Heartbeat:Wait()’ since it has been benchmarked to run around 2x faster than ‘wait()’. There are some other points listed in the post I linked about how wait can sometimes be delayed, etc…

Avoiding wait() and why

  1. Do stick with consistent casings when naming your variables whether its PascalCase or camelCase.
    Example: In your coroutine thread, you have a numeric loop in another numeric loop. For the outer - scoped one, you use ’ i ’ and in the loop inside that, you use ’ A '. This does not impact code performance at all, I’m just suggesting tips when it comes to readability.

  2. I don’t recommend using numerical loops over the generic loop (ipairs). Yes, numeric runs faster than ipairs but when it comes to retrieving the value it really isn’t. When you use a numeric loop you have to do ‘RainbowWood[A]’ to retrieve the value which is not instantaneous. However, the generic loop caches the value in a variable (for i, v in ipairs(array) do).

  3. I don’t understand the logic of this chunk of code

				if RainbowWood[A] then
					RainbowWood[A].Color = ComputedColor
				else
					table.remove(RainbowWood,A)
				end

If the key ‘A’ does not exist in your array, why would u remove it?

I’m not sure how collection service works, but here is how I would rewrite your code:

local CollectionService = game:GetService("CollectionService")
local RunServiceHeartbeat = game:GetService('RunService').Heartbeat

local RainbowWood = {}
local Speed = 3

for _, Object in pairs(CollectionService:GetTagged("RainbowWood")) do
	RainbowWood[#RainbowWood + 1] = Object
end

CollectionService:GetInstanceAddedSignal("RainbowWood"):Connect(function(Object)
      RainbowWood[#RainbowWood + 1] = Object
end)

CollectionService:GetInstanceRemovedSignal("RainbowWood"):Connect(function(Object)
      RainbowWood[Object] = nil
end)

CollectionService:GetInstanceAddedSignal("RainbowWood") 
CollectionService:GetInstanceRemovedSignal("RainbowWood")

while true do
	for i = 0, 1, 0.001 * Speed do
		local ComputedColor = Color3.fromHSV(i, 1, 1)
		for _, v in ipairs(RainbowWood) do
			v.Color = ( v and ComputedColor ) or v.Color
		end
	end
	RunServiceHeartbeat:Wait()
end

Due to the 6th reason I had, I decided not to include the part where you remove A from RainbowWood

2 Likes

the 6th part was there to make sure that it would not keep parts that were removed from the game in the table it was leftover from the old script which had it loop through a certain folder whenever parts were added and it was a huge mess and barely worked. I really appreciate the feedback I’m new to doing stuff with all of this I find that part about the tables the most useful because I don’t have a great understanding of them

1 Like

Not anymore, Luau iterator functions have been optimized.

wait()’ is a big code smell. Instead of calling that to yield your code, it’s better to use ‘runservice.Heartbeat:Wait()’. If you want faster checks it’s better to use ‘Heartbeat:Wait()’ since it has been benchmarked to run around 2x faster than ‘wait()’.

It isn’t necessarily a code smell. It runs on a 30hz frequency (30 fps) and is dependent on whether you use it on the client or server. wait() usually waits 2 frames if those 30 fps are consistent and then stops yielding when it has reached the argument time. If you use wait() on the client, it can be more consistent because the client may have an FPS unlocker (doesn’t mean wait() will run on a higher hz). If the client lags (fps reaching below 30), wait() will also take more time to yield since the frame time will be higher. Same goes for using it in a server.

Also, the reason RunService.Heartbeat:Wait() is faster is because that it runs on 60 fps (if used on the server) and even more if used on the client. Since the frequency is dependent but not limited to 30, each frame time takes less time than compared to those in wait(). If you use wait() on the client whose fps are somewhere around 240, RunService.Heartbeat:Wait() will have less frame time and will be far superior to wait() since its dependent on the fps of the client if used on the client side.

If you want faster checks it’s better to use ‘Heartbeat:Wait()’

Using it on the server and only if those 60 fps are consistent, then yes. Otherwise on the client and if the client is lagging, it will not yield faster checks. If you use this on either of these sides, it isn’t guaranteed to yield faster checks than wait() but is a lot more consistent in yielding faster checks than wait().

Infact, you can just use an alternative function to wait().

local RunService = game:GetService("RunService")

function smartWait(yield : number) --> probably place it in rep storage in a module so both the server and client can use this function
      local timePassed : number = 0

      while true do
          timePassed += RunService.Heartbeat:Wait()
          if (timePassed >= yield) then break end 
      end
end

smartWait(5)

I’m very confused by this. It seems throughout your entire explanation you’re defending the use of wait() in a certain environment when I feel like this misses the point of @DarthFS’s mentioning of Avoiding wait() and why

The primary reason you shouldn’t use wait() is because its frequency is not proportional to framerate. In fact, because of this, there’s no guarantee the code after wait() will execute the frame it’s expected to.

I highly recommend you reread the post provided as I’ve never seen wait preferred in quite some time. In fact, the main reason I’m confused is that you end your discussion with a wait alternative that uses RunService.Heartbeat like @DarthFS advocated.

All in all, instead of exploring obscure edge cases of when something becomes acceptable, you should always just opt for the best practice.

1 Like

According to this: Task Scheduler | Documentation - Roblox Creator Hub
wait() does not have the lowest priority, heartbeat does… everra does mention wait() is on a secondary queue or something but I never found the source for this or any reasonable explanation on why that is

" there’s no guarantee the code after wait()" that is only assuming that a secondary queue exist, and that wait() is on there since there no source dedicating that it is, its hard to really judge that. It also depends on the actual behavior of how the secondary queue works if it exist

If the secondary queue does not exist, code after wait() will always execute, it might not always execute at a expected time, but that applies to other yielding methods

what Silents said appears to be all completely correct, although I do disagree with the usage of smartWait in actual games

2 Likes

You’re right with the secondary queue shenanigans, I’ve updated my post to reflect this. However, since the use of wait() either is misinformed or often is used for polling instead of the much preferred event-driven architecture I simply can never advocate for its use, especially in production.

3 Likes

You are infact incorrect. I have benchmarked whether or not numeric is faster than the generic loop. Numeric loops seemed to be significantly faster whether or not the amount of elements in the array was 10 or 10,000,000. Here is the source code:

local array = {}

for i = 1, 10000000 do
	array[#array + 1] = math.random(1, 100)
end

for i = 1, 5 do
	local startTimeOne = os.clock()
	for index, value in ipairs(array) do
	end

	local ipairsTime = os.clock() - startTimeOne
	print('ipairsTime: ' .. ipairsTime)
	
	local startTimeTwo = os.clock()
	for i = 1, #array do
	end
	
	local numericTime = os.clock() - startTimeTwo
	print('numericTime: ' .. numericTime)
	print('')
end

The results I got when i appended 10 elements or 10,000,000 were the same. The reason I prefer ipairs over the numeric is that ipairs caches the value inside the numeric. The ‘speed’ of the loops don’t really matter when you are accounting for what goes inside the loop when using numeric ’ t[i] '. Luckily, the ipairs iterator caches the value. Numeric is indeed faster though.

It is best like @TheEdgyDev said, to use what is best practice rather than trying to find cases where something becomes acceptable over the latter.

If you read sections on the roblox task scheduler, it mentions that ‘The most direct way to add frame-by-frame game tasks is through the following members of RunService’. Instead of nitpicking ways to advocate for wait() usage you should pick an overall better yieldMethod that is practical in most scenarios. Also, having a custom wait function is not to be rude, a terrible idea for the following reasons.

  1. Polling is bad practice. You can read a more indepth explanation on why: Avoiding wait() and why

  2. Now you may be wondering. Whats wrong with polling? Polling is bad since you are constantly yielding even when not needed. You are adding constantly adding those instructions into the task scheduler. The issue is when you use this practically in game development and have lots of custom wait functions being called.

When you delay the task scheduler to do a certain task, it will push that task to the next frame. It’s good that you are using heartbeat which has the lowest priority, but other more relevant functions could be using heartbeat as well. As you can see from a game development standpoint it’s quite impractical.

  1. There is a much better way of creating a custom wait without polling!
local runService = game:GetService('RunService')
local signal = require(game:GetService('ReplicatedStorage'):WaitForChild('Signal'))


local function smarterWait(yieldTime)
	yieldTime = yieldTime or 0
	
	local signalOBJ = signal.new()
	local accumulated, connection = 0, nil
	connection = runService.Heartbeat:Connect(function(dt)
		accumulated = accumulated + dt
		
		if accumulated >= yieldTime then
			signalOBJ:Fire()
		end
	end)
	
	signalOBJ:Wait()
	connection:Disconnect()
	signalOBJ.BindableEvent:Destroy()
end

return smarterWait

Do I recommend using this?: Heck no. I’d rather just do wait(5). There still comes the creation costs of implementing bindables and destroying them.
Is this better than using a custom wait that polls: Heck yeah

Custom Signal Source Code

It also seems you use Heartbeat in your custom wait function instead of wait()? That dissaproves your point in using wait() over Heartbeat:Wait() in the first place.

1 Like

The benchmarks I provided in my post these are from support @SilentsReplacement’s claim.

Iterators are, in fact, all about equal in terms of performance with ipairs and pairs being preferred when iterating over a table.

1 Like

Yep, I never considered ‘numeric’ to be a true iterator. All it does is return the numerical index. That’s why from my previous replies, I supported ipairs over the numeric. All I stated was that numeric is not slower than ipairs and that it should not be used to iterate through arrays since ipairs and any other true iterator caches the value.

1 Like

That post was meant to clarify why wait() is not consistent and showing how Heartbeat:Wait() can be superior.

1 Like

The task scheduler was made precisely for yielding functions, so I don’t fully understand what you mean by yielding doesn’t add any instructions, if you executed instructions without yielding then it wont add any instructions but not the case for when there is yielding. You can even look at the task scheduler documentation on the roblox wiki

These tasks include detecting player input, animating characters, updating the physics simulation, and resuming scripts in a wait() state.

a issue with your smartWait() method is that ur using Heartbeat:Wait() the reason this is bad is because your repeatingly adding in a task every loop cycle, the smarterWait method by darth is better because it doesn’t add/remove task each time, the heartbeat event will simply fire to the function each time without having the need to remove it until you know its disconnected, not advocating for the smarterWait() I personally don’t think its ever a good idea to make those type of methods

2 Likes

The task scheduler was made precisely for yielding functions, so I don’t fully understand what you mean by yielding doesn’t add any instructions, if you executed instructions without yielding then it wont add any instructions but not the case for when there is yielding. You can even look at the task scheduler documentation on the roblox wiki

What I mean is that it doesn’t add any extra work on the scheduler.

a issue with your smartWait() method is that ur using Heartbeat:Wait() the reason this is bad is because your repeatingly adding in a task every loop cycle, the smarterWait method by darth is better because it doesn’t add/remove task each time, the heartbeat event will simply fire to the function each time without having the need to remove it until you know its disconnected, not advocating for the smarterWait() I personally don’t think its ever a good idea to make those type of methods

The point was that it was an alternative to wait(). But yes, I do agree with your point that it adds new task every time it is fired.

2 Likes

Adding tasks to the task scheduler by polling in ur custom wait function is the extra work.

The task scheduler isn’t a simple queue data structure that accesses each task like that. When tasks get added to the scheduler there is a whole priority process on which actions get sorted first.

Not a very optimal alternative to wait(). @Thedagz already stated why but I’m going to restate it to prove that I know what I’m talking about.

  1. Constantly calling a yieldMethod in a loop (polling) pushes those instructions onto the scheduler and causes issues in the long run when called multiple times or the longer you want your function to yield for.

  2. My ‘smarterWait’ function does not poll and constantly call a yieldMethod which is a significant pro. The only drawbacks are the creation costs of creating and destroying the bindable with signals.

  3. With such cons it’s better to stick with a consistent overall better way of yielding which is wait(yieldTime). The only exception to not using wait is when having to yield for a frame in which that case you use Heartbeat:Wait().

1 Like

Ipairs is faster than numeric, the reason your results show that numeric is faster is because you didn’t index or use the keys/values. The larger the iteration, the faster ipairs is compared to numeric.

2 Likes

yep, thats why I prefer ipairs over numeric. the value is cached unlike numeric where you have to index the value.

the reason I said numeric was faster was that I didn’t account for indexing the keys resulting in an unfair benchmark.

1 Like

what the generic loop returns is the index, and the value (array[index]). array[index] is cached in a variable so you don’t have to keep indexing the key doing array[index] which is inefficient.

https://gyazo.com/43fcd72c0534ea19e1035397457275ae

a local variable being only accessible to a loop’s scope is not relevant to caching.

1 Like

I know this is old, but I’m since I’m looking it up and finding it today and I’m sure other people are too, I figured I’d just let anyone else know (took me about 30 mins to figure out what was going on) that the RunServiceHeartbeat:Wait() in the solution should be INSIDE the for loop, not outside of it. Such a small detail that I’m surprised I didn’t notice, but it was confusing since I was more focused on connecting the right tags and making sure the connection was valid as opposed to the formatting.

3 Likes