godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.07k stars 69 forks source link

Add a shortcut to disable/deactivate a node in the editor #7715

Open JaimieVos opened 9 months ago

JaimieVos commented 9 months ago

Describe the project you are working on

When developing a game sometimes I want to set a node and it's children to an inactive state so I can test better without having to delete the entire node and losing track of what I removed.

Describe the problem or limitation you are having in your project

I cannot disable/deactivate nodes in the editor right now. This causes me to lose track of which nodes I deleted when I am testing something and I want a node to be gone for testing.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

If I can disable/deactivate a node I can visually still see the node in the scene view. Then I have a clear view of which nodes are active and which are not.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

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.

If this enhancement will not be used often, can it be worked around with a few lines of script?

I think it will be used quite often.

Is there a reason why this should be core and not an add-on in the asset library?

I think it is quite an essential part of being able to test.

RobProductions commented 2 weeks ago

Since it seems like we're settling in on a solution here, I've tried my hand at implementing these ideas and wanted to share my progress so far. Would appreciate your feedback/ideas so that I can account for everyone's concerns before I submit my PR and during the review process :)

First up, all nodes now have a property called "Active State" which overrides both visibility and process_mode so that you can quickly disable the entire node's impact with one field:

image

You can set the active state via code at runtime or in the editor! Then, I added a quick action icon to the hierarchy so that you can toggle active state on and off easily; it affects children nodes as well of course. Here's that in action:

https://github.com/godotengine/godot-proposals/assets/11141862/cc06ce34-a7e4-4ff5-8887-cdfca0d6b13c

To quell any concerns about clutter, I'm planning on adding an option here in the "Scene Tree" editor settings to enable/disable the checkmarks (default can be disabled so that there is no confusion over the new feature):

image

I did really want to implement @Snowdrama 's idea of having the checkmarks on the left side of the node, however it seems much more complicated to do so because: the tree.cpp file which controls the hierarchy doesn't use Nodes/buttons for the little icons, it renders out images based on offsets and uses a sort of click detection to tell what was pressed. There is a "checkbox" mode for tree items but they replace the node icons which isn't good. To implement it based on your image we would need some of "leading button" feature for tree.cpp that would probably be its own rabbit hole. Or if that feature already exists I can't find it currently and would appreciate some guidance there. Hopefully it's alright to leave that for a future PR.

The idea to use Blender-style icon filtering is also great but is probably best left up to its own PR as well, hence #9951 . For now I hope it is okay to rely on the editor setting mentioned above instead of that as I figured this base functionality is more crucial to have early on.

As a bit of a summary, here are the things I'm planning to do before the PR:

Let me know if those sound good and here is one last point I want to mention as I continue refining my changes:

The name "is_active()" was actually already taken by some classes such as the Animation Mixer etc. so I had to rename the getter to "is_node_active()" though it is not as ideal in terms of simplicity. Is there a better naming scheme for this feature as a whole? I'm cool with whatever name seems right but do note that "is_enabled()" is also already used in the node so this seemed like the only clear solution. Also, is_node_active() refers to whether it or a parent node is active, and I have is_node_active_self() to tell whether or not the node individually is active. Should this instead be is_node_active() and is_node_active_in_tree() to match the visibility functions found in Node3D?

Thanks, and I hope it's heading in a good direction for everyone :)

passivestar commented 2 weeks ago

Thanks, and I hope it's heading in a good direction for everyone :)

@RobProductions it is the right direction from the UI/UX perspective but if I read your post correctly it doesn't address the bigger problem - hiding a node and disabling its process doesn't fully deactivate it. Lifecycle methods are still called on it, which isn't how people expect inactive nodes to work and it's just not always very helpful

So it's a step in the right direction but the inactivity system needs to do more than disable visibility and processing. And it needs to be its own separate system from editor-only that @torcado194 has been cooking because there's value in being able to prevent lifecycle hooks of editor tools to fire when they enter the tree

Lifecycle hooks firing on nodes with processing disabled wouldn't have been a big problem if Godot made it easy to execute logic on enable/disable, but currently Godot isn't endorsing that in any way, those don't have their own methods and the only way to react to those lifecycle events is through _notification. I think inactivity in Godot needs a proper design phase beyond the UI/UX discussion that has been happening in this thread. Going with a half-measure on this will only pollute the namespace even more with methods that don't do what users expect, making it harder to solve later

OhiraKyou commented 2 weeks ago

I'm realizing a single bool for active state will probably be less complicated for users than an enum so unless there's potential for other "active states" besides enabled and disabled, I will simplify it to that. I just started with that because it was easier to match process_mode initially.

Yeah, I favor a Boolean over an enum. Enums encourage additional states, which I never want to see for an active/enabled toggle; those should not be unwieldy, convoluted, or conflated with other states.

RobProductions commented 2 weeks ago

