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

Unify disabling of nodes #1835

Closed
reduz opened this issue Nov 14, 2020 · 76 comments
Closed

Unify disabling of nodes #1835

reduz opened this issue Nov 14, 2020 · 76 comments

Comments

@reduz
Copy link
Member

reduz commented Nov 14, 2020

Describe the project you are working on:
Godot
Describe the problem or limitation you are having in your project:

Many users (specially those who come from Unity I guess) complain that it's not so easy to turn off objects like they do over there.

In Godot, you can turn on and off most types of objects:

  • Visual 2D and 3D objects can be hidden
  • Physics objects can be disabled
  • Animation player can stop their processing
  • process functions in script can be turned on and off via process property.
  • etc

But its kinda not unified nor always easily available from the UI, so this can be a hassle. I understand that in Godot you don't really need to do pooling like you do in Unity, so you can just create and free objects without much of a performance hit or GC unpredictability, but I still think having this may be of interest.

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

This proposal is about providing a way to unify this from the UI for most cases, so it's easier to toggle things off and on.
The proposal will consist of a few features working together

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:

This will be implemented via the following features:

Feature 1: Disable when hidden

The first feature will be a new property on 2D and 3D nodes: "disable_when_hidden"

The property will be enabled by default, and it will result in:

  • nodes that are hidden will no longer receive process or global input events.
  • physics objects will just go disabled and stop affecting collision or areas

Feature 2: Expose Process

For nodes that are neither 2D or 3D (or nodes that use scripts with the process function), if they can be inactivated, they will show a small gear icon instead of the eye in the scene tree, which will do a simliar function. If parents nodes are hidden, this will also
deactivate:

image

Please keep answers strictly on-topic or I will delete the comment. if you want to request or discuss a completely unrelated feature, open a separate proposal

@godotengine godotengine deleted a comment from avedar Nov 14, 2020
@hoontee
Copy link

hoontee commented Nov 14, 2020

Feature 2 is confusing; is it correct that clicking the gear icon will toggle the node's visibility?

If so, I feel like the icons could be more descriptive:

image

@Calinou
Copy link
Member

Calinou commented Nov 14, 2020

This is great, I would also love to be able to lock certain aspects of a node. For example: lock scaling so that I can't accidentally grab the scaling handles while trying to move nodes around.

This should be discussed in a separate proposal. Also, in the 3D editor, you can already choose which gizmos to display (translation/rotation/scale) using the buttons at the top of the 3D viewport.

@PLyczkowski
Copy link

I would do this: add an 'active' property to nodes, toggling this to false would gray out the node and sub nodes in the editor (no added clutter due to new icons), stopping all processing on them and making them invisible to other nodes.

@hoontee
Copy link

hoontee commented Nov 14, 2020

add an 'active' property to nodes

Yeah, that would probably be much simpler to implement and simpler to use.

Also important to note that for this property to be useful, toggling it needs to make any descendants active or inactive as well.

@RandomShaper
Copy link
Member

Maybe there will be some confusion between active and paused. What do you think about this?: the new icon in the tree items is a shorthand for the pause mode property.

@vybr
Copy link

vybr commented Nov 14, 2020

I would do this: add an 'active' property to nodes

Also agree with this as it's a lot more intuitive and would have better UX. The other problem I have with disable_when_hidden is that if the user doesn't want that functionality at all in their game, they'll have to disable it on every node if it's true by default. It's a two-step process of toggling that property and hiding the node as opposed to simply deactivating it like the suggestion above, and that is clearer because it's standalone and not linked to another property (visibility).

I would also be against using a gear icon on anything that is not a settings button.

@RandomShaper
Copy link
Member

Following up on my last comment, feature 1 may be implemented via a new pause mode: PAUSE_MODE_BY_VISIBILITY (or a better name).

@mrjustaguy
Copy link

mrjustaguy commented Nov 14, 2020

