Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When packing a PackedScene into a scene, signals and scripts of that PackedScene are duplicated into the parent scene #48064

Open
Synzorasize opened this issue Apr 21, 2021 · 11 comments · Fixed by #97303

Comments

@Synzorasize
Copy link

Godot version:
3.2.3

OS/device including version:
Windows 10

Issue description:
I was working on a level editor for my game, and I decided to use PackedScenes for the tested levels since scenes are the default format for my original levels.
I also have PackedScenes within those scenes, though. Those are the tiles, hazards, orbs, player, etc. They all obviously have scripts attached to them (except for the regular tiles). And the hazards and orbs' body_entered are connected to themselves (in their own scene) to detect if the player touched them or not.
When I play the level, it shows this error:

E 0:01:28.051   connect: Signal 'body_shape_entered' is already connected to given method '_on_Hazard_body_shape_entered' in that object.
  <C++ Error>   Method failed. Returning: ERR_INVALID_PARAMETER
  <C++ Source>  core/object.cpp:1515 @ connect() # I am not even using the connect() method, haha...
  <Stack Trace> CustomLevel.gd:75 @ _on_PlayButton_pressed()

It appears to be blaming this line in the CustomLevel node's (The main parent node for all the elements in the level editor) script:
get_tree().change_scene_to(GlobalVariables.level) # The variable in a singleton which the CustomLevel node packs into.

Of course, it isn't crashing or breaking my game, but it is annoying to see this error for each node pop up every time I go to this scene or reload it. The scene would probably also take slightly more time to load due to this error. I would also want to focus on some real issues, too.