@RobProductions it is the right direction from the UI/UX perspective but if I read your post correctly it doesn't address the bigger problem - hiding a node and disabling its process doesn't fully deactivate it. Lifecycle methods are still called on it, which isn't how people expect inactive nodes to work and it's just not always very helpful

That's a good point, I've heard about this before and I think it should be possible and fit in perfectly for this feature. I'll research that as part of my next steps and try to get that working, just might take a bit to get it running and tested properly. Is there anything else related to Node functionality/visibility that might not be caught already?

it needs to be its own separate system from editor-only that @torcado194 has been cooking because there's value in being able to prevent lifecycle hooks of editor tools to fire when they enter the tree

Yup if I understand @torcado194 's implementation correctly it should mesh well with that PR and work distinctly from it. The key difference in this implementation is to keep the node alive in memory so that references don't break but still remove their impact for debugging/performance purposes, working in both editor and runtime.

I think inactivity in Godot needs a proper design phase beyond the UI/UX discussion that has been happening in this thread. Going with a half-measure on this will only pollute the namespace even more with methods that don't do what users expect, making it harder to solve later

I feel that, and though this does add something new to keep track of, I think this concept is pretty simple to get and when encouraged correctly it can simplify workflows instead of complicating them. i.e. You only need to consider active state and it is potentially an easier concept to grasp for newcomers instead of distinguishing visibility and process_mode, which can be tucked away as more advanced features in the future. Without more details or a known timeline on design though this seems like a good first step to something happening at least(?)

If anything based on what I'm hearing from this thread, it's that this "universal active state" should be the one driving processing, lifecycle notifications, and visibility without needing separate fields for all of those mechanisms. But before we can cut all of that out we need a transition period so to speak where the new property is added and understood by the community. Though personally I like the granular control of having all 3 options available :)

passivestar commented 2 weeks ago

Though personally I like the granular control of having all 3 options available :)

I never said any existing options should be removed

Yup if I understand @torcado194 's implementation correctly it should mesh well with that PR and work distinctly from it

Yeah that's what it looks like to me as well

Is there anything else related to Node functionality/visibility that might not be caught already?

tbh nothing else comes to mind atm, as long as inactive means that no code can be executed on a node including all notifications, input callbacks, etc I'd personally be happy with that

zargy commented 2 weeks ago

See, would "notifications" include signals as well, or just certain ones like _process, _ready, _input ect. Because while I can't think of a specific example right this second, there are probably a few situations where I'd like signals to still fire on inactive nodes.

RobProductions commented 2 weeks ago

I never said any existing options should be removed

I know, just stating that in response to some people wanting everything to be condensed to a single checkbox

as long as inactive means that no code can be executed on a node including all notifications, input callbacks, etc I'd personally be happy with that

there are probably a few situations where I'd like signals to still fire on inactive nodes.

Sorry to drag out the convo but one important question: is it alright if another script referencing a disabled node is able to directly call a function on it? I think that might be what @zargy means. I believe something like this example is possible and will probably be hard to disable:

image

I'd also vouch for signals being useful to keep around on inactive nodes since you opt in to subscribing in the first place

zargy commented 2 weeks ago

is it alright if another script referencing a disabled node is able to directly call a function on it?

Yes, that would be my expectation when encountering a feature like this. To me, disabling a node effectively just means that it acts like it doesn't exist and doesn't do anything on its own, but there's no reason why I shouldn't be able to call functions on it from elsewhere, and that would include connected signals. A limitation like that doesn't exist in Unity with a GameObject's active state/component's enabled state. And if it's important that a specific node type is active when you call a function on it... just check to see if it's active in either the calling or receiving function.

passivestar commented 2 weeks ago

I don't think anything can be done about direct function calls but I'd really love an example of when you'd expect an inactive node to respond to a signal to better understand your usecase

My example of where I DON'T want it to happen is: There's a button and a door. Pressing a button triggers its "pressed" signal that's connected to the door's "open" function. If the door is inactive pressing the button should never execute any code on it. Opening the door when it's inactive will change its state which inactive objects shouldn't do, not to mention that the door may trigger its own "opened" signal when opened, potentially triggering some behaviors on other interested nodes. That's not how an inactive door should behave imo

zargy commented 2 weeks ago

I see what you mean by your example. Here's an alternate one that I can come up with off the top of my head. What if your described level is really big and complicated and you want to completely deactivate chunks of it depending on where the player is to save on any potential performance (let's say you have enough complex stuff in your game that it actually does matter). And the button that happens to open the door is way across the map and in a place where the player can't see (assume that, for whatever reason, you don't actually show the door opening to the player. Maybe you have an onscreen message telling them a door opened and they have to find it).

So the door definitely needs to open when the player presses this button, but it also definitely won't be active when the player is standing by the button. In a case like that, I think you'd definitely want to be able to say "this signal should fire on this target thing, even if it's inactive". (Of course, you could just not have a signal and set up an actual function on the door for making it open, but that bypasses the whole point of signals.)

