Continuously damaging block

Help and Feedback Code Review
1

Hi, I’ve created a block that does continuous damage while standing OR walking on top of it, based on the Touch event (instead of the other option, with “GetPartBoundsInBox” I believe). It works fairly okay, especially when comparing it to what I’ve found online.

Full script 
local part = script.Parent
local PlayersDamaging = {}

part.Touched:Connect(function(hit)
	local hum = hit.Parent:FindFirstChild("Humanoid")
	if hum then
		if not table.find(PlayersDamaging, hit.Parent) then
			table.insert(PlayersDamaging, hit.Parent)
			local jumpold = hum.JumpPower
			local emitter = game.ServerStorage.LavaParticles:Clone()
			emitter.Parent = hit.Parent.Head
			coroutine.resume(coroutine.create(function()
				while table.find(PlayersDamaging, hit.Parent) do
					local touches = part:GetTouchingParts()
					hum:TakeDamage(0.15)
					hum.JumpPower = 0
					hit.Parent.Health.Disabled = true
					if not table.find(touches, hit.Parent) and hum.MoveDirection.Magnitude == 0 then
						game["Run Service"].Heartbeat:Wait()
						if not table.find(touches, hit.Parent) and hum.MoveDirection.Magnitude ~= 0 then
							hum.JumpPower = jumpold
							hit.Parent.Health.Disabled = false
							emitter:Destroy()
							table.remove(PlayersDamaging, table.find(PlayersDamaging, hit.Parent))
						end
					end
					game["Run Service"].Heartbeat:Wait()
				end
			end))
		end
	end
end)

Players do get continuously damaged, by both walking and standing on the block. Below is the ‘difficult’ part.

Difficult part 
if not table.find(touches, hit.Parent) and hum.MoveDirection.Magnitude == 0 then
						game["Run Service"].Heartbeat:Wait()
						if not table.find(touches, hit.Parent) and hum.MoveDirection.Magnitude ~= 0 then

First off, it means that players who stand still have an extra heartbeat before getting damaged (which is fine I guess, they don’t regenerate health anyway). That wait is however necessary, because I’m first testing someone standing still, and then walking.
The first if includes the table.find argument which is true when standing still, both on and off the lava.

The second if includes the table.find argument which can only be true when walking off the lava. The thing is, this is quite inconsistent. You need to wiggle a bit when you’re off the block, before losing the effect. If I just remove the first condition, the effect does not longer work when walking on top of the block.

Splitting the two apart (when walking and when standing still) didn’t work for me, although there may be a possibility I have overlooked. The loop sometimes doesn’t close and the emitter and jumppower get messed up. Here it is below.

Not-working solution 
local part = script.Parent
local PlayersDamaging = {}
local deb = true

part.Touched:Connect(function(hit)
	local hum = hit.Parent:FindFirstChild("Humanoid")
	if hum then
		if not table.find(PlayersDamaging, hit.Parent) then
			table.insert(PlayersDamaging, hit.Parent)
			local jumpold = hum.JumpPower
			local emitter = game.ServerStorage.LavaParticles:Clone()
			emitter.Parent = hit.Parent.Head
			coroutine.resume(coroutine.create(function()
				while table.find(PlayersDamaging, hit.Parent) do
					local touches = part:GetTouchingParts()
					hum:TakeDamage(0.1)
					hum.JumpPower = 0
					hit.Parent.Health.Disabled = true
					if not table.find(touches, hit.Parent) and hum.MoveDirection.Magnitude ~= 0 then
						hum.JumpPower = jumpold
						hit.Parent.Health.Disabled = false
						emitter:Destroy()
						table.remove(PlayersDamaging, table.find(PlayersDamaging, hit.Parent))
					end
					print("Check1")
					game["Run Service"].Heartbeat:Wait()
				end
			end))
		end
	end
end)

part.Touched:Connect(function(hit)
	local hum = hit.Parent:FindFirstChild("Humanoid")
	if hum then
		if deb == true then
			deb = false
			jumpold = hum.JumpPower
			emitter = game.ServerStorage.LavaParticles:Clone()
			emitter.Parent = hit.Parent.Head
		end
		hum:TakeDamage(3)
		hum.JumpPower = 0
		hit.Parent.Health.Disabled = true
		print("Check2")
		game["Run Service"].Heartbeat:Wait()
		print(jumpold)
		hum.JumpPower = jumpold
		hit.Parent.Health.Disabled = false
		emitter:Destroy()
		deb = true
	end
end)

Any ideas on how to get this consistent? An idea using GetPartBoundsInBox or even Region3 is welcome as well if that works of course.

2

So before I delve into my suggested solution, I have a few notes on your implementation.

Firstly, I think that you’d be better suited using a dictionary for ‘PlayersDamaging’ as opposed to an array. table.find is a linear search algorithm, and table.remove of course has to shift back the indices that are higher than the index it removes. Depending on your server playercap, the array may never be populated enough for the consideration to matter, but I think these things are still worth keeping in mind.

Another pitfall I noticed, was that you’re searching the ‘touches’ table for ‘hit.Parent’; this is likely not your intended functionality… At that point in the code, hit.Parent is guaranteed to be the Model of the character that touched the lava. See the issue? Since the ‘touches’ table is constructed from :GetTouchingParts() (which is a function that returns an array of touching BaseParts), and you’re searching that array for the character (a Model), the expression will always evaluate to false.

Rather than using a loop w/ :GetTouchingParts(), to assess when the Character steps off of the lava, I’d instead consider leveraging the BasePart.TouchEnded event alongside BasePart.Touched, in order to maintain a table of the characters that are currently touching the lava. Then, from there, you could have a loop that distributes damage based on that dictionary every so often. If you wanted to get real fancy with it, you could even code it so that the loop only runs while the table is populated.

How I’d wager this accounting would work, is you’d have a dictionary where each key is a character that’s colliding with the lava, and each value is the number of parts within that character that are currently in contact. When new collisions are registered from .Touched, you’d add to this value; and when collisions end from .TouchEnded, you’d subtract from it. Then you’d remove the key entirely once the count reaches zero.

2 Likes
3

Thank you for your answer.
I’m not sure how to employ a dictionary at all, and since you say that it may never matter (I wager that at most 4 maybe 5 entries will ever be present at once), I’ll leave that.
Your second part is of course very right, and I feel stupid that I never considered that. Here’s a ‘fixed’ version of that.

for i,v in pairs(touches) do
	if v.Parent ~= nil then
		table.insert(touches, v.Parent)			
        table.remove(touches, table.find(touches, v))
	end
end

What confuses me a lot about this however, is that the function already worked somewhat, and did not improve at all with the fix above.

Then to your actual solution:
I may misunderstand you, but that’s what I (everyone) tried at first, but TouchEnded fires even when standing still on top of the block -Touch works when there’s a physics interaction. So this would not work, it would just remove the value from the array/dictionary when standing still and thus doing no damage and reenabling jumping and regenerating.
If you meant it in some smart way I did not get out of it (I’m not that talanted at Luau :D), can you please show an example on it?
Again, thank you for your response!

4

In Lua, tables are a hybrid of arrays and dictionaries. Utilizing dictionary lookups is as simple as setting a key to a value; I’d definitely recommend reading up on tables a bit more, having that understanding is super useful for just about everything in scripting.

PlayersDamaging[Character] = 3
print(PlayersDamaging[Character]) --> 3

You’re definitely correct in that TouchEnded is finicky with how frequently it fires. I didn’t consider that may not be ideal for your usecase, since the constant updating would definitely be unusual from the player’s perspective (and potentially breaking, depending on how you handle regeneration).

How would you feel about periodically doing a raycast downwards to see if the player is standing on something that’s tagged as lava? That might be a good simple solution. Though of course, the idea of having an endless loop raycasting away (potentially very quickly, and for every player in the game), doesn’t exactly strike me as super efficient…

What you could do, is once the character touches lava, you could tag it with a burn status. That way, you could write an implementation such that the raycast is only triggered on inflicted characters, with the status upheld for each raycast that finds that the character is indeed still ontop of lava.

(Here’s an article on raycasting incase that term is new Intro to Raycasting)

1 Like
5

Thank you, I’ll definitely read up on tables. And also on raycasting. In theory, your idea works splendidly, so I’ll just need to implement it. I’d still be open to ‘fix’ to my script as well, and I may just leave it as-is right now at least, since it costs more time than it’s worth haha.

1 Like
6

Oh lord when 2 long message makers talk to each other:

anyways you can just follow these steps

#1: See if player touched part.
#2: Do while humanoid.Health > 0 do.
#3: Add wait or task.wait.
#4: Add the damage function (Humanoid:TakeDamage(--amount))
#5: Test it!

7

As I stated in my first post, this is not what I’m looking for. This does not damage players while standing still (doesn’t detect them in any way), and is literally from the free models.

8

wow i didnt know it was well then buddy just use region3

9

Don’t use region3 as it is deprecated. Instead, you could possibly raycast to check if there is a player on top of the block.

10

I’ve decided on the following solution:

Script in part 
local part = script.Parent
local damage = 0.1
local PlayersDamaging = {}

