Client sided screenshake randomly not applying

[TheBigBad_MOP (Mop)]

I have a script that searches inside a region for players, and applies a client sided camera shake if said players are in the region. Through many trials, the client sided shake seems to apply completely at random, and I can not find any way to consistently replicate it breaking or working.

Server Script:

local EarthFolder = script.Meshes.Earth  --Meshes are stored in ServerScriptStorage.Script
local EarthPillar = EarthFolder.EarthPillar:Clone()
local EarthParticle = EarthPillar.ParticleMaker.ParticleEmitter
			
EarthPillar:SetPrimaryPartCFrame(CFrame.new(Pos) * CFrame.new(0,-7.5,0))
EarthParticle.Parent.CFrame = EarthParticle.Parent.CFrame * CFrame.new(0,10,0)
EarthParticle.Enabled = true
EarthPillar.Parent = game.Workspace:WaitForChild(player.Name.." Effects")
local pos1 = EarthPillar.PrimaryPart.Position - (EarthPillar.PrimaryPart.Size / 2)
local pos2 = EarthPillar.PrimaryPart.Position + (EarthPillar.PrimaryPart.Size / 2)
local Hitbox = Region3.new(pos1, pos2)
			
for i = 1, 20 do 
	task.wait(0.05)
	local partsInHibox = workspace:FindPartsInRegion3(Hitbox, nil, 20)
	print(partsInHibox)
	for _, v in pairs(partsInHibox) do 
		if v.Name == "Head" or v.Name == "Left Arm" or v.Name == "Left Leg" or v.Name == "Right Arm" or v.Name == "Right Leg" or v.Name == "Torso" then 
			local PlayerTarget = Players:GetPlayerFromCharacter(v.Parent)
			print(PlayerTarget)
			EarthAttack:FireClient(PlayerTarget, "ScreenShakeTrue") --EarthAttack is a remoteEvent in ReplicatedStorage
		else
			EarthAttack:FireAllClients("ScreenShakeFalse") --Probably bad practice but was the quickest solution I was able to find, Might be the issue?
		end		
	end
end

Client Script:

local Players = game:GetService("Players")
local LocalPlayer = Players.LocalPlayer

LocalPlayer.CharacterAdded:Wait()
local Char = LocalPlayer.Character
local Humanoid = Char:WaitForChild("Humanoid")
local Active = false

remoteEarth.OnClientEvent:Connect(function(info) --remoteEarth is same remoteEvent as EarthAttack
	if info == "ScreenShakeTrue" then
		Active = true 
		while Active == true do 
			local x = math.random(-100,100)/90
			local y = math.random(-100,100)/90
			local z = math.random(-100,100)/90
			Humanoid.CameraOffset = Vector3.new(x,y,z) 
			task.wait()
		end
	elseif info == "ScreenShakeFalse" then 
		Active = false
		Humanoid.CameraOffset = Vector3.new(0,0,0)
	end
end)

Video of bug: https://youtu.be/F1W1KT8wgTA

Any help would be appreciated! A similar thing like this has happened before where seemingly functional code just randomly decides to not work and I was never able to get to the bottom of it Oh and the screenshake persisting after the animation is completed is “intentional”! I have yet to code in a fire for “ScreenShakeFalse” after the animation is completed.

[Scottifly]

Ahead of any if checks print the variable(s) it’s checking.
For example:

    print("info = ", info, "   ScreenShakeTrue = ", ScreenShakeTrue)  --why not just have a 'ScreenShake' variable & set it true or false?
    if info == "ScreenShakeTrue" then
    --code

This will show you if there’s something strange about the variables, like one being nil or not what you expect.

[TheBigBad_MOP (Mop)]

I’ll try this thank you, and the “Active” variable is my ScreenShake variable. “ScreenShakeTrue” is just what im passing through the remoteEvent so that the client can know exactly which function action im trying to excecute.

[TheBigBad_MOP (Mop)]

I found an issue. When its checking for player parts, its also reading the parts inside the model and baseplate and running the else statement alongside the correct if statement. Thus its firing both “ScreenShakeTrue” and “ScreenShakeFalse” in near unison. A solution I would like is a better way of detecting if a player is inside the region, but I don’t know of any other way. Do you know of either any different ways to detect a player, or a way to exclude those specific parts from its detection (though this could lead to the same bug if it the spell is casted near other parts)?

[Scottifly]

GetPartsInPart might be better than Region3, and it says you can use OverlapParams to include or exclude parts so it doesn’t overwhelm your HitBox search.

[Fusionet]

I would also used @Scottifly’s recommendation of GetPartsInPart(). However, I would just focus on the issue on hand.

So just to clarify, by “random” do you mean it sometimes never goes on at all or it’s not consistent.

There are a few things I’d change.

1. First off, there is no reason to do a for loop in the server. This could be the issue as it may be throttling. To fix this, move the for loop from the server to the client.
2. Instead of individually checking the part name, simply check if it’s a child of the character.
3. Add a table of “tagged” players so you don’t FireClient() more person than once.

Improved server script:

--Server Script
local Duration = .5 -- How long screen shake lasts
local tagged = {}

for _, v in pairs(partsInHibox) do 
	if v.Parent:FindFirstChild("Humanoid") and not table.find(tagged, v.Parent) then 
		table.insert(tagged, v.Parent) -- Making sure it does not FireClient to the same person more than once.
		local PlayerTarget = Players:GetPlayerFromCharacter(v.Parent)
		print(PlayerTarget)
		EarthAttack:FireClient(PlayerTarget, "ScreenShakeTrue", Duration) --EarthAttack is a remoteEvent in ReplicatedStorage
	end
end

Now for the client script, there’s not much to do.

1. Remove the active variable since we have switched to a duration based system.
2. Add a while loop that runs for the Duration that we passed from the Server.

--Client Script
remoteEarth.OnClientEvent:Connect(function(info, duration) --remoteEarth is same remoteEvent as EarthAttack
	if info == "ScreenShakeTrue" then
		local start = os.clock() -- start time stamp
		local timePassed = os.clock() - start
		while timePassed <= duration do
			local x = math.random(-100,100)/90
			local y = math.random(-100,100)/90
			local z = math.random(-100,100)/90
			Humanoid.CameraOffset = Vector3.new(x,y,z)
			task.wait()
		end
		Humanoid.CameraOffset = Vector3.new(0,0,0) -- Revert to normal offset once it's done
	end
end)

And there it is! There may be some silly mistakes I left since I never tested this, but let me know if this solves your problem. This method reduces the amount of :FireClient’s drastically.

NOTE: If you didn’t want to use duration, it would be better to do this instead of firing to every player in the server (this is not to be used with the code I provided).

for i, player in pairs(tagged) do -- Disable screen shake for all people who were tagged
	EarthAttack:FireClient(player, "ScreenShakeFalse")
end

[TheBigBad_MOP (Mop)]

Thank you so much for the detailed response! It’s unfortunately not achieving what I’m looking for, as this seems to be effectively a fancy .Touched() event. The reason I had the for loop in server side was to check if a player is inside the zone at any particular time. Should they be inside the zone, a screen shake would apply until the zone disappears, or until they walk out of said zone. This is instead checking if a player is ever in the zone, and applies a set screen shake that might be out of sync with the animation.

I did want to ask, however, about os.clock() as this is the first time I’ve ever even seen it. Is it starter a timer without the need of a for loop? And why are you able to set/setting the same value to different variables?

Also, by “random” I did mean inconsistent. The script recognizes that a player is in the bounds, it just isn’t applying the screen shake properly. Though I think I found a solution that I will post here should it work! Thanks again for the help and information

[TheBigBad_MOP (Mop)]

The issue was, there were other parts in partsInHitbox that were getting passed through the argument, and firing the client with "ScreenShakeTrue" and "ScreenShakeFalse" near simultaneously. To remedy this, I tried finding any parts that weren’t a “player” part (what the if statement is checking for), and remove them from the partsInHitbox array. This is what I originally came up with:

(Server Script, the Client Script did not end up needing to be changed, though its most probably better to find a different method than firing every single client at once when there is nothing in the region lol)

for i = 1, 20 do 
	task.wait(0.05)
	local playerEffects = game.Workspace:FindFirstChild(player.Name.." Effects")
	local partsInHibox = workspace:FindPartsInRegion3(Hitbox, game.Workspace.Baseplate, 20) --Hitbox is predefined as Region3.new(pos1, pos2) [pos1 and pos2 are the bottom left and bottom right corner of a part]
	for _, v in pairs(partsInHibox) do 
					
			if v.Name == "Head" or v.Name == "Left Arm" or v.Name == "Left Leg" or v.Name == "Right Arm" or v.Name == "Right Leg" or v.Name == "Torso" then 
				local PlayerTarget = Players:GetPlayerFromCharacter(v.Parent)
				print(PlayerTarget)
				EarthAttack:FireClient(PlayerTarget, "ScreenShakeTrue") --remoteEvent in ReplicatedStorage
			elseif v.Name ~= "Head" or v.Name ~= "Left Arm" or v.Name ~= "Left Leg" or v.Name ~= "Right Arm" or v.Name ~= "Right Leg" or v.Name ~= "Torso" then
				local function findValue(array, itemName) --this is literally taken from the roblox documentation about changing array values, I half understand what its saying tbh
					for currentIndex = 1, #array do
						if array[currentIndex] == itemName then
							return currentIndex
						end
					end
				end
			local valueFound = findValue(partsInHibox, v.Name)
			table.remove(partsInHibox, valueFound)
		end
						
		print(partsInHibox)
		if partsInHibox == {} then
			EarthAttack:FireAllClients("ScreenShakeFalse") --same remoteEvent
		end			
	end