Aside from that, I'll add here: I think it may be a good idea to have the default be "inactive nodes can't receive them", but provide a way for the coder to override that. Either a toggle on the signal itself, a toggle on the whole node that lets every signal through (I'd add an argument to EmitSignal but that already uses a variable number of args for signal parameters).

passivestar commented 2 weeks ago

yeah that's an interesting example. but it seems like the existing toggles (visibility and processing) already solve your case perfectly:

Hope it makes sense

Snowdrama commented 2 weeks ago

tbh nothing else comes to mind atm, as long as inactive means that no code can be executed on a node including all notifications, input callbacks, etc I'd personally be happy with that

I don't necessarily agree we should be preventing the lifecycle calls like _EnterTree, _ExitTree, and _Ready when a node is disabled. To me they signify specific things that are still valid EVEN if the node is disabled. They should continue to fire even if the node is disabled because they have actually entered the tree, and are "readied" due to being loaded as part of a scene.

This is where the additional lifecycle function come in like _OnEnabled _OnDisabled and the _OnActiveChanged

If you previously did something in _EnterTree or _Ready you would switch it to _OnEnabled then you could mark it as disabled and then at some other point make it active. The initialization can then happen in _OnEnabled if it truly needs to happen when it's enabled like maybe it sends a signal. But you also get to frontload work like if you were making a procedural dungeon, and you wanted to generate the chunk of the map when it enters the tree, as part of the scene loading, and not when it becomes active. It can start disabled, but still do work as part of it entering the tree.

If we are to change it so _EnterTree, _ExitTree, and _Ready are not called, we would need to have a discussion about when in the lifecycle they are called, as well as the implications of lying to the parent telling that the children are _Ready even if a child was never readied or even entered the tree. Since the parent can have a reference to it, the triggering of the _Ready function of the parent signals that the children are initialized, even if they aren't?

passivestar commented 2 weeks ago

This is where the additional lifecycle function come in like _OnEnabled _OnDisabled and the _OnActiveChanged

I fully support adding those, yes. I don't even mind if enter/exit tree are executed if enable/disable are easily available because I can just never put any logic into those old lifecycle methods and only use _on_enabled _on_disabled. It's important though that _on_enabled is actually called at appropriate time when the node is first created (I know it's the case in Unity but I haven't checked in Godot yet)

Snowdrama commented 2 weeks ago

It's important though that _on_enabled is actually called at appropriate time when the node is first created (I know it's the case in Unity but I haven't checked in Godot yet)

Yeah for sure the idea would be that if it enabled in the serialized scene, then during scene loading the _on_enabled function would be called as part of the scene load lifecycle. Whether it comes before or after _ready is a question to be had and probably needs more investigation.

Similarly _on_disabled would be called along side _exit_tree during the process of removing a node when it's freed with queue_free

both _on_enable and _on_disable would be called just like current functions like _ready and _enter_tree called automatically when the state changes using something like myNode.active = false or myNode.set_active(true) and the hope would be that similarly to visibility it would also have a signal to listen for the state changed, like visibility_changed we'd have an on_active_changed

RobProductions commented 2 weeks ago

both _on_enable and _on_disable would be called just like current functions like _ready and _enter_tree called automatically when the state changes using something like myNode.active = false or myNode.set_active(true) and the hope would be that similarly to visibility it would also have a signal to listen for the state changed, like visibility_changed we'd have an on_active_changed

I like this idea, and on_active_changed is something I already implemented actually as you can see here:

image

Though in hindsight it should be exposed by Node not Node3D, it was just necessary there at first to help propagate visibility stuff. Will fix that when I can. Is this "active state callback" approach the best option then? From what I'm seeing it should be possible to create these new lifecycle calls for GDScript and should be less disruptive to the existing calls, but I just wanted to double check that everyone's workflow is satisfied by that!

One problem though is that in terms of naming enabled and disabled are already used internally:

image

These notifications were originally run by process_mode changes so that more specific nodes could run behaviors when processing is disabled, but the new active state stuff also runs them for this same purpose now. However, that means if I run the GDScript callbacks when these notifications are sent they also will be run when process_mode changes. I think we lose something with that approach because it removes the ability to get more granular with "disabled" vs "don't process" states if that makes sense, so I'm leaning towards keeping them separate from the existing notifications.

So if we want these new lifecycle calls to only be triggered by active state changes, it would make sense to use a more unique name. I can go with something like _on_active and _on_inactive for now but are there any other ideas/preferred naming? Or maybe we do want these calls to be called by process_mode changes? Let me know!

As a reminder this feature I'm building is meant to do two things:

  1. Act as a shortcut for debugging/culling through API and Editor UI (which is what this proposal was originally aimed at)
  2. Add a cleaner/more universal way to completely disable a node's impact in the scene in both editor and runtime