I agree with the Suggestion for a new Pause Mode, However I must disagree with the Feature Implementation, instead of having it as a new Property that will mean having to toggle "disable_when_hidden" for every node could quickly become tiresome for those that don't want that and could lead to unexpected client side issues, as if a node is missed when setting the feature to off, and it's parent is hidden (hence disabling all it's children with the property set to true) because human error is bound to fail to properly disable the property on a node or 2 and overconfidence will lead some people trying to diagnose in places where there's nothing wrong and possibly making plenty of bug reports for a bug that isn't there but is actually misuse.. Instead the implementation should in my opinion go something like:

1.Default Node Mode = Inherit from parent (much like the current pause works)
2.Disable When Hidden (DWH) = Explicitly tells the node to disable when hidden, all of it's Children with inherit will be disabled, no change if they have explicitly stated otherwise, and the Children of it's Children will inherit based on their parent's state, Example will be below, works for all the modes..
3.Don't Disable When Hidden (DDWH) = Explicitly tells the node to Stay active when hidden

Example:
Root Node Mode = DWH
Root/Child1 Mode = Inherit --> Root Node Mode --> Currently DWH
Root/Child2 Mode = DWH --> Doesn't matter what it's parent mode is
Root/Child2/Child1 Mode = Inherit --> Root/Child2 = DWH --> Root/Child2/Child1 DWH
Root/Child2/Child1/Child1 Mode = DDWH --> It's parent is DWH, however it doesn't inherit, so DDWH no matter the other nodes in the tree

@Bromeon
Copy link

Bromeon commented Nov 14, 2020

It seems to me that disabled and hidden are completely orthogonal properties:

  • Disable is for physics/logic processing, and toggling it will change the game's semantics.
  • Hidden is a purely visual setting, and will only change the game's graphics.

I don't think disable_when_hidden is desirable, especially not with default true. If the user needs those semantics, he/she can easily change both settings. As they're different concepts, the UI should probably make the distinction between the two clear, not trying to unify them into a single button.

@robbertzzz
Copy link

Will this just deactivate every aspect of the node, or are you planning to add options for only disabling process or only disable input?

I don't think the disable when invisible option is the best way to do this, it leads to UX problems that you're trying to solve with replacing an eye with a gear icon. What if the option is disabled, do you hide the gear icon? But the other objects always have an eye, that way you think you can tell from the hierarchy which objects get disabled, but you actually don't.

Theres also the option that I want to disable the script on a node (which is possible in unity), but not hide the node. This is something you're not accounting for, it still has to be done by calling quite a few functions from script.

In unity disabling works with a checkbox in the top left corner of the inspector. This disables the object and all its components. But, each component, like a renderer, can also be disabled separately. I'd rather see godot doing something similar and keeping these things separate altogether.

@reduz
Copy link
Member Author

reduz commented Nov 14, 2020

@PLyczkowski, @vybr , @Bromeon Ok, thanks for the feedback, I see the point of what you mean.

So then, what about this? following your sugestions, we remove the pause mode, and we add a new enum (exposed as a combo property) to nodes called ProcessMode, for which values would be:

  • Inherit (use parent, this is default)
  • Normal (will be suspended on pause, the default if no parent changes)
  • PauseOnly (will only run if pause is on)
  • Always (will run always, paused or not)
  • Disabled (Node will never do any processing)

So, this way you can turn off processing of objects entirely at any point in the scene tree. It needs to be noted however that this will not hide the nodes, as this would be orthogonal to hiding. If you want nodes to completely stop doing anything, you would need to disable them and hide them, both things separately. If you prefer this explicitly, then it should be doable, and does not break compatibility with existing scenes.

Because this can be difficult to track in the editor (as it makes no sense to put an extra icon besides the eye), we just gray out nodes if they are disabled.

I personally like this solution you guys suggested more than my original proposal, since I think its much clearer and likely useful.

@robbertzzz
Copy link

@reduz how do you distinguish greyed out (non-processing) nodes from inherited nodes in that case?

@RandomShaper
Copy link
Member

Sounds good, but I'd use a different word for the Normal mode (Inherit is quite normal too and it may be hard to remember the difference). Since Normal means 'use the global pause mode set for the scene tree', maybe Global or Tree can work, or a better alternative.

@Zireael07
Copy link

@reduz: What you are suggesting in terms of pause is extremely close to #1011, which has an open PR over at Godot: godotengine/godot#39606

@mrjustaguy
Copy link

mrjustaguy commented Nov 14, 2020

The ProcessMode idea seems Great to me, However as @RandomShaper pointed out Normal might be the wrong word, however I guess sufficient Docs on the matter would clear that problem somewhat.

How would "StandardProcess" or just "Standard" work instead of "Normal"?

@RandomShaper
Copy link
Member

Regarding the hide-on-pause thing, I was considering maybe making visibility a tri or quad-state or adding another property about forcing some visibility depending on the current pause state of the node.

But I think that would be adding clutter for something that you can easily implement manually, perfectly tailored for the needs of your game.

@reduz
Copy link
Member Author

reduz commented Nov 14, 2020

@Zireael07 That proposal is probably not doable in practice, because pause needs to be a system wide thing. When you enable pause, the physics are stopped entirely. Here when you stop processing of a branch, the object gets removed from the physics but things continue.

@RandomShaper
Copy link
Member

@reduz, if, according to @Zireael07's proposal, the idiom for pausing the whole game changed from pausing the SceneTree to setting pause on the root node, what would be the difference, besides an uglier syntax?

@danilw
Copy link

danilw commented Nov 14, 2020

For nodes that are neither 2D or 3D (or nodes that use scripts with the process function), if they can be inactivated, they will show a small gear icon instead of the eye in the scene tree,

is this only UI change or setting visible=false to root node will deactivate everything in child nodes include scripts?

I do not want more "confusion" in Godot because lots of confusing features that not documented...

what would be the difference, besides an uglier syntax?

adding "pause" button to Godot editor UI at "hide" button that in Godot now can be more "clean" to understand

@reduz
Copy link
Member Author

reduz commented Nov 14, 2020

@mrjustaguy @RandomShaper maybe WhenActive ?

@reduz
Copy link
Member Author

reduz commented Nov 14, 2020

@robbertzzz @danilw Agreed it's confusing, will need to think of something else for how to make it clearer that its not active.

@danilw if there is something you find confusing that is not documented, open an issue in the docs repo:
https://github.com/godotengine/godot-docs

@robbertzzz
Copy link

@reduz making the node name italic could work? Or giving active nodes a lighter background?

@mrjustaguy
Copy link

@reduz WhenActive works just fine in my opinion.

@RandomShaper
Copy link
Member

I like Normal better than WhenActive. I'm even starting to assimilate the mapping between Normal and its meaning. So it may be a good name after all. And @mrjustaguy is right in that the docs can clarify any doubt.

@mrjustaguy
Copy link

@reduz making the node name italic could work? Or giving active nodes a lighter background?

Maybe having different color node names to signal their status? White as Processing and Gray as Not Processing maybe? Italics would probably be a Little confusing and unintuitive as to which state is which.

@robbertzzz
Copy link

@reduz making the node name italic could work? Or giving active nodes a lighter background?

Maybe having different color node names to signal their status? White as Processing and Gray as Not Processing maybe? Italics would probably be a Little confusing and unintuitive as to which state is which.

This will create problems with distinguishing between not processing and inherited nodes, which are also greyed out. Hence the background colour suggestion :)

