Skip to content

Fix "Change Type" ignoring non-top-level nodes and editing foreign nodes#119909

Merged
Repiteo merged 1 commit into
godotengine:masterfrom
EdwardChanCH:fix_change_type_selection
Jun 3, 2026
Merged

Fix "Change Type" ignoring non-top-level nodes and editing foreign nodes#119909
Repiteo merged 1 commit into
godotengine:masterfrom
EdwardChanCH:fix_change_type_selection

Conversation

@EdwardChanCH
Copy link
Copy Markdown
Contributor

@EdwardChanCH EdwardChanCH commented May 31, 2026 •
edited
Loading

Closes #74236

Related: #119593

Note: Split from #119617. Can be merged separately.

Changes:

  • "Change Type" now applies to all nodes in the selection, instead of non-top-level nodes only.
  • "Change Type" is now correctly hidden when the selection contains any scene instance or yellow foreign node.
  • Properly removes _validate_no_foreign(); now uses _validate_no_foreign_selected().
  • Properly removes _validate_no_instance(); now uses _validate_no_instance_selected().

Video:

fix_change_type_selection.mp4

@EdwardChanCH EdwardChanCH requested a review from a team as a code owner May 31, 2026 05:37
@EdwardChanCH EdwardChanCH mentioned this pull request May 31, 2026
Open
@EdwardChanCH EdwardChanCH changed the title Fix Change Type ignoring non-top-level nodes and editing foreign nodes Fix "Change Type" ignoring non-top-level nodes and editing foreign nodes May 31, 2026
@AThousandShips AThousandShips added this to the 4.x milestone Jun 1, 2026
AThousandShips
AThousandShips reviewed Jun 1, 2026
View reviewed changes
Comment thread editor/docks/scene_tree_dock.h Outdated
@EdwardChanCH EdwardChanCH force-pushed the fix_change_type_selection branch 2 times, most recently from e82a446 to d70032f Compare June 2, 2026 17:44
@KoBeWi
Copy link
Copy Markdown
Member

KoBeWi commented Jun 2, 2026 •
edited
Loading

"Alert!" pop-up now correctly shows up when trying to edit scene instances or yellow foreign nodes.

This was already the case before this PR. When could foreign node change type?

This should no longer be possible to trigger (see the next bullet point).

Shortcuts exist.

@EdwardChanCH
Copy link
Copy Markdown
Contributor Author

EdwardChanCH commented Jun 2, 2026

This was already the case before this PR. When could foreign node change type?

You are right about this. I will remove it then.

Shortcuts exist.

My poor choice of words... I meant by default, since "Change Type" does not have a default shortcut like "Rename (F2)". I will remove it then.

@KoBeWi
Copy link
Copy Markdown
Member

KoBeWi commented Jun 2, 2026

You are right about this. I will remove it then.

So the note about correctly hiding it is also irrelevant, or...?

@EdwardChanCH
Copy link
Copy Markdown
Contributor Author

EdwardChanCH commented Jun 2, 2026 •
edited
Loading

So the note about correctly hiding it is also irrelevant, or...?

That one is relevant. In 4.6.1.stable, "Change Type" did not apply to non-top-level selected nodes, and did not check if any non-top-level selected node is foreign. Rename had the same issues (#119593, #119645), as seen in the screenshot.

Image 2026-06-02_15-07-30

KoBeWi
KoBeWi approved these changes Jun 2, 2026
View reviewed changes
Copy link
Copy Markdown
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.

The comments are questionable, but the code itself looks fine.

Comment thread editor/docks/scene_tree_dock.cpp Outdated

} else if (current_option == TOOL_CHANGE_TYPE) {
const List<Node *> selection = editor_selection->get_top_selected_node_list();
// TODO: Rename selection to full_selection?
Copy link
Copy Markdown
Member

@KoBeWi KoBeWi Jun 2, 2026

Choose a reason for hiding this comment

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

Either remove or act on this comment, it's not that many changes.

The code for TOOL_REPARENT_TO_NEW_NODE also has selection instead of top_selection, so IMO it just doesn't matter.

Copy link
Copy Markdown
Contributor Author

@EdwardChanCH EdwardChanCH Jun 3, 2026

Choose a reason for hiding this comment

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

Removed the comment (at line 3872).

Comment thread editor/docks/scene_tree_dock.cpp Outdated
ERR_FAIL_COND(!EditorNode::get_singleton()->get_edited_scene());
menu->clear(false);

// TODO: Rename selection to top_selection?
Copy link
Copy Markdown
Member

@KoBeWi KoBeWi Jun 2, 2026

Choose a reason for hiding this comment

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

Here the rename makes more sense.

Copy link
Copy Markdown
Contributor Author

@EdwardChanCH EdwardChanCH Jun 3, 2026

Choose a reason for hiding this comment

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

Acted on the comment (at line 3138).

Comment thread editor/docks/scene_tree_dock.cpp Outdated
Comment on lines +3873 to +3877
// Returns only the top-level selected nodes (i.e. excludes any selected node whose parent is also selected).
// The first node selected by the user is at the front of the list (i.e. not sorted in scene tree order).
const List<Node *> selection = editor_selection->get_top_selected_node_list();
List<Node *> full_selection = editor_selection->get_full_selected_node_list(); // Above method only returns nodes with common parent.
// Returns all selected nodes (list version of "get_selected_nodes").
// The first node selected by the user is at the front of the list (i.e. not sorted in scene tree order).
Copy link
Copy Markdown
Member

@KoBeWi KoBeWi Jun 2, 2026

Choose a reason for hiding this comment

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

I don't think these methods should be explained here.
Also there are similar exposed methods (get_selected_nodes() and get_top_selected_nodes()) so technically they're already documented.

Copy link
Copy Markdown
Contributor Author

@EdwardChanCH EdwardChanCH Jun 3, 2026

Choose a reason for hiding this comment

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

Updated the comment. I think some comments here would still be helpful, since the variable declarations are quite far away, unlike in the other methods.

@EdwardChanCH EdwardChanCH mentioned this pull request Jun 2, 2026
Merged
@EdwardChanCH EdwardChanCH force-pushed the fix_change_type_selection branch from d70032f to 97c7b9f Compare June 3, 2026 00:23
@EdwardChanCH EdwardChanCH force-pushed the fix_change_type_selection branch from 97c7b9f to 8984aff Compare June 3, 2026 00:30
@EdwardChanCH
Copy link
Copy Markdown
Contributor Author

EdwardChanCH commented Jun 3, 2026

Updated questionable comments.

Properly removed _validate_no_foreign() and _validate_no_instance() entirely, as discussed in #119910 (comment).

@Repiteo Repiteo modified the milestones: 4.x, 4.7 Jun 3, 2026
@Repiteo Repiteo merged commit dba2dd3 into godotengine:master Jun 3, 2026
20 checks passed
@Repiteo
Copy link
Copy Markdown
Contributor

Repiteo commented Jun 3, 2026

Thanks!

@EdwardChanCH EdwardChanCH mentioned this pull request Jun 6, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AThousandShips AThousandShips AThousandShips left review comments

@KoBeWi KoBeWi KoBeWi approved these changes

Assignees

No one assigned

Labels

bug topic:editor usability

Projects

None yet

Milestone

4.7

Development

Successfully merging this pull request may close these issues.

Cannot change type of child nodes

4 participants

@EdwardChanCH @KoBeWi @Repiteo @AThousandShips