The first point definitely has a greater impact on my specific workflow so I appreciate opinions on the second point and hope to shape it to match expectations there. We've got some great ideas so far I think :)

Snowdrama commented 2 weeks ago

I like this idea, and on_active_changed is something I already implemented actually as you can see here Though in hindsight it should be exposed by Node not Node3D, it was just necessary there at first to help propagate visibility stuff. Will fix that when I can.

Yeah sounds like you get the idea, I think on_active_changed would be on Node itself, but I could imagine having it change visibility on CanvasItem and Node3D could be a challenge so I get it haha

One problem though is that in terms of naming enabled and disabled are already used internally

Ooo I didn't know about that one, I'm just starting to dig into the engine side of things. So enable/disable are triggered when process is changed? Interesting.

I think we lose something with that approach because it removes the ability to get more granular with "disabled" vs "don't process" states if that makes sense, so I'm leaning towards keeping them separate from the existing notifications.

Absolutely, I do like the granularity around process_mode enabled/disabled vs a node's active/inactive state. I think the active/inactive triggering visiblility and process_mode callbacks is good for sure. I'd be curious to see how people think about what happens if I change the visibility or process_mode on an inactive node.

Does setting visibility/process_mode or anything like that would reactivate the node? Or we could throw an error like "Node is inactive, process_mode can't be changed while the node is inactive" like the active state overrides visibility, and process_mode.

Consider this:

@export var my_node : Node3D

func do_something():
    my_node.visible = true;
    my_node.node_active = false
    print("Is Node Visible?:", my_node.visible) # false? Did we change visible to false?

    my_node.visible = false;
    my_node.node_active = true
    print("Is Node Visible?:", my_node.visible) # what's the expectation here? is it forced visible?

If it's actually changing the value of visible under the hood, then we may be losing the state of the node before it was disabled leading to unintended effects when re-enabling.

I Personally think overriding the state makes the most sense to me as it preserves the state regardless of if it's active or inactive, but I want to see how other people feel. Here's an example of how that might look:

# my_node stays true while active, checking it while inactive returns false because it overrides the visibility state.
@export var my_node : Node3D
func do_something():
    my_node.visible = true
    print("Is Node Visible?:", my_node.visible) # true
    my_node.node_active = false
    print("Is Node Visible?:", my_node.visible) # false it's overridden
    my_node.node_active = true
    print("Is Node Visible?:", my_node.visible) # true because it was true before becoming inactive

# my_other_node stays false, setting it back to active doesn't change the visibility back to true
@export var my_other_node : Node3D
func do_something_else():
    my_other_node .visible = false
    print("Is Node Visible?:", my_other_node.visible) # false
    my_other_node.node_active = false
    print("Is Node Visible?:", my_other_node.visible) # false it's overridden
    my_other_node.node_active = true
    print("Is Node Visible?:", my_other_node.visible) # still false

Hopefully these examples help make what I'm getting at clear.

So if we want these new lifecycle calls to only be triggered by active state changes, it would make sense to use a more unique name. I can go with something like _on_active and _on_inactive

If NOTIFICATION_DISABLED is tied to the changing of process_mode maybe it might make sense as maybe another proposal/change to change they enum name to something like NOTIFICATION_PROCESS_MODE_DISABLED to clarify that it's the process_mode that's getting enabled/disabled.

I assume adding lifecycle callbacks for _on_active and _on_inactive would need notifications as well, so In the same vein as that though we could add "node" to it like NOTIFICATION_NODE_ACTIVE and NOTIFICATION_NODE_INACTIVE and maybe the lifecycle callbacks become _on_node_active and _on_node_inactive for clarity.

I do want to see if maybe someone else has any ideas besides active/inactive but I do think active/inactive makes sense here.

As a reminder this feature I'm building is meant to do two things:

  1. Act as a shortcut for debugging/culling through API and Editor UI (which is what this proposal was originally aimed at)
  2. Add a cleaner/more universal way to completely disable a node's impact in the scene in both editor and runtime

The first point definitely has a greater impact on my specific workflow so I appreciate opinions on the second point and hope to shape it to match expectations there. We've got some great ideas so far I think :)

I definitely get the use cases for debugging by isolating stuff by deactivating other things in a scene, but I think there's other benefits to other uses, like object pools, and tools and stuff so I'm glad there's a good discussion around it and I think there's a lot of really good ideas between everyone.

RobProductions commented 2 weeks ago

Ooo I didn't know about that one, I'm just starting to dig into the engine side of things. So enable/disable are triggered when process is changed? Interesting.

Yup but they are not exposed to GDScript, just "notifications" which help the nodes communicate in c++ world.

Does setting visibility/process_mode or anything like that would reactivate the node? Or we could throw an error like "Node is inactive, process_mode can't be changed while the node is inactive" like the active state overrides visibility, and process_mode.

