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.

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

Add node active feature and corresponding lifecycle methods #94144

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RobProductions
Copy link
Contributor

Implements godotengine/godot-proposals#7715 and is based on suggestions from users in that discussion :)

This change aims to accomplish 3 things:

  1. Add a faster/more convenient way to disable a Node for debugging and gameplay purposes. See here.
  2. Provide the groundwork for a more universal disable feature that can simplify workflows and eventually replace some existing "enabled" toggles. See here.
  3. Provide new initialization and cleanup methods (i.e. lifecycle calls) that are dependent on active state so that users can run code only when a node is supposed to have an impact in the scene. See here.

I should also note that this PR is meant to pair nicely with and build upon the work of @torcado194 since it is distinct from #92377 due to working in both runtime and the editor, and leaving references to disabled nodes intact.

Active Concept

All nodes now have a property called node_active which determines their active state:

image

Similar to visibility, if an ancestor node is inactive, the child behaves as though it is inactive as well. To match visibility, a new method called is_node_active_in_tree() exists to check whether it should behave as active or inactive.

From discussions in godotengine/godot-proposals#7715 we wanted an easy way to toggle/preview active state but some people noted a lot of icons in the scene tree already, so @Snowdrama suggested an icon filter as seen here. It's a great idea that should definitely be added imo but it's probably best as it's own PR, so for now I added a new editor setting called show_node_active_checkbox which toggles the icon and is off by default to alleviate concerns of clutter. Here is that in action:

Capture2024-07-09.11-19-06.mov

An inactive node is not visible, does not process scripts/physics/collision/lighting/etc., and will not work as a valid camera (a special case because it did not previously check can_process). It works directly from the editor via these new buttons, a tool script that sets the node_active property, or from the inspector:

Capture2024-07-09.11-26-51.mov

It also works in runtime! Simply set the node_active property or (in C++) use set_node_active to toggle it. You can connect to the node_active_changed signal to get a callback when the property has changed, or the node_active_in_tree_changed to get a callback when its "active in tree" status has changed. Here a script enables a node during a certain time frame and then turns it off again:

Capture2024-07-09.11-27-46.mov

Lifecycle Methods

Thanks to feedback from @passivestar and others in the proposal, another useful aspect of this feature is new lifecycle methods for users to run initialization/cleanup code that is dependent on active state. The new _node_active() call is run when a node becomes active in tree or after _ready() when it enters the tree (if active). The _node_inactive() call is run when a node becomes inactive in tree or before _exit_tree() when exiting the tree (if it was active before that point). Now, users can run code like this:

func _ready():
	print("_ready")

func _enter_tree():
	print("_enter_tree")

func _exit_tree():
	print("_exit_tree")

func _node_active():
	print("_node_active");
	
func _node_inactive():
	print("_node_inactive");

And like before, it works both in the editor and in runtime:

Capture2024-07-08.13-30-12.mov

Ultimately, this means you can put setup steps in _node_active so that code runs when it becomes active, not when the scene starts. This is useful for say an object that is culled by distance so that it only has an impact when the player gets closer to it rather than it having to do additional work to check whether it is culled in _ready().

There is room for further work here like adding a parameter to signal calling so that inactive objects do not receive them, but for now this is a good starting point for people to play with and covers the main use cases discussed in the proposal.

Other Notes

I generated and wrote out documentation descriptions for the new methods, notifications, and properties so let me know if that looks good and if there's other documentation I should edit as a result of this change. For example, it might be good to mention active state in the visibility section of the docs, but I wasn't sure how much I should mess with for now. I'm happy to help write explanations in the doc site as well to help explain the feature since I know communication of something like this is very important. I also added new test cases just to cover some simple scenarios but I can add more if necessary.

Thanks to everyone that helped shape this PR and I know that since this is a hefty change it will take some time to review/merge but I'm happy to clarify anything and make any necessary adjustments :) I'm hoping this will improve lots of workflows down the line, so let me know how it works during your testing! Thank you!

