Is this inefficient? (multiple while loops)

this is mah code:

local trees = script.Parent:GetChildren()

for _, v in ipairs(trees) do
	if v.Name == "Tree" then
		for _, aa in ipairs(v:GetChildren()) do
			if aa:IsA('BasePart') then
				if aa.Material == Enum.Material.Grass then
					task.delay(0, function()
						local pos = aa.Position
						pos = Vector3.new(pos.x, pos.y-0.2, pos.z)
						local x = 0
						local z = 0
						local T = -99999
						local tall = aa.Size.Y / 2
						math.randomseed(tick())
						local rand = (math.random(0,20))/10
						while task.wait() do
							x = pos.x + (math.sin(T + (pos.x/5)) * math.sin(T/9))/3
							z = pos.z + (math.sin(T + (pos.z/6)) * math.sin(T/12))/4
							aa.CFrame = CFrame.new(x, pos.y, z) * CFrame.Angles((z-pos.z)/tall, 0,(x-pos.x)/-tall)
							T = T + 0.12
						end
					end)
				end
			end
		end
	end
end

what it does is shake the trees around like there’s a wind storm or something
task.delay was to bypass the loop stopping the entire code

it looks to be running okay:
image

is this expensive on performance or not? and if so, is there an alternative? because i really like the effect it gives

also here is the script performance panel
image

Is this working for every tree? The code that I see seems like it will only work once.

Yes, this will be expensive on performance.

First, try using math.noise with an offset calculated from the position and the amount of time passed. If that doesn’t work, there is another solution which involves creating a set of points and interpolating the rotation according to the distance to the point and the point value.

it works if you put task.delay with a delay of 0, as i said it bypasses that for whatever reason

my friend made me the animation code and i made it so it would just animate all the trees without having a individual script for each tree, i dont necessarily have a problem with the animation code more so if all these loops can hurt performance

basically i just would like to know if i should just leave this code as is or alter it to be friendly to lower end devices

It’s not too inefficient in the first place, but here’s how I would optimize this.
(note: I have not and can not test this code, but any errors should be simple to resolve)

Original script:

local trees = script.Parent:GetChildren()

for _, v in ipairs(trees) do
	if v.Name == "Tree" then
		for _, aa in ipairs(v:GetChildren()) do
			if aa:IsA('BasePart') then
				if aa.Material == Enum.Material.Grass then
					task.delay(0, function()
						local pos = aa.Position
						pos = Vector3.new(pos.x, pos.y-0.2, pos.z)
						local x = 0
						local z = 0
						local T = -99999
						local tall = aa.Size.Y / 2
						math.randomseed(tick())
						local rand = (math.random(0,20))/10
						while task.wait() do
							x = pos.x + (math.sin(T + (pos.x/5)) * math.sin(T/9))/3
							z = pos.z + (math.sin(T + (pos.z/6)) * math.sin(T/12))/4
							aa.CFrame = CFrame.new(x, pos.y, z) * CFrame.Angles((z-pos.z)/tall, 0,(x-pos.x)/-tall)
							T = T + 0.12
						end
					end)
				end
			end
		end
	end
end

I move the function out of the loop so that it doesn’t get recreated for each part. Also use coroutine.wrap instead of task.delay to be able to supply an argument. (Both task.delay and coroutine.wrap have the same effect here: putting the function in a coroutine so that the wait does not stop the whole loop).

local trees = script.Parent:GetChildren()

local function animate(aa)
	local pos = aa.Position
	pos = Vector3.new(pos.x, pos.y-0.2, pos.z)
	local x = 0
	local z = 0
	local T = -99999
	local tall = aa.Size.Y / 2
	math.randomseed(tick())
	local rand = (math.random(0,20))/10
	while task.wait() do
		x = pos.x + (math.sin(T + (pos.x/5)) * math.sin(T/9))/3
		z = pos.z + (math.sin(T + (pos.z/6)) * math.sin(T/12))/4
		aa.CFrame = CFrame.new(x, pos.y, z) * CFrame.Angles((z-pos.z)/tall, 0,(x-pos.x)/-tall)
		T = T + 0.12
	end
end