Neither, the way I implemented this is that active state is simply another property like visibility that can be toggled on or off, without either of them throwing an error. In the code that checks whether a node should render, it simply checks both visibility and node_active, hence it acting like an override.

func do_something():
    my_node.visible = true;
    my_node.node_active = false
    print("Is Node Visible?:", my_node.visible) # false? Did we change visible to false?

Just tested this to be sure; so in this example my_node.visible is true because the "visibility field" is true. If you want to check whether or not the node is "currently visible in the scene" you could call my_node.is_visible_in_tree() which accounts for node_active and parent nodes that have visibility/active state set to false. This is basically what you have to do anyways currently in Godot if you want to check for parent node visibility so it should make sense to most users :) It's also the same as Unity if you've ever used it, where you can call SetActive() on an object that has it's renderer component set to disabled and basically get granular control both in the editor and in code for each toggle.

Similarly, as I mentioned above I've got is_node_active() to tell if it and all parents are active, and is_node_active_self() to tell if it is active individually. I can rename those if there's a better option :)

If NOTIFICATION_DISABLED is tied to the changing of process_mode maybe it might make sense as maybe another proposal/change to change they enum name to something like NOTIFICATION_PROCESS_MODE_DISABLED to clarify that it's the process_mode that's getting enabled/disabled.

I assume adding lifecycle callbacks for _on_active and _on_inactive would need notifications as well, so In the same vein as that though we could add "node" to it like NOTIFICATION_NODE_ACTIVE and NOTIFICATION_NODE_INACTIVE and maybe the lifecycle callbacks become _on_node_active and _on_node_inactive for clarity.

Strictly speaking I don't think it's necessary (so long as something runs the GDVIRTUAL_CALL() macro) but definitely good to have from what I can tell! I like these names so I'll go with that for now as I try my hand at implementing it

I'm glad there's a good discussion around it and I think there's a lot of really good ideas between everyone.

Absolutely, really happy to hear your thoughts on this stuff and make workflow improvements that hopefully help everyone in the long run :) Will report back when I make some progress with this stuff!

Snowdrama commented 2 weeks ago

Yup but they are not exposed to GDScript, just "notifications" which help the nodes communicate in c++ world.

Interesting, I'm surprised it doesn't expose a signal for it like on_visibility_changed an on_process_mode_changed signal I could see being useful, but I see now.

Neither, the way I implemented this is that active state is simply another property like visibility that can be toggled on or off, without either of them throwing an error. In the code that checks whether a node should render, it simply checks both visibility and node_active, hence it acting like an override.

Ohh that's interesting that makes sense to me, as long as having a node be in one state, setting it inactive, and then setting it active again keeps it the same way it was before setting it inactive I'm happy with that.

Just tested this to be sure; so in this example my_node.visible is true because the "visibility field" is true. If you want to check whether or not the node is "currently visible in the scene" you could call my_node.is_visible_in_tree() which accounts for node_active and parent nodes that have visibility/active state set to false.

Perfect! I was literally just thinking maybe something like an is_visible() would make sense, sounds like this covers that!

Similarly, as I mentioned above I've got is_node_active() to tell if it and all parents are active, and is_node_active_self() to tell if it is active individually

That sounds good to me! makes sense similarly my_node.node_active would be true, even if a parent was inactive, and is_node_active() is the same as is_visible_in_tree() but for node_active might be worth considering naming it is_node_active_in_tree() for consistency with the same function for visibility?

Strictly speaking I don't think it's necessary (so long as something runs the GDVIRTUAL_CALL() macro) but definitely good to have from what I can tell!

Yeah at least on the C++ side having those will probably enable someone to do something cool with them eventually right? haha

Kalto-Mate commented 2 weeks ago

If enable and disable are already taken naming-wise, would "awake" and "asleep" be a good compromise? I for one also expect a disabled node to still be reachable and "be there somewhere", just not using all that many resources. The sleeping analogy seems to work better in my head compared to "disable" anyways. For some reason I assume the later means the node is GONE gone.

AThousandShips commented 2 weeks ago

"sleeping" is already taken for physics bodies so it's not appropriate to use here

Kalto-Mate commented 2 weeks ago

D A R N I... hmmm... Unconscious? Hibernating? Frozen? (which should absolutely be the physics one)

AThousandShips commented 2 weeks ago

Frozen is a different physics thing, but the node isn't really sleeping so I don't think any synonym for it is useful

AThousandShips commented 2 weeks ago

But it'd say the naming discussion and confusion here highlights an important thing: This feature risks being confusing based on what it does or is expecting to do, and that the name is critical to not mislead people as to what it does

RobProductions commented 2 weeks ago

But it'd say the naming discussion and confusion here highlights an important thing: This feature risks being confusing based on what it does or is expecting to do, and that the name is critical to not mislead people as to what it does

