So I made this water ball script, that homes in on your mouse with really good accuracy, but I did this by spamming the client and sending information from the client to the server, and vice-verca.
In the video you can see the crazy accuracy, but I’m worried about the preformance cost in a real game.
How can I make this better without the spam? The heavy performance cost in a real game, worries me.
CODE
-- local script code
WaterBendingRE.WaterThrash.SendMouse.OnClientEvent:Connect(function()
WaterBendingRE.WaterThrash.SendMouse:FireServer(mouse.Hit.p)
end)
-- server script code
coroutine.wrap(function()
while waterBubble:IsDescendantOf(workspace) do
WaterBendingRE.WaterThrash.SendMouse:FireClient(player)
WaterBendingRE.WaterThrash.SendMouse.OnServerEvent:Wait()
WaterBendingRE.WaterThrash.SendMouse.OnServerEvent:Connect(function(player, newMousePos)
waterBV.Velocity = CFrame.new(waterBubble.Position, newMousePos).LookVector * 60
end)
end
end)()
The performance issue you might be seeing seems to be from a memory leak. You are connecting an event in a loop and never disconnecting it. Remember you only need to connect events once.
As for the rest of the code, a couple lines are unclear. Firing the client and waiting then discarding a server event is also weird.
The code seems a bit redundant, though. You don’t need to fire the client when you’re already expecting the client to be firing to you. You also don’t need a loop when you already connected the event.
My solution would look something like this:
local event_con
event_con = WaterBendingRE.WaterThrash.SendMouse.OnServerEvent:Connect(function(player, newMousePos)
-- code here I won't copy because mobile is pain
end)
waterBubble.AncestryChanged:Connect(function(_, parent)
if not parent then -- might not need to check if you only reparent on destroy
event_con:Disconnect()
end
end)
You wouldn’t need a coroutine either, because this doesn’t yield. Now as to the issue with the loop…
Events are made so that they can fire and be used multiple times, so a loop to keep connecting them is at best redundant and at worst a memory leak. When you see a pattern like that, you should try to re-evaluate what you’re doing.
Busy loops especially (aka “waiting” for something to happen) are generally bad practice. You can almost always get away with using an event to avoid it.
Also, I assume your FireClient was to inform the client to send the server the mouse position. If I’m wrong, ignore this part. If not, then you should consider carrying out the check for when the client should fire on the client. Whether it’s by checking mouse state or something, it should probably be handled there instead.
Lastly, onto how my proposed solution works. You can store the reference to the connection you make from the RemoteEvent to later be disposed of. You only create one connection, because it can be used indefinitely, and then attach an AncestryChanged handler. This checks for when your instance’s parent goes nil. When this is the case, you’ve probably destroyed it. So it disconnects the remote’s event. The ancestry event itself isn’t disconnected manually because Destroy already handles that when you call it.
-- local script code (brief and only goes over the relevant things)
WaterBendingRE.WaterThrash.Interfere.OnClientEvent:Connect(function(interferedState)
if interferedState then
interference = true -- defined at the top of the code
while interference do
WaterBendingRE.WaterThrash.SendMouse:FireServer(mouse.Hit.p)
RunService.Heartbeat:Wait() -- should I use renderstepped?
end
else
interference = false
end
end)
-- server code (brief as well)
local sendMouse = nil
WaterBendingRE.WaterThrash.Interfere:FireClient(player, true)
sendMouse = WaterBendingRE.WaterThrash.SendMouse.OnServerEvent:Connect(function(_, newMousePos)
waterBV.Velocity = CFrame.new(waterBubble.Position, newMousePos).LookVector * 60
end)
waterBubble.AncestryChanged:Connect(function(_, parent)
if not parent then
sendMouse:Disconnect()
WaterBendingRE.WaterThrash.Interfere:FireClient(player, false)
debounce = false
for i = 1, 6, -1 do
waterThrashTool.Name = i
wait(1)
end
waterThrashTool.Name = "Water Thrash"
end
end)
Thanks to your revisions, the code is a LOT less laggy when multiple bubbles are in the game.
Yeah that looks pretty good. The only thing I’d change might be adding a check inside the OnServerEvent to make sure the player passed into it as the first argument is the same as the outer scope just in case someone attempts to mess with it.
Something simple like if player ~= event_player then return end inside the event handler after renaming the first (_) param to event_player.
Why not use classic replication with BasePart:SetNetworkOwner()
This way you’ll only be firing one event to set the network owner and then you can control the ball on the client.
Just make sure you set the network owner to nil or destroy the ball afterwards.
If you’re concerned about security, doing sanity checks before doing damage is also helpful, be sure to lock out your events to make sure the person try to fire a ball should be able too.
PS: This isn’t really a solution to your problem but a different way to approach this.