Dropper activating multiple times

Help and Feedback Scripting Support
, ,
#1

I’m currently working on my first tycoon. You have to buy droppers that drop money so you buy more and so on… the problem is that sometimes when I buy the dropper upgrade it starts to drop more than one block which it’s not supposed to do. I have tried using boolean values to check if the dropper has been activated already but it seems to still happen sometimes.

this is the script that is activated when the player walks on the upgrade and is prompted for the purchase. This code eventually fires a remote to the server.

dropper.Touched:Connect(function()
		if touching_dropper == false then
			touching_dropper = true
			if dropper.active.Value == false then
				--prompt confirm purchase ui
				prompt_gui.Enabled = true
				prompt_gui.Frame.TextLabel.Text = "Would you like to buy dropper for " .. dropper.price.Value .. "$ ?"
				--connect functions for confiorm and cancel buttons
				local confirm
				confirm = prompt_gui.Frame.Frame.confirm.Activated:Connect(function()
					--fire remote event to server
					dropper_prompt:FireServer(dropper)
					prompt_gui.Enabled = false
					confirm:Disconnect()
				end)
				local cancel
				cancel = prompt_gui.Frame.Frame.cancel.Activated:Connect(function()
					prompt_gui.Enabled = false
					cancel:Disconnect()
				end)
				
				--wait until the ui is closed or player walks away
				repeat wait() until prompt_gui.Enabled == false or math.abs(player.Character.PrimaryPart.Position.X - dropper.Position.X) > 5 or math.abs(player.Character.PrimaryPart.Position.Z - dropper.Position.Z) > 5
				prompt_gui.Enabled = false
			end
			task.wait(0.5)
			touching_dropper = false
		end
	end)

this is the server script that fires when it receives the remote

dropper_prompt.OnServerEvent:Connect(function(player, dropper)
	local slots_store = DataStore2("slots", player)
	local coin_store = DataStore2("coins", player)
	local orb_store = DataStore2("orb", player)
	local money_store = DataStore2("money", player)
	
	--check if price is correct and deduct form current money amount
	if dropper.price.Value <= money_store:Get() then	
		dropper.active.Value = true
		money_store:Increment(-dropper.price.Value)
		dropper.Parent.fade.Color = Color3.new(0,1,0)
		dropper.model.Value.Transparency = 0
		--spawn function that continuously spawns orbs from the dropper
		spawn(function()
			while dropper.active.Value == true do
				local block = orb_store:Get():Clone()
				block.Position = dropper.model.Value.Position + Vector3.new(-2.1,-2.3,0)
				block.Parent = dropper.model.Value
				block.Anchored = false
				game.Debris:AddItem(block, 20)
				block.Touched:Connect(function(part)
					--add money when orb touches the collector
					if part == slots_store:Get().maindropper.collector then
						coin_store:Increment(block.multiplier.Value)
						slots_store:Get().maindropper.screen.SurfaceGui.TextLabel.Text = coin_store:Get()
						local orb_size = slots_store:Get().maindropper.dropper_orb.orb.Size   
						local multiplier = orb_store:Get().multiplier.Value
						if orb_size.x < 8 then
							slots_store:Get().maindropper.dropper_orb.orb.Size += Vector3.new(0.1/multiplier,0.1/multiplier,0.1/multiplier)
							slots_store:Get().maindropper.dropper_orb.orb.Fire.Size += 0.4/multiplier
						else
							slots_store:Get().maindropper.dropper_orb.orb.Fire.Heat += 1/multiplier
						end
						block:Destroy()
					end
				end)
				task.wait(1)
			end
		end)
	else
		print("price is: ", dropper.price.Value)
	end
end)

I’m very happy to receive possible solutions. (I’m also happy to receive feedback on the code if I have any bad practices as I’m quite new to coding)

3 Likes
#2

Add a variable that at the start is true and after you run the script and do what you need is false

1 Like
#3

Try that:

while wait(1) do
if dropper.active.Value == false then return; end;
				local block = orb_store:Get():Clone()
				block.Position = dropper.model.Value.Position + Vector3.new(-2.1,-2.3,0)
				block.Parent = dropper.model.Value
				block.Anchored = false
				game.Debris:AddItem(block, 20)
				block.Touched:Connect(function(part)
					--add money when orb touches the collector
					if part == slots_store:Get().maindropper.collector then
						coin_store:Increment(block.multiplier.Value)
						slots_store:Get().maindropper.screen.SurfaceGui.TextLabel.Text = coin_store:Get()
						local orb_size = slots_store:Get().maindropper.dropper_orb.orb.Size   
						local multiplier = orb_store:Get().multiplier.Value
						if orb_size.x < 8 then
							slots_store:Get().maindropper.dropper_orb.orb.Size += Vector3.new(0.1/multiplier,0.1/multiplier,0.1/multiplier)
							slots_store:Get().maindropper.dropper_orb.orb.Fire.Size += 0.4/multiplier
						else
							slots_store:Get().maindropper.dropper_orb.orb.Fire.Heat += 1/multiplier
						end
						block:Destroy()
					end
				end)
			end