In that case I think "node active" is probably the clearest name I can put to it as an inactive node will not process and will not render, whereas an active node can do either/both of those things depending on how you set visibility/process_mode. So unless the existing is_enabled() function name which is exposed to users is renamed to process_mode_enabled() (and similar renames for the internal notifications), I think this is the best option if that works for now :)

To that end, I've reworked some names for clarity and fixed up a few things that we discussed above! node_active is now a bool instead of an enum:

image

There are now two active-related signals which are implemented in Node instead of Node3D. node_active_changed is run when the node's active bool changes and node_active_in_tree_changed is run when it or a parent node has changed in such a way that it's become active or inactive in the tree:

image

The node active getters are now named is_node_active_self() and is_node_active_in_tree() to match the visibility functions, and are exposed to the user and work in runtime:

image image

I've now confirmed the set_node_active call works in runtime from GDScript:

# Called every frame. 'delta' is the elapsed time since the previous frame.
func _process(delta: float) -> void:
    timer += delta;
    child.set_node_active(timer > 1 && timer < 2);

https://github.com/godotengine/godot-proposals/assets/11141862/0d3dcf8f-acdd-431f-8683-0b03f3b48772

The new lifecycle functions _on_node_active() and _on_node_inactive() are now working in GDScript when a node's state changes:

func _on_node_active() -> void:
    print("I'm active!");

func _on_node_inactive() -> void:
    print("I'm inactive!");

Note that are called each time the "active in tree" status changes, just not just the first time. Behind the scenes they are called just before the node_active_in_tree_changed signal is sent. The last thing to do for these is to run them when a scene first loads up, I'm thinking just after _ready() so that users can perform critical setup steps before doing active-dependent start behavior :)

Lastly, I've implemented active state visibility changes for Node2D as well since it is handled independently of 3D nodes and it was much easier this time thanks to the signals being moved into Node:

image

Left to do: lifecycle calls on scene load, text fade feedback in the scene hierarchy, editor setting to disable checkmarks, testing/documentation. Signals currently still work like before (i.e. they always run regardless of active/process state on sender and receiver), and I might say that if we want signals to not be sent to inactive nodes it could be left for a future PR. There I would recommend we use two ways to call signals either via parameter or with 2 function calls, one that ignores inactive state and one that doesn't. That way we can keep the existing behavior and open up this ease of use feature for more complex stuff. Hope it's looking good for everyone that contributed ideas!

Snowdrama commented 2 weeks ago

This feature risks being confusing based on what it does or is expecting to do, and that the name is critical to not mislead people as to what it does. Understandable, though I'd personally argue that for a beginner with no engine knowledge, the "Active" toggle is actually MORE intuitive as a starting point. It's just a big checkbox that "makes it not do anything" no visibility, no physics, no processing. And then as they become advanced users, they can dig into the minutia of node active vs visibility vs process disabled

Similarly anyone coming from Unity to Godot will already be very familiar with the idea of the "active toggle" as it more closely mimics how Unity's gameObject.SetActive(false) works(funny enough I got to this proposal by searching for a way to "fully" disable a node not just set visible false and process disabled since physics bodies are still active if you do that)

Most gripes I imagine will likely be with people who are already advanced Godot users who are familiar with Visibility and Process, and run into edge cases where they use if node.visible: instead of if node.is_visible_in_tree(): and it returns true even though it's "not actually there" because it's not active. But there are similar edge cases with how visibility works already anyway.

Note that are called each time the "active in tree" status changes, just not just the first time. Behind the scenes they are called just before the node_active_in_tree_changed signal is sent. The last thing to do for these is to run them when a scene first loads up, I'm thinking just after _ready() so that users can perform critical setup steps before doing active-dependent start behavior

That's basically exactly what I expected, they'd be called every time the node becomes active/inactive and are good places to do initialization for tools and pooled objects and stuff!

@RobProductions just to ask because I'm not sure if I've seen it mentioned, but for nodes with collision like StaticBody3D does this remove the body from the physics simulation? For example, if I have a StaticBody3D platform and a Rigidbody3D object on top, and I set it inactive, will the body on top of it fall? Just curious if that was something this covered or has been tested at all. I assume if not, I could probably use the _on_node_active and _on_node_inactive calls to disable it myself but I figured I'd see if it was already covered and I just missed that part.

RobProductions commented 2 weeks ago

I got to this proposal by searching for a way to "fully" disable a node not just set visible false and process disabled since physics bodies are still active if you do that

Actually from what I can tell setting process to disabled in 4.3 does stop collision and forces, see below

or nodes with collision like StaticBody3D does this remove the body from the physics simulation? For example, if I have a StaticBody3D platform and a Rigidbody3D object on top, and I set it inactive, will the body on top of it fall?

Let's try it! First I run Rigidbody over Static Body with default state, then I set process mode on the static body to disabled, then I set process mode back to default and disable node_active for the static body:

https://github.com/godotengine/godot-proposals/assets/11141862/2141e134-659e-4c82-92f1-c586c475cc1f