end

As some may have noticed however, this “discarding unwanted parts in the array method” had a
few flaws in its code. Firstly, not only was the elseif redundant, as else would function the exact same in this situation. But I also didn’t even properly make the elseif redundant! The else if argument was basically a double negative where it was returning, “This index value’s name is this part’s name, but its not this other part’s name so the argument is true.” A simple fix if this is your only issue is to change the or operators to and so that the statement is only true if the value your searching for is not equal to any of the values the argument is checking for. The second flaw was that when I was removing an unwanted part from the array, the i,v pair (index, value pair that the part is stored under in the array) right below it would move up, changing its index to the previously removed index. This was creating gaps in my array, thus leaving unwanted parts still in partsInHitbox. And lastly, when checking if there were no parts in the region, thus meaning it was empty of players. I was instead created a new empty table, NOT checking if the current table was empty. So, with the help of @RoBoPoJu, he was able to create a much more efficient block that read:

for i = 1, 20 do 
	task.wait(0.05)
	local playerEffects = game.Workspace:FindFirstChild(player.Name.." Effects")
	local partsInHitbox = workspace:FindPartsInRegion3(Hitbox, game.Workspace.Baseplate, 20) --Hitbox is predefined as Region3.new(pos1, pos2) [pos1 and pos2 are the bottom left and bottom right corner of a part]
	for arrayIndex = #partsInHitbox, 1, -1 do
		local v = partsInHitbox[arrayIndex]
					
		if v.Name == "Head" or v.Name == "Left Arm" or v.Name == "Left Leg" or v.Name == "Right Arm" or v.Name == "Right Leg" or v.Name == "Torso" then 
			local PlayerTarget = Players:GetPlayerFromCharacter(v.Parent)
			print(PlayerTarget)
			EarthAttack:FireClient(PlayerTarget, "ScreenShakeTrue") --remoteEvent in ReplicatedStorage
		else		
			table.remove(partsInHibox, arrayIndex)
		end
						
		print(partsInHitbox)
		if #partsInHitbox == 0 then
			EarthAttack:FireAllClients("ScreenShakeFalse") --same remoteEvent
		end			
	end
end

This is redefining certain variables in the loop, and changing the bounds of the loop so that it starts from the lowest/last (numerically highest) index. This allows the loop to remove unwanted i,v pairs without it creating gaps, as there is no i,v pair under it. It’s also checking if the number of indexes in partsInHitbox is 0, thus meaning the table is empty. This above code fixed the bug I was experiencing entirely, and should be taken as the finite solution to this problem as of now. Thank you to everyone who helped solve this for me, it was a huge help and a huge learning experience!

[Fusionet]

Nice job on figuring it out. I still think this is bad practice, and if you can explain to me what exactly you are trying to accomplish, I can give you a more efficient method. Right now I believe you are firing way more remote events than necessary which causes server lag and high ping.

You asked was os.clock() is. To put it simply, it’s very useful for bench marking as you can figure out how much time has passed since it will return the CPU run time every time you call it. A simple example:

local start = os.clock()
wait(1)
print(os.clock() - start) -- would print approximately 1 since 1 second has passed after calling os.clock() again

Now I’d just like to clear up some confusion. Your loop should last for approximately ~1 second (20 * .05). Does this mean you want the zone to last for 1 second?

You can do something like this to account for how much time has passed and base the camera shake duration on that.

You also said that if they leave the zone, my method won’t work which is true. However, we can solve this by using a while loop and comparing the tagged table vs characters currently in the zone. I wrote this on the spot and never tested it so apologies for any bugs/mistakes.+

Updated code:

