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

Improve SceneTreeEditor usability #85386

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Improve SceneTreeEditor usability #85386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions editor/animation_track_editor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5006,12 +5006,17 @@ void AnimationTrackEditor::_new_track_node_selected(NodePath p_path) {
}

void AnimationTrackEditor::_add_track(int p_type) {
if (!root) {
AnimationPlayer *ap = AnimationPlayerEditor::get_singleton()->get_player();
if (!ap) {
ERR_FAIL_EDMSG("No AnimationPlayer is currently being edited.");
}
Node *root_node = ap->get_node_or_null(ap->get_root_node());
if (!root_node) {
EditorNode::get_singleton()->show_warning(TTR("Not possible to add a new track without a root"));
return;
}
adding_track_type = p_type;
pick_track->popup_scenetree_dialog();
pick_track->popup_scenetree_dialog(nullptr, root_node);
pick_track->get_filter_line_edit()->clear();
pick_track->get_filter_line_edit()->grab_focus();
}
Expand Down
2 changes: 1 addition & 1 deletion editor/connections_dialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,7 @@ void ConnectDialog::init(const ConnectionData &p_cd, const PackedStringArray &p_
signal_args = p_signal_args;

tree->set_selected(nullptr);
tree->set_marked(source, true);
tree->set_marked(source);

if (p_cd.target) {
set_dst_node(static_cast<Node *>(p_cd.target));
Expand Down
11 changes: 9 additions & 2 deletions editor/editor_properties.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2751,7 +2751,15 @@ void EditorPropertyNodePath::_node_assign() {
add_child(scene_tree);
scene_tree->connect("selected", callable_mp(this, &EditorPropertyNodePath::_node_selected));
}
scene_tree->popup_scenetree_dialog();

Variant val = get_edited_property_value();
Node *n = nullptr;
if (val.get_type() == Variant::Type::NODE_PATH) {
n = get_base_node()->get_node_or_null(val);
} else {
n = Object::cast_to<Node>(val);
}
scene_tree->popup_scenetree_dialog(n, get_base_node());
}

void EditorPropertyNodePath::_update_menu() {
Expand Down Expand Up @@ -3184,7 +3192,6 @@ void EditorPropertyResource::_resource_changed(const Ref<Resource> &p_resource)
add_child(scene_tree);
scene_tree->connect("selected", callable_mp(this, &EditorPropertyResource::_viewport_selected));
}

scene_tree->popup_scenetree_dialog();
}
}
Expand Down
25 changes: 24 additions & 1 deletion editor/gui/scene_tree_editor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -925,6 +925,27 @@ void SceneTreeEditor::_notification(int p_what) {

_update_tree();
} break;

case NOTIFICATION_VISIBILITY_CHANGED: {
if (is_visible()) {
TreeItem *item = nullptr;
if (selected) {
// Scroll to selected node.
item = _find(tree->get_root(), selected->get_path());
} else if (marked.size() == 1) {
// Scroll to a single marked node.
Node *marked_node = *marked.begin();
if (marked_node) {
item = _find(tree->get_root(), marked_node->get_path());
}
}

if (item) {
// Must wait until tree is properly sized before scrolling.
callable_mp(tree, &Tree::scroll_to_item).call_deferred(item, true);
Copy link
Member

Choose a reason for hiding this comment

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

This PR seems to be causing crashes. Probably deferred call should use ObjectID instead of raw pointer, otherwise the tree item might be removed before callback execution and pointer will become invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you show me other code locations that do something like this? Also if there's a reliable way to reproduce the crash would help.

Copy link
Member

Choose a reason for hiding this comment

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

See #90235.

Copy link
Member

@bruvzg bruvzg Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can you show me other code locations that do something like this?

Not sure if there's exactly same situations, buy you can do something like:

Add a new callback:

void Tree::_scroll_to_item_id(ObjectID p_item_id, bool p_center_on_item) {
	TreeItem *item = Object::cast_to<TreeItem>(ObjectDB::get_instance(p_item_id));
	if (item) {
		scroll_to_item(item, p_center_on_item);
	}
}

And call it like this

if (item) {
	ObjectID item_id = item->get_instance_id();
	callable_mp(tree, &Tree::_scroll_to_item_id).call_deferred(item_id, true);
}

}
}
} break;
}
}

