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

CanvasItemEditor: Fix losing position for drag'n'dropped scenes #39754

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Jun 22, 2020

Fixes #26549.
Supersedes #36309.

Note: Tested on 3.2 as drag and drop is broken on Linux/X11 in master.


Some issues I noticed (not created by this fix):

  • Drag and drop of scenes in an empty scene doesn't work well. It properly creates an inherited scene, but it doesn't get it marked as unsaved with (*), and it also opens a new, empty scene which takes focus, so it looks like it didn't even work.
  • The drag preview doesn't take the target node's transform into account, so if you drag a Sprite or scene e.g. on a rotated parent node, or a scaled one, the preview will be unaffected but once dropping, the new node will properly be subject to the transform. This could be good to fix so that WYSIWYG when dragging and dropping.

@akien-mga akien-mga added bug topic:editor cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Jun 22, 2020
@akien-mga akien-mga added this to the 4.0 milestone Jun 22, 2020
@akien-mga akien-mga requested review from groud and KoBeWi June 22, 2020 12:23
@groud
Copy link
Member

groud commented Jun 22, 2020

I am not sure this is the expected behavior in fact. If I am not mistaking, when you position a node2d as instantiated into a scene, you override the instantiated scene's position in the instantiating scene. So, basically, it's like you ignore the original position in the subscene. This behaviors continue once you dropped the node anyway, as you won't have the origin as considered as the initial position in the instantiated scene.

I think the right way to solve the problem would be to simply set the position to (0,0) during the drag&drop preview, it would probably be more consistent IMHO.

@akien-mga
Copy link
Member Author

See the screenshots below that show the behavior after this PR.
Note: the "fire" sprite is not the root node, but it's positioned at the origin of the scene being instanced to mark it. The root node is actually the tree on the bottom right (which therefore has a non-zero position in its own scene).

Original scene:
Screenshot_20200622_143842

Preview when dragging that scene in a new scene:
Screenshot_20200622_144040
(Same as before, the origin of the dragged scene is under the cursor.)

After dropping the scene:
Screenshot_20200622_144119
(The origin of the dragged scene was dropped at the position of the cursor, as I would expect it to be.)

Before this PR, the center of the bottom-right tree (root node of the dragged scene) would be at the cursor position, since the previous code would discard its pre-existing position information.

@akien-mga
Copy link
Member Author

The whole point being that if you design your scene with a root node that has a non-zero position, you have a valid reason to do so. The editor shouldn't discard this information.

@groud
Copy link
Member

groud commented Jun 22, 2020

The whole point being that if you design your scene with a root node that has a non-zero position, you have a valid reason to do so. The editor shouldn't discard this information.

Well, that's the question. What I mean is that this information will be discarded anyway once you drop the subscene, as the original position is overridden in the parent scene. So basically, once the subscene is dropped, you have no more way to adjust the subscene position considering the original position in the subscene as there is not even a pivot icon considering this origin.

Basically, for now, the position information of the root node in the subscene is only used for the drag&drop preview, but nowhere else. So I think the simple solution would be to reset the position to zero in the preview.

We can also go for your solution, which might be a little bit more expected for the drag&drop, but I fear that it also could mislead users into thinking that they can still manipulate the scene with an origin which is no more considered once the scene is instantiated.

@akien-mga
Copy link
Member Author

akien-mga commented Jun 22, 2020

Right, I see your point.

I still tend to think that it's better to respect what the devs designed in their original scene, even if I don't see much use case for having a root node with non-zero position in the first place. One could imagine a scenario where you make your scene with such an offset specifically because that's what you want to enforce when instantiating them in other scenes, but then you don't bother about keeping that initial offset information.

Morever, the fix in this PR makes the behavior consistent with what happens when you instantiate the scene from code or via the "Instance a scene" button of the scene tree panel. The scene will be instantiated at the target node's origin, while preserving the transform of the instantiated scene.

See:

Scene to instantiate, root node has a non-zero position:
Screenshot_20200622_152109

After instantiating it using the button in the Scene tree dock, the new node's position is target_node.position + new_node.position:
Screenshot_20200622_152231

The information about the original scene's offset is indeed lost, but I haven't seen anyone complain about it so far (again, having a scene with a non-zero position for the root node is quite a niche situation already).

So this PR makes the drag behavior match the instantiation behavior (instantiating is equivalent to drag and dropping with the original scene's origin dropped on top of the target node's position).

And indeed, if you drag and drop a scene as a child of a pre-existing node, this original offset will be preserved as long as you only move the parent node and not the instantiated one.

@groud
Copy link
Member

groud commented Jun 22, 2020

Sure it make sense too. If this is the most expected behavior I guess it makes sense. :)

@akien-mga akien-mga merged commit b740f64 into godotengine:master Jun 26, 2020
@akien-mga akien-mga deleted the canvasitemeditor-fix-dropped-scenes-position branch June 26, 2020 18:54
@KoBeWi
Copy link
Member

KoBeWi commented Jun 26, 2020

I forgot to leave a review, sorry ;_;

@akien-mga
Copy link
Member Author

Post-merge reviews are still welcome :) Especially if you disagree with the change ;)

@akien-mga
Copy link
Member Author

Cherry-picked for 3.2.3.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dragged resource file over scene should appear at it's drop position, not be centered at cursor
3 participants