@@ -60,7 +60,7 @@ Transform2D CanvasItem::_edit_get_transform() const {

bool CanvasItem::is_visible_in_tree() const {
ERR_READ_THREAD_GUARD_V(false);
return visible && parent_visible_in_tree;
return visible && parent_visible_in_tree && is_node_active_in_tree();
Copy link
Member

Choose a reason for hiding this comment

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

is_node_active_in_tree() seems to be checking activity recursively in its ancestors. It should be a cached value, like parent_visible_in_tree.

Copy link
Contributor Author

@RobProductions RobProductions Jul 9, 2024

Choose a reason for hiding this comment

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

I can do that, though the way that node_3d.cpp handles visibility in tree is actually to traverse each parent, seen here. Is there a reason its like that? And this one was modeled after the process_owner system i.e. it doesn't check all parents one by one but just the topmost active parent in the chain thanks to this code:

if (data.parent && data.node_active) {
	data.active_owner = data.parent->data.active_owner;
} 

I could instead turn this into the way that canvas_item.cpp handles visibility in tree with a bool, would that be preferred?

Copy link
Member

Choose a reason for hiding this comment

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

CanvasItem visibility was optimized to avoid tree lookup, 3D code was not updated yet (assuming it's possible; there might be differences preventing it).

The process_owner doesn't make recursive calls, so the method does not get more expensive based on node depth. But this is possible, because the owner determines only process mode; the processing itself is determined based on mode. It can't be applied to activity, because it depends on whether the parent is active.

For optimal solution, active should match the CanvasItem visibility implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks! Let me know if this implementation looks better 😄

@Mickeon Mickeon self-requested a review July 9, 2024 18:58
@passivestar
Copy link
Contributor

Tested, can confirm that these work:

  • Disabled node doesn't call _input and _process
  • Disabling a Node3D hides its children
  • Disabling Camera3D switches to another camera as expected
  • Lifecycle methods _node_active and _node_inactive work
  • node_active_changed signal works
  • Instantiating a scene with a disabled root node and a tool script doesn't call _node_active method on it in editor
  • Disabling a rigid body excludes it from the simulation, same with particles
  • Clicking on a checkbox while multiple nodes are selected properly toggles all of them just like the visibility toggle

Problems and nitpicks:

  • Disabling a CanvasLayer doesn't hide its children. Toggling its visibility does hide its children
  • Remote tab of scene tree doesn't have the checkboxes
  • Would be nice for checkboxes in the scene tree to have indication for "inactive in tree", just like the visibility icon
  • Disabling Environment doesn't have any effect, would be nice if it did (not necessarily for this PR)

I wasn't able to keep up with the discussion in the proposal thread but I'm pretty happy with this design. A much needed feature. The only thing missing is a native option for disabled nodes to ignore signals. Whether a disabled node will react to a signal can be handled by adding a new flag to ConnectFlags

@Snowdrama
Copy link

Snowdrama commented Jul 10, 2024

Remote tab of scene tree doesn't have the checkboxes

Good catch! Being able to set active/inactive in the remote is super useful.

Would be nice for checkboxes in the scene tree to have indication for "inactive in tree", just like the visibility icon

That makes sense, I'm not sure how that would work with the blue checkbox icon, does the visibility icon just change the opacity to fade it when it's inactive in tree? But I think this makes makes sense.

It might also a good change to possibly update the icon to match the style of the other icons while we're at it rather than using the blue checkbox?

I threw together an SVG in Affinity Grey/Black for Dark/Light mode so I'd just need like file formats and export resolutions for them if we wanted to use them.
imageimage

The only thing missing is a native option for disabled nodes to ignore signals. Whether a disabled node will react to a signal can be handled by adding a new flag to ConnectFlags

That would be valuable for sure! I think that part did get lost in the conversation, thanks for bringing it up.

@reduz
Copy link
Member

reduz commented Jul 10, 2024

I think this is a good feature usability wise, but to be honest, the process mode and visibility properties together achieve a similar effect and its good that they are separate to me. Adding a yet third state to coordinate both seems unnecessary and adding more confusion to a system that you have to learn anyway and it won't go away just because you add this state.

I totally get it makes sense if you come from Unity, but Godot is not Unity and I think the solution should make more sense in the context of how Godot works internally.

So, instead of having an active mode, you could have the processing exposed in the scene tree (this an Idea I had for a while, and kind of the original intention when it I created the process modes in Godot) something like:

image

This way you can disable node visibility and not processing, and processing and no visibility. Don't want any? disable both, so it's much clearer, obvious and flexible what is going on while editing the state.

And this basically just reflects the state of the process mode similar to the eye reflects the state of the visibility, keeping the Godot core APIs cleaner and the editor interface reflecting what it actually happens internally.

@passivestar Currently, you have NOTIFICATION_PAUSED and NOTIFICATION_UNPAUSED that do exactly the same as setting a node active or not, though I am not sure there is a callback, though it should be possible to add this easily (such as _activated and _deactivated callbacks for more ease of use). Not sure if worth it, though.

@passivestar
Copy link
Contributor

@passivestar Currently, you have NOTIFICATION_PAUSED and NOTIFICATION_UNPAUSED that do exactly the same as setting a node active or not, though I am not sure there is a callback, though it should be possible to do this

Did you mean NOTIFICATION_ENABLED and NOTIFICATION_DISABLED? I know about those, but no, there's no callbacks for those currently

They are also not called when the node enters and leaves the tree. There's currently no place to put your code in so that it doesn't run when you disable a node in editor before running the game

There's no API that would be a contract for inactivity. There's no agreement. If you download something from the asset store, put it in your scene, change properties on it, and then need to temporarily disable it to check how things work without it and without losing your data, what do you do? Will disabling process on it work as if it was never in the scene? No, because it runs code in _ready() and _enter_tree() and most likely isn't checking the process mode before doing so. It wasn't designed with inactivity in mind, because inactivity isn't in Godot's lexicon

Everybody seems to be doing whatever. You say that disabling process should be good enough for runtime, some people say that it's better to take it out of the tree to properly "disable" it. Taking it out of the tree will at least trigger an actual callback that is exposed (_exit_tree), but you need a reference to a parent to add it back later. And it will be gone from the Remote tab of the Scene dock, which isn't very convenient either

To this day, after playing with Godot for over half a year I still have no confidence that when I disable something by setting its process to disabled it will actually become disabled. If I disable a camera this way, will it stop being the "active" camera and will another camera become active? If I disable a collider by changing its process mode, will it stop colliding? As I'm writing this, I genuinely don't remember. Right now I don't even remember if disabling process will also disable _input. To me it sounds like it won't, because just from the name of it I'd expect it to only disable _process and _physics_process. This is confusing. Maybe I'm a bit slow and should learn the engine more to better appreciate it. Or maybe there's a better design

Having visibility and processing separately makes more sense in code than it does in the scene dock. More often than not you'll want to disable game entities completely, not just their visibility or their processing. More often than not I'll have to click both buttons

This isn't copying Unity for the sake of copying Unity. Unity has its problems. This is taking a look at how Godot solves inactivity and thinking if it can be improved. Ending up with a system that is similar to Unity in some ways isn't a problem when there are things that it solves better, namely in-editor UX for disabling objects, playmode debugging experience and having an API that actually gives you confidence that an object will become disabled as you expect no matter if it's your code or not

This PR could be the first step to solving these issues. The exact API can be refined to make sure it works for Godot and makes sense to everybody, both existing and new users

@allenwp
Copy link
Contributor

allenwp commented Jul 10, 2024

At first glance, I expect this feature to be a shortcut to set the Node’s process_mode to disabled and also turn off visibility in one click… So from a usability perspective, I would be confused about why this new feature isn’t modifying these properties. (Please correct me if I’m wrong.)

Is there any way to add the new lifecycle functionally while also using the existing visibility and process_mode?

Edit: Hmm. Maybe not. At second glance, if the conversation is around activating and deactivating stuff that you aren’t the original author of, then my idea can’t work.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 10, 2024

If we add activity, Godot will have visibility, process mode, and activity, which is the same as the former 2 with some extras. It just feels redundant to have multiple basically competing systems. You can toggle visibility, process mode or activity and each has a callback that the node is supposed to react to, with lots of overlap. Sounds like a mess.

I'm also in favor of process toggle, but to make it a proper replacement for activity, it needs some improvements. If disabling a node does not yield expected results (like in camera or colliders), it needs to be fixed to properly handle disabling.

If I disable a collider by changing its process mode, will it stop colliding?

Disabling active of a collider won't stop colliding either, or at least nothing in the code indicates so.

@reduz
Copy link
Member

reduz commented Jul 10, 2024

@passivestar

Did you mean NOTIFICATION_ENABLED and NOTIFICATION_DISABLED? I know about those, but no, there's no callbacks for those currently

They are also not called when the node enters and leaves the tree. There's currently no place to put your code in so that it doesn't run when you disable a node in editor before running the game

Oh yes sorry, I forgot we renamed them in 4.0. I see why the workflow you intend to do is cumbersome. One possibility may be to make something like NOTIFICATION_ACTIVATED that is called when the node is enabled, so you can use it with NOTIFICATION_DISABLED and it would work as you suggest. Likewise no harm in adding callbacks if there is demand.

To this day, after playing with Godot for over half a year I still have no confidence that when I disable something by setting its process to disabled it will actually become disabled.

Disabling processing effectively disables it in practice, node does nothing. No processing, no input, no physics (which you can decide whether you want it to make it static or just remove it from processing) etc. That is warranted. It is still added to the scene and initialized, of course, but no processing code will be executed that comes from the scene tree, input, physics etc. That is a contract that the engine honors and it does in a way it makes sense. It also adds the possibility to specify what can be paused and how, which is really helpful when you want to pause the game state.

You just have to wrap your head around the fact that due to how Godot works, It's not Unity. There is no such thing as a globally active thing because it does not make sense in how the engine works, and hacking it will make things worse. Visuals and processing are 100% separate in Godot, while on Unity they are even the same thing internally. It sounds to me that this is something that needs better documentation, as it is new to 4.0 and likely has not much examples.

Having visibility and processing separately makes more sense in code than it does in the scene dock. More often than not you'll want to disable game entities completely, not just their visibility or their processing. More often than not I'll have to click both buttons

Actually you would be surprised that most of the time you don't want to do both and this is not a problem Godot users face often (as you can see it's been a decade, so it shouldn't surprise you this is not very requested). The most common thing you want to do in games it do disable objects when not in view (something Godot does very well), or disable them when not active (due to gameplay reasons) or in the right room, but you don't necessarily want to hide it because it still needs to be visible. Of course, if you also want to show/hide them its also super easy.

In Unity, users disable objects for different reasons, most of the time because they can't freely take them in/out of the scene. As in Godot doing this is not really a problem and users are very accustomed to just remove/readd the node.

This is why adding this to Godot feels pretty foreign, specially when you are used to how the engine works.

That said, I think having a process_disable property to completely disable the node processing no matter what is set in process_mode, and expose it in the scene tree dock does have a lot of value in cases where you want to edit a level and more visually leave parts of the scene disabled until you can enable them later.

I agree that going to the inspector and changing the process mode just for this is quite cumbersome and complex.

@Snowdrama
Copy link

I think this is a good feature usability wise, but to be honest, the process mode and visibility properties together achieve a similar effect and its good that they are separate to me.

I 100% understand why they're separate as well, and I wouldn't want to change that, so the intention wasn't to affect that directly. Instead the idea behind this for me was to improve the experience around when it's desired to set and unset both without removing the object from the tree. It's totally valid to want to change one or the other, and we intentionally don't change that, instead opting for a single override for when we desire it to change both, that continues to respect the state of those separate values.

At first glance, I expect this feature to be a shortcut to set the Node’s process_mode to disabled and also turn off visibility in one click… So from a usability perspective, I would be confused about why this new feature isn’t modifying these properties. (Please correct me if I’m wrong.)

I wanted to make sure we continued to respect that separation, so in the implementation the active state acts as an override that changes both without changing the underlying state. When returning a node to the active state it still respects the state it was in before becoming inactive.

So for me I wanted to avoid any kind of "hidden" or "magic" changes.

my_node.visible = false
my_node.node_active = false
my_node.node_active = true
# we never modified the underlying visible flag, so the expectation is that visible should still be true
# setting node_active to true doesn't magically change the state of the underlying visible property
print(my_node.visible) # false

Keep in mind the goal is that this does correctly call things like visibility_changed as well and is consistent with some other visibility quirks when polling for the visibility state, and needing to use is_visible_in_tree() instead of polling the visibility property itself.

# In both cases the value of the property is true, but it's not visible 
# due to something other than the property itself modifying it's visibility
my_parent_node.visible = false
my_child_node.visible = true
print(my_child_node.visible) # true
print(my_child_node.is_visible_in_tree()) # false

my_node.visible = true
my_node.node_active = false
print(my_node.visible) # true 
print(my_node.is_visible_in_tree()) # false

I'm also in favor of process toggle, but to make it a proper replacement for activity, it needs some improvements. If disabling a node does not yield expected results (like in camera or colliders), it needs to be fixed to properly handle disabling.

If I disable a collider by changing its process mode, will it stop colliding?

Disabling active of a collider won't stop colliding either, or at least nothing in the code indicates so.

From my perspective the this implementation is an improvement here. The name process_mode is ambiguous enough to be confusing, the intuitive answer is stops calling _process and _physics_process since process is right in the name. However this is actually apparently not the case. Currently colliders do in fact get disabled when process_mode is disabled, but cameras don't?

This attempts to solve this by having a unified definition that a node that is not active should not have any affect including physics, collision, cameras, scripts, viability, nothing. So this acts as an override for all of those at once, while also continuing to respect the individual choices for if/when it gets re-enabled.

@reduz
Copy link
Member

reduz commented Jul 10, 2024

@Snowdrama Yes, as I said, I think a simpler version of this PR that just disables processing and exposes this in the scene tree dock, while leaving visuals alone makes more sense.

from code you could use it like:

my_node.visible = false
my_node.process_enabled = false # property needs to be named like this for proper grouping

And be it.

(And likewise I think NOTIFICATION_ACTIVATED may make sense).

@passivestar
Copy link
Contributor

@reduz

One possibility may be to make something like NOTIFICATION_ACTIVATED that is called when the node is enabled, so you can use it with NOTIFICATION_DISABLED and it would work as you suggest

I'm sorry again, but did you mean to say NOTIFICATION_DEACTIVATED?

NOTIFICATION_DISABLED as I understand isn't called when a node leaves the tree. If there's a new pair of callbacks that fire both when a node enters/leaves and getting enabled/disabled, that should solve the biggest problem with the current lifecycle. I'll just put all my init/uninit logic into those and encourage everyone to prefer them over ready and enter/exit_tree where possible so that we could have a more predictable behavior in shared assets

If we also address the inconsistencies of how process works for some built-in nodes that @KoBeWi mentioned (like cameras), and add a ConnectFlag for an option to prevent signals from calling methods on disabled nodes then I believe this will cover it all

@RobProductions
Copy link
Contributor Author

RobProductions commented Jul 10, 2024

To clear a few things up:

At first glance, I expect this feature to be a shortcut to set the Node’s process_mode to disabled and also turn off visibility in one click… So from a usability perspective, I would be confused about why this new feature isn’t modifying these properties.

node_active is an override, so regardless of the state of those 2 properties, an inactive node will never render or process. The shortcut aspect is just point 1. of the purpose behind this PR though indeed for me personally it is the most important.

Disabling active of a collider won't stop colliding either, or at least nothing in the code indicates so.

It does actually! Collision is disabled when process_mode is disabled and as a result an inactive node will not collide either. I demonstrated this here.

I think a simpler version of this PR that just disables processing and exposes this in the scene tree dock, while leaving visuals alone makes more sense.

But in that approach we lose point 1. which is to act as a shortcut for both properties. I know one extra click in the scene tree isn't exactly the end of the world but conceptually it seems a lot easier for users to just have to worry about node_active and basically never have to touch visibility. As @passivestar mentioned above I would have to think the more common use case is to disable a node completely and the fact that people are used to the visibility toggle is just a consequence of it being a part of Godot for so long. I wouldn't want to eliminate that workflow but as discussed in godotengine/godot-proposals#7715 , we believe that in the long term this new property will simplify things if people use this in place of the other two.

Totally get that this is a big change and I don't want to presume any specific "correct" approach, but this was the way that made most sense while leaving everything else intact and adding some extra functionality for users to boot. If we were to go with the process override instead, it would definitely be an improvement to current situation in terms of workflow, but I feel like we'd miss out on a chance to rethink the system and make it even easier for newcomers. I also believe the concept of a universal disable is useful in its own right and makes the most sense for new lifecycle functionality. But those are just my thoughts and a reflection of the discussion that was happening in the proposal :)

@Snowdrama
Copy link

Disabling processing effectively disables it in practice, node does nothing. No processing, no input, no physics (which you can decide whether you want it to make it static or just remove it from processing)
...
I think a simpler version of this PR that just disables processing and exposes this in the scene tree dock, while leaving visuals alone makes more sense.

To be fair as long as the lifecycle callbacks align with what we have here It would probably be fine to just have a toggle button for process mode.

That said I think this is still a good compromise between a universal toggle, and continuing to respect the individual separate toggles for more granularity

The one thing a process_mode toggle button in the scene view doesn't do, which I still think is valuable is visibility at the Node class level which this does. One of the best Godot features is that the Node class doesn't have a transform, but I have run into cases where the root of my scene is intentionally a Node because I don't want the scene have a transform/offset and I want to be able to load but fully disable the scene and all it's children, however since the root is a Node type, it's not possible. We could move visibility to Node, even though Node doesn't have any visuals, the fact that visibility affects children, would make it valuable even on the root node, and would benefit from being able to toggle visibility of a Node that we only know is of type Node and not need to check and cast to a Node2D or Node3D to toggle visibility.

If we also address the inconsistencies of how process works for some built-in nodes that KoBeWi mentioned (like cameras), and add a ConnectFlag for an option to prevent signals from calling methods on disabled nodes then I believe this will cover it all

Yeah I think any rendering-tangential nodes would probably need to be looked at how they are affected by process_mode like WorldEnvironment

As passivestar mentioned above I would have to think the more common use case is to disable a node completely and the fact that people are used to the visibility toggle is just a consequence of it being a part of Godot for so long.

It's anecdotal but when I posted about this PR on social media, the first response I got was "I always assumed the 'show/hide' status was basically it" and "I always assumed Show/Hide did just what Active does in Unity." But if you just search for things like "godot disable" you find a bunch of posts asking "how do I fully disable something in godot" and with that many that there must be at least a significant portion of users who expect there to be a straightforward checkbox to just "turn off" a node.

@RobProductions
Copy link
Contributor Author

RobProductions commented Jul 10, 2024

The one thing a process_mode toggle button in the scene view doesn't do, which I still think is valuable is visibility at the Node class level which this does.

Apologies for the double post but I really wanted to highlight this as well, there can often be "breaks" in the scene tree with plain Nodes instead of Node3D which can make it tricky to disable whole groups of nodes and you may need to add transform data (i.e. memory overhead) just to get a parent that has a visibility toggle.

We could move visibility to Node, even though Node doesn't have any visuals, the fact that visibility affects children, would make it valuable even on the root node, and would benefit from being able to toggle visibility of a Node that we only know is of type Node and not need to check and cast to a Node2D or Node3D to toggle visibility.

This would be one solution to the above but at the cost of breaking compatibility with the existing API. Hence in addition to the reasons above & your comment about visibility perception, there should be no ambiguity with this new property :)

@reduz
Copy link
Member

reduz commented Jul 10, 2024

@RobProductions

But in that approach we lose point 1. which is to act as a shortcut for both properties. I know one extra click in the scene tree isn't exactly the end of the world but conceptually it seems a lot easier for users to just have to worry about node_active and basically never have to touch visibility.

Yes, I already expressed disagreement on this and explained on my comments why I don't like this idea.

I think having them separate makes more sense on how Godot works.

@reduz
Copy link
Member

reduz commented Jul 11, 2024

@Snowdrama

As mentioned before I also think it's fine if we want to treat them fully separately, and then have toggles for both, but I'd want to see a change that adds the separate toggles for things to the base Node due to the state of the parent's toggle affecting children, then we wouldn't need to have a single on/off toggle at the root.

The nodes already have a process mode and this determines whether it is inherited or not towards children. The extra bool I mentioned would simply disable the node, so if children are set to inherit they will be disabled, otherwise they can be set to enable and unless you disable they will remain disabled.

By default they inherit, so it already works the way you mention.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 11, 2024

This change would be for cases where the base Node is used as an organizational tool, but we still want to toggle something from the root.

Just use Node3D for organizing. Or Node2D in case of 2D scenes.

@adamscott
Copy link
Member

the process mode and visibility properties together achieve a similar effect and its good that they are separate to me. Adding a yet third state to coordinate both seems unnecessary and adding more confusion to a system that you have to learn anyway and it won't go away just because you add this state.

I don't agree, @reduz. There's a lot of behavior that occurs outside the process mode and the visibility properties. How many times there's something that I just want to deactivate, to toggle without having to delete an entire node leaf.

@zydeas
Copy link

zydeas commented Jul 11, 2024

Having visibility and processing separately makes more sense in code than it does in the scene dock. More often than not you'll want to disable game entities completely, not just their visibility or their processing. More often than not I'll have to click both buttons

Actually you would be surprised that most of the time you don't want to do both and this is not a problem Godot users face often (as you can see it's been a decade, so it shouldn't surprise you this is not very requested).

Just because people aren't making a massive amount of noise about it doesn't mean it isn't an issue, or something that can be improved upon. I ran against it the first time I tried to seriously use Godot (finished making an enemy for my prototype, went "okay, let's move onto the next feature", set it to invisible, and I was then very confused and annoyed when the now invisible enemy was attacking me). This interrupted my flow state, as I now had to actually think about what to do about this problem, as opposed to just checking a single checkbox and forgetting it. I'm still frustrated that Godot doesn't behave this way regularly, and whenever it happens it makes me not want to use it. I have a very strong feeling that this might be more common of an experience than you think.

I'm trying to think of a time where I was making game content in the editor (not coding) and actually wanted to set the process and visibility mode as separate things. 99% of the time I want to set the node into one of two states:

  • Completely active.
  • Not active (Nothing runs, including any startup lifecycle callbacks)

If I need to switch to one of the other process modes (pausing for example), then that's done at runtime. I like that I have the option to control process/visibility separately at runtime for the cases where I'd need it, but a editor single checkbox solves almost any "game editor" use case that I've encountered in past decade that I've been a game developer. Making me click twice to fully disable a node instead of once just adds more work, which may sound ridiculous till you extrapolate out the number of times the users will be performing the action across the years that games take to develop (I personally enable/disable things very often and I think a lot of other users will too, given the option). There are absolutely valid design critiques of the Unity "active state" system, but I'm also genuinely struggling to think of a time where that system has caused me any actual massive headache.

It also wouldn't stop _ready() and _enter_tree() from being run. Passivestar had a very good point there and unless I'm mistaken, I don't think Reduz addressed it at all. Being able to easily stop these from happening is actually very useful, because otherwise you have to manually check for the process mode inside any given script where you'd want this to happen, which is just not reasonable. And again, you can't control what plugins do without modifying their code.

As far as being confusing to users, I'm pretty sure you could explain it with a one or two diagrams that outlines how the three variables effect the node's state, and how lifecycle callbacks get executed. It really does not come off as complicated to me from the PR description.

So far in this thread I haven't seen any concrete details on the specific technical issues that'll result from adding this one extra layer of state to the node system. It's not an exceedingly complex mechanism and seems fairly simple in implementation.

Reduz's core dispute seems to be that this conflicts with a core aspect of the engine's design (Process/Rendering are supposed to be kept separate). I don't personally see how adding one toggle that overrides both of these conflicts with that, but if that's the case then... yes. By making and supporting this PR, the users are implicitly saying that an aspect of the engine design does not actually account for a specific behavior that users want, and that this aspect of design should be expanded in a minor way to account for that said behavior. I think that's pretty reasonable.

@eisegesis-623
Copy link

eisegesis-623 commented Jul 11, 2024

Just adding support as another non-Unity user (and a less technically-inclined Godot user). Having a simple toggle that provides the peace of mind that a node and its children are fully disabled, including _ready() and _enter_tree() and the like, would be a lifesaver.

As of right now, it's needlessly difficult to implement (as far as I've been able to tell at least, so forgive me if I've missed something obvious). Creating a component to auto-delete its parent at runtime ends up having to worry about the scene tree order. Having to add code and checks to the _ready() or _enter_tree() methods of every single node that might need to be disabled is a huge waste of time, cluttering nodes that already have scripts and adding scripts to nodes that don't need them. A simple checkbox would be a wonderful solution, but I don't care what it is--I'd be fine digging through menus to get to it as long as it provides the certainty that the node is as good as nonexistent until I specifically ask for it to start existing sometime at runtime, and as long as it is considered the default, universal way to do it in Godot. (And that's generally ignoring the potential of its applicability to disabling @tool scripts and the like.)

Regarding the separation of Process/Rendering, I'm curious, how does adding a toggle that affects both fundamentally break this rule? There are already situations where changing one toggle in a node will automatically disable others iirc, so how does this do anything but simplify the user's interaction with those systems while providing additional control beyond that?

@RobProductions
Copy link
Contributor Author

Just use Node3D for organizing. Or Node2D in case of 2D scenes.

Yes, but that's why I brought up:

you may need to add transform data (i.e. memory overhead) just to get a parent that has a visibility toggle

Not a huge increase in memory I imagine but it's possible this could add up in some projects and gives people the opportunity to optimize further. This is just a small technical advantage but conceptually/UX-wise I believe it can have a much bigger impact as we're hearing from the users chiming in on the thread (thank you!) and will ensure node type isn't even a consideration for this scenario...

Having a simple toggle that provides the peace of mind that a node and its children are fully disabled, including _ready() and _enter_tree() and the like, would be a lifesaver.

I want to clear up potential confusion on this, the toggle will not interfere with the way _ready() or _enter_tree() works. Instead the idea is that if you'd like to care about node_active, you can hook into 2 new lifecycle methods called _node_active() and _node_inactive() which are called when the node becomes active or inactive, including when it enters/exits the scene tree. So you can use these for initialization/cleanup instead of the other functions which remain the same as before. As we learned in the proposal, messing with ready() would have too many bad consequences so this solution was much better as it does not disrupt any existing workflow :)

@eisegesis-623
Copy link

I want to clear up potential confusion on this, the toggle will not interfere with the way _ready() or _enter_tree() works.

Ah, my bad. Based on various statements made during the discussion I thought that was implemented here. Regardless, being able to move the code from those methods to one that respects an in-editor toggle is a good enough solution to the issue, just one that requires some re-writing of a project to make fullest use of.

@mrawlingst
Copy link
Contributor

This is just a small style/nitpick but I would probably suggest alternative style for the "disabled" nodes (when they are disabled) in the tree list.

In the list of nodes, there are already few cases where the nodes are grayed out (e.g. uneditable child of an inherited scene). I feel that if we changed the style for the disabled node, it would be easier to tell why this node is not normal.

Suggested style: red name with strikethrough - I believe this would help stand out especially in a large scene with huge number of nodes in the list.

Also add filter support to the filter input with @disabled or something similar to display only disabled nodes.

@RobProductions
Copy link
Contributor Author

@mrawlingst Good point, though I have to say red name and strikethrough is way too visually distracting for something like this that is meant to be used in place of previous toggles. I also believe that faded is already the best way to communicate it along with the (optional) scene tree checkboxes, though they could also receive some sort of faded treatment in certain situations like a child of inactive node for better clarity. The reason faded makes most sense to me is that is the exact same fade as process_mode being disabled, which is exactly what this toggle does and tries to convey. The same goes for the fade on the visibility toggle. It actually took no extra code to create the effect, just had to update it at the right time and that's because the specific fade colors are already associated with can_process being false and visible_in_tree being false. Hopefully that makes sense why UI-wise I think people will find this most familiar to the existing style, but I'm open to trying out other ideas if there are specific justifications!

For your note about filtering I like that idea a lot! I will look into that :)

@AThousandShips
Copy link
Member

Since there seems to be a lot of confusion about what this PR does and doesn't do (and what the proposal proposed and expectations from that) I think it'd be good to clear up those questions, like:

  • How does this affect different methods, notifications, and signals, like _ready, _process, etc.
  • How does this relate to the original proposal idea, which was to essentially be equivalent to removing the enode while not having to remove it, i.e. stopping all processing, both that which is affected by process_mode and that which isn't
  • How does this behave when loading a scene, does it prevent a node from entering the scene and interacting if disabled from the start?

@RobProductions
Copy link
Contributor Author

RobProductions commented Jul 11, 2024

How does this affect different methods, notifications, and signals, like _ready, _process, etc.

While a node is inactive, it will:

  • Still receive _ready, _enter_tree, and _exit_tree like normal
  • Not be "visible in tree", i.e. the visibility toggle is overridden to false
  • Act as though process_mode is set to disabled, which means it will...
  • Not run _process, _physics_process, or _input
  • Still receive all signals and notifications like normal
  • Still allow external scripts to run code in a script on this node directly, i.e. via direct function call
  • Not act as valid camera in 2D or 3D
  • Not collide with anything

Ultimately anything that was previously affected by process_mode or visible is now affected by this. The only special case I accounted for was cameras because they were previously not affected by process_mode. Though the CanvasLayer bug that @passivestar found has yet to be accounted for as well; I will fix that up shortly :)

