Issue with custom toolbar module

Help and Feedback Scripting Support
,
#1

so i have an issue with my module script, i have a part with a click detector in it an a script whenever the button is pressed the module script is fired, however for the second if statement bellow breaks the for, loop without the break statement, the script doesnt work as intended. with the break vallue the script doesnt contuie to the next s value in the for i ,s loop. is there somethign i can replace the break statement with, so that it stops if its true and skips to the next value?

elseif s:IsA("TextLabel") and s.Taken.Value == true and amt < 10 and debounce == false then
					debounce = true
					amt = amt +1
					debounce = false
					break

local rs = game:GetService("ReplicatedStorage")

local module = {}
local amt = 0
local debounce = false

module.skillgiver = function(player,SkillName)
	--// Save the old amount at the time of function call, so we can track changes
	local OldAmount = amt

	for _, v in player.Backpack:GetChildren() do
		--// Here we check if the value has changed. If changed, we break the main loop
		if amt ~= OldAmount then
			break
		end

		if v.Name == SkillName then
			for _,s in player.PlayerGui.keybindFrame.Frame:GetChildren() do
				if s:IsA("TextLabel") and s.Taken.Value == false and debounce == false then
					local toolui = rs.TextLabel
					local UiClone = toolui:Clone()
					debounce = true

					UiClone.Parent = player.PlayerGui.HotbarFrame.Frame
					UiClone.Position = s.Position
					s.Taken.Value = true

					amt = amt + 1

					debounce = false

					break
				elseif s:IsA("TextLabel") and s.Taken.Value == true and amt < 10 and debounce == false then
					debounce = true
					amt = amt +1
					debounce = false
					break
				elseif s:IsA("TextLabel") and s.Taken.Value == true and amt >= 10 and debounce == false then
					debounce = true
					local toolui = rs.TextLabel
					local UiClone = toolui:Clone()


					UiClone.Parent = player.PlayerGui.Inventory.ScrollingFrame
					UiClone.TextLabel.Visible = false
					debounce = false
				end
			end
			print(OldAmount)
		else
			print("else")
		end
	end
end

return module
#2

Maybe use the continue keyword, though I’m not sure how it will help compared to removing the breaks completely

#3

i removed the second break and not the first, because i intended for the loop to break once it creates the gui and sets the value as taken, however still,
image
the amt values are all over the place, not sure why

#4

I don’t know what the :31, :51 and :48 lines are, the code you sent only has 1 print that would print numbers so I don’t know what those mean

#5

my bad i added a print statmentbefore the break to print amt

#6

break stops a loop, continue skips one iteration of the loop.
If you want to only skip one iteration, use continue.

However break and continue don’t matter here because the other code will not run since it is inside of an elseif block. Just remove the breaks entirely.

Here’s your code:

module.skillgiver = function(player, SkillName)--stick to a single naming convention
     local OldAmount = amt
     for _, v in ipairs(player.Backpack:GetChildren()) do
          if amt ~= OldAmount then break end
          --if v.Name ~= SkillName then break end--unless you actually plan on doing things in the else block
         if v.Name == SkillName then
             for _, s in ipairs(player.PlayerGui.keybindFrame.Frame:GetChildren()) do
                if not s:IsA("TextLabel") or debounce then continue end--just check this once, not in every condition.
                debounce = true --this is always set to true, so just do this once outside of the if.

                if not s.Taken.Value then
                    local UiClone = rs.TextLabel:Clone()
                    UiClone.Position = s.Position
                    UiClone.Parent = player.PlayerGui.HotbarFrame.Frame--assign parent last when modifying properties for performance
           
                    s.Taken.Value = true
                    amt += 1
                elseif amt < 10 then --don't check s.Taken.Value again since we know it's true here
                    amt += 1
                    --no breaks here, other code won't run
                else --don't check anything since we know s' value is greater or equal from the previous check
                    local UiClone = rs.TextLabel:Clone()
                    UiClone.TextLabel.Visible = false
                    UiClone.Parent = player.PlayerGui.Inventory.ScrollingFrame--consider making global variables for the player gui since you're accessing it a lot.
                end

                print(OldAmount)
                debounce = false --always set to false, just do this outside of the if statement. (what's the point of debounce anyway?)
             end
         else
             print("else")
         end
     end
end

If you are planning to add code after the if statement, then use continue, otherwise you just keep the if-elseif-else block.

Hope this helps!

1 Like
#7

Hey man, the code you wrote out was extremely helpful, it made complete sense, yet the whenevr i run the module it crease 10x of the frame, everything works but now the amt is multiplying by 10?
image
i pressed it 6 times, thats why there is a 10 and 50, each time i press it once it adds ten? like the code is written logically, there doesnt seem to be any errors.

here is the script i use to run the module

local click = script.Parent.ClickDetector
local module = require(game:GetService("ReplicatedStorage").ModuleScript)
local debounce = false

click.MouseClick:Connect(function(plr)
	if debounce == false then
		debounce = true
		module.skillgiver(plr,"GURA")
		task.wait(1)
		debounce = false
	end
end)
#8

Okay, can you provide more context. What is inside of the keybindFrame.Frame. Why specifically are you looping through the children? What does the Taken value do? What’s the purpose of amt? Finally, is there other code that modifies the global amt other than the skillgiver? What exactly is the code trying to achieve?

edit: the loop prints 10 times, does that mean one can have multiple tools in their backpack with the given skill name?
edit: while copying the code to test it myself I realised the first loop is useless since we don’t use v other than to check its name, so I have another question, why are we looping through backpack anyway? also what’s the use of debounce? Can it be removed?

edit: Okay, what’s keybindframe? I’m assuming hotbar is your hotbar, inventory the inventory, but what’s keybindframe? All ui elements are parented directly to playerGui (inventory, hotbarframe, keybindframe), why?

#9

Sorry for the late response was a bit busy.
I’ve rewritten the code a bit to make it more coherent. Essentially, the for i,v loop is looping through a skills folder to find a skill that matches the skill name. If it does, then it loops through the keybindframe, which is where the hotbar is, where the player can see the keybinds: here is what I’m talking about

image
image810×393 43.2 KB

The bottom bar is the keybind frame, the top portion is the inventory.
Whenever the inventory is active, the keybind frames appear. This is done to show the players where they can put the skills; however, while the inventory is disabled, the hotbar only shows what skills they have in their hotbar.
it loops through those 10 squares, and if it’s taken, it adds 1 to the taken amount.
The amount value is there to let the system know whether the player has any space on their hotbar. If all the values are taken, then the item is put into the inventory.
the debounce is honesty useless i only added it to try and fix the amt issue.
Ideally, I don’t want more than one of the same skills. The looping through the backpack in my new script should ensure that if the player already has the skill, it doesn’t give it to them again.

local rs = game:GetService("ReplicatedStorage")

local module = {}
local amt = 0
local debounce = false

module.skillgiver = function(player, SkillName)
	local OldAmount = amt
	local skillsFolder = rs:WaitForChild("SkillsFolder")
	for i,v in skillsFolder:GetChildren() do
		print('works')
		if v.Name ~= SkillName then return end
		if v.Name == SkillName then
			
			local SkillClone = v:Clone()
			SkillClone.Parent = player.Backpack
			for _, s in ipairs(player.PlayerGui.keybindFrame.Frame:GetChildren()) do
				if not s:IsA("TextLabel") or debounce then continue end
				debounce = true 

			    if not s.Taken.Value then
					local UiClone = rs.TextLabel:Clone()
					UiClone.Position = s.Position
				    UiClone.Parent = player.PlayerGui.HotbarFrame.Frame--assign parent last when modifying properties for performance

					s.Taken.Value = true
					amt += 1
				elseif amt < 10 then --don't check s.Taken.Value again since we know it's true here
					amt += 1
							--no breaks here, other code won't run
				else --don't check anything since we know s' value is greater or equal from the previous check
					local UiClone = rs.TextLabel:Clone()
				    UiClone.TextLabel.Visible = false
					UiClone.Parent = player.PlayerGui.Inventory.ScrollingFrame--consider making global variables for the player gui since you're accessing it a lot.
				end

				print(OldAmount)
				debounce = false --always set to false, just do this outside of the if statement. (what's the point of debounce anyway?)
			end
		end
	end
end
	
return module

Finnaly, here my inventory system file if you want to check it out indepth.
Inventory system.rbxl (73.9 KB)

#10

so I’m assuming you’re looping through the skills folder to check if a skill exists with that name, but you don’t need to loop through the other children, right?

if so you can delete the outer loop and just do this:

local skill = skillsFolder:FindFirstChild(skillName)
if not skill then return end--if no skill with such name exists, cancel the function
--do your processing with the skill here

also I realised that ‘amt’ is a server global, but it is used as if it is local to a single player, this is a bit concerning because other players could be modifying this while other algos for other players are running, so I suggest you handle UI related things on the client. Just use a remote to tell the client what you’ve added.

I think I now know what the code wants to do, so your code wants to add a tool to the player’s backpack, and if a tool with the same name is in the hotbar, increment the value? Otherwise store it in the inventory scrollingframe?

Okay so I sorted things, I moved some objects to other locations since I think they would benefit from being there instead. An example is the skillsfolder, in replicated storage any exploiter can grab any skill and give it to themselves. The screen gui in serverstorage is questionable at least, I moved it to replicated storage, the server shouldn’t worry about vfx, sfx or UI, the client has to do those things, the server does math. So I moved it to replicated storage so the client as easy access to what it might need. I also added a remote for adding UI on the client. Also the amt variable is not required.

here’s my file:
Inventory system.rbxl (74.6 KB)

Click the part, and you’ll see it adds frames until there are 10 frames, then it adds them to the inventory. You might keep track of how many frames there are to make it faster, idk. This could be done better, but I just kinda used what was already there instead of doing an overhaul, I also don’t know what you want with all of the other stuff so I tried to touch only the things I felt like needed to change.

Hope this helps, and I hope that this is what you want.

#11

This works wonderfully, Thanks alot for helping me :slight_smile:

1 Like