Another way of making an efficient snap to grid build system?

Help and Feedback Scripting Support
1

Current way I’m doing is: on the player’s base, I have many 5x1x5 sized blocks named “MouseDetector” and if the mouse hovers over them and clicks, the item will be placed there

for i, v in pairs(BuildFrame:GetChildren()) do
	if v:IsA("ViewportFrame") and v:FindFirstChild("ClickGui") then
		local clonedModel = v:FindFirstChildWhichIsA("Model"):Clone()
		clonedModel.Parent = workspace
		v.ClickGui.MouseButton1Down:Connect(function()
			RS_connection = RunService.RenderStepped:Connect(function()
				local target = mouse.Target
				if target.Name == "MouseDetector" then
					clonedModel:PivotTo(target.CFrame)
				end
				mouse_connection = mouse.Button1Down:Connect(function()
					if RS_connection then RS_connection:Disconnect() end
					if mouse_connection then mouse_connection:Disconnect() end
				end)
			end)
			ItemsBG:TweenPosition(UDim2.new(0,0,1,0),nil,nil,0.1,true)
		end)
	end
end

I’m using RenderStepped for now, I don’t know if there’s a better way of doing this without needing to fire a function every frame, so if there’s a better way of doing lemme know

Thanks in advance

2

This doesn’t work like you expect:

if RS_connection then RS_connection:Disconnect() end
if mouse_connection then mouse_connection:Disconnect() end

RS_connection and mouse_connection will never be nil because calling Disconnect on a RBXScriptConnection object doesn’t remove the references to it. It just sets connection.Connected to false and stops calling the function when the event is fired.

The reason it hasn’t broken yet is that the function where those statements occur will never be called more than once, since the connection that invokes it is disconnected by the listener function. Be confident that disconnecting events works and just do

RS_connection:Disconnect()
mouse_connection:Disconnect()

In cases where you really do need an if statement like that to work, instead do

if RS_connection then RS_connection:Disconnect(); RS_connection = nil end
if mouse_connection then mouse_connection:Disconnect(); mouse_connection = nil end

… to actually make the if statement do something, assuming the function isn’t actually disconnecting the connection that it’s the listener of.

Anyway, I also have some feedback on the overall way of doing things

RS_connection = RunService.RenderStepped:Connect(function()
    ...
    mouse_connection = mouse.Button1Down:Connect(function()
        if RS_connection then RS_connection:Disconnect() end
        if mouse_connection then mouse_connection:Disconnect() end
    end)
end)

You’re setting up and cleaning up a Button1Down connection every frame. It’s extra unnecessary work for the CPU, but not something that’s likely to make a noticeable difference. What’s much worse is that the code is pretty confusing to read. That’s bad because it makes it harder to get help, and will make you introduce bugs that break your game if you’re lucky or introduce memory leaks that are hard to debug if you’re unlucky.

Here are the first changes I’d make:

for _, v in pairs(BuildFrame:GetChildren()) do
	if v:IsA("ViewportFrame") and v:FindFirstChild("ClickGui") then
		local clonedModel = v:FindFirstChildWhichIsA("Model"):Clone()
		clonedModel.Parent = workspace
		
        --On item clicked in the GUI...
        v.ClickGui.MouseButton1Down:Connect(function()
            --Moved this first because it's kinda easy to miss down at the bottom, and the timing works out the same either way
            ItemsBG:TweenPosition(UDim2.new(0, 0, 1, 0), nil, nil, 0.1, true) --Spaces after commas pls, it makes a big difference when reading

            --Split into two separate connections
            --Easier to understand because:
            --  Everything is at most 2 levels deep instead of 3 in terms of connections inside connections etc...
            --     Which is better because both events are something can can happen in any order and independently of each other,
            --     so it's more in-line with the logic to have the event connections be side-by-side instead of one being inferior and one superior (hiearchy)
            --  No connections get unnecessarily set up and cleaned up every frame
            local rendersteppedConnection
            local button1DownConnection
            
            --Move the cloned model to where the mouse is every frame.
            --Gets disconnected in Button1Down event listener.
			rendersteppedConnection = RunService.RenderStepped:Connect(function()
				local target = mouse.Target
				if target.Name == "MouseDetector" then
					clonedModel:PivotTo(target.CFrame)
				end
			end)

            --Next time a click occurs...
            button1DownConnection = mouse.Button1Down:Connect(function()
                button1DownConnection:Disconnect() --But not the times after that...
                
                --Stop moving the cloned model to the mouse every frame, 
                rendersteppedConnection:Disconnect()

                --Do other things related to placing the model in the game world, presumably?
            end)
		end)
	end
end

Having fewer things going on (and comments to explain why) makes it a lot easier to verify that connections get disconnected when appropriate.

EDIT: The Maid library is a cool way of managing state like this, check it out if you want

2 Likes
3

Thanks for the clear cut explanation! Appreciate it

1 Like