The moment a node's active state changes, it will run:

  • NOTIFICATION_VISIBILITY_CHANGED notification
  • visibility_changed signal
  • NOTIFICATION_PAUSED or NOTIFICATION_UNPAUSED notification
  • NOTIFICATION_DISABLED or NOTIFICATION_ENABLED notification
  • NOTIFICATION_NODE_ACTIVE_CHANGED notification
  • node_active_changed signal
  • node_active_in_tree_changed signal
  • And the _node_active and _node_inactive lifecycle callbacks and their corresponding notifications

How does this relate to the original proposal idea, which was to essentially be equivalent to removing the enode while not having to remove it, i.e. stopping all processing, both that which is affected by process_mode and that which isn't

The proposal topic was aimed at creating 2 improvements:

  1. Creating a faster/more convenient way to disable the visibility/processing of a node
  2. Creating a more universal disable system that stops "all" node impact (some things cannot be accounted for) that is simple to use. Example quote from the original proposal description: "In the inspector view there will be a disable/deactivate checkbox. If this is checked the node becomes faded or another color in the scene view."

With the new property, features that were previously toggled via process_mode are now also turned off by node_active, and other features that we expect to be disabled when a node is "inactive" can now poll this property to disable themselves. The first example of this is the camera behavior. If there are other features that need the same treatment they can be added here or in a future PR, but crucially this aims to lay the groundwork for a more unified system that satisfies both points in the proposal.

