Godot: Nodes shouldn't be added to 'idle_process' group

[razcore-art]

I don't know if this is a new thing in v2.1.2, but I just noticed that nodes are added to this 'idle_process' group internally. This is a flaw in the design. Only groups added by the developer should be present at run time. If you add a node to only one group then you expect that the node will have only one group at run time and might think to retrieve that with self.get_groups()[0] or something like that. Because godot internally adds some nodes to 'idle_process', the above action can lead to errors.

[CodeDoes]

Either this or it should be made clear that there's magic happening when a game runs

[bojidar-bg]

Or maybe it should be made into __idle_process, and get_groups should remove all groups starting with _

[razcore-art]

As long as the developer gets only the groups that he himself put in, doesn't really matter how the internals of get_groups works. It's a good idea to have get_groups remove everything starting with _ or maybe even __ in case someone decides for whatever reason to use a group name starting with _? Doing this wouldn't change much in terms of the internals of GD

[CodeDoes]

i don't agree with this. it should rather just be made clear that _ is
used for system groups.

On 30/01/2017, Răzvan Cosmin Rădulescu [e-mail] wrote:

As long as the developer gets only the groups that he himself put in,
doesn't really matter how the internals of get_groups works. It's a good
idea to have get_groups remove everything starting with _ or maybe even
__ in case someone decides for whatever reason to use a group name
starting with _? Doing this wouldn't change much in terms of the internals
of GD

--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/godotengine/godot/issues/7673#issuecomment-276070864

[razcore-art]

@ClikCode no, it's not enough, because you end up at run time with more groups than you as a developer want. It's all got to do with getting the group back. If I as a developer add a node to ONE group and ONLY ONE group then I expect at run time to be able to get that back with get_groups()[0]. Currently there's no guarantee for this.

[CodeDoes]

@razvanc-r That sounds nice and all. But, you're forgetting about plugins, and modules. Which have a chance of wanting to use groups for their own purposes as well. ignoring group names that begin with "_" is a roundabout way of doing things. because then you'd also need another function that gives you groups that start with "_".

Another way would be. Godot shouldn't be putting groups onto onto node to begin with. It should keep the information in a list instead.

[razcore-art]

Yeah if you follow the title of the issue that was the first thought I had, but it depends how difficult it is to change this behavior inside of godot. If it's too difficult we could have like an internal random string or something so instead of idle_process it would be added to ALKJFfjkdfjKDJF_idle_process etc. and then when you get them back with get_groups() these would be removed from the list or something. In any case this needs to be fixed somehow :)

[bojidar-bg]

@ClikCode Well, if a module/plugin does add _-starting groups, we would filter them out as well, no issues. We might just have a param for it, e.g. get_groups(include_internal=false)

[CodeDoes]

that could work, it would make using groups a lot safer. plugins can
also use groups then, without having to worry about messing with game
logic.

On 30/01/2017, Bojidar Marinov [e-mail] wrote:

@ClikCode Well, if a module/plugin does add _-starting groups, we would
filter them out as well, no issues. We might just have a param for it, e.g.
get_groups(include_internal=false)

--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/godotengine/godot/issues/7673#issuecomment-276176067

[razcore-art]

