Allow TreeItem nodes to toggle visibility #60061

monkeyman192 · 2022-04-09T11:17:46Z

I have attached an example godot project (it's pretty lazy and dirty without some optimisations but it's simply to show how the functionality works).

~~filtered_tree.zip~~
filtered_tree2.zip

As far as I can see for testing, the functionality works really nicely. One thing that may be confusing for people using the property can be explained by an example:

Let's say I have a Tree with TreeItem's as follows (assuming a hidden root, and everything is current expanded):

Fruits
├─ apple
└─ pear
Vegetables
└─ cucumber

If I set apple to be invisible then it will not be drawn, and everything else will.
If I then set Fruits to be invisble then that branch will not be drawn.
If I then set Fruits to be visible I'd expect it to be drawn again and same with pear, but not apple.
This is how it will currently work, however, if I checked pear.visible when Fruits.visible = false, then it will be true since I am not propagating the value to children (so that if a parent TreeItem is made visible again, it doesn't cause unintentded changes of state of child TreeItems.

This could be slightly confusing to users, however it could be potentially clarified by documentation, or potentially have the is_visible() getter return a computed value based on the visibility of the parent(s).
Very open to discussion and suggestions regarding these points 😄

Calinou · 2022-04-09T16:29:05Z

The editor's node tree filtering could be modified to make use of this new Tree visibility. It will likely speed up filtering large scene trees.

KoBeWi · 2022-05-01T11:24:46Z

I get crash with this code:

func _ready() -> void:
	var tree = $Tree as Tree
	
	var root = tree.create_item()
	root.set_text(0, "Test0")
	
	for i in 8:
		var item = tree.create_item(root)
		item.set_text(0, "Test1")
	
	for i in 4:
		var item = root.get_first_child().create_child()
		item.set_text(0, "Test2")
	
	root.get_first_child().visible = false

[0] TreeItem::get_parent (C:\godot_source\scene\gui\tree.cpp:656)
[1] Tree::get_column_minimum_width (C:\godot_source\scene\gui\tree.cpp:4207)
[2] Tree::get_internal_min_size (C:\godot_source\scene\gui\tree.cpp:3647)
[3] Tree::_do_incr_search (C:\godot_source\scene\gui\tree.cpp:3673)
[4] Tree::_notification (C:\godot_source\scene\gui\tree.cpp:3818)
[5] Tree::_notificationv (C:\godot_source\scene\gui\tree.h:365)
[6] Object::notification (C:\godot_source\core\object\object.cpp:850)
[7] CanvasItem::_update_callback (C:\godot_source\scene\main\canvas_item.cpp:143)
[8] call_with_variant_args_helper<CanvasItem> (C:\godot_source\core\variant\binder_common.h:241)
[9] call_with_variant_args_dv<CanvasItem> (C:\godot_source\core\variant\binder_common.h:384)
[10] MethodBindT<CanvasItem>::call (C:\godot_source\core\object\method_bind.h:336)
[11] Object::callp (C:\godot_source\core\object\object.cpp:839)
[12] Callable::call (C:\godot_source\core\variant\callable.cpp:62)
[13] MessageQueue::_call_function (C:\godot_source\core\object\message_queue.cpp:230)
[14] MessageQueue::flush (C:\godot_source\core\object\message_queue.cpp:277)
[15] SceneTree::physics_process (C:\godot_source\scene\main\scene_tree.cpp:421)
[16] Main::iteration (C:\godot_source\main\main.cpp:2692)
[17] OS_Windows::run (C:\godot_source\platform\windows\os_windows.cpp:677)
[18] widechar_main (C:\godot_source\platform\windows\godot_windows.cpp:175)
[19] _main (C:\godot_source\platform\windows\godot_windows.cpp:197)
[20] main (C:\godot_source\platform\windows\godot_windows.cpp:211)
[21] WinMain (C:\godot_source\platform\windows\godot_windows.cpp:225)
[22] __scrt_common_main_seh (D:\a01\_work\26\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[23] BaseThreadInitThunk
-- END OF BACKTRACE --

Caused by the last line.

scene/gui/tree.cpp

monkeyman192 · 2022-05-02T00:00:44Z

I get crash with this code:

func _ready() -> void:
	var tree = $Tree as Tree
	
	var root = tree.create_item()
	root.set_text(0, "Test0")
	
	for i in 8:
		var item = tree.create_item(root)
		item.set_text(0, "Test1")
	
	for i in 4:
		var item = root.get_first_child().create_child()
		item.set_text(0, "Test2")
	
	root.get_first_child().visible = false

Thanks for finding this! I have fixed this issue but it made me realise another issue which I'll need to fix. When the parent TreeItem is set as visible = false, if it isn't collapsed then the child TreeItem's are still visible (and actually get rendered in the same "level" as the parent that is no longer visible. Will work on fixing this.

monkeyman192 · 2022-05-02T12:50:40Z

Ok. I have made a fix for the above issues mentioned.
As part of this I have also updated the example project (initial comment has been updated to include the new version)

KoBeWi · 2022-05-02T14:15:01Z

Folding is still active when all children all invisible:

Is this intended?

scene/gui/tree.cpp

monkeyman192 · 2022-05-03T01:48:50Z

Folding is still active when all children all invisible:

Is this intended?

Ah, good catch. I think it should not be.

This is a setter for a variable, so it should only return that variable. If you want function like this one here, it should be called is_visible_in_tree() or similar (like in CanvasItem).

EDIT: Ah, but also you are traversing the whole tree upwards. This basically kills any performance optimizations you tried to achieve.

I assume you mean getter? I thought that in general there was nothing against having a getter return a modified version of the variable? Although it would lead to cases where you can do ti.visible = true; print(ti.visible) # false if the parent is not visible.
I think is_visible_in_tree() is a better idea though.
The main issue is that the visible property doesn't always actually reflect whether it will be visible as it depends on the parent (as I recently added).
The upward traversal does at least stop when it is found to not be visible. I guess the performance hit would be more when the tree is deeply nested. I'll see if there is a better away around this.

KoBeWi · 2022-05-03T02:03:55Z

I assume you mean getter?

Ah, yes.

The main issue is that the visible property doesn't always actually reflect whether it will be visible as it depends on the parent (as I recently added).

The same behavior is in e.g. Node2D. If it's visible, it doesn't mean you can see it, as the parent might be invisible. This is perfectly fine, because the node can exist outside of tree and the property needs to be independent.

I guess the performance hit would be more when the tree is deeply nested.

Yes. But even with shallow tree, if you have e.g. 1000 items on 2 levels, each of the items would check parent and grandparent visibility needlessly every time you update the tree. is_visible_in_tree() will be called very often, so it needs to be performant.

monkeyman192 · 2022-05-03T03:07:09Z

The same behaviour is in e.g. Node2D. If it's visible, it doesn't mean you can see it, as the parent might be invisible. This is perfectly fine, because the node can exist outside of tree and the property needs to be independent.

Ok cool. If there is precedent for this behaviour and it's understood, then I'm happy with that. I was just concerned because I felt it seemed unintuitive at first glance.

Yes. But even with shallow tree, if you have e.g. 1000 items on 2 levels, each of the items would check parent and grandparent visibility needlessly every time you update the tree. is_visible_in_tree() will be called very often, so it needs to be performant.

Yep, very true. I'll work on this more tonight and see what I can do.

monkeyman192 · 2022-05-04T03:03:16Z

scene/gui/tree.cpp

 TreeItem *TreeItem::get_child(int p_idx) {
 	_create_children_cache();
 	ERR_FAIL_INDEX_V(p_idx, children_cache.size(), nullptr);
 	return children_cache.get(p_idx);
 }

+int TreeItem::get_visible_child_count() {


I didn't publicly expose this function, but if it might be useful then I can add it. I can see it having use, but it's also not as efficient for determining whether to conditionally hide something based on whether it has any visible children as simply counting how many children are not hidden as you hide them to avoid doing a second loop.
It still may be useful though in other situations...

KoBeWi · 2022-05-14T17:46:50Z

scene/gui/tree.cpp

-TreeItem *TreeItem::get_next_visible(bool p_wrap) {
+TreeItem *TreeItem::get_prev_visible(bool p_wrap) {
+	TreeItem *loop = this;
+	TreeItem *prev = this->_get_prev_visible(p_wrap);


It's weird that _get_prev_visible() can return a TreeItem that isn't visible. We should rename this method, but it can be done in later PR.

Yeah, I agree. I really couldn't think of a better name 😅

KoBeWi · 2022-05-14T18:12:06Z

scene/gui/tree.cpp

@@ -4150,7 +4220,7 @@ int Tree::get_column_minimum_width(int p_column) const {
 					depth += 1;
 				} else {
 					TreeItem *common_parent = item->get_parent();
-					while (common_parent != next->get_parent()) {
+					while (common_parent != next->get_parent() && common_parent) {


Why this change? Doesn't look related.

This is to fix the issue that you raised near the top of this review.
Without this change your example code that didn't work will still not work.
I am not 100% sure why it is needed either, but adding this indeed fixes it.

Ah, I see why is this needed now. I mean, I know when it could crash without this check, just not sure when this happens. Probably not important 🤔

You could swap the order though, so common_parent check is first.

scene/gui/tree.h

KoBeWi · 2022-05-14T18:22:38Z

scene/gui/tree.cpp

+	}
+	visible = p_visible;
+	if (tree) {
+		tree->update();


This will update the tree even if a parent item is invisible (so needlessly). I wonder if it's worth to optimize it 🤔

I am not sure if there is a way to do this without checking parents upwards until one is found to be invisible.
I am guessing that this would still be more efficient than updating the entire tree each time.
If you think it's worth including this in this PR I can do that otherwise we can leave for a future one?

I had some further thought on this and realised I don't think that the set_collapsed method has an optimisation like you described above either (so if you collapse or un-collapse a node which is underneath an already collapsed node you would toggle an update of the whole tree.
Maybe it might be worth doing an investigation on some optimisations in a separate ticket/PR.

TreeItem should have a second property called parent_visible_in_tree which is set when TreeItem is added to Tree. The first TreeItem has it always as true, then its children will have parent_visible_in_tree true if the parent has both visible and parent_visible_in_tree set to true. So it's only checked once and propagated as nodes are added. When item changes visibility, child items get notified and update accordingly.

This is how it works in CanvasItem. It can be done in a separate PR, as it requires some more changes.

doc/classes/TreeItem.xml

KoBeWi

Looks good overall. I left some comments, but any bigger changes can be done in a future PR.

monkeyman192 · 2022-05-16T14:05:04Z

Looks good overall. I left some comments, but any bigger changes can be done in a future PR.

Thanks very much for the review. I have made the suggested small changes, and have left the others with comments here.
If you are happy with the state of this PR, I am happy for it to be merged and have no other changes to make (unless extra suggestions are made).
Also, let me know if a back-port is required for the 3.x branch.

akien-mga · 2022-05-24T08:53:47Z

Thanks! And congrats for your first merged Godot contribution 🎉

monkeyman192 requested review from a team as code owners April 9, 2022 11:17

YeldhamDev added enhancement topic:gui labels Apr 9, 2022

YeldhamDev added this to the 4.0 milestone Apr 9, 2022

KoBeWi reviewed May 1, 2022

View reviewed changes

scene/gui/tree.cpp Outdated Show resolved Hide resolved

monkeyman192 force-pushed the allow_treeitem_visible branch from ed276a3 to 92993c1 Compare May 2, 2022 04:20

KoBeWi reviewed May 2, 2022

View reviewed changes

scene/gui/tree.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed May 2, 2022

View reviewed changes

scene/gui/tree.cpp Show resolved Hide resolved

monkeyman192 force-pushed the allow_treeitem_visible branch from 92993c1 to 22b4b74 Compare May 4, 2022 02:54

monkeyman192 commented May 4, 2022

View reviewed changes

monkeyman192 requested a review from KoBeWi May 8, 2022 23:18

KoBeWi reviewed May 14, 2022

View reviewed changes

scene/gui/tree.h Outdated Show resolved Hide resolved

KoBeWi reviewed May 14, 2022

View reviewed changes

doc/classes/TreeItem.xml Outdated Show resolved Hide resolved

KoBeWi approved these changes May 14, 2022

View reviewed changes

monkeyman192 force-pushed the allow_treeitem_visible branch from 22b4b74 to 7feb364 Compare May 16, 2022 13:53

Allow TreeItem nodes to toggle visibility

31381f8

monkeyman192 force-pushed the allow_treeitem_visible branch from 7feb364 to 31381f8 Compare May 16, 2022 13:55

akien-mga merged commit 2998be4 into godotengine:master May 24, 2022

monkeyman192 deleted the allow_treeitem_visible branch May 24, 2022 12:19

This was referenced May 24, 2022

Optimize SceneTreeEditor filtering #61371

Merged

Relationship lines draw for invisible TreeItems #61372

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow TreeItem nodes to toggle visibility #60061

Allow TreeItem nodes to toggle visibility #60061

monkeyman192 commented Apr 9, 2022 •

edited

Loading

Calinou commented Apr 9, 2022

KoBeWi commented May 1, 2022

monkeyman192 commented May 2, 2022

monkeyman192 commented May 2, 2022

KoBeWi commented May 2, 2022

monkeyman192 commented May 3, 2022

KoBeWi commented May 3, 2022 •

edited

Loading

monkeyman192 commented May 3, 2022

monkeyman192 May 4, 2022

KoBeWi May 14, 2022

monkeyman192 May 16, 2022

KoBeWi May 14, 2022 •

edited

Loading

monkeyman192 May 16, 2022

KoBeWi May 16, 2022

KoBeWi May 14, 2022

monkeyman192 May 16, 2022

monkeyman192 May 16, 2022

KoBeWi May 16, 2022

KoBeWi left a comment

monkeyman192 commented May 16, 2022

akien-mga commented May 24, 2022

Allow TreeItem nodes to toggle visibility #60061

Allow TreeItem nodes to toggle visibility #60061

Conversation

monkeyman192 commented Apr 9, 2022 • edited Loading

Calinou commented Apr 9, 2022

KoBeWi commented May 1, 2022

monkeyman192 commented May 2, 2022

monkeyman192 commented May 2, 2022

KoBeWi commented May 2, 2022

monkeyman192 commented May 3, 2022

KoBeWi commented May 3, 2022 • edited Loading

monkeyman192 commented May 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KoBeWi May 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KoBeWi left a comment

Choose a reason for hiding this comment

monkeyman192 commented May 16, 2022

akien-mga commented May 24, 2022

monkeyman192 commented Apr 9, 2022 •

edited

Loading

KoBeWi commented May 3, 2022 •

edited

Loading

KoBeWi May 14, 2022 •

edited

Loading