How would I make this script efficient? (Small script)

So, I am trying to create MS2 (Mining Simulator 2) Solar System system.
If you don’t know what it is it’s basically an order system…
As you can see here in this image, it’s like that. (Mine)


Well the problem is the script is very inefficient and It doesn’t even work (sometimes)…
So how would I make a system like that by an efficient way?
Here is my code, Keep in mind it’s really inefficient.
Inefficient code

Map.PlanetTouchPartFolder.PlanetTouchPart.Touched:Connect(function()
	if Player.StatsFolder.Prestiege.Value <= 1 then
		Camera.CameraType = Enum.CameraType.Scriptable
		Camera.CFrame = Map.SolarSystem.Planets.Earth.CameraPart.CFrame
		
		
		for i,UI in pairs(Player.PlayerGui:GetChildren()) do
			if UI:IsA("Frame") or UI:IsA("TextButton") or UI:IsA("TextBox") or UI:IsA("ImageLabel") or UI:IsA("ImageButton") or UI:IsA("TextLabel") then
				UI.Visible = false
			end
		end
		
		MainGui.PlanetFrame.Visible = true
		
		
		Player.PlanetValue:GetPropertyChangedSignal("Value"):Connect(function()
			if Player.PlanetValue.Value == 1 then
				Camera.CFrame = Map.SolarSystem.Planets.Earth.CameraPart.CFrame
			elseif Player.PlanetValue.Value == 2 then
				Camera.CFrame = Map.SolarSystem.Planets.Mars.CameraPart.CFrame
			elseif Player.PlanetValue.Value == 3 then
				Camera.CFrame = Map.SolarSystem.Planets.MysteriousPlanet.CameraPart.CFrame
			elseif Player.PlanetValue.Value >= 1 then
				Player.PlanetValue.Value = 3
			end
		end)

		MainGui.PlanetFrame.NextButton.MouseButton1Click:Connect(function()
			Player.PlanetValue.Value += 1
		end)

		MainGui.PlanetFrame.BackButton.MouseButton1Click:Connect(function()
			Player.PlanetValue.Value -= 1
		end)
	end
end)
2 Likes

Any help asap would be appreciated…

I’d first suggest to replace that big if statement with another smaller if statement:

  1. I’d add variable set to an array that lists the different CFrames. For example:
local myTable = {
       Map.SolarSystem.Planets.Earth.CameraPart.CFrame,
Map.SolarSystem.Planets.Mars.CameraPart.CFrame, 
etc. 
}
  1. I’d change the if statement to say:
local cf = myTable[Player.PlanetValue.Value] 
if cf then Camera.CFrame=cf end

Can’t really see what else I’d do (I’m on my phone). I suggest these 2 changes for a bit more convenience, and easier to read code. For performance I’d say this is fine, getting a value from a table by index is faster than looping through it. Since you have number values for your different planets.

im not sure if your script is actually inefficient as you’ve claimed;
do you mean performance wise or readability wise?;
or is there something going wrong in your script?;
and also there is no debounce so im trying to figure out why your script doesnt have one considering that your using a touch event;
did you make a mistake by not putting in a debounce?;
or is it on purpose?

if Player.PlanetValue.Value == 1 then
				Camera.CFrame = Map.SolarSystem.Planets.Earth.CameraPart.CFrame
			elseif Player.PlanetValue.Value == 2 then
				Camera.CFrame = Map.SolarSystem.Planets.Mars.CameraPart.CFrame
			elseif Player.PlanetValue.Value == 3 then
				Camera.CFrame = Map.SolarSystem.Planets.MysteriousPlanet.CameraPart.CFrame
			elseif Player.PlanetValue.Value >= 1 then
				Player.PlanetValue.Value = 3
			end

what i did below is just a way that i would code the above part of your code.
there is nothing wrong with your code since its only really 4 if statements.

--top scope
local table = {
   Camera.CFrame = Map.SolarSystem.Planets.Earth.CameraPart.CFrame,
   Camera.CFrame = Map.SolarSystem.Planets.Mars.CameraPart.CFrame,
   Camera.CFrame = Map.SolarSystem.Planets.MysteriousPlanet.CameraPart.CFrame
}

--PlanetValue:GetPropertyChangedSignal("Value") event scope
Player.PlanetValue.Value = Player.PlanetValue.Value >= 1 and 3 or Player.PlanetValue.Value

Camera.CFrame = table[Player.PlanetValue.Value]

i wrote the above because i dont really know what to help you with.
Did you ran into a bug and thats why you think your code is inefficient?

You might be having problems because of the connections inside a connection. I do not know the full context of the script, these are my assumptions of what could be going wrong.

Since you connect property changed signal inside the touched connection you end up having a new connection every time it is touched, which is often dozens of times per frame. Furthermore the next/back button also get connected during touch.

I would recommend moving all three connections outside of the Touched Connection and validating inside those functions.