Expand Down Expand Up @@ -1567,7 +1588,9 @@ SceneTreeEditor::~SceneTreeEditor() {

/******** DIALOG *********/

void SceneTreeDialog::popup_scenetree_dialog() {
void SceneTreeDialog::popup_scenetree_dialog(Node *p_selected_node, Node *p_marked_node, bool p_marked_node_selectable, bool p_marked_node_children_selectable) {
get_scene_tree()->set_marked(p_marked_node, p_marked_node_selectable, p_marked_node_children_selectable);
get_scene_tree()->set_selected(p_selected_node);
popup_centered_clamped(Size2(350, 700) * EDSCALE);
}

Expand Down
6 changes: 3 additions & 3 deletions editor/gui/scene_tree_editor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,8 @@ class SceneTreeEditor : public Control {
void set_as_scene_tree_dock();
void set_display_foreign_nodes(bool p_display);

void set_marked(const HashSet<Node *> &p_marked, bool p_selectable = false, bool p_children_selectable = true);
void set_marked(Node *p_marked, bool p_selectable = false, bool p_children_selectable = true);
void set_marked(const HashSet<Node *> &p_marked, bool p_selectable = true, bool p_children_selectable = true);
void set_marked(Node *p_marked, bool p_selectable = true, bool p_children_selectable = true);
void set_selected(Node *p_node, bool p_emit_selected = true);
Node *get_selected();
void set_can_rename(bool p_can_rename) { can_rename = p_can_rename; }
Expand Down Expand Up @@ -201,7 +201,7 @@ class SceneTreeDialog : public ConfirmationDialog {
static void _bind_methods();

public:
void popup_scenetree_dialog();
void popup_scenetree_dialog(Node *p_selected_node = nullptr, Node *p_marked_node = nullptr, bool p_marked_node_selectable = true, bool p_marked_node_children_selectable = true);
void set_valid_types(const Vector<StringName> &p_valid);

SceneTreeEditor *get_scene_tree() { return tree; }
Expand Down
1 change: 0 additions & 1 deletion editor/plugins/cpu_particles_3d_editor_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ void CPUParticles3DEditor::_menu_option(int p_option) {
switch (p_option) {
case MENU_OPTION_CREATE_EMISSION_VOLUME_FROM_NODE: {
emission_tree_dialog->popup_scenetree_dialog();

} break;

case MENU_OPTION_RESTART: {
Expand Down
6 changes: 4 additions & 2 deletions editor/plugins/multimesh_editor_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,13 +255,15 @@ void MultiMeshEditor::edit(MultiMeshInstance3D *p_multimesh) {

void MultiMeshEditor::_browse(bool p_source) {
browsing_source = p_source;
std->get_scene_tree()->set_marked(node, false);
std->popup_scenetree_dialog();
Node *browsed_node = nullptr;
if (p_source) {
browsed_node = node->get_node_or_null(mesh_source->get_text());
std->set_title(TTR("Select a Source Mesh:"));
} else {
browsed_node = node->get_node_or_null(surface_source->get_text());
std->set_title(TTR("Select a Target Surface:"));
}
std->popup_scenetree_dialog(browsed_node);
}

void MultiMeshEditor::_bind_methods() {
Expand Down
7 changes: 1 addition & 6 deletions editor/reparent_dialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ void ReparentDialog::_reparent() {

void ReparentDialog::set_current(const HashSet<Node *> &p_selection) {
tree->set_marked(p_selection, false, false);
//tree->set_selected(p_node->get_parent());
tree->set_selected(nullptr);
}

void ReparentDialog::_bind_methods() {
Expand All @@ -74,7 +74,6 @@ ReparentDialog::ReparentDialog() {

VBoxContainer *vbc = memnew(VBoxContainer);
add_child(vbc);
//set_child_rect(vbc);

tree = memnew(SceneTreeEditor(false));
tree->set_show_enabled_subscene(true);
Expand All @@ -86,10 +85,6 @@ ReparentDialog::ReparentDialog() {
keep_transform->set_pressed(true);
vbc->add_child(keep_transform);

//vbc->add_margin_child("Options:",node_only);

//cancel->connect("pressed", this,"_cancel");

set_ok_button_text(TTR("Reparent"));
}

Expand Down
2 changes: 1 addition & 1 deletion modules/multiplayer/editor/replication_editor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ void ReplicationEditor::_pick_new_property() {
EditorNode::get_singleton()->show_warning(TTR("Not possible to add a new property to synchronize without a root."));
return;
}
pick_node->popup_scenetree_dialog();
pick_node->popup_scenetree_dialog(nullptr, current);
pick_node->get_filter_line_edit()->clear();
pick_node->get_filter_line_edit()->grab_focus();
}
Expand Down
Loading