#4

I can see a couple errors in your practices, but I’m generally surprised by the fact that you’re quite new to this stuff.
You can completely cut out the == true from while dropper.active.Value == true do, as it will return the same boolean value.
You used a wait() and then task.wait() immediately after? Nevertheless, only use task.wait(). A more performant way would be to add a delay or connect to a function that fires if the prompt’s Enabled property changes or the player’s position changes.

Anyways, is there a reason for the spawn() in the server script? It feels like it could be the cause of the issue, I don’t see anything after the function that the script needs to reach.

#5

The spawn() is indeed not needed, but even after removing it the problem is still there. Could you maybe show me how I can have a function that fires when the prompt’s Value changes?

2 Likes
#6

Sorry, can’t immediately tell what’s wrong. You should clean up your code by using early returns instead of nesting where possible, e.g. if statements with no else block and nothing after.

So

dropper.Touched:Connect(function()
		if touching_dropper == false then
			touching_dropper = true

should be

dropper.Touched:Connect(function()
		if touching_dropper then return end
                
		touching_dropper = true

This will make your code a lot more readable and easier to debug. Speaking of which, you can narrow it down by using the debugger as well as print debugging. Start by placing a print statement just before the FireServer call, to see if the bug originates in the client code and to see the timing of when the FireServer calls are made. If it does print twice when it should only print once, then place a breakpoint in the Touched listener function and step through the code line-by-line, seeing if the correct control flow is followed and inspecting the variables when the wrong flow is followed to see what variables have the wrong value.

3 Likes
#7

Thank you for the response, but this didn’t fix the issue as it only removed the spawn() function (if dropper.active.Value == false then return; end; is not supposed to fire during normal gameplay)

#8

Thank you! I cleaned up the code and started debugging. I will be sure to mark your answer as the solution if I find the issue. The problem is that I don’t exactly know how to replicate the bug.

update after debugging the issue seemed to that the functions for the buttons would fire even after the ui was closed so hte simple fix was to disconnect the functions when the ui closes.

repeat task.wait() until prompt_gui.Enabled == false or math.abs(player.Character.PrimaryPart.Position.X - dropper.Position.X) > 5 or math.abs(player.Character.PrimaryPart.Position.Z - dropper.Position.Z) > 5
			confirm:Disconnect()
			cancel:Disconnect()
			prompt_gui.Enabled = false
2 Likes
#9

I see! You can avoid bugs like these in the future by using a state cleanup management library (I like Trove, Maid is another popular one). Using Trove, your Touched listener could look like

dropper.Touched:Connect(function()
	if touching_dropper then return end
	if dropper.active.Value then return end

	--Enter the "prompting state"
	local cleaner = Trove.new()

	local cleaned = false
	touching_dropper = true
	prompt_gui.Enabled = true --prompt confirm purchase ui
	prompt_gui.Frame.TextLabel.Text = "Would you like to buy dropper for " .. dropper.price.Value .. "$ ?"
	
	cleaner:Add(function()
		prompt_gui.Enabled = false
		cleaned = true
		
		--Wait a bit before allowing the "prompting state" to be entered again
		task.wait(0.5)
		touching_dropper = false
	end)
	
	--connect functions for confiorm and cancel buttons
	cleaner:Add(prompt_gui.Frame.Frame.confirm.Activated:Connect(function()
		--fire remote event to server
		dropper_prompt:FireServer(dropper)

		--Exit the "prompting state"
		cleaner:Clean()
	end))
	
	cleaner:Add(prompt_gui.Frame.Frame.cancel.Activated:Connect(function()
		--Exit the "prompting state"
		cleaner:Clean()
	end))
	
	--Exit the "prompting state" if the player walks away
	repeat 
		if math.abs(player.Character.PrimaryPart.Position.X - dropper.Position.X) > 5 or math.abs(player.Character.PrimaryPart.Position.Z - dropper.Position.Z) > 5 then
			cleaner:Clean()
		end
		wait() 
	until cleaned --cleaner doesn't totally manage this kind of loop, that's why we need this additional variable to manually manage it. Use promises instead to clean this up further.
end)

This has some advantages:

  1. It enforces consistent way of dealing with states
  2. It automates a lot of cleaning up (notice there are no Disconnect calls, Trove handles that so there’s no way you can forget it)
  3. Any manual cleanup code can be put right next to the setup code
  4. You could write a function that sets a property then returns a function that sets the property back to the old value, to automate property setup/cleanup

The “waiting for player to walk away” code could be cleaned up with Promises:

--Returns a promise that resolves once the player is out of a given range of some given PVInstance
function onPlayerLeftRange(rangeOf: PVInstance, range: number)
	return Promise.new(function(resolve, reject, onCancel)
		local cancelled = false
		onCancel(function()
			cancelled = true
		end)

		repeat
			local distance = ... you figure it out
			task.wait(0.25)
		until distance > range or cancelled
	
		if not cancelled then
			resolve()
		end
	end)
end

Then the loop can be replaced with

    cleaner:AddPromise(onPlayerLeftRange(dropper, 5)):andThen(function()
		cleaner:Clean()
	end)

and then you can get rid of the cleaned variable.

I can’t test this code right now, so let me know if it has bugs or something.

1 Like
#10

Thanks a lot for your response! I will definitely look into Trove and promises but what would be the advantage of using the code for the “waiting for player to walk away” you coded instead of the original?

1 Like
#11

I think it’s because if you dotn do that then your button/dropper will fire multiple times?

#12

It makes the code read more like “what happens” (declarative) and less like “how does it happen” (imperative), because it gives a name to an implementation of the idea of “doing something once the player has walked away”. That’s what abstraction is, and using good abstractions is how part of how we make large software projects understandable and maintainable. It also makes the control flow follow the reading flow better. Theoretically this also makes it reusable, but that’s not really the goal. The goal is to make the code more readable.

A common saying is that functions shouldn’t be more than 10 lines long (or 15 or w/e, the actual number doesn’t matter). If you have a longer function, it’s probably too detailed and should be split into smaller, well named functions. The goal is to let you read and understand the program as essentially a series of english-ish sentences instead of keeping track of variables being set and control flow happening in your head, because that stuff is hard. Don’t actually follow that saying, just take away the useful lesson it tries to teach.

You could do the same thing without promises, but Trove is made to work with promises among other things, and promises are a common, standard way of working with things that do things that may not be done right away and may fail or have to be cancelled instead of finishing normally (like waiting for the player to walk away).

If you take it even further you could have your entire gui script look like this:

local AreaHelpers = require(game.ReplicatedStorage.AreaHelpers)
local Promise = require(game.ReplicatedStorage.Promise)
local FSM = require(game.ReplicatedStorage.FSM)

local dropper = game.Workspace.Dropper

local states = {}

function init()
	FSM.new(states):SetState(states.idle)
end

function states.idle(fsm)
	wait(0.5)
	
	return {
		[dropper.Touched] = function() return states.promptPurchase end
	}
end

function states.promptPurchase(fsm)
	local promptGui = script.Parent
	promptGui.Enabled = true
	promptGui.TextLabel.Text = ("Would you like to buy dropper for $%d?"):format(dropper.price.Value)

	return {
        function()
			promptGui.Enabled = false
		end,
		[promptGui.ConfirmButton.Activated] = function() game.ReplicatedStorage.Purchase:FireServer(); return states.idle; end,
		[promptGui.CancelButton.Activated] = function() return states.idle end,
		[AreaHelpers.onPlayerLeftRange(game.Players.LocalPlayer, dropper, 8)] = function() return states.idle end,
		
	}
end

init()

As you can see it’s quite short and there’s no “manual cleanup” code in the gui script, just things that happen when each state is entered or exited, and some events that may return a state to transition to. It’s very easy to figure out what connections are active while each state is active, because they all get set up and cleaned up along with the state they’re defined in. Adding new states is easy and requires very little code.

A small (H)FSM class that works with the above. 
local Trove = require(game.ReplicatedStorage.Trove)
local Promise = require(game.ReplicatedStorage.Promise)
local Signal = require(game.ReplicatedStorage.GoodSignal)

local FSM = {}
local FSM_Public = {}
local FSM_MT = {__index = FSM_Public}
local FSM_Protected = {}

function FSM.new(states, parentFSM)
	local self = {}
	FSM_Protected[self] = {
		States = states,
		Cleaner = parentFSM and FSM_Protected[parentFSM].Cleaner:Extend() or Trove.new(),
		ParentFSM = parentFSM,
	}
	return setmetatable(self, FSM_MT)
end

function FSM_Public:SetState(stateFactory)
	local prot = FSM_Protected[self]
	local cleaner = prot.Cleaner
	local states = prot.States

	cleaner:Clean()

	if not stateFactory then return end

	for k, handler in pairs(stateFactory(self)) do
		if typeof(stateFactory) ~= "function" then error() end
		
		local function handleHandler(...)
			local nextState = handler(...)
			if nextState then
				self:SetState(nextState)
			end
		end
		
		if isEvent(k) then
			cleaner:Add(k:Connect(handleHandler))
		elseif Promise.is(k) then
			cleaner:AddPromise(k):andThen(handleHandler)
		elseif typeof(k) == "number" then
			cleaner:Add(handler)
		else
			error()
		end
	end
end

function isEvent(obj)
	if not obj then return false end
	if typeof(obj) == "RBXScriptSignal" then return true end
	if typeof(obj) == "table" and getmetatable(obj) == Signal then return true end
	return false
end

return FSM

Once you get used to this way of programming I think it makes things much easier to keep track of, but if you don’t see the advantage in your case then that’s also fine.

1 Like
#13

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.