Roblox Cloning Issue with ServerScript

[welcomedude3 (soupkong)]

Hello! This is my first post.
I’m a beginner scripter, and was working on testing out different scripts to see how they would work.
I made a script that would open a small description box with information on the item clicked, then it allows you to purchase the item. I made a ServerScript inside of ServerScriptService that would respawn the item every 3 minutes, if it wasn’t currently inside of workspace. I’m not sure if this is relevant, but the script basically checks if there is certain text inside of a TextLabel before proceeding. Here’s my workspace:

Order of the scripts: The Purchase Script uses the Remote Event to fire the Open Menu local script, which makes the gui box visible. When the purchased button is clicked, the Script Buttons local script fire to the server, which takes away and adds values.

PurchaseScript:

function pressed(plr)
	script.Parent.RemoteEvent:FireClient(plr)
	script.Parent.ClientGame.OnServerEvent:connect(function(plr)
		if plr:WaitForChild("Currency").Coins.Value >= 25 and plr:WaitForChild("PlayerGui").ShopGui.ShopTextFrame.TextValue.Text == "Apple" then
			plr.Currency.Coins.Value -= 25
				plr.Ingredients.Apple.Value += 5
				script.Parent:Destroy()
		end
	end)
end

script.Parent.ClickDetector.MouseClick:connect(pressed)

OpenMenu Localscript:

local plr = game.Players.LocalPlayer

function receiveeventapple(play)
	script.Parent.ShopTextFrame.Visible = true
	script.Parent.ShopTextFrame.Description.Text = "A shipment of 5 apples. They probably found these on the ground, but they're being sold here, so obviously they have a purpose."
	script.Parent.ShopTextFrame.Image.Image = "http://www.roblox.com/asset/?id=6436213195"
	script.Parent.ShopTextFrame.PurchaseButton.Text = "Purchase for 25 Coins"
	script.Parent.ShopTextFrame.TextValue.Text = "Apple"
end

workspace.AppleShipment.RemoteEvent.OnClientEvent:connect(receiveeventapple)

ScriptButtons Localscript:

local plr = game.Players.LocalPlayer

function gameing()
	if script.Parent.ShopTextFrame.TextValue.Text == "Apple" then
		workspace.AppleShipment.ClientGame:FireServer(plr)
		script.Parent.ShopTextFrame.Visible = false
end
end

script.Parent.ShopTextFrame.PurchaseButton.Activated:connect(gameing)

RespawnScript:

bop = true

game.Players.PlayerAdded:connect(function(plr) --Store cargo respawn
	repeat
		wait(5)
		print("game")
	if workspace:FindFirstChild("AppleShipment") == nil then
	local appleship = game.ServerStorage.Cargo.AppleShipment:Clone()
	appleship.Parent = workspace
	appleship.Position = Vector3.new(65, 11.2, 157.5)
		end
until bop == false
end)

I know the code may be extremely sloppy, however it worked perfectly until it was cloned. If the solution involves rewriting the entirety of the code then I’m willing to do that. However, even if I don’t have to rewrite the entirety of the code, any criticism for my code would be great. Again, this is my first post, but I hope I included enough information to help find my solution. Thank you!

Edit: The respawn script was shortened to 5 seconds rather than 3 minutes for testing purposes.
Also, the gui is in the StarterGui service, and the Apple part is in workspace.

[EssenceExplorer]

Hi!

For a beginner script, you are very good! I wish you a successful learning journey!

It’s probably better if you move this topic to code review.
I am not completely sure what is preventing your cloned items to appear in workspace as there are multiple potential reasons, but you may find a solution here.

---

Because I needed something relatively similar done for my project, I decided to quickly assemble a uncopylocked place you can find at the end of this post. End result is similar, but with a slightly different concept. Please take into account that this is one way of how one could approach creating a market system, so none of it has to be done this way. Another quick warning: the market I’m posting below doesn’t not focus on server security. I left it out, because it’s a separate part of market development and has a lot to do with data saving and remote function spamming, which is different story.

---

Time to explain the process step by step.

1) Detect player clicks

Each item only contains one short script that sends a remote event to the client that clicked. Checking whether player can afford the item or not right away is not advised. Why? Because it becomes inconvenient when we have lots of items. If each script compares player’s wealth with the price individually, it’s very hard to change prices manually in every single item. DRY (don’t repeat yourself) principle is golden here. How to simplify the process? Read futher.

Let’s put the identical scripts into every single item. When the item is clicked, send remote signal to the client, containing name of the item (script.Parent.Name). Still don’t like having so many scripts everywhere? Use CollectionService and so called tagging.

2) Open GUI

Nothing new here. All the same as you did it previously. Almost. There is, however, something new. In ReplicatedStorage, we put a module script, containing a list of all the items, prices, descriptions, and everything that comes with it. I haven’t included images in my place, but you can add them later any time. How does it work?

-- Item database structure.

["Spongebob's pineapple"] = {
		50, -- price in coins
		"Who lives in a PINEAPPLE under the sea!?" -- description
	}

-- ["Sponglebob's pineapple"][1] --> 50

Why in ReplicatedStorage and not elsewhere (e.g in ServerStorage). We could do that, but I chose ReplicatedStorage because everyone receives the list that way. Clients can read and edit this list, but no changes they do are visible in other replicas (server’s version, other clients’ versions).

require() this module and read the values. When item script we talked about before sends us the name of chosen item, we use that name and search for it in our dictionary-type list in module script.

3) Purchase confirmation

When GUI becomes visible, button .Activated event connects. Now the player can click it, confirm their decision and send remote singal to one, main server script in ServerScriptService. Data contains player’s name (by default) and desired item.

4) The purchase

Server accepts that remote event and validates player’s request. It checks whether the item exists, whether it is available, and if the player has enough coins to buy it (it’s worth warning again that the place I provided doesn’t cover server security, only basic item giving).

5) Actual selling - ABOUT CLONING HERE

Server calls two functions. One copies a tool (copied object can be anything, not necessary a tool), and places it in player’s backpack. The second is a coroutine. It clones the item, destroys the original, and defines a parent to cloned object after a period of time. This means that after the purchase, item disappears from workspace for a period of time, until new one is copied to the same spot.

Purchase handler

There is at least one alternative to coroutines. They are fine solution, as long as items are not purchased by a lot of players every second.

-- IMPORTANT: This script does not cover server security!

local ServerStorage = game:GetService("ServerStorage")
local RepStorage = game:GetService("ReplicatedStorage")

local ItemDatabase = require(RepStorage.ItemDatabase)

local RESPAWN_TIME = 5
local ITEMS_PATH = workspace.Fruits
local TOOLS_PATH = ServerStorage.Fruits

local buyItemEvent = game:GetService("ReplicatedStorage").Events.BuyItem

local function restockItem(marketItem)
	local clone = marketItem:Clone()
	marketItem:Destroy()
	wait(RESPAWN_TIME)
	clone.Parent = ITEMS_PATH
end

local function priceLookup(player, item)
	local price = ItemDatabase[item][1]
	local marketItem = ITEMS_PATH:FindFirstChild(item)
	if (player:FindFirstChild("leaderstats") and marketItem) then
		if (player.leaderstats.Coins.Value >= price) then
			coroutine.wrap(restockItem)(marketItem)
			player.leaderstats.Coins.Value -= price
			return true
		end
	end
	return false
end

buyItemEvent.OnServerEvent:Connect(function(player, item)
	local pass = priceLookup(player, item)
	if (pass) then
		local clone = TOOLS_PATH[item]:Clone()
		clone.Parent = player.Backpack
	end
end)

-- When player joins, give them leaderstats folder and some money.
game:GetService("Players").PlayerAdded:Connect(function(player)
	local leaderstats = Instance.new("Folder")
	leaderstats.Name = "leaderstats"
	leaderstats.Parent = player

	local coins = Instance.new("IntValue")
	coins.Name = "Coins"
	coins.Value = 100
	coins.Parent = leaderstats
end)

Finally, here is the uncopyloced place, check the source code:

https://www.roblox.com/games/6451472932/Simple-market

EDIT

@Hexcede wrote a very comprehensive post addressing the respawning part and code tunning. As far as the repeat-loop goes, I tried to avoid it. It isn’t necessary, because you can call a respawn function when it’s needed, instead of endless checking. I also wanted to simplify the script, so you don’t have to look for right vector position manually, but rather take the properties of previous object and respawn it by cloning it back after a period of time you can customly set. Instead of adding plr.Ingredients.Apple.Value, player receives that item in form of a tool as a confirmation that the script is working, for I didn’t know what value’s purpose is.

[Hexcede]

I believe your issue is in your OpenMenu script. You use workspace.AppleShipment.RemoteEvent, but, workspace.AppleShipment won’t exist yet since you haven’t created it yet. Do you have an error in your console that shows you that? You should use workspace:WaitForChild("AppleShipment"). Usually you should also use :WaitForChild when you can, because sometimes things won’t load or be available to scripts in time. Usually its a good idea to put what you can into variables, for example, you might have a Folder in the workspace you use to store your AppleShipments, so you might use local shipmentsFolder = workspace:WaitForChild("AppleShipments") to wait for that folder in case it takes too long to load on the client.