@mrjustaguy
Copy link

Didn't think about that. In that case, Background Coloring makes more sense.

@chucklepie
Copy link

chucklepie commented Nov 16, 2020

A simple question before I go into detail. Why does calling set_process(false) not disable timers?

Regardless of my timer question, I'd love this feature. Here's why: I have a large tilemap/flip screen game and so have to disable/enable everything not in screen. Currently this is pretty painful and error prone. Currently I do this for every child:

set physics off
set process off
set visible to false
set all collision boxes off

But as I've just found out, timers are still running and all my nodes are still active in the game! these nodes just don't want to stop :(

What makes it worse is when you have nested nodes, it makes coding pretty much a tortuous affair.

@markdibarry
Copy link

You're looking for set_process_internal()and set_physics_process_internal().

@chucklepie
Copy link

chucklepie commented Nov 16, 2020

@markdibarry what is this 'set_process_internal'? I've read the docs and it makes no sense to make, simply refers to an internal process and says it's for advanced use... does it stop timers running, collision events being raised, screen notifiers, yield returns being triggered, etc?

@chucklepie
Copy link

chucklepie commented Nov 17, 2020

Unless I'm doing something wrong, I'm calling this in code, and while all nodes seem to be disabled (they aren't visible and have no collision), they are still raising area2d body entered events and timers are still running?

		for container in children.get_children():
			Globals.debugprint("Disabling items in container " + container.name)
			#refresh_map_objects(container.name,false,true,current_level,true)
			container.propagate_call("set_process",[false])
			container.propagate_call("set_process_internal",[false])
			container.propagate_call("set_physics",[false])
			container.propagate_call("set_physics_internal",[false])

@stebulba
Copy link

@reduz
ProcessMode:

  • Inherit (use parent, this is default)
  • Normal (will be suspended on pause, the default if no parent changes)
  • PauseOnly (will only run if pause is on)
  • Always (will run always, paused or not)
  • Disabled (Node will never do any processing)

I like that. And keep visibility separately as how is working now in the Editor.

But in the code, add a method enable() and disabled() on each node :

node.enable(visible=true) ---->  that will set up visibility to true and setup ProcessMode to original arguments
or
node.disable(visible=false) ---->  that will set up visibility to false and setup ProcessMode to Disabled

On that way I will replace in my gdscript:

self.set_process(false)
self.set_process_input(false)
self.visibility = false

by
self.disable()
or
self.enable(visibility=true, ProcessMode=Always) to enable everything set up the ProcessMode to Always and visibility to true by default

why not to add :
node.enable_childrens/ node.disable_childrens(visibility, ProcessMode) to enable/disable the process and visibility for all childrens.

The visibility arguments, (argument 1), can be true, false or null. if is NULL the visibility value of the node doesn't change with the method.
if the second argument, the ProcessMode is null, enable() will give the previous/original value before the disabled.

That can be a compromise between the godot style and the Unity style where Unity's users will do : node.enable() or node.disable() with the default arguments.

@chucklepie
Copy link

@markdibarry set_process_internal() does not work as expected (or at least not as I expected), see godotengine/godot#43689

When you call the method, it doesn't just enable/disable but this _internal version also actually starts all the timers running.

reduz added a commit to reduz/godot that referenced this issue Feb 18, 2021
Implements godotengine/godot-proposals#1835 (comment)

* PauseMode is now ProcessMode, containing the following states:
	```
	PROCESS_MODE_INHERIT, // same as parent node
	PROCESS_MODE_NORMAL, // process only if not paused
	PROCESS_MODE_PAUSE_ONLY, // process only if paused
	PROCESS_MODE_ALWAYS, // process always
	PROCESS_MODE_DISABLED, // never process
	```
* NOTIFICATION_PAUSED and NOTIFICATION_UNPAUSED are received effectively when the node is paused and unpaused (not any longer when pause mode is set in SceneTree).
* Renamed some nodes that used ProcessMode/process_mode to specify a callback type to ProcessCallback to avoid clashes.
@akien-mga
Copy link
Member

Fixed by godotengine/godot#46191.

@akien-mga akien-mga added this to the 4.0 milestone Feb 19, 2021
@chucklepie
Copy link

chucklepie commented Feb 19, 2021

Sorry, slightly confused. Is this just for the editor or will turning off visibility with this flag enabled in code off disable the node and so there's no need to now call_deferred or call disable/changing monitoring status?

Will it also fix the bug (forgotten number) where particles continue to process (at a slow speed) when debug is entered?

@aaronfranke
Copy link
Member

@chucklepie Your confusion is understandable. What was implemented in godotengine/godot#46191 is not the same as what the title and OP of this proposal describe, because the plans were changed in the course of this discussion. The behavior that godotengine/godot#46191 implemented was basically this comment.

@hsandt
Copy link

hsandt commented Feb 18, 2023

And finally, for people looking for a way to change process mode in the editor, just scroll to the bottom of the inspector to modify the Mode property via dropdown. Effect is the same as the code equivalent.

pasted_image

Granted, it's not as quick as toggling the eye icon. But we can add a shortcut for quick enable/disable later in the UI.

@kendallroth
Copy link

...speaking about a shortcut in the UI for tweaking this... 😁

In all seriousness, is that something that we may see arrive in the future?

@Calinou
Copy link
Member

Calinou commented Sep 20, 2023

...speaking about a shortcut in the UI for tweaking this... 😁

In all seriousness, is that something that we may see arrive in the future?

This is being tracked in #7715.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests