Is this expensive/inefficient, and how can I optimize it?

Help and Feedback Code Review
#1

So, I have code that effectively makes a rainbow trail. Every object inserted into the objects array is a Trail instance. I was wondering if that was expensive, or if there’s a better way to accomplish this?

objects = {}

game:GetService('RunService'):BindToRenderStep('rgb', Enum.RenderPriority.Last.Value - 3, function()
	if #objects > 0 then
		local c = Color3.fromHSV(os.clock()*1.5%1, 0.75, 1)
		local sequence = ColorSequence.new({ColorSequenceKeypoint.new(0 , c), ColorSequenceKeypoint.new(1 , c)})
		for i = #objects, 1, -1 do
			local v = objects[i]
			if (v and v.Parent) then
				v.Color = sequence
			else
				table.remove(objects, i)
			end
		end
	end
end)
2 Likes
#2

I suggest benchmarking this yourself because it may be fine as it is, but creating a ColorSequence every frame for every object may be a waste. You could precompute a set of color sequences and then just assign them to trails as needed, other then that I cant see any meaningful improvements.

2 Likes
#3

It only creates a single colour sequence per frame. I’d be surprised if this had any performance impact even if there are hundreds of trails.

1 Like
#4

I shouldn’t be too expensive, but you can simplify the code however.

if possible, use tags instead of a table:

local CollectionService = game:GetService("CollectionService")

game:GetService('RunService'):BindToRenderStep('rgb', Enum.RenderPriority.Last.Value - 3, function()
	local objects = CollectionService:GetTagged("RGB")
	if #objects > 0 then
		local c = Color3.fromHSV(os.clock()*1.5%1, 0.75, 1)
		local sequence = ColorSequence.new({ColorSequenceKeypoint.new(0 , c), ColorSequenceKeypoint.new(1 , c)})
		
		for i = #objects, 1, -1 do
			local v = objects[i]
			v.Color = sequence 
			--we dont need to check for parent since :GetTagged doesn't return
			--instances that have parent set to nil
		end
	end
end)

To use this code just simply add a tag to the trail named RGB.

And to simplify further here inverse the condition for early return (guard clause) and use descriptive naming. We can also simplify the loop now.

local CollectionService = game:GetService("CollectionService")
local RunService = game:GetService("RunService")

local RENDER_PRIORITY = Enum.RenderPriority.Last.Value - 3

RunService:BindToRenderStep("RGB", RENDER_PRIORITY, function()
	local objects = CollectionService:GetTagged("RGB") :: { Trail }
	if #objects < 0 then
		return
	end

	local color = Color3.fromHSV(os.clock() * 1.5 % 1, 0.75, 1)
	local sequence = ColorSequence.new({ ColorSequenceKeypoint.new(0, color), ColorSequenceKeypoint.new(1, color) })

	for _, object in objects do
		object.Color = sequence
	end
end)
1 Like