How does this behave when loading a scene, does it prevent a node from entering the scene and interacting if disabled from the start?

You can still reparent, instantiate, and manipulate an inactive node without issue. A Node that enters the tree while inactive will still run _enter_tree and _ready as before but since it acts as though process_mode is disabled, it will not process/run code itself unless a signal or external script triggers something in it. As I mentioned above, future PRs could tie in signals to this system as well with a parameter or something like that but for now this is a good first step. Hopefully between this and the original PR description that communicates everything that this aims to do! Happy to clarify anything else if needed!

@Norrox
Copy link
Contributor

Norrox commented Jul 11, 2024

@RobProductions

what if there is one "checkbox" and that box can toggle between 4 states? ( not in this order but you get the idea )

first nothing is disabled
image

second visibility false
image

third everything disabled
image

fourth all process modes disabled
image

@RobProductions
Copy link
Contributor Author

what if there is one "checkbox" and that box can toggle between 4 states ( not in this order but you get the idea )

Hi, we have discussed concepts like that heavily in the proposal and ultimately people found it too convoluted to use a multi-state icon for something like this. I'd have to agree, for both clarity and speed purposes, that it should be on or off for each property. For concerns about too many buttons in the scene tree, that's why there exists a setting to enable them and it is defaulted to false so that nobody will be bombarded with checkboxes when they first load it up :)

