Question about having a loop or base it of a returned value

Help and Feedback Scripting Support
#1

Sorry for the vague title, but i have an idea to change my script to make it “Cleaner” and somewhat better.

Basically how it is now is a loop to find if the guns been purchased


local player = game.Players.LocalPlayer
local SelGui = script.Parent

local Modules = game.ReplicatedStorage:WaitForChild("Modules")
local Events = game.ReplicatedStorage:WaitForChild("Events")
local Inventorys = game.ReplicatedStorage:WaitForChild("InvHolder")
local PlayerFolder = Inventorys:WaitForChild(player.Name .. "DataFolder")

local GunMod = require(Modules.GunShop)

local GunEvent = Events:WaitForChild("ShopEvent")
local PurchaseEvent = Events:WaitForChild("PurchaseEvent")
local EquipEvent = Events:WaitForChild("EquipEvent")

local Image = SelGui:WaitForChild("GunPicture(TEMP)")
local CostText = SelGui:WaitForChild("CostLabel")
local ReqText = SelGui:WaitForChild("ReqLevelLabel")
local PurchaseButton = SelGui:WaitForChild("PurchaseButton")
local EquipButton = SelGui:WaitForChild("EquipButton")

local GunSelected = nil

local function BuyGun(Gun)
	if not Gun then return "No Gun Selected" end
	if not GunMod[Gun] then return "NoGun" end
	PurchaseEvent:FireServer(Gun)
end

local function EquipGun(Gun)
	if not Gun then return "Still No Gun" end
	if not GunMod[Gun] then return "Gun Doesnt Exist" end
	EquipEvent:FireServer(Gun)
end

GunEvent.Event:Connect(function(Gun)
	if Gun == GunSelected then
		GunSelected = nil
	else
		GunSelected = Gun
	end
end)

PurchaseButton.Activated:Connect(function()
	BuyGun(GunSelected)
end)

EquipButton.Activated:Connect(function()
	EquipGun(GunSelected)
end)


while wait() do
	if GunSelected ~= nil then
		Image.Image = if GunMod[GunSelected].PictureId then "http://www.roblox.com/asset/?id=" .. GunMod[GunSelected].PictureId else 0
		CostText.Text = GunMod[GunSelected].Cost .. "$"
		ReqText.Text = GunMod[GunSelected].LevelReq
		if PlayerFolder:WaitForChild("ShopOwned")[GunMod[GunSelected].GunType .. "Guns"]:FindFirstChild(GunSelected) then
			local GunType = GunMod[GunSelected].GunType
			if PlayerFolder.Game["Equipped" .. GunType].Value == GunSelected then
				EquipButton.Visible = true
				EquipButton.Text = "UnEquip"
			else
				EquipButton.Visible = true
				EquipButton.Text = "Equip"
			end
		else
			PurchaseButton.Visible = true	
		end
	else
		Image.Image = 0
		CostText.Text = " "
		ReqText.Text = " "
		PurchaseButton.Visible = false
		EquipButton.Visible = false 
	end
end

Ya know, mainly so if they purchase / equip the gun they dont have to reclick the guns button to see the new equip / unequip button

Although im pretty sure someone mentioned another way i could do it in another thread which ive been thinking about

I was thinking to use RemoteFunctions and return values if the player bought the gun / if they equipped / if they unequipped and just change the button depending on the returned value (Basically getting rid of the need of a loop), although i dont know if this is would benefit the code for the better or for the worse

(edit)
Added more information i left out

#2

It’s definitely not crazy to change state using events, and have a dedicated loop handling the visuals like this.

However, I would remove any yields (e.g. WaitForChild) from the update loop and maybe use BindToRenderStep instead of while wait().

For the sake of terminology:

  • You (sort of) have an “immediate mode GUI”, where you update properties every frame based on some state

  • You’re considering switching to a “retained mode GUI”, where you update properties only when they change

    (more details)

Both work. Immediate mode tends to make nicer code that’s very declarative and easy to change, but can be slower (relatively—it’s fast enough).

The downside, and why maybe I would suggest switching to a retained-mode system, is that roblox’s GUI APIs are all retained mode anyways. Things like creating/destroying instances or using TweenService are going to be hard with the loop-based method.

Also, you have to be careful that all your if-branches set all the properties you care about. If you had something like:

while wait() do
  if on then
    frame.Color = Color3.new(1, 0, 0)
  end
end

Now your frame will turn red when on is true, but will never undo that change. You have to start with a default:

while wait() do
  frame.Color = Color3.new()
  if on then
    frame.Color = Color3.new(1, 0, 0)
  end
end

or do something like:

while wait() do
  frame.Color = on and Color3.new(1, 0, 0) or Color3.new()
end

that will probably get tedious as your GUI gets more complicated. You could wrap some of that in a framework—that’s what roact does to get a usable state-based GUI API in roblox.

How much does using RunService.RenderStepped affect optimization?
#3

I’m not sure you why you have that big while wait() do logic. Why not put all of that code in your GunEvent function?

GunEvent.Event:Connect(function(Gun)
	
	if Gun == GunSelected then
		Image.Image = 0;
		CostText.Text = " ";
		ReqText.Text = " ";
		PurchaseButton.Visible = false;
		EquipButton.Visible = false;
		GunSelected = nil;
	else
		GunSelected = Gun;
		Image.Image = if GunMod[GunSelected].PictureId then "http://www.roblox.com/asset/?id=" .. GunMod[GunSelected].PictureId else 0
		CostText.Text = GunMod[GunSelected].Cost .. "$";
		ReqText.Text = GunMod[GunSelected].LevelReq;
		if PlayerFolder:WaitForChild("ShopOwned")[GunMod[GunSelected].GunType .. "Guns"]:FindFirstChild(GunSelected) then
			local GunType = GunMod[GunSelected].GunType;
			if PlayerFolder.Game["Equipped" .. GunType].Value == GunSelected then
				EquipButton.Visible = true;
				EquipButton.Text = "UnEquip";
			else
				EquipButton.Visible = true;
				EquipButton.Text = "Equip";
			end
		else
			PurchaseButton.Visible = true;
		end
	end
	
end)
#4

Because, if the player buys the gun, they will have to reselect the gun to see the equip button, and so on if they decide to equip it and want to unequip it.

#5

I did this before, using findfirstchild
And it just decides to error 50% of the time for no reason.

#6

Well, it’s erroring for a reason. You have to check the return value of FindFirstChild. If it’s nil, don’t do anything and just try again on the next event loop.

Essentially re-implementing WaitForChild within your own loop.

#7 
if PlayerFolder:WaitForChild("ShopOwned")[GunMod[GunSelected].GunType .. "Guns"]:FindFirstChild(GunSelected) then

It would error here everytime, which is why i used :WaitForChild instead of FindFirstChild

Dont know why it would even error, it would have a long time to load as the loop doesnt even work until a gun is selected which gives it a good enough time to load everything

Either way, This is just for the loops, i could change it to the other way and find a better way to achieve it