Map.PlanetTouchPartFolder.PlanetTouchPart.Touched:Connect(function()
	if Player.StatsFolder.Prestiege.Value <= 1 then
		Camera.CameraType = Enum.CameraType.Scriptable
		Camera.CFrame = Map.SolarSystem.Planets.Earth.CameraPart.CFrame
		
		
		for i,UI in pairs(Player.PlayerGui:GetChildren()) do
			if UI:IsA("Frame") or UI:IsA("TextButton") or UI:IsA("TextBox") or UI:IsA("ImageLabel") or UI:IsA("ImageButton") or UI:IsA("TextLabel") then
				UI.Visible = false
			end
		end
		
		MainGui.PlanetFrame.Visible = true
    end
end)

Player.PlanetValue:GetPropertyChangedSignal("Value"):Connect(function()
    -- put your validation. "When is the player not allowed to use this"
    if Player.StatsFolder.Prestiege.Value > 1 then
        return
    end

	if Player.PlanetValue.Value == 1 then
		Camera.CFrame = Map.SolarSystem.Planets.Earth.CameraPart.CFrame
	elseif Player.PlanetValue.Value == 2 then
		Camera.CFrame = Map.SolarSystem.Planets.Mars.CameraPart.CFrame
	elseif Player.PlanetValue.Value == 3 then
		Camera.CFrame = Map.SolarSystem.Planets.MysteriousPlanet.CameraPart.CFrame
	elseif Player.PlanetValue.Value >= 1 then
		Player.PlanetValue.Value = 3
	end
end)

MainGui.PlanetFrame.NextButton.MouseButton1Click:Connect(function()
    -- put your validation. "When is the player not allowed to use this"
    if Player.StatsFolder.Prestiege.Value > 1 then
        return
    end

	Player.PlanetValue.Value += 1
end)

MainGui.PlanetFrame.BackButton.MouseButton1Click:Connect(function()
    -- put your validation. "When is the player not allowed to use this"
    if Player.StatsFolder.Prestiege.Value > 1 then
        return
    end

	Player.PlanetValue.Value -= 1
end)
2 Likes

why are you connecting events inside of an event without disconnecting the events inside that event when that event’s function is over?

wouldnt that cause memory leak which that memory leak would also result in a bug?

1 Like

because you didnt disconnect these events below

nextButton click event
backButton click event
GetPropertyChangedSignal event --this event also causes a memory leak but wont cause a bug

i assume you ran into a bug and lag so therefore thats why you thought your code is inefficient
because it was somewhat laggy in the long run and it didnt do what you want

because you didnt dobunce, the touch event have a very good chance of running 4 or 15 times at once, connecting events the same amount of times as the amount of times the touch event ran.

This results in the PlanetValue being numbered at 4 or 15 because there is 4 to 15 events adding the value “1” and subtracting the value “1” which means none of your if statements will run or event if one of them does run correctly, it basically messes up your logic even more

i know that you have a memory leak and it is not intentional because you didnt even reference your events and usually when i dont need to disconnect an event, i dont reference it. So there was no attempt at disconnecting the events after the player is done using the Gui which is how i know that you definitely have a bug relating to memory leak

i wouldnt say your code is inefficient but just that the logic is wrong overall for what your attempting to make

the title “make script efficient” would be a bit misleading

one solution would be to put a local script inside of the screenGui your working with
and handle the events using that script
and so your script should look something like this

local debounce
Map.PlanetTouchPartFolder.PlanetTouchPart.Touched:Connect(function()
	if Player.StatsFolder.Prestiege.Value <= 1 then
        if debounce then return end
		Camera.CameraType = Enum.CameraType.Scriptable
		Camera.CFrame = Map.SolarSystem.Planets.Earth.CameraPart.CFrame
		
		
		for i,UI in pairs(Player.PlayerGui:GetChildren()) do
			if UI:IsA("Frame") or UI:IsA("TextButton") or UI:IsA("TextBox") or UI:IsA("ImageLabel") or UI:IsA("ImageButton") or UI:IsA("TextLabel") then
				UI.Visible = false
			end
		end
		
		MainGui.PlanetFrame.Visible = true
        debounce = true
        task.wait(0.4)
        debounce = nil
    end
end

your other script handling the events should look something like this

local Player --a reference of the local player
local MainGui --a reference of the gui your working with
Player.PlanetValue:GetPropertyChangedSignal("Value"):Connect(function()
    if Player.PlanetValue.Value == 1 then
        Camera.CFrame = Map.SolarSystem.Planets.Earth.CameraPart.CFrame
    elseif Player.PlanetValue.Value == 2 then
        Camera.CFrame = Map.SolarSystem.Planets.Mars.CameraPart.CFrame
    elseif Player.PlanetValue.Value == 3 then
        Camera.CFrame = Map.SolarSystem.Planets.MysteriousPlanet.CameraPart.CFrame
    elseif Player.PlanetValue.Value >= 1 then
        Player.PlanetValue.Value = 3
    end
end)

MainGui.PlanetFrame.NextButton.MouseButton1Click:Connect(function()
    Player.PlanetValue.Value += 1
end)

MainGui.PlanetFrame.BackButton.MouseButton1Click:Connect(function()
    Player.PlanetValue.Value -= 1
end)

I have found another solution than Tables Thanks all for trying to help