Is this function as optimal as it gets?

Help and Feedback Code Review
1

This function is bound to the render step:

local function findPartsInSelectionBox()

	local a1 = selectionBox.AbsolutePosition
	local b1 = a1 + selectionBox.AbsoluteSize

	--- make sure a is the top left and b the bottom right
	local a = Vector2.new(math.min(a1.X, b1.X), math.min(a1.Y, b1.Y)) -- both are math.min
	local b = Vector2.new(math.max(a1.X, b1.X), math.max(a1.Y, b1.Y)) --both are math.max
	
	local newTable = {}
	local hasChanges = false

	for part, position in inputSelectablePartReturnExtentsCFramePosition do

		local point, onScreen = camera:WorldToScreenPoint(position)

		if not onScreen then
			continue
		end

		if a.X < point.X and point.X < b.X and a.Y < point.Y and point.Y < b.Y then
			
			if previousTable[part] == nil then -- means a new part was added
				hasChanges = true
			end
			
			newTable[part] = part
			
		end

	end
	
	local function Length(Table)
		local counter = 0 
		for _, v in Table do
			counter += 1
		end
		return counter
	end
	
	if hasChanges == false and Length(newTable) ~= Length(previousTable) then
		hasChanges = true
	end
	
	previousTable = newTable
	
	return newTable, hasChanges
	
end

-- Cases:
-- if exist in both
-- if only exist in previousTable
-- if only exist in newTable

Is the Length function in the right place? Should I move it up above and outside the scope of the findPartsInSelectionBox()? I literally don’t need it anywhere else, so I’m not sure. I don’t think it’s smart creating an entirely new function every frame. Could the line in the code that contains “-- means a new part was added” be optimized, by first checking if hasChanges is already true?

This is the code on the receiving end:

local returnedTable = findPartsInSelectionBox()

if returnedTable == nil then
	return
end

-- Check what was changed. Maybe do that in the function as well, since it already loops?

Any suggestions? Thanks!

2

Hey. Since it runs on RenderStepped, its better to move the legnth function outside of findPartsInSelectionBox() to avoid creating it every single frame. Also, yes, it can be optimised. You can change the block from:

if previousTable[part] == nil then -- means a new part was added
				hasChanges = true
			end

to

if not hasChanges and previousTable[part] == nil then -- means a new part was added
				hasChanges = true
			end

This will skip the check once a change has been detected, and basically prevent it from cycling even if a change has been detected.

1 Like
3

Thank you for confirming my suspicions :D

4 
local a = Vector2.new(math.min(a1.X, b1.X), math.min(a1.Y, b1.Y))
local b = Vector2.new(math.max(a1.X, b1.X), math.max(a1.Y, b1.Y))

You can use the builtin min and max instead.

local a = Vector2.min(a1, b1)
local b = Vector2.max(a1, b1)

local point, onScreen = camera:WorldToScreenPoint(position)

You should move this outside the loop.

if a.X < point.X and point.X < b.X and a.Y < point.Y and point.Y < b.Y then

You might be able to replace this with this, but I’m not entirely sure if the behavior will be the same.

if Vector2.min(Vector2.max(point, a), b) == point then

Instead of counting the dictionary afterwards, you should keep a counter inside the loop and compare it with a previousLength.

1 Like
5

Why so? For every part position in my table, I check where it is on my screen.

Otherwise, my new code looks like this:

local previousTable = {}
local previousLength = 0

local function findPartsInSelectionBox()

	local a1 = selectionBox.AbsolutePosition
	local b1 = a1 + selectionBox.AbsoluteSize

	local a = Vector2.min(a1, b1)
	local b = Vector2.max(a1, b1)
	
	local newTable = {}
	local newLength = 0
	local hasChanges = false

	for part, position in inputSelectablePartReturnExtentsCFramePosition do

		local point, onScreen = camera:WorldToScreenPoint(position)

		if not onScreen then
			continue
		end

		if a.X < point.X and point.X < b.X and a.Y < point.Y and point.Y < b.Y then
			
			if hasChanges == false and previousTable[part] == nil then -- means a new part was added
				hasChanges = true
			end
			
			newTable[part] = part
			newLength += 1
			
		end

	end
	
	if hasChanges == false and newLength ~= previousLength then
		hasChanges = true
	end
	
	if hasChanges == true then
		previousTable = newTable
		previousLength = newLength 
		return newTable
	else
		return nil
	end
	
end

Edit: I forgot to mention that point is a vector3, with it’s Z being the distance in studs that position is away from the camera.

6

My bad, I skimmed over position being inside the loop!

2 Likes
7

No worries :hugs: