Changing node parent produces Area2D/3D signal duplicates
Zylann opened this issue · 32 comments
Godot 2.1.4, Godot 3.0 beta 1
Windows 10 64 bits
I found that if you teleport-away and reparent a node which is inside an Area2D, you will get an area_exit, but ALSO extras notifications body_enter and body_exit, two in my case, even though the object is not anymore in the area.
This gave me headaches for several hours in my game until I narrow it down to this, because this behavior causes my teleport system to trigger everything multiple times, and I can't find a way to avoid this so far without atrocious hacks...
It does this wether I teleport from fixed_process or not, and emptying layer/collision masks before teleport has no effect.
Here is a demo project reproducing it:
2.1.4: TeleportArea2D.zip
3.0b1: TeleportArea2D_Godot3b1.zip
Repro:
- Launch main scene, notice you get one
body_enterbecause the body overlaps with the area, wich is expected - Press any key, this teleports the body and reparents it (in code, the node is removed from tree, translated, and then added to the tree)
- Notice you get an expected
body_exit, but also two unexpected pairs ofbody_enterandbody_exit
I've done some probing in Area2D, and I got the following:
-----_body_inout
Body enter
-----_body_exit_tree
Body exit
-----_body_enter_tree
Body enter
-----_body_inout
Body exit
-----_body_inout
Body enter
-----_body_inout
Body exit
Looking at the code, first thing strange thing I notice is:
Area2D stays connected to the body when it leaves the tree, I see no signal disconnection here and connections aren't one-shots:
Lines 134 to 148 in 13c2ff9
Then, when the body is re-added to the tree, body_enter is emitted but there is no check wether it still overlaps with the area, but in my opinion it's wrong to not have disconnected the signals:
Lines 116 to 132 in 13c2ff9
The 3 next emissions in _body_inout are still a mystery to me, but I have to go to sleep for now
This bug/behavior has existed for a long time (03.2016). My only solution was to delete the body and re-create it. You can also try to remove all connections.
I've tried several times to explain that the node2d function
set_pos, set_rot and set_scale has problems with RigidBody2D, KinematicBody2D and Area2D.
Partly works and partly there is a strange behavior. By scale I mean flipping the body around the x/y axis.
In your case, the engine thinks that the body is still in Area2D although it was moved with set_pos to a different location.
example: try teleporting a falling rigidbody2d in Godot3
(set_scale = flipping body on x/y axis)
@puppetmaster- deleting the body (so the entirety of any character entering a teleport) is going to be quite annoying for me. I would like to find a solution to fix this in engine at least for teleporting because it's a PITA to deal with for something that simple... otherwise I'm not doing anything fancy with the kinematic body (just using move(), no rotation, no scale).
The problem on node side seems obvious, but if the issue also extends in the physics engine I'll need some help...
@Noshyaar note this behaviour also exists in 3.0
Edit:
I found that connections made by the area to the body in C++ seem to be completely redundant, because the physics engine already notifies the node with the monitoring callback.
But even with these removed, I still get an extra enter/exit from the Physics2DServer itself.
I tried to yield() one frame at various points of the teleport code to account for the delay the physics engine might have, but it had no effect.
I'm clueless now...
@puppetmaster- if you're interested, I lost some hair of trial and error in order to encapsulate a workaround for my use case, specifically for Godot 2.1.4 (it likely won't work in 3.0), maybe it can be useful to others:
# See https://github.com/godotengine/godot/issues/14578
class BodyTeleporter:
signal done
func exec(body, new_parent, new_local_pos):
# Memorize and clear collision and layer masks,
# so the physics server will stop detecting overlaps
var cached_collision_mask = body.get_collision_mask()
var cached_layer_mask = body.get_layer_mask()
body.set_collision_mask(0)
body.set_layer_mask(0)
# Move the node first, so we get a body_exit coming FROM the Physics Server
# (it's important because I believe Area2D itself sends body_exit when the body exits the tree)
var gpos = new_parent.get_global_transform().xform(new_local_pos)
body.set_global_pos(gpos)
# Wait one physics step for the PhysicsServer to flush the things
yield(body.get_tree(), "fixed_frame")
# Wait again because in my game that wasn't enough apparently...
yield(body.get_tree(), "fixed_frame")
# Now the body is out of the area, has no contact and can't collide anything:
# time to reparent it
body.get_parent().remove_child(body)
new_parent.add_child(body)
# Set position locally,
# because the global one set earlier isn't valid anymore after reparenting
body.set_pos(new_local_pos)
# Restore collision masks so the body works again
body.set_collision_mask(cached_collision_mask)
body.set_layer_mask(cached_layer_mask)
# Emit a signal so the caller can yield until teleport is done
emit_signal("done")
Usage:
var tp = BodyTeleporter.new()
tp.exec(character, dst_room, target_pos)
yield(tp, "done")This is how you teleport bodies under a new parent, folks.
Fixing this would be really nice, although I understand the frame delay of the physics engine, I believe none of this workaround should be required^^"
@Zylann OMG!! 😮 Great workaround! Thanks for your contribution. Let me know if you need some hair, I've got enough left and I'm sure I can borrow some.
Is this still reproducible in the current master branch?
It reproduces in 3.1 alpha 1.
On scene start:
Body enter
Which is expected.
Then the reparent-teleport happens, and all these signals get fired, while I expected only one Body exit:
Body exit
Body enter
Body exit
Body enter
Body exit
I'm pretty certain this is the same issue as #20107. When re-inserting the node the position will be still be the original position and retrigger the enter/exit signals.
Still reproducible in 3.2:
https://www.reddit.com/r/godot/comments/g9cosc/area2d_body_entered_firing_when_it_should_not/
The enter signal is emitted immediately after re-adding the red_level scene back to the tree. When I check "body.position" in the "entered" function, it clearly shows the body is far away, so the signal should not be emitted.
I've simplified my minimal project some more, replaced the green level with a button. This makes the bug even more apparent.
After the player collided the first time and the red_level scene got removed and the button enabled,
on clicking the button the fade Animation that should only be triggered if the player is colliding with the Area2D starts immediately playing:
Minimal project:
Area2D_enter_on_remove_bug2.zip
I think this is still reproducible.
I have 2 Node2D with a player hanging from one of them. When player touches Area2D (a door) it is supposed to be removed from one Node2D and add it as a child to the other Node2D and also changing its position, but project crashes because this throws infinite body_entered signals
I have included a minimal project in a zip file where you can reproduce this bug. (I am aware that this is easily avoided by just changing the position of the node and not doing the reparenting stuff, but in my real project i need to do the reparenting as I have 2 different scenes below 2 different viewports and I want the player to travel from one viewport to another)
bugarea.zip
Forgot to mention, I've been testing and apparently, after removing child by using the method remove_child(child_to_remove) if you put yield(get_tree(), "idle_frame") afterwards, the whole issue gets fixed, even on my other project with multiple viewports.
bugarea-fixed.zip
I've simplified my minimal project some more, replaced the green level with a button. This makes the bug even more apparent.
After the player collided the first time and the red_level scene got removed and the button enabled,
on clicking the button the fade Animation that should only be triggered if the player is colliding with the Area2D starts immediately playing:
Minimal project:
Area2D_enter_on_remove_bug2.zip
As I did comment in #45131 I've find out that using the yield with the body_exited signal right after changing the body position is the way to go to fix this or maybe putting it in the _on_Area2D_body_exited().
Solution waiting the body_exited signal:
*Edit: The better solution is connecting the body_entered with the CONNECT_DEFERRED | CONNECT_ONESHOT options every time you remove and add the child node.
red_level.gd:
func _on_Area2D_body_entered(body):
print(body.name, " entered red Area")
$AnimationPlayer.play("transition")
func _on_AnimationPlayer_animation_finished(_anim_name):
print("AA")
stage_number += 1
$Label.text = str("stage ", stage_number)
$player.hide()
$player.position = Vector2(128, 256)
yield($Area2D, "body_exited")
call_deferred("emit_signal","signal_next_level")
main.gd:
func change_to_next_level():
if red_level in get_children():
$Button.disabled = false
remove_child(red_level)
else:
add_child(red_level)
red_level.get_node("player").show()
$Button.disabled = true
Forgot to mention, I've been testing and apparently, after removing child by using the method
remove_child(child_to_remove)if you putyield(get_tree(), "idle_frame")afterwards, the whole issue gets fixed, even on my other project with multiple viewports.
bugarea-fixed.zip
I tested your MRP, and it is entering and exiting 2 times from each function. You can test this putting an counter inside of the functions.
Like this:
extends Node2D
var num: int = 0
func _ready():
$LEFT/Door.connect("body_entered", self, "_player_to_right")
$RIGHT/Door.connect("body_entered", self, "_player_to_left")
func _player_to_right(player):
num += 1
print(num)
player.get_parent().remove_child(player)
yield(get_tree(), "idle_frame")
$RIGHT.add_child(player)
player.set_position(Vector2())
func _player_to_left(player):
print(num)
num += 1
player.get_parent().remove_child(player)
yield(get_tree(), "idle_frame")
$LEFT.add_child(player)
player.set_position(Vector2())
You can avoid this connecting using CONNECT_DEFERRED | CONNECT_ONESHOT every time you change the room. Anyway the bug will still be there in the engine side.
extends Node2D
var num = 0
func _ready():
$LEFT/Door.connect("body_entered", self, "_player_to_right", [], CONNECT_DEFERRED | CONNECT_ONESHOT)
$RIGHT/Door.connect("body_entered", self, "_player_to_left", [], CONNECT_DEFERRED | CONNECT_ONESHOT)
func _player_to_right(player):
num += 1
print("num ", num)
player.get_parent().remove_child(player)
# yield(get_tree(), "idle_frame")
$RIGHT.add_child(player)
player.set_position(Vector2())
$LEFT/Door.connect("body_entered", self, "_player_to_right", [], CONNECT_DEFERRED | CONNECT_ONESHOT)
func _player_to_left(player):
num += 1
print("num ", num)
player.get_parent().remove_child(player)
# yield(get_tree(), "idle_frame")
$LEFT.add_child(player)
player.set_position(Vector2())
$RIGHT/Door.connect("body_entered", self, "_player_to_left", [], CONNECT_DEFERRED | CONNECT_ONESHOT)
I can't test this right now, but will do so later. Thank you @AttackButton!
Still very annoying having to reconnect every time as this workaround only works with ONESHOT.
I tested your MRP, and it is entering and exiting 2 times from each function. You can test this putting an counter inside of the functions.
Damn you are absolutely right, I didn't notice this...
You can avoid this connecting using
CONNECT_DEFERRED | CONNECT_ONESHOTevery time you change the room. Anyway the bug will still be there in the engine side.
After a lot of testing, it seems that what you mention is the only reliable way of guaranteeing one signal received instead of multiple
I also tested putting some timers between lines of code and it seems that the problem also got fixed, but i find my solution very arbitrary and not very clean, but may help in finding out the issue. For example:
extends Node2D
var count = 0
func _ready():
$LEFT/Door.connect("body_entered", self, "_player_to_right")
$RIGHT/Door.connect("body_entered", self, "_player_to_left")
func _player_to_right(player):
count += 1
print(count)
player.set_global_position(Vector2(-100,-100)) # Move the player away from the area
yield(get_tree().create_timer(0.5), "timeout")
player.get_parent().remove_child(player)
$RIGHT.add_child(player)
player.set_position(Vector2())
[...]
^ This makes the signal emit only once, but if you change the timer for something shorter, let's say 0.01 like this...
[..]
func _player_to_right(player):
count += 1
print(count)
player.set_global_position(Vector2(-100,-100)) # Move the player away from the area
yield(get_tree().create_timer(0.01), "timeout")
player.get_parent().remove_child(player)
$RIGHT.add_child(player)
player.set_position(Vector2())
[...]
then the signal emits twice.
For me it seems that the "add" and "remove" child are not in sync with the physics process or something like that (i know nothing about Godot's internals so I might be completely wrong)
Just wanted to point out that this happens too in 3d. The workaround proposed by @AttackButton works in this case too
Just tested this and it's still reproducible in Godot 3.3.2
Here's my workaround for bugarea3d.zip in Godot 3.3.2:
extends Spatial
func _ready():
$LEFT/DOOR.connect("body_entered", self, "_player_to_right")
$RIGHT/DOOR.connect("body_entered", self, "_player_to_left")
func _player_to_right(player):
$LEFT/DOOR.set_block_signals(true)
$RIGHT/DOOR.set_block_signals(true)
player.get_parent().remove_child(player)
$RIGHT.add_child(player)
player.transform.origin = Vector3()
$LEFT/DOOR.set_block_signals(false)
$RIGHT/DOOR.set_block_signals(false)
func _player_to_left(player):
$LEFT/DOOR.set_block_signals(true)
$RIGHT/DOOR.set_block_signals(true)
player.get_parent().remove_child(player)
$LEFT.add_child(player)
player.transform.origin = Vector3()
$LEFT/DOOR.set_block_signals(false)
$RIGHT/DOOR.set_block_signals(false)
Of course, it's a test project. In my own project, this workaround is fine with the addition of yielding 2 idle frames before blocking the signals, and 2 idle frames before unblocking the signals, so YMMV. Although I'm not actually reparenting in my project, but the same bug is triggered by changing the underlying collider (CollisionShape2D / RectangleShape2D) used by an Area2D.
Just tested this and it's still reproducible in Godot 3.4. Seems like it will probably be fixed in 4.0
@knightofiam I think your workaround might be the cleanest so far tbh, thanks for pointing it out!
I can reproduce this in 3.3.4 in 2D.
I thought I found a workaround for it but so far the best way I've found to prevent it is to just not use the event system and just use _process(delta) with get_overlapping_bodies, but that's not ideal.
I'm going to try and think of something else, I feel like this is a pretty bad bug because IMO the docs lead you right into it https://docs.godotengine.org/en/stable/tutorials/best_practices/scene_organization.html#choosing-a-node-tree-structure
I wonder if some other workaround the docs kind of recommend against because of "bad coding practices" might also work (e.g., adding the player to the scene every time).
I'll hopefully have more ideas for this later.
Pretty sure I am battling this issue in 3.4.5.stable right now. Here is a minimal reproduction project (Move the capsule with left and right arrow keys): area2d_body_entered_triggered_multiple_times.zip
using call_deferred() for the add_child() seems to fix this for me, but when I exit the game I get errors that I leaked memory:
Is this reproducible in the current master branch?
@Anutrix Copy/paste of what I wrote in #61584 (comment)
As of 880a017 (the latest from master as of right now) it looks like it's behaving better.
When I hit 3 from position B I am not getting that body_entered event anymore, which is great!
It seems like the body_entered issue has improved in the last 4 months.
I tested this against master branch.
I downloaded the bugarea.zip and converted it to Godot 4 (had to fix some collision layers because it didn't collide with the door) then tested it and it seems to keep happening, signal is fired an indefinite amount of times making the game freeze. I haven't tried the workarounds though.
That's for the 2D version, the 3D version (bugarea3d.zip) directly closes the game upon opening it without moving so I assume something in the automatic conversion to Godot 4 went wrong, but I don't know how to debug that.
This is still a problem in 4. Reparenting a node while inside an area2d triggers an additional body_entered signal.
Can confirm, still seeing this in v4.1.3
Is this reproduceable with 4.3 dev1? https://godotengine.org/article/dev-snapshot-godot-4-3-dev-1/
Yes I can still reproduce it in 4.3 dev1. I quickly assembled a project to test it. I included 2D and 3D examples.
I'll leave a zip attached in this comment. If you want to test it, please run (using F6) the world.tscn scene for each example (I didn't set a main scene sorry)
EDIT: Sorry, I confused this issue with another one (#61584).
I also confirm that the bug is reproducible in 4.3 dev 1. Trying to workaround this by temporarily changing process_mode doesn't work, the only workaround is to change the collision_layer (and set_physics_process(), so as not to process a body with collision disabled).
I'm guessing the reason is that the body doesn't update the coordinates in the physics server when needed (you can ensure that the node coordinates are updated at the right time).
I encountered this problem with the Object Pooling I implemented. I have a kill box where I remove my Node, when i reenter the Node into my scene tree it triggered the on body entered no matter how and where I changed the position or global position.
My workaround looks like this:
func _enter_tree():
if isReady:
respawn()
func respawn():
Shape.disabled = true
await get_tree().physics_frame
await get_tree().physics_frame
Shape.disabled = falseI had the same issue with double taps of my collider, and also had the issue of having to use call_deferred to reparent. Searching lead to this solution type which handled both problems (node type is Area2D):
func picked_up(player: Player) -> void:
_being_carried = true
#reparent(player) # demanded call_deferred
monitoring = false
await change_parent(player)
monitoring = true
position.y -= 0.1
func change_parent(player: Player) -> void:
await call_deferred("reparent",player, true)