Some people in the proposal actually also preferred to see more buttons in the scene tree for operations like this as they found it useful. In the future we should look towards @Snowdrama 's idea of creating Blender-style icon filters to more easily manage user preferences:

336996963-0d53d514-9497-418c-8168-9ce9f12082f0

@Norrox
Copy link
Contributor

Norrox commented Jul 11, 2024

@RobProductions

what if there is one "checkbox" and that box can toggle between 4 states ( not in this order but you get the idea )

Hi, we have discussed concepts like that heavily in the proposal and ultimately people found it too convoluted to use a multi-state icon for something like this. I'd have to agree, for both clarity and speed purposes, that it should be on or off for each property. For concerns about too many buttons in the scene tree, that's why there exists a setting to enable them and it is defaulted to false so that nobody will be bombarded with checkboxes when they first load it up :)

Some people in the proposal actually also preferred to see more buttons in the scene tree for operations like this as they found it useful. In the future we should look towards @Snowdrama 's idea of creating Blender-style icon filters to more easily manage user preferences:

336996963-0d53d514-9497-418c-8168-9ce9f12082f0

Alright, fair enought :D

What about the "right click" menu?
image

@Snowdrama
Copy link

Snowdrama commented Jul 11, 2024

Some people in the proposal actually also preferred to see more buttons in the scene tree for operations like this as they found it useful.

