What does the code do and what are you not satisfied with?
This is a snippet of a local script that handles a Main Menu Gui. I’m confused, is it optimal the way it is?
My current code:
playButton.MouseButton1Click:Connect(function()
playContainer.Visible = true
buttonsContainer.Visible = false --the playButton is inside of this frame so it becomes invisible.
local firstConn
local secondConn
firstConn = playContainer.Original.MouseButton1Click:Connect(function()
local success, err = pcall(function()
tpService:Teleport(placeID, player)
end)
if not success then
warn("Teleport failed: " ..err)
end
end)
secondConn = playContainer.X.X.MouseButton1Click:Connect(function()
playContainer.Visible = false
buttonsContainer.Visible = true
firstConn:Disconnect()
firstConn = nil
secondConn:Disconnect()
secondConn = nil
end)
end)
Or could I just do this and it would be fine:
playButton.MouseButton1Click:Connect(function()
playContainer.Original.MouseButton1Click:Connect(function()
local success, err = pcall(function()
tpService:Teleport(placeID, player)
end)
if not success then
warn("Teleport failed: " ..err)
end
end)
end)
You disconnect functions from events only when you don’t need that connection anymore. For example, if you have RenderStepped event connected to a function while holding a tool, you would disconnect that connection when you un-equip that tool.
No, this is wrong. You need to be cautious of when and how you connect and disconnect. Having code like the OP does willl lead to infinite connections calling the same function. He will lag his game like this.
If you reference a connection in a variable, that connection will remain there untill you set the var to nil or disconnect it.
It is absolutely possible to have multiple identical functions connected to a single event. Which is a huge hazard in coding.
So im not entirely clear on what youre trying to do and im almost certain there is a more elegant way to do this.
However. I will tell you what to look out for with your code. In both examples it doesnt seem like you check if the connections already exist. What your code will do is each time playButton is clicked, you are creating additional identical connections to playContainer.
This will cause the firstConn to run multiple times on execution which could build up and cause lag. Now you do disconnect when players activate secondConn but what if players never activate secondConn and continually press playButton. Now when someone triggers fistConn the function will run a bunch of times in an instant causing slowdowns.
I would recommend replanning what youre trying to do or declare first and second conn in the global scope and check if they already exist when the player clicks.
But again, im nore sure what youre trying to make here. Maybe you do need multiple identical connections. There are some valid cases for it, but its generally looked down upon.
When the playButton is pressed, it becomes invisible, therefore it cannot be clicked again until the playerContainer.X.X is clicked, which disconnects all of the connections. In this case I don’t actually need to check if the connections already exist, right? Because Im pretty sure there can be only one of each connection at once, the way I have the script currently set up.
There is no reason to disconnect the event for your playContainer.Original button, and also no reason to create the connection every time the playButton is clicked. Why not just connect the signal for all of your buttons regardless of whether or not they’re visible? It’s not going to have any noticeable performance cost, and it won’t be clickable if it’s not visible either way, so you don’t need to worry about someone accidentally clicking it when they shouldn’t be able to.
While in theory yes this works and in practice will probably function well, if you plan on continuing this project in the future, you should design things in as efficient and clean a way as possible. You never know what you might change in the future.
Its a common mistake to put problems like this off for the future because you almost always forget that you had the buttons set up this way. While you could just make them much more flexible now. Because now you have to make absolutely sure that the button goes invisible with no issues at all for the rest of the time spent on the project.
Last point you could create the first and second conn in global scope with no change in performance. You wouldnt need to disconnect them since all youd have to do is disable the buttons.
I understand what you are trying to say, however, once you disconnect an RBXScriptConnection you cannot reconnect it. There is no :Reconnect() method. To “reconnect” you will have to create a new connection.
Thus, you disconnect when you do not need it anymore. When you need that connection again, you create a new connection. As in the example in my previous reply, if you want something to run every frame only when holding a particular tool, you would disconnect the connection when the player un-equips the tool and create a new connection when the player re-equips it.
Sorry I haven’t been replying, anyway I edited the post a little because I feel like I wasn’t clear enough originally. Have a look at the post again, I might not have described it well initially
Are you making only computer ? because you could do like this (if your game for all plaftorm)
i probably made this Incorrectly
local GuiButton = --TextButton, whatever it might as counts button
GuiButton:GetPropertySignalChanged("GuiState"):Connect(function()
if GuiButton.GuiState=Enum.GuiState.Press then
-- do something here
end
end)
Don’t disconnect if you can see PlayButton again (with a Visible = true) and you want the same code to run on click.
If you ever have a situation where you won’t see a GUI instance again, :Destroy() it instead of doing Visible = false. The connection will be disconnected automatically.
Ok, say I don’t disconnect, won’t there be a bunch of connections created when I press the button and just keep closing the frame (and don’t trigger any other events)
The connections should be separate, one per button.
playButton.Activated:Connect(function()
playContainer.Visible = true
buttonsContainer.Visible = false
end)
playContainer.Original.Activated:Connect(function()
if not pcall(tpService.Teleport, tpService, placeID, player) then
warn("Teleport failed")
end
end)
playContainer.X.X.Activated:Connect(function()
playContainer.Visible = false
buttonsContainer.Visible = true
end)
(Activated vs. MouseButton1Click is down to personal preference; MouseButton1Click fires with touch and gamepad inputs too, but I use Activated because it’s shorter, more universal, and provides better event parameters)
Now you have three connections total, that never change. Like you said,
so there’s no need to worry about disconnecting if you always want the button to run that code when clicked.
Further, check out the Interactable property, which will prevent buttons from receiving input even if they’re visible. You don’t need to worry about it if you’re going to keep using the Visible property on the parent, but in case you want to start tweening transparency or disabling buttons, you still don’t need to disconnect the events.
For buttons, think of the connection akin to a property. You wouldn’t do
local function open()
parent.Visible = false
child.Text = "" --This is unnecessary, we already don't see it
end
local function close()
child.Text = "The equivalent of reconnecting with the same function"
parent.Visible = true
end