--Server Script
local start = os.clock()
local duration = 1
local waitTime = .25 -- time in between zone check (make sure it's a factor of the duration, ex: 1/.25 = 4)
local tagged = {}


while os.clock() - start <= duration do
	local partsInHibox = workspace:FindPartsInRegion3(Hitbox, nil, 20)
	local currentCharacters = {} -- players that are inside the zone on each check
	
	
	-- This section if code will find characters inside the zone and send the camera shake
	for _, v in pairs(partsInHibox) do 
		if v.Parent:FindFirstChild("Humanoid") and Players:GetPlayerFromCharacter(v.Parent) then -- player is inside
			table.insert(currentCharacters, v.Parent) -- This is separate from tagged table, it will keep track of people still inside the zone
			
			if not table.find(tagged, v.Parent)  then -- Make sure tagged players only :FireClient ONCE
				table.insert(tagged, v.Parent) -- Making sure it does not FireClient to the same person more than once.
				local PlayerTarget = Players:GetPlayerFromCharacter(v.Parent)
				-- new addition
				local timePassed = os.clock() - start
				local adjustedDuration = duration - timePassed -- If the player entered the zone .25 seconds later, 
				-- the camera shake will only last .75 seconds (thats how much is left over)
				EarthAttack:FireClient(PlayerTarget, "ScreenShakeTrue", adjustedDuration) --Make sure to send the adjusted duration
			end
		end
	end
	
	-- This section will check if any tagged players have left the zone
	
	
	for i, character in pairs(tagged) do
		local player = Players:GetPlayerFromCharacter(character)
		
		if table.find(currentCharacters, character) then -- Player is STILL inside the zone
			-- Do Nothing! Let the camera shake go.
		else -- Player has LEFT the zone
			print(character.Name.." has left the zone.")
			
			local index = table.find(tagged, character)
			table.remove(tagged, index) -- Removing character from tagged since they are no longer in the zone
			EarthAttack:FireClient(player, "ScreenShakeFalse")
			-- If they re-enter the zone they will be re tagged
		end
	end
	
	task.wait(waitTime)
end

[TheBigBad_MOP (Mop)]

While this is definitely way more efficient and lag friendly, I don’t enjoy taking other peoples’ work and just plugging it into my script. I was using a for loop at first so that I could run an if statement saying “on the the last loop do blah”. I see now that I can just check if os.clock() - start == duration and it would achieve the same thing. I will still stick with my original idea, as it feels more rewarding to have come up with it on my own (even if I got help tweaking a specific nested loop).

That being said, the reason I was using :FireAllClients() was because I did not know of a way to access the same PlayerTarget that I had defined in an earlier nested function. I do like the way you are putting the PlayerTarget into a table though and have transferred that aspect of your method into my code which has worked wonders! The spell is now completely functional and lag friendly

The full for loop now reads:

local TaggedPlayers = {}
local AllTaggedPlayers = {}
for i = 1, 20 do 
	task.wait(0.05)
	local playerEffects = game.Workspace:FindFirstChild(player.Name.." Effects")
	local partsInHibox = workspace:FindPartsInRegion3(Hitbox, game.Workspace.Baseplate, 20)
	for arrayIndex = #partsInHibox, 1, -1 do 
		local v = partsInHibox[arrayIndex]

		if v.Name == "Head" or v.Name == "Left Arm" or v.Name == "Left Leg" or v.Name == "Right Arm" or v.Name == "Right Leg" or v.Name == "Torso" then 
			for i=1, 1 do 
				local PlayerTarget = Players:GetPlayerFromCharacter(v.Parent)
				table.insert(TaggedPlayers, PlayerTarget)
				table.insert(AllTaggedPlayers, PlayerTarget)
				EarthAttack:FireClient(PlayerTarget, "ScreenShakeTrue")
				break
			end	
		else
			table.remove(partsInHibox, arrayIndex)
		end

		if #partsInHibox == 0 then
			for pNumber, p in pairs(TaggedPlayers) do 
				EarthAttack:FireClient(p, "ScreenShakeFalse")
				table.remove(TaggedPlayers, pNumber)
			end
		end

		if i == 20 then 
			if v.Name == "Head" or v.Name == "Left Arm" or v.Name == "Left Leg" or v.Name == "Right Arm" or v.Name == "Right Leg" or v.Name == "Torso" then 
				local PlayerTarget = v.Parent

				for pNumber, p in pairs(AllTaggedPlayers) do 
					EarthAttack:FireClient(p, "ScreenShakeFalse")
					table.clear(AllTaggedPlayers)
					table.clear(TaggedPlayers)
				end
				task.spawn(PlayerEffects.KnockUpEffect,PlayerTarget, 10, 30, 0.5) --a modular function
				break
			end
		end
	end
end

Im sure there is an even more efficient way to script this, however I found most of the answers on my own and feel comfortable sticking with this for now. Thank you so much for the help and teaching me new methods and practices!

[Fusionet]

No problem, glad I could help. Good luck on your game!