In the proposal I did a mock up and I just recently updated the mock up using the gear icon that was posted in in reduz's first post about processing disable.

image

Edit: Removed the mocks with the buttons on the left side to prevent confusion.

But yeah I still think a good idea eventually might be to add Blender style toggles to the top to let the user choose if they want to see icons. Don't currently care about processing icons, You don't have to see them.

What about the "right click" menu?

Definitely a good idea for sure!

@bappitybup
Copy link

bappitybup commented Jul 12, 2024

Some people in the proposal actually also preferred to see more buttons in the scene tree for operations like this as they found it useful.

In the proposal I did a mock up and I just recently updated the mock up using the gear icon that was posted in in reduz's first post about processing disable. image

I also did a variant that moved these to the left side of the window that could be done in a future PR if it was something that people were interested in I know currently I don't think that's possible but I think it looks good. image

Both of these also include a small transparency/fade change experiment that makes the icons of "inherited" disabled nodes darker to set them apart from the node that actually disabled them.

I also mocked up an icon change for disabled nodes similar to how the eye closes when clicked. The icon only has a has a single gear vs two gears(Symbolically the gear isn't being driven by another gear so it's not "processing") image

But yeah I still think a good idea eventually might be to add Blender style toggles to the top to let the user choose if they want to see icons. Don't currently care about processing icons, You don't have to see them.

