Repository: godot
Issue: is_visibility_in_tree() parenting issue with Spatial and CanvasItem
Labels: enhancement,discussion,topic:core

<!-- Please search existing issues for potential duplicates before filing yours:
https://github.com/godotengine/godot/issues?q=is%3Aissue
-->
Further discussion of #22373 and extending to cover Spatial as well.

**Godot version:**
<!-- Specify commit hash if non-official. -->
Current master b6c10c2

**Issue description:**
<!-- What happened, and what was expected. -->
Calling is_visible_in_tree() fails to detect visibility changes higher up in the tree for spatials and canvas items if a parent is a different type (i.e. not derived from the corresponding type).

In the case of spatials data.parent is set to NULL, when the parent is a non-spatial.

**Steps to reproduce:**
_Spatials_: Create a Spatial root node, add a Node child, then another spatial below the child. The visibility of the 2nd spatial will not be affected by toggling the visibility of the root spatial.
_CanvasItems_: Create a Spatial root node, add a Sprite child. The visibility of the sprite isn't affected by toggling the visibility of the parent.

**Minimal reproduction project:**
[Visibility.zip](https://github.com/godotengine/godot/files/3385211/Visibility.zip)
Load the example project. Toggle the root spatial in the editor, it does not change visibility of the ball. View the sprite in 2d view. Toggle the spatial parent of the sprite. It does not affect the visibility of the sprite.

**Discussion:**
I'm pretty new to working on the engine but I was looking at ways of turning off processing when nodes had visibility turned off in the scene graph and I was suggested to use is_visible_in_tree(), as is_visible() is local to the node. I had a look at the source code for spatial:
```
bool Spatial::is_visible_in_tree() const {

	const Spatial *s = this;

	while (s) {
		if (!s->data.visible) {
			return false;
		}
		s = s->data.parent;
	}

	return true;
}
```
And immediately was concerned what happened if the parent node was not Spatial (or derived). After a test it seemed that the data.parent is set to NULL if you have e.g. a Node as parent to the Spatial, rather than getting a crash. This seems by design and means visibility doesn't pass through here.

After a little look it seems the same applies to CanvasItems:
```
bool CanvasItem::is_visible_in_tree() const {

	if (!is_inside_tree())
		return false;

	const CanvasItem *p = this;

	while (p) {
		if (!p->visible)
			return false;
		p = p->get_parent_item();
	}

	return true;
}
```
where:
```
CanvasItem *CanvasItem::get_parent_item() const {

	if (toplevel)
		return NULL;

	return Object::cast_to<CanvasItem>(get_parent());
}
```
So again traversal stops once it meets a non-canvas item.

I guess my question would be, is this the intended situation, and if so it should be mentioned in the docs? Currently it says:

_CanvasItem_
> bool is_visible_in_tree() const
> Returns true if the node is present in the SceneTree, its visible property is true and its inherited visibility is also true.

_Spatial_
> bool is_visible_in_tree() const
> Returns whether the node is visible, taking into consideration that its parents visibility.

For me, an obvious question is why is visible bool being maintained separately in Spatial and CanvasItem, rather than just being part of the Node (and being able to do this whole traversal via the Node). However I am sure there is some non-obvious design reason for this .. like being able to totally separate the 2d and 3d components of the engine.

Anyway this is just in the hope of some enlightenment, at the least we can simply mention it in the docs. :smile: 

Another possibility to remove the need for this in most cases is some kind of method for optional parameters for notifications (i.e. whether being changed to visible, or invisible). Obviously the notification system wants to be as efficient as possible, and passing unused parameters around umpteen times is not a good solution, but you could easily store an optional notification parameter (or several) in the engine to be queried. This would have almost zero cost unless being used. Unless there were issues with multiple threads trying to call notifications at the same time.

You could also simply change the single visibility notification from changed to two alterative notifications for showing and hiding, however this would require changes throughout.