So it would appear that process mode disabled does in fact disable collisions and because the active state overrides the can_process and is_enabled check, the same applies here. As you can see in the video I posted earlier, I've already confirmed it stops forces in Rigidbody as well. Hope that's helpful :)

Kalto-Mate commented 2 weeks ago

At the end of the day, this addition will work like a shortcut to do things quicker, a broader stroke, because I'm positive some Unity refugees currently go "Oh well, it's not a single call anymore" and have routinely made their own reusable code snippets that mess with several node properties in bulk. This would just make things official, so I can't wait for it to be accepted

RobProductions commented 1 week ago

I noticed that since cameras aren't affected by process_mode or visibility, they needed extra logic to disable when inactive, so I'm currently working on that. However, it's made me think about the lifecycle calls because this is one case where utilizing them internally will make things a lot easier. Here's a quick question I wanted to ask about that:

Currently, after a node has run _ready() it will check whether it's active/inactive and send a notification to run _on_node_active() or _on_node_inactive() for GDScript. Should it instead only run an _on_node_active() on startup if it's active and not do anything if its inactive? I think this is what you guys meant above but I just wanted to make sure! i.e. remove the "else" here:

image

I'm also going to assume that we want _on_node_inactive() to run before the node exits the tree if it was active before that point, right? This would also match Unity's OnDisabled call which is what I think we were going for. If we go this route it should make the camera code a little cleaner, and then after that I pretty much only have documentation/testing left to do! Once I have that started I will make one last update here with screenshots before the PR :) Thanks!

Snowdrama commented 1 week ago

Should it instead only run an _on_node_active() on startup if it's active and not do anything if its inactive? I think this is what you guys meant above but I just wanted to make sure! i.e. remove the "else" here: ... I'm also going to assume that we want _on_node_inactive() to run before the node exits the tree if it was active before that point, right?

Exactly you got it. I consider these functions like secondary constructors/deconstructors that are triggered when active/inactive state changes.

So from my perspective yes only _on_node_active() would be called after _ready() if active is true when loading the node.

Then similarly to how _on_node_active is called during the process of setting up the node, _on_node_inactive is called during the process of the node's removal.

So node initialization would be:

_enter_tree
_ready
if node_active == true : _on_node_active

Teardown would be:

_exit_tree
if node_active == true : _on_node_inactive()

though I could also consider flipping these so inactive is before _exit_tree I could see both, but I think having it triggered by the exiting of the tree is probably easier to implement.

That covers cases where if we set it inactive during runtime then it doesn't call_on_node_inactive() when freeing the node when exiting the tree, since it's already inactive.

OhiraKyou commented 1 week ago

In theory, the inactive function should be called before exit tree. On disable/inactive would often be used for cleanup of things set up during on enable/active. If possible, that cleanup should also work the same way even when exiting the tree (i.e., while the node is still in the tree).

Also, as a rule, I believe that lifecycle function pairs should be nested so that their setup/cleanup scopes are preserved.

Rough XML visualization of what I mean:

<tree-scope>
    <!-- On enter tree -->
    <active-scope>
        <!-- On active -->
          <game-loop></game-loop>
        <!-- On inactive -->
    </active-scope>
    <!-- On exit tree -->
</tree-scope>
Snowdrama commented 1 week ago

Also, as a rule, I believe that lifecycle function pairs should be nested so that their setup/cleanup scopes are preserved.

Yeah which is why I suggested that it possibly could be switched to before _exit_tree() however for me that's not a blocker if it made the implementation significantly more difficult, I'd rather get it implemented and fight over getting that changed later than prevent it from being implemented. Given that the _node_active() is called as a response to _ready() so if it needs to be as a response to _exit_tree() I don't think it it should be a blocker.

RobProductions commented 1 week ago

however for me that's not a blocker if it made the implementation significantly more difficult

No worries, it was easily added before exit_tree is called as part of the exit tree propagation step, so it will occur in the order you guys suggested :) I've also updated it to be _node_active() instead of _on_node_active() (and similarly for inactive) in order to match consistency of _ready() and _exit_tree() etc. which have no "on" prefix.

Currently facing a bug that when I switch scene tabs in the editor, the new node_active property is reset to false for the root node of the scene you switch to. I'm at a bit of a loss as to why that's happening, best idea is that I'm missing some serialization step or something in regards to root node properties during scene switching, but I don't see any special treatment on any of the other properties for this case so I'm not quite sure. Appreciate any ideas there if anyone knows why that may happen!