This is the code in the CustomLevel script to pack everything (since the CustomLevel node isn't the root node, all the children won't be packed if I don't set the owner to itself):

func saveLevel():
	position = Vector2(0,0)
	descendants = get_children() + [UI] # the variable "UI" stands for the UI node of the game which is a child of a CanvasLayer.
	for i in descendants:
		i.owner = self
	GlobalVariables.level.pack(self)

func _on_PlayButton_pressed():
	UI.show()
	saveLevel()
#	ResourceSaver.save("glitchedLevel.tscn", GlobalVariables.level) # I wanted to test what it looked like as a .tscn file
	get_tree().change_scene_to(GlobalVariables.level)
	GlobalVariables.editor = false
	GlobalVariables.editor_playing = true

I decided to save it as a .tscn file to see what it looked like and it seemed to have the same signals and scripts as the nodes already have in their source scenes... Hence why it is showing the error. I edited the files to remove those signals and scripts and it still works perfectly... so those extra bytes of data are unnecessary.

I am assuming this might be a core problem and probably can't be fixed with a script? Maybe it can, but with a complicated workaround. I also took a look at core/object.cpp:1515 but it is just the emit_signal() method which is not helpful, I'm pretty sure since I am not using connect() or emit_signal() in the first place. I am connecting the signals through the editor (in their own scenes)

Steps to reproduce:
Have a root Node, then add a child to that Node (Node0 in this case). This second Node will be the parent of all the other nodes. Make one or two nodes the children of this Node. Then make a new scene, make a Node (Node3 in this case), add a script to it. Then connect any signal (it can be a custom signal) to itself in the local scene, then make it print something in _ready() when the node is emitted (just to know if it is working).
Go back to the original scene and instance Node3 to Node0. Add a script to Node0 and make a PackedScene.new() variable like "level". Then in _ready(), put something like this:

for i in get_children():
	i.owner = self
level.pack(self)
get_tree().change_scene_to(level)

The error will appear.

Minimal reproduction project:
PackedSceneProblem.zip
(For some reason, this is glitching out (it is printing "hello" every frame even though it is only supposed to be emitted once at _ready(). It might have to do with this bug, but I am not sure.)

@Calinou
Copy link
Member

Calinou commented Apr 21, 2021

Possibly related to #3393.

@Yesyoor
Copy link

Yesyoor commented Jun 2, 2022

Hey I ran into a similar problem. My signals get wired up in the scenes _ready function.
Unfortunately they also get saved, and some how Godot is not checking if a signal is already connected when loading from a packed scene. Is there any workaround available? I couldn't find ways to avoid the signal from getting packed into the scene

@jitspoe
Copy link
Contributor

jitspoe commented Aug 19, 2023

I think this issue is present in Godot 4.1 as well. I'm working on a BSP importer, and I remap Quake entities to packed scenes, so in this case I map trigger_changelevel to a changelevel.tscn.

Everything functions, but I get errors in the log on import: Signal 'body_entered' is already connected to given callable 'Area3D(changelevel.gd)::_on_body_entered' in that object.

@jitspoe
Copy link
Contributor

jitspoe commented Aug 23, 2023

Actually, this is worse than I thought initially. I thought it was just on the import that I was getting the errors, but it seems every time I run the imported level, I get spammed with these errors if there are a lot of area triggers, making it difficult to identify real issues.

@jitspoe
Copy link
Contributor

jitspoe commented Oct 5, 2023

Just an interesting note, but if I save the bsp that I imported (which is internally stored as a scn file) as a tscn file, the error goes away. I was hoping to be able to look at the text scene and see exactly what's going wrong. Haven't tried importing directly as a tscn file, yet, but figured I'd share that info in case it's helpful.

Edit: Just tested storing the imported file as a tscn file instead of a .scn file and can confirm the duplicated signal being added.

[connection signal="body_entered" from="ChangeLevel" to="ChangeLevel" method="_on_body_entered" flags=18]
[connection signal="body_entered" from="AreaTriggerMultiple" to="AreaTriggerMultiple" method="_on_body_entered" flags=18]

These were already in their respective scenes:
Ex, change_level.tscn had

[connection signal="body_entered" from="." to="." method="_on_body_entered"]

@KoBeWi
Copy link
Member

KoBeWi commented Dec 12, 2023

(For some reason, this is glitching out (it is printing "hello" every frame even though it is only supposed to be emitted once at _ready(). It might have to do with this bug, but I am not sure.)

You are packing the scene that does the packing and scene change, which means that it repeats after every scene change.

@cixil
Copy link
Contributor

cixil commented Sep 10, 2024

I can confirm this behavior is still the same today in 4.3.

In my project I am packing/loading scenes in a tool script. Checking the text of the .tscn files as @jitspoe suggested last year, I can see that the signals that should only be in child tscn files are falsely duplicated in the parent scene that I had packed.

@cixil
Copy link
Contributor

cixil commented Sep 10, 2024

I'm packing and instantiating scenes in a tool script and I get the "signal already connected" errors every time I instantiate one of these botched scene files whether in the editor or in game.

I noticed that saving the packed scenes to files with ResourceSaver and then opening those files in the editor and saving them once will fix the signals, this can also be verified by checking the difference in the .tscn file after its been opened and saved in the editor.

Maybe this is a helpful workaround for some use cases. As I'm often repacking many scenes, opening them in the editor to save them manually each time is still a hassle if I want my game error free.

@Yesyoor
Copy link

Yesyoor commented Sep 13, 2024

i can't recommend to use ResourceSaver and PackedScences and so on at all. The next problem will occur if you try to save nested scenes. I tried, many times with others on discord and it just stays unuseable for me. I serialize everything myself and that way keep control of everything.

@cixil
Copy link
Contributor

cixil commented Sep 18, 2024

Not sure if this has been said, but this bug seems to only affect signals coming from the root of the child scenes. Not every signal within the child scene. For example a scene whose root is an Area2D node that has a body_entered signal connected to itself.

So a workaround is to have a plain Node or Node2D as the root of all subscenes and connect from children to the root node, and just have no signals emitting from the root of any child scenes.

But I'm seeing if I can make a PR to fix for 4.4

@akien-mga
Copy link
Member

Reopening as #97303 was reverted to fix #100097.

@akien-mga akien-mga reopened this Dec 11, 2024
this-is-envy added a commit to Small-Loan-Studio/TGO that referenced this issue Dec 11, 2024
So there was a bug where child scenes that have signals emitted from and
then connected to themselves would have their signals connected to the
**parent** scene that was hosted them. This was tracked upstream in
godotengine/godot#48064 (comment).

I reworked the switch component to have relevant signals either not
connected via auto-wiring in the scene or to be emitted / connected to
not the root object. It's kinda trash and more confusing but (I think 😰)
it will work and as long as we don't have to come back and make
substantial changes it should be "fine"

Hello tech debt.

While here I setup all moveable blocks to be included in the
switch-actor collision layer by default because even I was forgetting to
set that up.

---------

Co-authored-by: envy <this-is-envy@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants