Is there a better way to write my SpawnPoisFromGivenList function?

Help and Feedback Scripting Support
#1

should this be in code review? idk
i’m not necessarily asking for a global code review, just asking if there’s a better way to do what i did

the problem is:

  • i technically loop through all the pois in the given folder
  • each iteration, i call the :GetChildren() method so i can get the random poi

this is my main complaint, i don’t really like this part

function SpawnPoisFromGivenList(listOfPois: Folder, tileToSpawnOn: BasePart)
	-- loop through all the pois in the list
	local amountOfPois = #listOfPois:GetChildren()
	
	for i = 1, amountOfPois, 1 do
		-- then, for every iteration, generate a number between 1
		-- and the amount of pois in the folder
		local randomNumber = math.random(1, #listOfPois:GetChildren())

		-- after generating the random number, get the poi
		local chosenPoi = listOfPois:GetChildren()[randomNumber]
		
		-- see if the poi exists already, if it does
		-- skip the iteration
		if mapContentFolder:FindFirstChild(chosenPoi.Name) then continue end
		
		-- otherwise, clone it, parent it and then pivot it
		-- along with a random orientation
		local clonedPoi: Model = chosenPoi:Clone()
		
		clonedPoi.Parent = mapContentFolder
		clonedPoi:PivotTo(tileToSpawnOn.CFrame)
		
		-- get a random orientation (0, 90, 180, 270)
		-- by generating a random number between 0 and 3, and then
		-- multiplying it by 90
		-- rocket science!
		local randomOrientation = math.random(0, 3) * 90
		
		-- and now, rotate the clonedPoi by the random orientation
		-- on the Y axis
		clonedPoi:PivotTo(clonedPoi:GetPivot() * CFrame.Angles(0, math.rad(randomOrientation), 0))
		
		-- now, we gotta move the spawn parts of the poi to the main folder
		-- that's tracked by everything, otherwise the game won't
		-- spawn objects (and enemies)
		MoveSpawnPartsOfPoiToGlobalFolder(clonedPoi)
		
		-- after all of that, set the occupied attribute of the tile to true
		tileToSpawnOn:SetAttribute("Occupied", true)
	end
end
1 Like
#2

Personally, I’d use a while loop for this

function SpawnPoisFromGivenList(listOfPois: Folder, tileToSpawnOn: BasePart)
	-- loop through all the pois in the list
	local listOfPois = listOfPois:GetChildren()
	
	while (#listOfPois > 0) do
		-- then, for every iteration, generate a number between 1
		-- and the amount of pois in the folder
		local randomNumber = math.random(1, #listOfPois)
		
		-- remove poi from the list
		table.remove(listOfPois, randomNumber)

		-- after generating the random number, get the poi
		local chosenPoi = listOfPois[randomNumber]
		
		-- see if the poi exists already, if it does
		-- skip the iteration
		if mapContentFolder:FindFirstChild(chosenPoi.Name) then continue end

		-- otherwise, clone it, parent it and then pivot it
		-- along with a random orientation
		local clonedPoi: Model = chosenPoi:Clone()

		clonedPoi.Parent = mapContentFolder
		clonedPoi:PivotTo(tileToSpawnOn.CFrame)

		-- get a random orientation (0, 90, 180, 270)
		-- by generating a random number between 0 and 3, and then
		-- multiplying it by 90
		-- rocket science!
		local randomOrientation = math.random(0, 3) * 90

		-- and now, rotate the clonedPoi by the random orientation
		-- on the Y axis
		clonedPoi:PivotTo(clonedPoi:GetPivot() * CFrame.Angles(0, math.rad(randomOrientation), 0))

		-- now, we gotta move the spawn parts of the poi to the main folder
		-- that's tracked by everything, otherwise the game won't
		-- spawn objects (and enemies)
		MoveSpawnPartsOfPoiToGlobalFolder(clonedPoi)

		-- after all of that, set the occupied attribute of the tile to true
		tileToSpawnOn:SetAttribute("Occupied", true)
	end
end

Also, your code isn’t removing the picked pois from the list after they’re used, which allows for a poi to be picked more than once. Meaning, you would most likely end up having some pois uncloned.

  • i technically loop through all the pois in the given folder
1 Like