Well with that said, we should probably go for a list of reserved group names (I'm sure the internal ones are very limited) and go with _idle_process etc. and remove them upon request with get_groups() and make this clear in the docs.

edit: yeah it seems there are quite a few nodes being added to groups internally, most of them use groups starting with '_', like in scene/2d/canvas_modulate.cpp for example, but there are a few anomalies. There are these groups on the Node objects that don't start with _ and then there's the Camera2D node which is added to some groups starting with __... we should probably just stick with one _ and update docs with these restricted group names + strip them when retrieving with get_gropus(). I'm not sure if it make any sense to include the internal ones with get_groups(include_internal=true), I can't think of any scenario when you might want that..

[bojidar-bg]

@razvanc-r Well, I can't make much sense about it either, but I think it is better to have it than not to have it :smiley:

[tamkcr]

I'm against complicating whats working fine, if we want something untouched by engine internals we could add an extra system to scene tree with functionality identical to groups but call it something like tags instead.

[razcore-art]

Why would anyone want to duplicate existing functionality? That doesn't make any sense. And in any case the only "touching of the engine internals" would be imposing a standardized naming convention, which there isn't at the moment & just stripping the internal groups so they're not accessible from the editor. I don't know about you, but this makes sense to me

[bojidar-bg]

@tamkcr Well, why keep node then anyway, they are used for internal things too (e.g. scrollbars by scrollcontainer, etc.)? Let's make and extra system to scene tree identical to nodes, but called something like gameobjects, with a nice Component-Entity approach...
(just continuing your logic above)

I agree with @razvanc-r about this -- duplicating exiting functionality for the sake of it is just not worth it enough. In Godot we give the developer access to nearly all levels of functionality, from low-level Servers giving away RID-s to high-level Nodes and Resources/References. But all levels use the same terminology, and only servers have some internal, hard to get to state.

The other possible solution we might do is store set_process(true) nodes, etc. in some other, internal hashmap or list, but this automatically remove the possibility of module/plugin makers to reuse said internal hashmaps or lists, unlike the _ propsal.

[tamkcr]

@bojidar-bg I think you have mistaken my response since your last paragraph is in essence same as my suggestion (I also don't see how the node continuation relates at all, maybe I have failed in wording my response and the tag name triggered some unintended feelings :D). I just don't think this a healthy direction to take on as opposed to clearer documentation on existing functionalities (if the _ proposal is accepted we still need to document that _prefixed groups could get special care).

Moreover, it seems the main benefit this brings is letting people do get_groups()[0] in a single group situation, as in cases where more than 1 groups are used iterating with case check (or any wrapped equivalent) seem inevitable to me as the order of groups returned using get_groups is not guaranteed. Personally, I would workaround with meta instead for this specific case.

[bojidar-bg]

@tamkcr I actually agree that get_groups()[0] is dangerous and nonmodular code in all, and that is_inside_group is a probably much better way to check for similar (not saying that the first has no uses), but if one iterates over get_groups and finds a bunch of completely unrelated groups in there, it becomes a nuisance :smile:

I actually thought about the internal storage idea from the start, and maybe it indeed makes sense, but what should we do about Camera2D's usage of groups? (I personally would be amused to find a __camera_viewport_XXXX group on any node of mine..). That's why, I think we might go with both proposals at once, just to get the benefits of both (using is_in_group("_idle_process") is ugly anyway, and manually adding something to it would be even dangerous)

[akien-mga]

Remember that this is a game engine, and to provide this number of high level features that are necessarily internal components that are instanced even if not asked for specifically by the game developer. If you iterate on the root node's get_children() you'll likely also find some internal nodes which are not visible in the editor's scene tree (e.g. a timer to handle your Control's helpful tooltip).

I think it's actually great that those are accessible to the programmer so that you have full control over what the engine is doing (who knows, you could want to let a signal redefine the tooltip's timer value dynamically if meaningful for your UI).

So the main question is whether this is actually a use case for hiding those internal nodes? As said avoid using get_groups()[0] to try to get the first user-defined group is bad practice anyway.

[razcore-art]

How is that bad practice? Example usage. I have Enemy1, Enemy2, EnemyPotentialName and I assign all to group 'enemy' and only to this group. I then have sprites, audio FX, whatever who's name is something like 'enemy_%d'. I then proceed to use 'enemy_%d' % randi() or something to use random resources based on the name. Unsurprisingly I'm using this approach. But tell me, why is get_groups()[0] because you're not giving any motivation to not using it. It sure beats slicing get_name(), what if I change the names from EnemyXXX to, I don't know, SomethingXXX? All I have to do is change the group name and the code still works, but with get_name() I need to redo the slices as well. And what's wrong with get_groups(internal=true,false)?!

[akien-mga]

It's bad practice (IMO) because if you decide to add those enemies to a new "flying" group for flying enemies, your code might break. Writing code that assumes that you'll hopefully use the right group name to match your node names is hazardous. It's IMO much better to write one more line and be future proof:

