Cloned model is cloned multiple times

I’ve seen something like my issue, but it didn’t make sense in my condition. I’m making it so that where a player clicks, a model is cloned at the mouse position. It does exactly that, but if I do it for the second time, it clones two there. A third time is three, and so on. I’m not sure why it’s doing this

rs:FindFirstChild("cce2").OnServerEvent:Connect(function(player,key)
	on = true
	workspace:FindFirstChild("spawnedcpu").screen.BrickColor = BrickColor.new("Really black")
	workspace:FindFirstChild("spawnedcpu").screen.Material = Enum.Material.Plastic
	rs.mouse:FireClient(player)
	if on == true then
		rs:FindFirstChild("mouse").OnServerEvent:Connect(function(player,mousepos)
			local teacher = rs:WaitForChild("block"):Clone()
			teacher.Parent = workspace
			local cframe = CFrame.new(mousepos)
			teacher:SetPrimaryPartCFrame(cframe)
			wait(0.5)
			rs.enable:FireClient(player)
		end)
	end
end)

Because you are checking if the on variable is true, that’s why it is still cloning. You need to set it to false.

That’s not the issue. Even if I remove the variables fully the script works but the cloning is still out of hand

local on = false

rs:FindFirstChild("cce2").OnServerEvent:Connect(function(player,key)
	workspace:FindFirstChild("spawnedcpu").screen.BrickColor = BrickColor.new("Really black")
	workspace:FindFirstChild("spawnedcpu").screen.Material = Enum.Material.Plastic
	rs.mouse:FireClient(player)
	if not on then
		rs:FindFirstChild("mouse").OnServerEvent:Connect(function(player,mousepos)
			local teacher = rs:WaitForChild("block"):Clone()
			teacher.Parent = workspace
			local cframe = CFrame.new(mousepos)
			teacher:SetPrimaryPartCFrame(cframe)
			wait(0.5)
			rs.enable:FireClient(player)
                        on = true
		end)
	end
end)
2 Likes

What’s the code on the client? This is for the server and I don’t really see anything that would cause the issue. Perhaps the client is sending the request multiple times?

Besides that, I see other issues with your code…

workspace:FindFirstChild(spawnedcpu).screen.BrickColor = BrickColor.new(Really black)
workspace:FindFirstChild(spawnedcpu).screen.Material = Enum.Material.Plastic

can be shortened to this:

local item = workspace:FindFirstChild(spawnedcpu)
item.screen.BrickColor = BrickColor.new(Really black)
item.screen.Material = Enum.Material.Plastic

That way, you are only running functions once. It’s not much of a performance improvement, but on the server, every clock cycle counts. Then there is this code:

			local teacher = rs:WaitForChild("block"):Clone()
			teacher.Parent = workspace
			local cframe = CFrame.new(mousepos)
			teacher:SetPrimaryPartCFrame(cframe)
			wait(0.5)
			rs.enable:FireClient(player)

Always set the parent at the end. Setting it to the beginning will cause the server to replicate each change to all clients which means more work for the server which means more of a chance for lag. Here’s the new code:

			local teacher = rs:WaitForChild("block"):Clone()
			local cframe = CFrame.new(mousepos)
			teacher:SetPrimaryPartCFrame(cframe)
			teacher.Parent = workspace
			wait(0.5)
			rs.enable:FireClient(player)

I would also strongly recommend that you use a debounce in your code. What a debounce does is that is limits the rate of requests to the server so it doesn’t get bogged down so much. The general form of a debounce is this:

local debounce = false

some_function_definition(some_parameters)
	if not debounce then
		debounce = true
		-- Do Something
		delay(timeout, function()
			debounce = false
		end)
	end
end

The delay function spawns a new thread that doesn’t start executing the nested function until the timeout has elapsed. This is the preferred way because it doesn’t hang up the thread context that the some_function is executing in.

So with the debounce, your code would look like this:

local on = false

rs:FindFirstChild("cce2").OnServerEvent:Connect(function(player,key)
	workspace:FindFirstChild("spawnedcpu").screen.BrickColor = BrickColor.new("Really black")
	workspace:FindFirstChild("spawnedcpu").screen.Material = Enum.Material.Plastic
	rs.mouse:FireClient(player)
	if not on then
		on = true
		rs:FindFirstChild("mouse").OnServerEvent:Connect(function(player,mousepos)
			local teacher = rs:WaitForChild("block"):Clone()
			local cframe = CFrame.new(mousepos)
			teacher:SetPrimaryPartCFrame(cframe)
			teacher.Parent = workspace
			delay(0.5, function()
				rs.enable:FireClient(player)
				on = false
			end)
		end)
	end
end)
1 Like

you should not set the on variable to false again if you don’t want to clone it twice. Its better to use task.delay() instead of task.wait()

No. This is the form a of debounce. I use it everywhere in my code and it works just fine. By waiting, the thread is put to sleep for the duration. Using delay to spawn a new thread keeps the original thread executing to the end. And you have to set the on variable to true again at some point if you want to clone a different item.

The solutions presented address the cloning issue on the server, but isn’t the root cause. The real problem is on the client. The problem that the OP describes is that the client is sending multiple requests for each click. That needs to be addressed.

1 Like

The real issue here is the nested OnServerEvent connection, and lack of a subsequent disconnection. Each time the outer OnServerEvent is fired, its callback function is called resulting in a connection being made to the inner OnServerEvent event. This means that if the outer event is fired twice, the inner event is connected twice, if the inner event is then fired its connected callback function(s) is/are executed twice.

2 Likes

After I read your post, I went back and looked at it. Sure enough you are right. I don’t know how I missed that. I stand corrected.