What about the "right click" menu?

Definitely a good idea for sure!

left side for the cog icon does look really nice in my opinion and makes more sense to me. the way I see it is the right side icons are mainly references of that node in other bits of code and left side are direct controls of that node 👍

@RobProductions
Copy link
Contributor Author

The icon ideas here are neat! But definitely out of scope for this PR; as I mentioned in the proposal discussion in order to put icons on the left it would require some sort of "leading button" feature for the scene tree that would likely be a PR on its own. Since there is no visibility option in the right click menu currently, that also seems like its own PR for the future. I also want to keep clarity and the intent of the new property intact and not confuse it with processing, so would prefer to leave it as a checkbox, though I'd be happy to investigate different stylings for it :)

I made 2 changes to the code: CanvasLayer is now working with node_active, see here with active on:

image

And with active off:

image

Though since it is implemented differently than Node3D (CanvasLayers by design are not supposed to cascade visibility from parent CanvasLayers) it's a little tricky to mesh that with the existing visibility field and you get a visual issue here where the eye thinks visible is false. Will take a bit more tinkering to get the visual quirk eliminated, the best way is probably to implement "visible in tree" equivalent here but without the parent cascade part. (The scene tree literally searches for that binding name to use the fade colors so it seems the most straightforward option)

Second change is stopping _node_active() from being called twice on initialization. If you set to inactive and then back to active in _ready it previously didn't know that it already sent the active callback, so it would do it again when the scene loads. With this change, that's no longer an issue since it now tracks when it has fired off the method! And lastly, if it was missed before I updated is_node_active_in_tree to use the "CanvasItem-like bool implementation" so that there is no more recursion 😄

@Snowdrama
Copy link

Snowdrama commented Jul 12, 2024

The icon ideas here are neat! But definitely out of scope for this PR; as I mentioned in the proposal discussion in order to put icons on the left it would require some sort of "leading button" feature for the scene tree that would likely be a PR on its own.

100% I knew that the left side ones wouldn't happen in this for sure, the main one being the right side. For now I've deleted the left side mocks, in an attempt to not confuse things.

I also want to keep clarity and the intent of the new property intact and not confuse it with processing, so would prefer to leave it as a checkbox, though I'd be happy to investigate different stylings for it :)

My main reason for posting the mock was that it seemed that the only real pushback for node_active was that the checkbox combined visibility and processing together(I don't mind it being a single checkbox personally but as long as both buttons together work the same as the one toggle I don't think I'd mind it being split either, as long as it's implemented at the base Node class)

To make up for me making things confusing haha, here's a mock using checkboxes on the right, re-styled to use similar iconography as the existing icons including fading the children, mimicking visibility
image

@L4Vo5
Copy link
Contributor

L4Vo5 commented Jul 13, 2024

A simple checkbox would be a wonderful solution, but I don't care what it is--I'd be fine digging through menus to get to it as long as it provides the certainty that the node is as good as nonexistent until I specifically ask for it to start existing sometime at runtime, and as long as it is considered the default, universal way to do it in Godot. (And that's generally ignoring the potential of its applicability to disabling @tool scripts and the like.)

I wonder if it would be valuable for this toggle to take the node out of the tree completely. Keeping it on an internal list owned by the node's parent so it can be easily re-added/deleted when the owner is deleted.
Currently taking a node out of the scene is a way to 100% disable them without a doubt, but you can only do it with cumbersome custom code and great care to avoid memory leaks. Providing a shortcut for it would achieve the effect this PR is going for (I think) while having less collision with the current process/visibility system.

@zydeas
Copy link

zydeas commented Jul 14, 2024

I wonder if it would be valuable for this toggle to take the node out of the tree completely.

Personally, this really wouldn't be satisfactory for the feature set I'd want. Part of the whole idea behind this is that the node still exists in the tree, and if I really wanted to I could walk the hierarchy to find them and activate them at runtime.

@reduz
Copy link
Member

reduz commented Jul 15, 2024

After discussing with other contributors and community members, I opened a proposal with what is in my view a cleaner (more in line with how the engine works internally) solution to the original problem.

godotengine/godot-proposals#10212

To me the bottom line of the discussion is that, if you want to disable nodes in the cleanest way possible, It's just not possible to do this in Godot while they remain in the scene tree, because the engine simply does not expect this to happen at several levels, so as suggested by @KoBeWi above, the proposal describes a different way to achieve exactly the same.

Added node active property to the base Node class which acts as a universal "disable node" feature. When a node is inactive, it is not visible and cannot process scripts, physics, or be a valid camera. Much like visibility, if an ancestor node is inactive, this node is as well.

There are new signals and methods for querying the node active state such as `is_node_active_in_tree()` to match the visibility features. There's a new option in the settings to turn on checkboxes in the scene tree for easier access of the node active state.

Finally, there are two new virtual methods that are accessible via user which can act as lifecycle calls. `_node_active()` can be used for initialization and is called after `_ready()` has completed if the node is active. It is also called if a node becomes active in tree. `_node_inactive()` can be used for cleanup and is called before a node exits the tree or if it becomes inactive in tree.
@RobProductions RobProductions requested review from a team as code owners November 1, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.