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

Implement project-wide node groups #60965

Merged

Conversation

DarkMessiah
Copy link
Contributor

@DarkMessiah DarkMessiah commented May 12, 2022

This commit based on #24262.
Fix godotengine/godot-proposals#3789 and maybe godotengine/godot-proposals#3351 :)
Bugsquad edit:

  • Global Groups - stored in the godot.project and visible to all nodes.
  • Local Groups - these are just a list of groups derived from nodes in the current scene, excluding global groups.

Global groups editor in Project Settings.

Editor

image

Remove global group dialog

image

Reworked Groups Editor.

  • Removed Manage Groups button + dialog
  • Added filter for groups
  • Added create button
  • Reworked tree. Added categories for Local and Global groups.
Groups Editor

image

Ability to create Global and Local groups.

image

Ability to Covert Global to Local and Local to Global. Rename. Delete.

image

image

image

image

P.S. I have one problem. I store local groups cache inside Map<Node *, Set<StringName>>. When I opened scene, I'm storing all local groups into this Set where key is a edited_scene_root_node. I want to know the moment, when scene is closing and I can release a data from the Map.
I use Map<Node *, Set<StringName>> , because when you adding local group and unckeck it from all nodes, you can lose it when you are switching tabs.

@DarkMessiah DarkMessiah force-pushed the global-groups-implementation branch 4 times, most recently from 844d8e9 to a876f8f Compare May 12, 2022 10:29
@Chaosus Chaosus added this to the 4.0 milestone May 12, 2022
@Calinou
Copy link
Member

Calinou commented May 12, 2022

Should "Local Groups" be renamed to "Scene Groups" to make it more obvious that those groups are scoped to the currently open scene in the editor?

@fire-forge
Copy link
Contributor

This is great! I like this workflow much better than the old one.

A few suggestions:

  • Move the + button to the right side. Most other Add/+ buttons throughout the editor are on the right.
    image
  • Change the + button tooltip to just "Add a new group" instead of "Add/Create".
    image
  • Change the "Groups" tab to "Global Groups" in Project Settings to make it clear that this is only for global groups and not all groups.
    image
  • Split "Remove Completely" and "Make Local" into 2 buttons in the tree instead of combining them in the same button. The dialog is a bit confusing right now.
    image

@DarkMessiah
Copy link
Contributor Author

What icon could I use for another remove button? :)

@fire-forge
Copy link
Contributor

What icon could I use for another remove button?

I have no idea. I looked around for a good icon to use and I can't find one. Maybe someone else has an idea?

@DarkMessiah
Copy link
Contributor Author

I have no idea. I looked around for a good icon to use and I can't find one. Maybe someone else has an idea?

There is a Close icon, but I think it will be not clear for users.
image

What do you think about CheckBox in dialog? :D
image

@fire-forge
Copy link
Contributor

fire-forge commented May 12, 2022

A checkbox in the dialog is good. But the text to the right of the checkbox be a part of the checkbox (using the text property) instead of a separate label, so that the whole thing can be clicked.

@DarkMessiah DarkMessiah force-pushed the global-groups-implementation branch from a876f8f to be14cc3 Compare May 13, 2022 15:56
@DarkMessiah
Copy link
Contributor Author

  • Rename Local groups to Scene groups

  • Moved the + button to the right side and changed tooltip text.
    image

  • Rename tab in Project Editor to Global Groups

  • Redesigned removing dialog
    image

@fire-forge
Copy link
Contributor

fire-forge commented May 13, 2022

It looks great! One change I would make is removing "This action is undoable!", because other actions that are undoable don't have anything saying that. Usually it's the other way around, with actions that can't be undone having a warning and everything else is just assumed to be undoable.

Also, I don't know if this will make it too cluttered, but you could try adding icons to "Scene Groups" and "Global Groups" in the tree. The PackedScene icon will work for scene and Environment for Global:

Screenshot_20220513-113209~2.png Screenshot_20220513-113220~2.png

I don't have any more suggestions regarding the design. I'll do a review of the code when I get a chance to :)

@DarkMessiah DarkMessiah force-pushed the global-groups-implementation branch from be14cc3 to 166997c Compare May 13, 2022 18:16
@DarkMessiah
Copy link
Contributor Author

Done!

Screenshots

image

image

image

image

@fire-forge
Copy link
Contributor

I would review this right now, but it doesn't compile :( The error messages are referring to HashMap, so I assume it's a difference introduced by #60881 or #60999.

https://github.com/godotengine/godot/runs/6427783069?check_suite_focus=true

editor/group_settings_editor.cpp Outdated Show resolved Hide resolved
editor/groups_editor.cpp Outdated Show resolved Hide resolved
editor/groups_editor.cpp Outdated Show resolved Hide resolved
editor/groups_editor.cpp Outdated Show resolved Hide resolved
editor/groups_editor.cpp Outdated Show resolved Hide resolved
editor/groups_editor.cpp Outdated Show resolved Hide resolved
editor/groups_editor.cpp Outdated Show resolved Hide resolved
editor/groups_editor.cpp Outdated Show resolved Hide resolved
editor/groups_editor.cpp Outdated Show resolved Hide resolved
@DarkMessiah
Copy link
Contributor Author

@fire-forge I need to know when node added to the scene tree. Is there any signal for this?

@DarkMessiah DarkMessiah force-pushed the global-groups-implementation branch 3 times, most recently from b6bb8c1 to 92e7648 Compare July 4, 2022 04:28
@DarkMessiah
Copy link
Contributor Author

I removed my caching code, because it didn't work as I expected. Now the list of groups is updated every time when:

  1. New node added
  2. Any node removed
  3. Scene tab changed

Also I added icons for groups from packed scenes:
image

Groups from a packed scene cannot be removed or renamed outside of the packed scene.
image

When you'll try to remove these groups, you just remove them locally from the current scene, the packed scenes will not be changed. For the rename action I added TODO. Now I'm not showing anything, but maybe we need to show a warning message or create a new group.

@DarkMessiah DarkMessiah force-pushed the global-groups-implementation branch 3 times, most recently from 897f23b to cf72d13 Compare November 9, 2023 18:06
@DarkMessiah
Copy link
Contributor Author

Done :)

Though I noticed that when you rename or delete a scene, the old entry is kept in the cache.

Connected to the files_moved and file_removed signals from FileSystemDock, now the cache is updating correctly.

EDIT:
Also a more serious issue is that modifying scene externally does not update cache entry. Not sure how to fix that; if the editor is currently opened then there is a way to detect that, but when it's not...
It's very relevant when working in a team, as VCS causes lots of external changes. Though I think scripts have the same problem.

I don't know what to do about caching after external editing. I could suggest Project -> Re-cache groups, but this way seems to be the easiest but bad.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Seems to be working correctly now.

As for the external changes issue, scripts have the same problem and it's currently unresolved (AFAIK). Worst that it can cause is that renaming global group might miss some references, but I think renaming groups globally is not very common action.

@DarkMessiah
Copy link
Contributor Author

Removed hacks related to disabled Checkbox state, because Checkbox in Tree now has a disabled state: #84845

image

@YuriSizov YuriSizov dismissed reduz’s stale review December 20, 2023 13:41

Stale review, cache implemented

@YuriSizov
Copy link
Contributor

The OP is unclear whether this closes godotengine/godot-proposals#3351 or not. So, does it?

@YuriSizov YuriSizov merged commit 6296333 into godotengine:master Dec 20, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks! And thank you for sticking around for over a year to see this finished 🙂

@Exerionius
Copy link

Just saw this PR in the changelog for 4.3 dev1.

The description of local groups is kinda confusing.

derived from nodes in the current scene

What does this mean?

Does this mean that if I have two separate scenes with a bunch of nodes in them, and both scenes define a group called "test", then calling get_tree().get_nodes_in_group("test") from one of the scenes will return nodes only from the same scene regardless if a group with the same name exists in a different scene?

Does this mean that the old behavior of groups was what is now called "global"? But now we have "local" groups in addition to that?

How can we know in code which group is local and which is global?

This needs to be thoroughly documented on this page: https://docs.godotengine.org/en/stable/tutorials/scripting/groups.html

@Mickeon
Copy link
Contributor

Mickeon commented Dec 22, 2023

The description of local groups is kinda confusing.

derived from nodes in the current scene

What does this mean?

Does this mean that if I have two separate scenes with a bunch of nodes in them, and both scenes define a group called "test", then calling get_tree().get_nodes_in_group("test") from one of the scenes will return nodes only from the same scene regardless if a group with the same name exists in a different scene?

It's much simpler than you're imagining.

The underlying logic of groups is unchanged. Groups with the same name are the same.
The way groups are organized and displayed in the editor has changed drastically to make keeping track of them easier.

All groups are stored alongside the node they belong to in a PackedScene. When you load the scene, every node's group is kept track of and displayed in the Group Editor for easy access.
This is the exact same behavior as prior to this PR.

How can we know in code which group is local and which is global?

The only difference between "local" and "global" groups is...

The "global" groups and their descriptions are stored in the Project Settings (underneath [global_group] in the .godot file). The PR also implements some internal methods in the ProjectSettings class to retrieve them, but they are not exposed.

In code, you may write something like this (I don't see why you'd need to but...):

static func is_global_group(group_name):
    return ProjectSettings.has_setting("global_group/" + group_name)

This needs to be thoroughly documented on this page: https://docs.godotengine.org/en/stable/tutorials/scripting/groups.html

I wouldn't say "thoroughly" because again, your code would be entirely unaffected, but the page does need to be entirely refreshed for the new interface, yes.

@Exerionius
Copy link

@Mickeon thanks for the explanations, it makes it a bit clearer.

So it's the opposite of what I was imagining: the old groups are what "local" means, but now we have "global" groups which is editable in the project settings and that's the only difference between the two (from the usage perspective).

@fire
Copy link
Member

fire commented Jan 7, 2024

Many people are able to reproduce a crash at #86844

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.

Allow to manage node groups from the project settings Add project-wide group management