Scripting HelpersARCHIVE
A little piece of Roblox history. Preserved for everyone, one question at a time.Read about the archive →
COMMUNITY QUESTION

How come this script is not working?

I asked about this script when it had an error. Well, the error is gone, but it is not working for some reason when I played it Solo. I hit the Try button and it didn't function the script to do it:

dist = 10

function onClicked()
    local players = game.Players:GetChildren()
    for i = 1, #players do
    if (players[i].Character.Torso.Position - script.Parent.Position).magnitude < dist then
    local name = players[i].Name
    local clothing = players[i]:GetChildren()
    for i = 1, #clothing do
if clothing[i]:IsA("Clothing") then
    local f = game.Workspace:FindFirstChild(name)
    local s = f:FindFirstChild("Shirt")
    local p = f:FindFirstChild("Pants")
    local s1 = script.Parent.Parent.Parent.Clothes.Shirt:clone()
    local p1 = script.Parent.Parent.Parent.Clothes.Pants:clone()
    s.Parent = script.Parent.Parent.PlayerStorage
    p.Parent = script.Parent.Parent.PlayerStorage
    wait()
    s1.Parent = f
    p1.Parent = f
    wait(5)
    s1:remove()
    p1:remove()
    s.Parent = f
    p.Parent = f
end
end
    end
    end
end

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


1 answer

Basic Style Improvements

  1. Tab your code correctly

  2. Use generic for loops (You're reusing i which is a little scary)

function onClicked()
	local players = game.Players:GetChildren()
	for _, player in pairs(players) do
		if (player.Character.Torso.Position - script.Parent.Position).magnitude < dist then
			local name = player.Name
			local clothing = player:GetChildren()
			for _, cloth in pairs(clothing) do
				if cloth:IsA("Clothing") then
					local f = game.Workspace:FindFirstChild(name)
					local s = f:FindFirstChild("Shirt")
					local p = f:FindFirstChild("Pants")
					local s1 = script.Parent.Parent.Parent.Clothes.Shirt:clone()
					local p1 = script.Parent.Parent.Parent.Clothes.Pants:clone()
					s.Parent = script.Parent.Parent.PlayerStorage
					p.Parent = script.Parent.Parent.PlayerStorage
					wait()
					s1.Parent = f
					p1.Parent = f
					wait(5)
					s1:remove()
					p1:remove()
					s.Parent = f
					p.Parent = f
				end
			end
		end
	end
end

Address Issues

Now, some obvious problems: clothing doesn't make sense. A player won't have any useful children -- its clothing are in its Character.

You define f to be the character, but in a completely roundabout way. You also might as well use workspace[name] since you don't do error checking on f.

player.Character could be nil, too, so we have to check for that.


Also, you don't ever use the variable cloth, which is usually a big warning flag. (You instead attempt to indirectly refer to it using s and p)


s and p could be nil, but you do stuff to them anyway.

You use a lot of script.Parents, but never script on its own, so why not make that a variable?

It seems like instead you should move all the cloth objects to PlayerStorage, move the new objects in (not in the loop) and then move them back:

local button = script.Parent;

function onClicked()
	local players = game.Players:GetChildren()
	for _, player in pairs(players) do
		local character = player.Character
		if character and (character.Torso.Position - button.Position).magnitude < dist then
			local clothing = character:GetChildren()
			for _, cloth in pairs(clothing) do
				if cloth:IsA("Clothing") then
					cloth.Parent = button.Parent.PlayerStorage
				end
			end
			local shirt = button.Parent.Parent.Clothes.Shirt:clone()
			local pants = button.Parent.Parent.Clothes.Pants:clone()
			shirt.Parent = character
			pants.Parent = character
			wait(5)
			shirt:Destroy()
			pants:Destroy()
			for _, cloth in pairs(button.Parent.PlayerStorage:GetChildren()) do
				if cloth:IsA("Clothing") then
					cloth.Parent = character
				end
			end
		end
	end
end

button.ClickDetector.MouseClick:connect(onClicked) 

Actually though we probably don't want to store things in PlayerStorage -- other players might use that at the same time. We can just make a temporary model:

storage = Instance.new("Model")
....
		cloth.Parent = storage
....

Use Functions

We are reusing code: we use two four loops to moves Clothing, and also we also move Clothing in two other places. Why don't we make a moveClothes function?

If we're doing something 4 times, the answer is probably to use a function.

function moveClothes(from, to)
	for _, child in pairs(from:GetChildren()) do
		if child:IsA("Clothing") then
			child.Parent = to
		end
	end
end

function onClicked()
	local players = game.Players:GetChildren()
	for _, player in pairs(players) do
		local character = player.Character
		if character and (character.Torso.Position - button.Position).magnitude < dist then
			local storage = Instance.new("Model")
			moveClothes(character, storage);
			moveClothes(button.Parent.Parent.Clothes:Clone(), character);
			wait(5)
			moveClothes(character, nil);
			moveClothes(storage, character);
		end
	end
end

Now it reads very simply:

  • Move the Character's clothes into storage
  • Move a copy of the model clothes into the Character
  • Wait a few seconds
  • Get rid of the character's clothes (which are a copy of the model)
  • Move the clothes back from storage onto the character

1 comment

0

Thank you soo much bro. But I want to apologize since this must have been a lot of work for you to explain and type all those for me.

This discussion is preserved as it was. New answers, comments, and votes are closed.