Selection:Get() returning more objects than are actually selected

[stormmaster9090]

So im making a plugin that gives parts studs again.
and i want a scrolling frame to show the selected items
but when the i select something it duplicates the item 3x
so if i selected 3 parts its cloning the frame 9 times.

here is my code

selection.SelectionChanged:Connect(function()
	for i, v in pairs(MainFrame.Scroller:GetChildren()) do
		if v:IsA("Frame") then
			v:Destroy()
		end
	end
	wait(0.1)
	for i, v in pairs(selection:Get()) do
		if v:IsA("Part") then
			local clone = template:Clone()
			clone:WaitForChild("Selection").Text = v.Name
			clone.Parent = MainFrame.Scroller
		end
	end
end)

ive tried looking but again could not find someone with the same issue.
does anyone know how to fix this?

[jaymeyzy (jaymey)]

have you checked if the selection changed event is firing multiple times? test it by adding a print statement at the start of the selectionchanged event

[stormmaster9090]

EDIT: the event fires anywhere from 5 to 21 times when I drag my mouse to select the objects.

[BendsSpace]

@stormmaster9090

it automatically clears all children so this shouldnt be the case

The problem could be that it fires multiple times, and each function call removes the current stuff, but then waits a second and adds the new stuff.

So, for example, two function calls start, both try to remove any current stuff, then it waits a bit, then they add new stuff.

If done more properly it shouldn’t be a problem though, but the code above will definitely have this problem because of the yielding it does throughout. A more proper way might be to store the children in a list, remove the wait, and edit the text in a new thread with task.spawn so it doesn’t hold up the main thread making the frames.

[BendsSpace]

If you do it like this with no yielding, then it’s not possible for the function calls to “overlap” each other in what they’re doing.

local frames = {}
local template = template
template:WaitForChild("Selection")  -- Wait for the selection only for the initial template, since it should exist when the Clone function is finished
selection.SelectionChanged:Connect(function()
	-- ipairs is faster for arrays
	for i, v in ipairs(frames) do
		v:Destroy()
	end
	frames = {} -- All frames have been destroyed, reset the list
	for i, v in pairs(selection:Get()) do
		if v:IsA("Part") then
			local clone = template:Clone()
			clone.Selection.Text = v.Name -- Because we asserted the Selection child exists in the template above, waiting isn't needed
			table.insert(frames, clone)
			clone.Parent = MainFrame.Scroller
		end
	end
end)

[stormmaster9090]

i havent used task.spawn
but would something like this work?

function removechildren()
	for i ,v in pairs(MainFrame.Scroller:GetChildren()) do
		if v:IsA("Frame") then
			v:Destroy()
		end
	end
end

function AddChildren(Table)
	for i, v in pairs(Table) do
		if v:IsA("Part") then
			local clone = template:Clone()
			clone:WaitForChild("Selection").Text = v.Name
			clone.Parent = MainFrame.Scroller
		end
	end
end

selection.SelectionChanged:Connect(function()
	print("EEE")
	removechildren()
	task.spawn(AddChildren)
end)

[BendsSpace]

I think that would have the same problem: because adding the children is split from the removing children, it’s not guaranteed that the things will happen in exactly the removechildren, addchildren, removechildren, addchildren, etc order. If the functions removechildren, removechildren then addchildren, addchildren, are run, then it’s a problem because we get two sets of added children.

When a function is called in response to an event, its code is run. The responses to events are handled in order, so one changed response is handled, and then the next is handled, and so on.

The problem is that if there is yielding, the entire game doesn’t just stop and wait, so it goes on to handling the next events and comes back to that function call. This means to guarantee the order above, you need to do the order without yielding in between.

This means that yielding at the addchildren level doesn’t work. Instead, you need to break out the yielding used to wait for the “Selection” child, or avoid waiting for that child all together. This makes sure all the children are added without yielding, but also allows the text to be updated.

Here is how that would look:

function removechildren()
	for i ,v in pairs(MainFrame.Scroller:GetChildren()) do
		if v:IsA("Frame") then
			v:Destroy()
		end
	end
end

function addChildren(Table)
	for i, v in pairs(Table) do
		if v:IsA("Part") then
			local clone = template:Clone()
			-- task.spawn is kind of like saying "we need this done, but not *exactly* right now"
			task.spawn(function()
				-- If we left this wait in sequence, it would be possible for another function call to slip in between while this function call was waiting
				clone:WaitForChild("Selection").Text = v.Name
			end)
			clone.Parent = MainFrame.Scroller
		end
	end
end

selection.SelectionChanged:Connect(function()
	print("EEE")
	-- It's very important we do these two right in a line, so they don't get split up
	removechildren()
	addChildren()
end)

This stuff can take a while to get use to. In languages like JavaScript for web development, a ton of developers go without knowing about this at all since the entire thing largely just runs in sequence.

[stormmaster9090]

Thank you for the reply, your solution worked and I will use that one, but I didnt know about some of this stuff so thank you for showing me how important it is!