assert(that_node.is_inside_group("enemy"))
var name = "enemy_%d" % randi()

than

# hope I won't break this by mistake in 6 months
var name = that_node.get_groups()[0] + "_%d" % randi()

And what's wrong with get_groups(internal=true,false)?!

I haven't said anything was wrong with it, I'm just raising questions as so far the discussion has only been about "whooo Godot is spilling its internals all over the floor" :)

[razcore-art]

Alright, point taken on the group best "best practices". In my case I do know that I will only have one group per nodes (ever), but alright I can understand it. On the other hand I feel like when you as a developer add nodes to 3 groups say, then at runtime you as a developer want 3 and only 3 groups, not some random number depending on the internals of the engine. I am so far in favor of this get_groups(internal=true,false) with false being the default.

[CodeDoes]

On the other hand idle_process group should be unnecessary to begin with

[razcore-art]

You know @akien-mga after thinking about it a bit more. I kind of prefer getting the info from the groups. Here's why. Say you have multiple resources, some start with 'enemy', some start with 'this' some with 'that' etc. You get the idea. If you spread all over the place code like:

assert(...is_inside_group('constant'))
etc.

You still have to do massive refactoring. Alright, so don't use hard coded string values, let's instead use a class variable to store these things... like an array with a specific order. But wait, isn't that what groups should do in the first place?! So I noticed that the groups mechanism forces an alphabetical order (except for the internal states for some reason, which I think can either go at beginning or at end of the array).

So in the end of it all I think we should have get_groups(internal=true,false) as mentioned but also have order in the group array so that would mean enhancing the group panel to allow ordering of groups. They're arrays after all, not sets or maps or other things and arrays have order to them. Also allow editing in the panel, as of now you can only remove and add groups. So then there would be no problem with get_groups()[0, 1, 2, 3.. etc.] as the order would be guaranteed to be the one you set.

[tamkcr]

I think get_groups fetches the groups by iterating through Map< StringName, GroupData> grouped; in Node::Data then adding them to a list, any order you notice is probably due to the way they are hashed.

[CodeDoes]

so you can't rely on the ordering.

On 01/02/2017, tamkcr [e-mail] wrote:

I think get_groups fetches the groups by iterating through Map< StringName, GroupData> grouped; in Node::Data then adding them to a list,
any order you notice is probably due to the way they are hashed.

--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/godotengine/godot/issues/7673#issuecomment-276657994

[ghost]

First of all thank you for your report and sorry for the delay.

We released Godot 3.0 in January 2018 after 18 months of work, fixing many old issues either directly, or by obsoleting/replacing the features they were referring to.

We still have hundreds of issues whose relevance/reproducibility needs to be checked against the current stable version, and that's where you can help us.
Could you check if the issue that you described initially is still relevant/reproducible in Godot 3.0 or any newer version, and comment about it here?

For bug reports, please also make sure that the issue contains detailed steps to reproduce the bug and, if possible, a zipped project that can be used to reproduce it right away. This greatly speeds up debugging and bugfixing tasks for our contributors.

Our Bugsquad will review this issue more in-depth in 15 days, and potentially close it if its relevance could not be confirmed.

Thanks in advance.

Note: This message is being copy-pasted to many "stale" issues (90+ days without activity). It might happen that it is not meaningful for this specific issue or appears oblivious of the issue's context, if so please comment to notify the Bugsquad about it.

[AfterRebelion]

This doesn't seem to be valid anymore.
this_node.get_groups() returns an empty array on my testing.
Also this_node.is_in_group("idle_process") returns false.
Tried searching for a commit referencing this change, but neither Github or Google returned anything.

[akien-mga]

Thanks for testing, this was indeed fixed by f3f4a11cfb9767e1d691aec431dd2f1a87a31977 and later related changes. All builtin nodes now use NOTIFICATION_INTERNAL_PROCESS or NOTIFICATION_INTERNAL_PHYSICS_PROCESS for their processing needs, and no longer the user-exposed non-internal ones (which trigger the addition to the mentioned group).