Besides that I still have to generate the XML docs and test thoroughly, perhaps with new unit tests as well. Everything else is pretty much finished and cleaned up (should be done within a week or so if I'm lucky with fixing that bug), and it's gearing up to be a hefty PR so I imagine it will take some months to get it reviewed/merged. Will do the best I can though and appreciate your support when it's ready since community interest helps with getting it seen faster :)

Snowdrama commented 2 days ago

Currently facing a bug that when I switch scene tabs in the editor, the new node_active property is reset to false for the root node of the scene you switch to. I'm at a bit of a loss as to why that's happening, best idea is that I'm missing some serialization step or something in regards to root node properties during scene switching, but I don't see any special treatment on any of the other properties for this case so I'm not quite sure. Appreciate any ideas there if anyone knows why that may happen!

That is interesting, so it doesn't happen for visibility, I'd have to actually look into it but that's interesting, I wonder what happens when you switch tabs. Does it keep the whole scene loaded? Or does it like cache it to disk or something temporarily. I haven't looked too much at the engine code itself but I can check it out. Is there a fork/branch I could see the current changes you've made for this? Not that I think I'll have any luck haha

RobProductions commented 2 days ago

@Snowdrama appreciate the ideas, I think I figured it out! What happens when you switch tabs is that the scene is removed from the special "editing nodes" list and the new contents are loaded into the tree, so since the new lifecycle call _node_inactive() was setting nodes inactive on exiting tree, it was happening during tab switch. I've fixed it now but noticed a new problem in that the children of the root aren't receiving the notification when the application ends, however I'm pretty confident I can track this one down easily.

Feel free to check out my progress so far here, though it still has the issue I mentioned above and no tests/documentation yet. I was busy with another PR recently but I'm back on it this week to put the finishing touches on this!

Snowdrama commented 2 days ago

Ohh interesting, I was thinking about this and how this may be a desirable thing? I was curious of how @tool affects this, as I know some people were planning to use the active/inactive lifecycle calls for tools in the editor.

Does _node_active() get called when opening a tscn into a tab? Does closing the tab/scene call _node_inactive()? Is it desirable that _node_active() and _node_inactive() get called when switching tabs?

At the very least it probably shouldn't do these things if it's not marked as @tool but if it is a tool, then might be worth discussing, as it would reflect a similar expectation to how _process and _ready also gets called in the editor if it's marked as a @tool

Consider this script, if you close or open the tab _exit_tree _enter_tree and _ready are all called

@tool
extends Node

func _ready():
    print("_ready")

func _enter_tree():
    print("_enter_tree")

func _exit_tree():
    print("_exit_tree")

#_process and _physics_process are also called, but I omitted them just for clarity

Note that if marked as a @tool the _exit_tree and _enter_tree methods ARE called but interestingly _ready is only called on scene load, which I think makes sense.

So I'd imagine that the expectation would be that _node_active() and _node_inactive() would also be called? At least if it's marked as a @tool But maybe not, maybe once it's active, it doesn't become inactive unless it's toggled inactive OR the scene exits, similar to how _ready is only called on load, but not on tab switch.

https://github.com/godotengine/godot-proposals/assets/1271916/ab36382f-3f42-48d3-879d-2e340e175119

RobProductions commented 2 days ago

I was curious of how @tool affects this, as I know some people were planning to use the active/inactive lifecycle calls for tools in the editor.

It should and does currently, same as the other lifecycle calls because it uses the same system to run them. Your demo actually helped a lot with getting me to think about when _node_active() should be called, and I've realized instead of being tied to _ready it should simply happen after ready but still on each time _enter_tree happens. I've adjusted it and it makes a lot more sense this way I think!

Does _node_active() get called when opening a tscn into a tab? Does closing the tab/scene call _node_inactive()? Is it desirable that _node_active() and _node_inactive() get called when switching tabs?

Yes, yes, and yes imo. Here we have the same code you applied with the new lifecycle calls:

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 the script is marked as @tool. Then, this is what we get in the console with the updated method I mentioned above:

https://github.com/godotengine/godot-proposals/assets/11141862/c7661350-32b6-4b0c-9fd6-ca8c297964d4

As you can see, _ready() is only called once on scene load (i.e. in the editor's case, when it loads) but all subsequent tab switching and enter/exit calls will run the lifecycle functions in the desired order :) As for this:

noticed a new problem in that the children of the root aren't receiving the notification when the application ends

It turns out that exit_tree is not actually called on nodes when you manually close out of the game window, so there's no way for _node_inactive to be run while it's hooked to that. I think this is actually fine since it is consistent with the tree lifecycle calls and it would be much more convoluted & make less sense to try and also link it to application close. So I will leave it as is for the PR! Now looking into creating test cases for the new property so we'll see how that goes lol

RobProductions commented 1 day ago

Finally submitted the PR which will close this proposal :) Feel free to check it out, test it, or show your support here: https://github.com/godotengine/godot/pull/94144

Since it's a big change and especially since we are in 4.3 feature freeze right now, it will probably take a while to iron out issues and get it merged but I will keep an eye on it and make any changes necessary. Let me know how it works for you if you'd like to play around with it! Thanks again to everyone that helped with ideas/feedback!