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

Adding a new scene from FileSystem dock can misfire if a Scene has an @export … PackedScene #94959

Closed
inhalt120g opened this issue Jul 30, 2024 · 7 comments · Fixed by #95090

Comments

@inhalt120g
Copy link

inhalt120g commented Jul 30, 2024

Tested versions

v4.3.rc1.official [e343dbb]

System information

MacMini M1 with newest MacOS

Issue description

If there's a Scene that has an
@export var my_scene : PackedScene
and I try to drag something from the FileSystem to it as a child, instead of getting added as a child the newly added scene will for some reason get inserted as a my_scene instead.

I understand this might be considered a feature, but then it's not consistent with how a dragged-in scene behaves when it's added to a scene that doesn't have an @export … PackedScene bit.

This is super annoying when I have to drag lots of scenes from FileSystem panel into the various game levels and I want to drag them directly into Nodes in specific place in the list, but then I have to first drag them somewhere randomly (wherever there are no "@export … PackedScene"s) and then drag them manually to where they have to be.

Steps to reproduce

In the attached project:
Drag the following scenes to the Scenes in the Scene panel directly from the FileSystem panel:

  1. Drag "Expl01" to "model parent". The Expl01 will be added to the bottom of the Scene list.
  2. Drag "Expl02" to "N1": "Expl02" will be added as a child of N1.
  3. Drag "Expl03" to N3. The Node will not be added to the Scene list! Instead, if you look into the N2's my_scene, it will be assigned there.
  4. To make sure everything is in order with drag and dropping, go back to Expl03 and drag it into N1. Expl03 will appear as a child of N1 as expected. (edit: fixed typo)

Here is the video showing it in action (right click and Save As to avoid stuttering):
https://archive.org/download/ShowFlowFleetingWorld/画面収録%202024-07-30%2021.24.43.mov

Minimal reproduction project (MRP)

adding-a-new-scene-gets-swallowed.zip

@yahkr
Copy link
Contributor

yahkr commented Jul 31, 2024

This does seem counterintuitive, however it is consistent with exports pertaining to assets. So this is just a case of PackedScenes pulling double duty (Can be instantiated by drag and drop, and can be assigned to exports by drag and drop where the latter takes priority in this case).

# Drag and drop onto node from FileSystem populates
@export var my_texture : Texture2D
@export var my_scene : PackedScene

# Drag and drop onto node from Scene adds it as child
@export var my_node : Node

@inhalt120g
Copy link
Author

inhalt120g commented Jul 31, 2024

I can't see how this behavior makes sense.

I tried dragging a Scene from FileSystem into a Scene in the Scene List where the Scene has a few @export var … PackedScene slots, and indeed Godot offered me a drop down list of all the @export var … PackedScene available for that node. So… it is indeed meant to work like that. Very confusing.

But! If I dragged in the same Scene into the upper part of the target Scene name in the Scene panel, for example here:
image
…the target scene would be highlighted but then the dragged in Scene would be added ABOVE the target scene. Basically as if I did this:
image
So the new scene would not appear neither within the target scene (which I'd like), nor in its export (which is meant to

(Edit: shortening for the sake of clarity)

Not sure under what circumstances could the current behaviors be considered useful, but I'd like to compose all the enemies in my game by populating levels with basic enemy "skeletons" and then just dd various abilities to each by dragging in and adding various weapons, equipment or "brains" directly from the FileSystem panel into each enemy, so current behavior is getting in the way big time.

@AeioMuch
Copy link
Contributor

AeioMuch commented Aug 2, 2024

Hello. I could not reproduce it in 4.2.2, I think the regression was introduced since the change on the _files_dropped method in scene_tree_dock.cpp here : #92004

cc @timothyqiu

@timothyqiu
Copy link
Member

This is an intended behavior change rather than an regression, see #92004 (comment)

But I guess we can add a setting to treat PackedScene differently.

@inhalt120g
Copy link
Author

@timothyqiu This behavior definitely gets in the way if you want to compose your level by first populating levels with enemies, and then adding various component "modules" to them by dragging them from the FileSystem into each enemy (my case).

In fact I'd argue the current behavior is inconsistent with the basic "add a child to a target scene" operation when drag and dropping a Scene into another Scene within Scene list panel. Right now, if I drag a scene C into scene P within Scene list panel, C will become a child of P as expected. But by the "add it to the export if possible" logic, if I drag a scene C into scene P within Scene tree, instead of C becoming a child of P, C would get plugged in into one of P's exports. Surely the latter behavior would be considered wrong.

Same with copy / pasting a scene into another scene: if we follow the "add it to the export if possible" logic, the pasted scene would end up in one of the target scene exports. I think this would be very impractical.

So even though we're performing the same "adding a child to a scene" operation (by drag and dropping it into another scene), the end result is different depending on where we added the child scene from (FileSystem or Scene list panel).

@timothyqiu
Copy link
Member

We can examine the question of consistency from various perspectives, leading to potentially opposing conclusions, all of which are nonetheless valid 😛

I understand your concerns, so I have submitted a PR to restore the old behavior.

@inhalt120g
Copy link
Author

Yes you are right. Thank you for the PR, it's appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Unassessed
Development

Successfully merging a pull request may close this issue.

5 participants