How can I improve this magic spell code? It works but not well

Help and Feedback Code Review
#1

So, I’m making this magic spell that plays an animation, locks the player down and does damage in an area until they let go of their mouse. It works but there’s lots of issues with it I had to band-aid fix but there’s many issues with it being inconsistent, cancelling early, etc. I’m sure it could be simplified and made shorter too.

I also have the issue of everything lagging behind, where I’m guessing the client and server sees two different things, so if I run/jump and use it, the particles and effect will in the past.

local tool = script.Parent
local animation = tool.Animation
local rep = game:GetService("ReplicatedStorage")
local remoteEvent = tool.ReleaseEvent --remote event for releasing LMB
local triggerEvent = tool.TriggerEvent --remote event for pressing LMB
local players = game:GetService("Players")
local enabled = true

local cloneExplosion
local cloneFire
local hitbox 

local mouseHeld = false


tool.Activated:Connect(function()
	local character = tool.Parent
	local humanoid = character:FindFirstChildOfClass("Humanoid")
	local player = players:GetPlayerFromCharacter(character)
	
	if enabled == true then --intended to stop spam 
		enabled = false
		triggerEvent:FireClient(player,false) --fires event to hide backpack coreGUI so you cant unequip

		mouseHeld = true 

		character.HumanoidRootPart.Anchored = true --to stop the player from moving, doesnt always work

		local anim = humanoid.Animator:LoadAnimation(animation)
		anim:Play()
		anim.Looped = false

		anim:GetMarkerReachedSignal("Shine"):Connect(function() --anim marker to start particle, but particle part will lag behind the animation
			task.wait(0.01)
			tool.Shine:Play()
			local clone = rep.SparkleSymbol:Clone()
			clone.Parent = game.Workspace

			clone.CFrame = character["Right Arm"].RightGripAttachment.WorldCFrame

			task.wait(0.5)
			clone:Destroy()
		end)

		anim:GetMarkerReachedSignal("Explosion"):Connect(function() --marker to start ability 
			local pauseFunction = function()
				if mouseHeld == true then --making sure its held
					tool.FlameLoop.Looped = true
					anim:AdjustSpeed(0) --pausing animation until player lets go
				end
			end

			tool.Shine:Play()

			task.wait(0.01)

			cloneExplosion = rep.InitialExplosion:Clone() --particles
			cloneExplosion.Parent = game.Workspace
			cloneExplosion.Position = character["Right Arm"].RightGripAttachment.WorldCFrame.Position

			cloneFire = rep.FireCircle:Clone() --particles
			cloneFire.Parent = game.Workspace
			cloneFire.Position = character["Right Arm"].RightGripAttachment.WorldCFrame.Position + Vector3.new(0,2,0)

			tool.Explosion:Play()
			tool.Flame1:Play()
			tool.FlameLoop:Play()
			tool.Impact:Play()

			task.delay(0.5,pauseFunction) --delays pausing the animation for the while loop below so the while loop doesnt yield further code 

			while mouseHeld == true  do --makes damage hitboxes while you hold 
				task.wait(0.1)
				hitbox = rep.Hitbox:Clone()
				hitbox.Parent = game.Workspace
				hitbox.Position = character["Right Arm"].RightGripAttachment.WorldCFrame.Position + Vector3.new(0,2,0)
				hitbox.Size = Vector3.new(25,25,25)
				hitbox.Creator.Value = character.Name
				hitbox.Damage.Value = 0.25
				hitbox.Duration.Value = 0.25
			end

		end)

		remoteEvent.OnServerEvent:Connect(function() --triggers when mouse is no longer held, makes many issues and inconsistencies 
			print("server event")
			mouseHeld = false
			tool.FlameLoop.Looped = false
			task.wait(0.5)
			anim:AdjustSpeed(1)

			if cloneExplosion ~= nil then --gets rid of particles after ability is finished
				local particles = cloneExplosion:GetChildren()
				for _, part in ipairs(particles) do
					if part:IsA("ParticleEmitter") then
						part.Enabled = false
					end
				end
			end

			task.wait(1)

			if cloneFire ~= nil then --gets rid of particles after ability is finished
				local particles = cloneFire:GetDescendants()
				for _, part in ipairs(particles) do
					if part:IsA("ParticleEmitter") then
						part.Enabled = false
					end
				end
			end

			character.HumanoidRootPart.Anchored = false --releases player
			
			anim.Ended:Connect(function()
				enabled = true --resets enable state
				triggerEvent:FireClient(player,true) --reenables coreGUI
			end)

		end)
	end
end)

And the local script for the events.

local uis = game:GetService("UserInputService")
local tool = script.Parent
local remoteEvent = tool.ReleaseEvent
local triggerEvent = tool.TriggerEvent
local enabled = false
local starterGUI = game:GetService("StarterGui")

triggerEvent.OnClientEvent:Connect(function(state)
	starterGUI:SetCoreGuiEnabled(Enum.CoreGuiType.Backpack,state) --for making sure the player doesnt unequip or swap tools in the middle of a spell
end)

tool.Equipped:Connect(function()
	enabled = true
	
	if enabled == true then --this is an attempt to stop the fire event after the tool is equipped so it doesn't run on other tools, but this doesn't stop it
		uis.InputBegan:Connect(function(input)
			if input.UserInputType == Enum.UserInputType.MouseButton1 then
				print("pressed")
			end
		end)


		uis.InputEnded:Connect(function(input)
			if input.UserInputType == Enum.UserInputType.MouseButton1 then
				print("released")

				remoteEvent:FireServer() --this causes a lot of issues, its supposed to end the ability only when its equipped, you're currently using the ability and you release LMB, but its pretty faulty
			end
		end)
	end
end)

tool.Unequipped:Connect(function()
	enabled = false
end)

Sorry if it’s a lot of code, but this is my first attempt at making something like this and it’s kind of wack.

2 Likes
#2

This isn’t bad! Using RunService (on the client) would most likely be a better idea if you want to create a tool that does stuff while the mouse is clicked. With that, you can just check a variable (ex: isMouseClicked). You should also create the visual effects only on the client, and replicate them to the other clients when needed. Use the Debris service instead of task.wait to cleanup instances. That should help quite a bit. Good luck!

1 Like
#3

One thing I noticed is you’re not disconnecting your UIS input events in your local script. Which means every time you’re equipping the tool, a new connection is being connected and stacking on top of each. So now your callback functions are going to run more than once.

Are you getting multiple print statements for when you click or events firing more than once. If so, then what I just said is why.

1 Like
#4

Sorry this is so late but I only now just got a notification. I was curious about the visual effects on client and replicating it to the server as I need to change a lot of things in my game to do that because of how many issues I have with that. Is there an easy or fast way to go around creating a client effect on the server and syncing it up?

#5

Essentially, you’d be making a function for the clients. ex: VisualEffect(effect, position)
When a client wants to make an effect, play the effects for the player, and then fire a RemoteEvent to the server. From there, the server will fire the event for all of the other players, calling the VisualEffect function.

This may seem like more work instead of just doing everything on the server. However, think of this scenario: what if, say, 30 players were using the spells to create the effects. The server could definitely handle it, but it would be more performant to have the clients create the effects themselves locally, instead of polluting the server’s workspace.

I’d also recommend creating a separate module for handling effects. I noticed that you’re creating variables such as the “anim” variable inside of the Activated event - those should be outside of the event and already ready at the top of the script. It probably won’t make much of a difference in terms of performance, but its good practice!

Lastly for general programming advice, logically break down what you want the code to do in small steps. This kind of tool may seem complex, but in reality, it’s just a few fundamentals of Roblox game dev put together. Once you start breaking stuff down, it will all start to click.

By the way, no worries about replying late! I’ll respond as soon as I have the chance. Good luck, you can do this!

#6

The one thing I’m confused about is why, or how would I use RunService on the client without tons of debounces? If for example, I want to trigger an effect at an animation marker, and I do that in RunService, I would need to make sure it only does it once and not a hundred times. Or if I want to play an animation, I have to make sure it only does it once, but with RunService, it will do anything multiple times if ever single function and command isn’t debounced. I can’t really find a way to work around it or apply it.

#7

Have a variable, maybe “canFire”, and only let the player use the tool if it’s true. In a RunService loop you can just say “if canFire then …”