blocks: Add groups option block extension #240
Conversation
|
I now noticed that if I change the edited scene, the groups shown in the picker block won't change until I start dragging it out of the picker. I think I have to watch for the context node changing and emit BlockExtension.changed. |
I think we're okay there. I tested with Godot 4.2.2 to be sure, and it still works as expected except for an unrelated issue with simple_spawner.gd. Groups defined in the scene file still appear in the block, although global groups are pretty helpful in this case. |
Indeed. Since the implementation is just to scrape them out of the ProjectSettings properties, all that will happen on 4.2 is that there are never global groups. |
Actually, this is an existing problem in the picker. When the context has changed and blocks are being reused, the context node change needs to be passed down to the block extension. I think I have something working. |
In the category display, blocks reused when switching the current BlockCode node. When that happens, the BlockExtension needs to have its context update so that it can provide options for the current BlockCode node. https://phabricator.endlessm.com/T35647
Several blocks work with node groups. Rather than having the user enter the group, offer the current groups as options. This is trickier than expected since Godot offers no API to get the current groups. Finding them involves 2 parts: * Get scene local groups by calling get_groups() on each node in the scene. * Get project global groups (starting in 4.3) by getting the `global_group` values in the project settings. Monitoring for changes to the global groups is possible by listening for project settings changes. Monitoring for local group changes is not possible since the only events that occur are internal to Godot and not available in GDScript. https://phabricator.endlessm.com/T35647
dbnicholson
force-pushed
the
T35647-group-option
branch
from
d4ce733 to
b6b4944
Compare
|
I changed a few things:
|
| func refresh_context(): | ||
| if _context and _block_extension: | ||
| _block_extension.context_node = _context.parent_node |
There was a problem hiding this comment.
Can this be a signal handler for _context.changed, instead, or does that do something weird? (Oh noo so many changed signals!). Alternatively I don't mind if we move this stuff over to BlockExtension itself. For BlockExtension I didn't want to add anything more than that one context_node property until it was needed, but as long as individual extensions aren't poking at BlockEditorContext it's fine.
There was a problem hiding this comment.
That was my initial instinct, but that's not what we want. If the BlockExtension watched for context changes, then the options would change every time you changed the block code node. We only want to do this in the picker where the blocks are reused for all contexts. I presume that's why you passed in the context node to BlockExtension instead of having it just grab the current context.
There was a problem hiding this comment.
Oh, yeah, I see where you're coming from there. Thanks for the explanation!
|
This looks good to me! I'll merge it now :) |
| display_template = "is in group {group: NIL}" | ||
| code_template = "is_in_group(\\\"{group}\\\")" | ||
| defaults = { | ||
| "group": SubResource("Resource_d0v0d") | ||
| } |
There was a problem hiding this comment.
Ack, I completely missed this change! We shouldn't have needed to change group to a NIL :( I'll make a PR fixing this in a moment.
There was a problem hiding this comment.
To explain what's going on, previously we needed to do that for options lists because the value was reproduced verbatim in this case, but now it can be treated as a string and auto-quoted just like any other string parameter. The old behaviour was used in places, and that can be achieved by using something like {{param}} in the code template to substitute just the raw value.
There was a problem hiding this comment.
What was the change that made that different? I've been basing my work on 41afa00 where several string types changed to NIL.
There was a problem hiding this comment.
Oh, I see what happened there! Some crossed wires :) I think I switched them to NIL to keep the previous behaviour of not being able to snap weird things to options lists, but then #219 happened around the same time so it was kind of moot, and the parameter type is indeed pretty irrelevant to anything. The only thing that's important is we don't need to do anything special to quote the strings here.
The thing that caused the behaviour change in that commit is where we remove the special case for TYPE_OBJECT in raw_input_to_code_string (seeing as option values are no longer OptionData objects).
Several blocks work with node groups. Rather than having the user enter the group, offer the current groups as options. This is trickier than expected since Godot offers no API to get the current groups. Finding them involves 2 parts:
Get scene local groups by calling get_groups() on each node in the scene.
Get project global groups (starting in 4.3) by getting the
global_groupvalues in the project settings.Monitoring for changes to the global groups is possible by listening for project settings changes. Monitoring for local group changes is not possible since the only events that occur are internal to Godot and not available in GDScript.
https://phabricator.endlessm.com/T35647
This is a naive implementation. The groups are specific to the scene and project. Rather than have each
Blockmaintain its own list, it should be gathered in theBlockCodenode or even theBlockCodePlugin. I spent far too long trying to make that work well, but I decided to just put up this simple implementation now.As noted above, the biggest problem is that the scene local groups are not monitored. So, if you add a node to a group, the Blocks won't be updated with the new group. Also, since the
add_to_groupblocks only offer existing groups, it takes away the means of creating scene local groups dynamically. That may not be that bad, though, since that would only happen at runtime and you wouldn't be able to query for the presence of the group.