part.Touched:Connect(function(hit)
	local hum = hit.Parent:FindFirstChild("Humanoid")
	if hum then
		if hit.Parent ~= nil then
			local char = hit.Parent
			if not table.find(PlayersDamaging, char) then
				table.insert(PlayersDamaging, char)
				local jumpold = hum.JumpPower
				local emitter = game.ServerStorage.LavaParticles:Clone()
				emitter.Parent = char.Head
				while table.find(PlayersDamaging, char) do
					local objects = workspace:GetPartBoundsInBox(part.CFrame, Vector3.new(part.Size.X, part.Size.Y + 12, part.Size.Z))
					for i,v in pairs(objects) do
						if v.Parent ~= nil and not table.find(objects, v.Parent) then
							table.insert(objects, v.Parent)
							table.remove(objects, table.find(objects, v))
						end
					end
					hum:TakeDamage(damage)
					hum.JumpPower = 0
					char.Health.Disabled = true
					if not table.find(objects, char) then
						hum.JumpPower = jumpold
						char.Health.Disabled = false
						emitter:Destroy()
						table.remove(PlayersDamaging, table.find(PlayersDamaging, char))
						break
					end
					game["Run Service"].Heartbeat:Wait()
				end
			end
		end
	end
end)

This uses a Touch event to trigger the damaging effect once, and then checks continuously with workspace:GetPartBoundsInBox whether the player is still on the lava. It also disables jumping, and to prevent spam jumping and actually never touching the part to start with, you have to include a jump cooldown.

Jump cooldown script 
local char = script.Parent
local hum = char.Humanoid
hum:SetStateEnabled(Enum.HumanoidStateType.Jumping, true)

hum.StateChanged:Connect(function(state)
	if state == Enum.HumanoidStateType.Jumping then
		hum:SetStateEnabled(Enum.HumanoidStateType.Jumping, false)
	end
	if state == Enum.HumanoidStateType.Landed or state == Enum.HumanoidStateType.Climbing then
		task.wait(0.22)
		hum:SetStateEnabled(Enum.HumanoidStateType.Jumping, true)
	end
end)

The jump cooldown script goes into StarterCharacterScripts as a local script.

Also, it is possible to stop touching the lava part, but still getting damaged as an arm is hovering above the lava. So, this solution is not perfect -also doesn’t include dictionaries or raycasting (maybe raycasting is included in the GetPartBoundsInBox function?)-, but I guess it’s an improvement to my first post -which just used Touch events really.

11

This is some good improvement. I like how you removed the unnecessary additional coroutine this time around. When events are fired, the functions bound to them of course already run in a new thread; that was a fault with your original code that I neglected to mention.

I’m of course still going to advocate for using the PlayersDamaging table as a dictionary, as opposed to performing all of these searches, and reordering of indices. But I’ve already covered that ground, so I’ll move on.

Rather than caching the humanoid’s old jump power, and then resetting it, consider instead taking advantage of Humanoid:SetStateEnabled() in order to disable jumping; just as you’re doing in that ‘Jump cooldown script’:

Humanoid:SetStateEnabled(Enum.HumanoidStateType.Jumping, false) -- Disabled
Humanoid:SetStateEnabled(Enum.HumanoidStateType.Jumping, true) -- Enabled

I’m not necessarily opposed to the idea of checking to make sure that hit.Parent isn’t nil, but given that this is a Touched event, anything that fires it is guaranteed to be physically simulated, which in this case implies that hit will always be a descendant workspace. The only hypothetical circumstance I can think of where this wouldn’t be accurate, would be if the Touched event was deferred long enough for the triggering BasePart to be parented to nil; though I’d wager that would be exceedingly unlikely. If you want to keep the check around, I’d still revaluate its placement; put it before you search for the character’s Humanoid, as you already access hit.Parent when you do so.

Next, since that Vector3 you’re creating for each :GetPartBoundsInBox() call remains constant, you might consider caching it as a variable, rather than making a new one each time. Really, caching in general is a good idea. Even if it’s just to save some indexing time, having direct references to what you need will significantly reduce the length of some of these lines. The emitter for example; it might be nice to have a reference to that Instance, so that you don’t have to go fishing for it each time that you want to make a new copy.

Another note that kind of goes with that last point, is how you’re indexing RunService directly, with each iteration. Typically with services (ex; RunService, ServerStorage, etc…), scripters like to see them retrieved using game:GetService(), and defined as a variable near the top of the script. I’m fairly lenient when it comes to these kinds of things, but it’s what tends to be expected. Besides, if you cache it as a variable, it’ll save you some indexing time, so you might as well do it, if not for readability, then for the micro-optimization.

local RunService = game:GetService("RunService")

Incase you’re wondering, :GetService() tends to be the preferred way of retrieving services, as it’s resilient against the renaming of services, and it will actually create services if they do not yet exist. This also runs a bit into preservation; Roblox likely expects developers to be using :GetService(), and so while directly indexing RunService works as of now, you never know for certain what changes Roblox might make in the future. I doubt it’ll ever come to that, but it’s best to be on the safer side of updates.

Continuing on neatness, it may be worth organizing some of this logic into separate functions. A little compartmentalization goes a long way; even if you only separate out the section that handles damage, from the section that detects the character, I think it would do a world of good in terms of promoting readability (and ease of expandability for that matter).

1 Like
12

Thank you for the extra tips! I’ll implement most of them.
One thing that isn’t possible is to disable to HumanoidStateType.Jumping. That can only be done by the network owner, which is always the client of the player.

1 Like