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

Fix duplicating multiple nodes at different depths in SceneTreeDock #86211

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

TheSofox
Copy link
Contributor

Fixes #84333.

Duplicating multiple nodes at different depths seemed to put all the new nodes on the same level, which was counterintuitive. Also, undoing this operation was bugged.

I've fixed it so that each duplicated node appears next to the node it was duplicated from at whatever depth it was at. This also fixed the Undo functionality.

@TheSofox TheSofox requested a review from a team as a code owner December 15, 2023 19:37
@AThousandShips AThousandShips added this to the 4.3 milestone Dec 15, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Dec 21, 2023

This changes how nodes on the same level are duplicated:

godot.windows.editor.dev.x86_64_dmAeG1gXuZ.mp4

vs old

godot_Hsbxsczcj5.mp4

I don't think that's expected behavior. Especially the order is garbled (reminiscent of this old issue #38162, which the code you removed fixed)

@TheSofox
Copy link
Contributor Author

TheSofox commented Dec 21, 2023

Question: You duplicate (both nodes, same level):

  • TypeA1
  • TypeB1

Is the desired result:

  • TypeA1
  • TypeA2
  • TypeB1
  • TypeB2

or:

  • TypeA1
  • TypeB1
  • TypeA2
  • TypeB2

@KoBeWi
Copy link
Member

KoBeWi commented Dec 21, 2023

I'd say the latter.

In case of

  • TypeA1
  • TypeA2

Expected result is

  • TypeA1
  • TypeA2
  • TypeA3
  • TypeA4

In your example, the latter result is consistent with the above.

@TheSofox
Copy link
Contributor Author

TheSofox commented Dec 21, 2023

Understood, but I need to ask another question about desired behaviour:

  • AudioNode
  • Player Node
  • AnimationNode
  • Particle Node

You select AudioNode and AnimationNode (I'm assuming order you select them in doesn't matter, let me know if it does) and duplicate. This obviously creates two more nodes (AudioNode2 and AnimationNode2) but where are they positioned?

@KoBeWi
Copy link
Member

KoBeWi commented Dec 22, 2023

Currently you'd end up with

  • AudioNode
  • PlayerNode
  • AnimationNode
  • AudioNode2
  • AnimationNode2
  • ParticleNode

I think you should keep the current behavior for nodes on the same level and only fix duplication with different parents.

@TheSofox
Copy link
Contributor Author

Understood. Thank you for explaining.

@TheSofox TheSofox force-pushed the node-duplicate-undo-fix branch from 7809622 to ed64879 Compare December 22, 2023 15:20
@TheSofox
Copy link
Contributor Author

Amended PR and pushed. Like you asked, it sticks with current behavior for nodes on the same level, but still handles having nodes with different parents.

@YuriSizov YuriSizov changed the title Fixed duplicating multiple nodes at different depths in SceneTreeDock Fix duplicating multiple nodes at different depths in SceneTreeDock Dec 22, 2023
@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 2, 2024
@akien-mga akien-mga merged commit e78c5d0 into godotengine:master Jan 2, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 11, 2024
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.

Incorrect UndoRedo when duplicating nodes at different depths.
4 participants