Additionally, since your apple shipment gets deleted when the player purchases it, your OpenMenu script won’t connect to any new AppleShipment’s remote events. You can fix this using what I mentioned above, and, instead of putting your AppleShipments into the workspace, you can put a folder in the workspace named AppleShipments, and you can parent apple shipments to there. Then you can use .ChildAdded, and, you won’t need to do any special checking to see if what you are getting is an apple shipment, you can just assume that it is since you wouldn’t want to use your AppleShipments folder for anything else, and on top of that, you won’t need to use :WaitForChild() since you know the instance is a child now, you can just use the child that was added.

I put a little version of your OpenMenu script at the bottom I did, which, combines all of the stuff above and all of the stuff below too.

---

As for code suggestions and stuff, there’s probably a lot of little things you can do to your code that will help you organize it and help it run a little faster (although that probably doesn’t matter too much to you in this case since its not too important)

In your RespawnScript you are connecting to PlayerAdded and running a loop. Every player that joins will start a new loop and then your loop will never exit so you will get more and more loops over time whenever a player joins. That’s probably not a big concern for you, but, its a good thing to mention.

Additionally, rather than using a repeat until loop it might be better if you use a while loop instead, because, if your bop variable starts out as false, the contents of the loop will still run once every time a player is added since the check happens at the end of the loop instead of at the beginning.

You can move your loop outside of the PlayerAdded event and run it in a new thread, that way it won’t block code below it. You can use coroutine.wrap (or coroutine.create and coroutine.resume) to create a thread. Here is how I would personally do it:

coroutine.wrap(function()
	while bop do
		-- Stuff
	end
end)()

You should swap the position of these two lines, since, if you parent it first, it has a little bit of time to be effected by physics. You should always parent after you set properties, that way things will work as you expect them to.

appleship.Parent = workspace
appleship.Position = Vector3.new(65, 11.2, 157.5)

Also, I would suggest putting the services you use into variables (excluding the workspace of course), like this:

local Players = game:GetService("Players")
local ServerStorage = game:GetService("ServerStorage")

Players.PlayerAdded:Connect(function(plr)
	
end)

This will give you less to type and will help you read your code a little better. You can copy and paste your service names too, by double clicking on them to select them and hitting control+c. I use that all of the time when I script, and it saves me a lot of time typing while also looking a lot nicer, since you only have to glance at the word right after your indents.

And, with your indents, you can select a few lines and hit tab to add tabs before those lines automatically or shift+tab to remove tabs before those lines. That’s very helpful for keeping your code neat (and a further tip, you can use shift+arrow keys to change your selection, so, if you want to select the current line and the line above it you can use shift and then the up arrow).

---

Here is how I (personally) might write your OpenMenu script. Some of it comes down to my personal preferences, but, hopefully it might help you a little:

local Players = game:GetService("Players")

local player = Players.LocalPlayer
local appleShipments = workspace:WaitForChild("AppleShipments")

-- Info
local shopInfo = {
	Name = "Apple",
	Description = "A shipment of 5 apples. They probably found these on the ground, but they're being sold here, so obviously they have a purpose.",
	Cost = 25
	Image = "http://www.roblox.com/asset/?id=6436213195",
}

-- Gui stuff
local shopGui = script.Parent
local shopFrame = shopGui:WaitForChild("ShopTextFrame")

local shopDescription = shopFrame:WaitForChild("Description")
local shopImage = shopFrame:WaitForChild("Image")
local purchaseButton = shopFrame:WaitForChild("PurchaseButton")
local textValue = shopFrame:WaitForChild("TextValue")

local function receiveEvent()
	shopDescription.Text = shopInfo.Description
	textValue.Text = shopInfo.Name
	purchaseButton.Text = "Purchase for "..shopInfo.Cost.." Coins"
	shopImage.Image = shopInfo.Image

	shopFrame.Visible = true -- We can set the stuff then make it visible which will be a little more consistent
end

appleShipments.ChildAdded:Connect(function(appleShipment)
	appleShipment:WaitForChild("RemoteEvent").OnClientEvent:Connect(receiveEvent)
end)

Additionally, since :WaitForChild doesn’t interact with properties, you can do something like :WaitForChild("Name") and you won’t have issues, so your TextValue bit (which I am assuming is the item name) can be named Name if you wanted to name it that.

[welcomedude3 (soupkong)]

Sorry I didn’t respond earlier, I just saw these notifications now. Thank you guys for replying! Making the folder with the apple shipments and using .ChildAdded, as @Hexcede said, seems like it would a lot. I’ll definitely look into the @EssenceExplorer method, though. I’ll be sure to check out the market system you used, and maybe use some parts of it in the future. I’ll tell you how the outcome comes along!

[welcomedude3 (soupkong)]

Marked as solution. I used your method and added a couple things and it worked like a charm! Thank you!