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

Undo does not reset size on TextureRects which were automatically resized by being put into a CenterContainer #66092

Closed
Proggle opened this issue Sep 19, 2022 · 6 comments

Comments

@Proggle
Copy link
Contributor

Proggle commented Sep 19, 2022

Godot version

v3.5.stable.official [991bb6a]

System information

Windows 10

Issue description

Let's say you have a texturerect and a centercontainer.
image
You drag the texturerect onto the centercontainer, so it's now a child of the centercontainer. The centercontainer automatically resizes it.
image
You decide you didn't want to do that, and press control+Z to undo. The tree heirarchy is undone, but the change in size to the texturerect is not undone.
image

Expectation: Undo should restore the editor entirely to the state before the drag-and-drop operation, not partially.

Steps to reproduce

  1. drag and drop a texturerect onto a center container.
  2. press control + Z

Minimal reproduction project

CenterContainerBug.zip

@KoBeWi
Copy link
Member

KoBeWi commented Sep 19, 2022

The undo action uses generic add_child() method from Node and it has no information about the side-effects of this action. Resolving this would be tricky and require some special handing for specific nodes. I don't think it's worth it.

@SirQuartz
Copy link
Contributor

The undo action uses generic add_child() method from Node and it has no information about the side-effects of this action. Resolving this would be tricky and require some special handing for specific nodes. I don't think it's worth it.

You could probably do this by storing the size of the node within its previous parent with the undo action, then simply check if the current size is different and set the size of the node accordingly. Of course, that's just a hypothesis.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 19, 2022

Yeah the problem is that Node has no size, so you first need to check if it's a Control and do this conditionally. Doable, but sounds hacky. Also you can reparent a node in multiple ways, so this should be done with a specialized method to avoid duplicating this code.

This is minor bug that rarely has a real impact. In worst case you can just revert your scene if it was saved recently enough.

@lostminds
Copy link

I was just planning on reporting this bug for 4.0.beta6 and the issue is more general (perhaps it should be renamed?) affecting all Control nodes being placed in any of the layout Container nodes that resize them, and working with control nodes I think it's not that rare. Basically it means that undo doesn't undo layout changes caused by containers. This is annoying and I think it can decrease the willingness of users to try using these container nodes, since you can't undo the changes they cause. Which is a pity since they're powerful and useful once you get them set up correctly.

Having implemented undo-systems in other applications I think a workable more general approach would be to record any changes that the layout container applies when the new control node child is being added as part of the same undo action group if one is active. This way these or any other changes changes would also be undone if the user then undoes the change in parent, but you won't need to save anything in some undo state for all nodes where it's not relevant.

@TheSofox
Copy link
Contributor

Duplicate of #44629

I've made pull request that fixes this issue and has been approved for 4.3 : #85181

@lostminds
Copy link

I've made pull request that fixes this issue and has been approved for 4.3

Great work @TheSofox! Looks like it's been cherrypicked for 4.2 so hopefully it should be included in 4.2.1 even if it didn't make it into the first 4.2 release

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

No branches or pull requests

6 participants