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.

Sign up for GitHub

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

Jump to bottom

Fix behavior of 'Editable Children' toggle #60974

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Fix behavior of 'Editable Children' toggle #60974

merged 1 commit into from
Jan 9, 2024

Conversation

SaracenOne
Copy link
Member

Toggling 'editable_children' off in the SceneTree will now no longer result in nodes owned by the currently edited scene, but assigned as children of foreign nodes, from being accidentally lost, where they before would still be visible in the scene, but not viewable since their parents were made hidden. They will instead be moved to the root of the instance, with their global transform perserved in the case of Node2D and Node3D derivatives. The editable children toggle will now also be subject to functional undoing and redoing.

Closes #46726

@SaracenOne SaracenOne added this to the 4.0 milestone May 12, 2022
@SaracenOne SaracenOne requested review from a team as code owners May 12, 2022 15:45
@SaracenOne SaracenOne force-pushed the editable_children_toggle_improvements branch from f1d2e12 to aabc39f Compare May 12, 2022 18:56
lyuma
lyuma reviewed May 13, 2022
View reviewed changes
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
@SaracenOne SaracenOne force-pushed the editable_children_toggle_improvements branch 3 times, most recently from b0600fb to 117dbea Compare July 21, 2022 20:48
@fire fire force-pushed the editable_children_toggle_improvements branch 2 times, most recently from 71ec344 to 4e75005 Compare August 22, 2022 22:41
@KoBeWi
Copy link
Member

KoBeWi commented Sep 21, 2022

#36301 would make the implementation simpler.

@fire fire force-pushed the editable_children_toggle_improvements branch from 4e75005 to 9f3fd75 Compare December 7, 2022 04:56
@fire fire force-pushed the editable_children_toggle_improvements branch from 9f3fd75 to 6ea1ac2 Compare January 6, 2023 16:44
KoBeWi
KoBeWi reviewed Jan 10, 2023
View reviewed changes
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Jan 10, 2023
View reviewed changes
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 16, 2023
@SaracenOne SaracenOne force-pushed the editable_children_toggle_improvements branch from 6ea1ac2 to af6fa56 Compare October 26, 2023 10:32
@SaracenOne
Copy link
Member Author

Okay, this has been updated with the reparent function. Would recommend a new review since this bug is still relevant and still a bad UX trap.

KoBeWi
KoBeWi reviewed Oct 26, 2023
View reviewed changes
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Oct 26, 2023
View reviewed changes
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Oct 26, 2023
View reviewed changes
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Oct 26, 2023
View reviewed changes
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Oct 26, 2023
View reviewed changes
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
@SaracenOne SaracenOne force-pushed the editable_children_toggle_improvements branch 4 times, most recently from f7c80d1 to 9fce264 Compare October 27, 2023 21:36
@SaracenOne
Copy link
Member Author

Okay, I've made a bunch of updates and corrected most of what @KoBeWi pointed out. A lot of the issues were holdovers from the initial PR and I've corrected them now. There's a couple of points I've provided explanations for why its done that way though.

KoBeWi
KoBeWi reviewed Oct 27, 2023
View reviewed changes
editor/scene_tree_dock.h Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Oct 28, 2023

Something went wrong:

godot.windows.editor.dev.x86_64_SjHVs93e5s.mp4 
Can't add child 'A' to itself.
Invalid owner. Owner must be an ancestor in the tree

@SaracenOne SaracenOne force-pushed the editable_children_toggle_improvements branch from 9fce264 to 8ed50c6 Compare November 3, 2023 22:12
@SaracenOne SaracenOne requested a review from a team as a code owner November 3, 2023 22:12
@SaracenOne
Copy link
Member Author

Okay, sorry, I think I've fixed this properly this time.

  • Fixed main node disappearing when removing editable children
  • Made it so that update_all_gizmos method is binded properly and put it back seperately in the UndoRedo action to prevent the editable children status not correctly updating.
  • Removed the empty method defintion.

KoBeWi
KoBeWi reviewed Nov 4, 2023
View reviewed changes
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi approved these changes Nov 4, 2023
View reviewed changes
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine.

@SaracenOne SaracenOne force-pushed the editable_children_toggle_improvements branch from 8ed50c6 to 62b3dfb Compare November 5, 2023 04:50
@KoBeWi KoBeWi mentioned this pull request Nov 6, 2023
Open
@SaracenOne SaracenOne force-pushed the editable_children_toggle_improvements branch from 62b3dfb to 3177a72 Compare November 7, 2023 04:39
@SaracenOne SaracenOne changed the title Improvement to 'Editable Children' toggle Fix behaviour of 'Editable Children' toggle Nov 7, 2023
@SaracenOne
Copy link
Member Author

Changed PR and commit title to better match preferred submission format.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.3 Dec 14, 2023
AThousandShips
AThousandShips reviewed Dec 14, 2023
View reviewed changes
editor/scene_tree_dock.h Outdated Show resolved Hide resolved
editor/scene_tree_dock.h Outdated Show resolved Hide resolved
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga changed the title Fix behaviour of 'Editable Children' toggle Fix behavior of 'Editable Children' toggle Jan 9, 2024
@SaracenOne @akien-mga
Fix behavior of 'Editable Children' toggle.
cb8a743 
Prevents losing nodes owned by the edited scene when
toggling editable_children off on an instanced scene,
and makes the toggle compatible with undo-redo.
@akien-mga akien-mga force-pushed the editable_children_toggle_improvements branch from 3177a72 to cb8a743 Compare January 9, 2024 12:34
@akien-mga
Copy link
Member

Rebased and amended to apply @AThousandShips' suggestions.

@akien-mga akien-mga merged commit bcd3bc9 into godotengine:master Jan 9, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@bhottovy
Copy link

After this was merged I have started getting errors like:
scene/resources/packed_scene.cpp:1648 - Index p_idx = 6 is out of bounds (nodes.size() = 3).
spammed in the editor output whenever I save a scene containing an instanced child scene that is inherited and 'editable_children' is enabled. If 'editable_children' is disabled I don't have any issue.

See #87424

@fire fire deleted the editable_children_toggle_improvements branch February 18, 2024 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga left review comments

@lyuma lyuma lyuma left review comments

@KoBeWi KoBeWi KoBeWi approved these changes

@AThousandShips AThousandShips AThousandShips approved these changes

Assignees
No one assigned
Labels
bug topic:editor usability
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

A node instanced as a child of a foreign node doesn't get saved after Editable Children is unchecked.
7 participants
@SaracenOne @KoBeWi @akien-mga @bhottovy @lyuma @AThousandShips @YuriSizov