-- NOTE: the nested loop remains. It's not a problem at all. The real work actually isn't and never has been inside a deeply nested loop.
for _, v in ipairs(trees) do
	if v.Name == "Tree" then
		for _, aa in ipairs(v:GetChildren()) do
			if aa:IsA('BasePart') then
				if aa.Material == Enum.Material.Grass then
					coroutine.wrap(animate)(aa)
				end
			end
		end
	end
end

Some pedantic cleanups/tweaks, but the structure stays the same. It’s mostly variable removal.

local trees = script.Parent:GetChildren()

-- EDIT: changed variable name (aa -> part)
local function animate(part)
	-- EDIT: calculated pos in one step
	local pos = part.Position + Vector3.new(0, -0.2, 0)
	-- EDIT: x, z are now locals inside the loop
	local tall = part.Size.Y / 2
	-- EDIT: removed rand and randomseed because it was not being used at all
	while true do
		-- EDIT: replaced T with os.clock(), renamed to t and placed inside the loop
		local t = os.clock()
		local x = pos.x + (math.sin(t + (pos.x/5)) * math.sin(t/9))/3
		local z = pos.z + (math.sin(t + (pos.z/6)) * math.sin(t/12))/4
		-- EDIT: added a space after a comma
		part.CFrame = CFrame.new(x, pos.y, z) * CFrame.Angles((z-pos.z)/tall, 0, (x-pos.x)/-tall)
		-- EDIT: moved task.wait() into the loop
		task.wait()
	end
end

for _, v in ipairs(trees) do
	if v.Name == "Tree" then
		for _, aa in ipairs(v:GetChildren()) do
			-- EDIT: merged the two ifs using `and`
			if aa:IsA('BasePart') and aa.Material == Enum.Material.Grass then
				coroutine.wrap(animate)(aa)
			end
		end
	end
end

Now this is the real optimization: I store information on each leaf part in a table and animate all of them in one loop.
The earlier approach did the same thing, but accessing a table is somewhat cheaper than resuming a coroutine (that is, the task scheduler timing each animate() individually and having to resume each of them).

local trees = script.Parent:GetChildren()
local parts = {} -- array of parts
local positions = {} -- array of positions
-- this could've also been done with an array of {part, position}s for easier expansion, but this reduces the amount of tables, saving memory somewhat

-- discover parts
for _, v in ipairs(trees) do
	if v.Name == "Tree" then
		for _, aa in ipairs(v:GetChildren()) do
			if aa:IsA('BasePart') and aa.Material == Enum.Material.Grass then
				table.insert(parts, aa)
				table.insert(positions, aa.Position + Vector3.new(0, -0.2, 0))
			end
		end
	end
end

-- animate parts
while true do
	local t = os.clock()
	
	for i = 1, #parts do
		local part = parts[i]
		local pos = positions[i]
		local tall = part.Size.Y / 2
		
		local x = pos.x + (math.sin(t + (pos.x/5)) * math.sin(t/9))/3
		local z = pos.z + (math.sin(t + (pos.z/6)) * math.sin(t/12))/4
		part.CFrame = CFrame.new(x, pos.y, z) * CFrame.Angles((z-pos.z)/tall, 0,(x-pos.x)/-tall)
	end
	
	task.wait()
end

In addition, I seriously hope you’re running this on the client! If this loop is running on the server, it will create a lot of pointless network traffic updating all the parts. If the network jitters a little, then the trees will also animate with some stutter.

2 Likes

This works because task.delay() runs the code contained inside on a new thread, this is normally used for running code after an elapsed time on a new thread (to not yield code).

You can use task.spawn() for the same effect, (running the code on a new thread) without the time parameter

2 Likes

probably shouldve thinked of that, thanks for all of the tips, i think thisll work!

Just worth noting that one additional reason to run this on the client is because if you have a lot of trees this can actually start to get rather processing intensive. When running on the client though you will be able to turn it off for lower end devices or lower graphics settings.

Further optimizations if you really want to implement them would be to make it so that it will either only update leaves within a certain distance from the camera or change the frequency of updates for leaves that are further away to reduce how much is done per frame. These optimizations might not be worth it though, especially if you only have a few trees. But if you were making large forests these should be considered.

2 Likes