Add dynamic options lists for blocks #206

dylanmccall · 2024-08-29T01:33:25Z

We need a way to customize blocks depending on the node being edited. To address this, I found myself doing a few things:

Replaced the big, global CategoryFactory with a BlockBuilder that is instantiated for the currently selected block script. I don't super like how I did this - in particular that both picker.gd and block_canvas.gd instantiate their own (redundant) BlockBuilder instances. Getting it right means further adjusting how data flows between these things.
(I don't actually like the name BlockBuilder since it implies we're using the Builder pattern, which was my original intent but it is not what it is doing. Probably just BlockFactory would make sense).
But the big thing this gives us is BlockBuilder has a single function which generates a single big list of blocks, including variable blocks. It knows what node our block script belongs to. I think that is a reasonable thing for it to know, but we could specify some rules here like "don't do anything that makes it impossible to move a block script to another node."
With that said, I broke the variables category here - categories in general are a disaster in this branch - as well as
setup_custom_blocks() in node scripts.
I added a concept of extension scripts for block definitions. This is an extra GDScript property, so you can optionally attach a plain old Object with some functions defined. I did this for animationplayer_play.tres. We should take the time to actually specify an API here, and perhaps a BlockExtensionScript base class just to be sure. For now, I just have animationplayer_play.gd with func get_defaults_for_node(context_node: Node). It's a little broken still.
I broke a bunch of tests :(

https://phabricator.endlessm.com/T35564

manuq · 2024-09-05T13:02:39Z

addons/block_code/code_generation/block_extension.gd

+# FIXME: We should use some kind of prefix for these things. Is this okay?
+class_name EBC_BlockExtension


Yes I love the idea of a EBC_ prefix for every class that needs a name. We started to follow this pattern in f3cbacb for internal scripts:

const BlockExtension = preload("res://addons/block_code/code_generation/block_extension.gd")

But this doesn't allow extending the class like in extends EBC_BlockExtension.

I got rid of this here since there are dozens of other new things in this branch, but let's file away a wishlist item somewhere about adding a prefix :)

Is that... a bike shed?! I feel like we either want underscores or camel case but not both. So, EbcSomeClass seems like the natural continuation of the camel case way.

dylanmccall · 2024-09-12T04:18:32Z

Okay, finally back on track here. The remaining work is pretty much go through and delete some now-obsolete code, fix some GUT tests, reorganize commits, and fix one stubborn new bug (probably from clumsy rebasing) where a block script isn't loading if it has loose non-Entry blocks placed in it.

While I was here, I tore out some unnecessary optimization around BlocksCatalog / BlockFactory / CategoryFactory. I feel a bit guilty doing it, but I don't think we need to do any kind of indexed lookups here. A function that iterates through all the blocks and looks at their category names and then forgets about it is fine for the scale at hand.

A brave person in the future might come back and improve our plumbing a bit so we're passing a single BlockFactory from main_panel.gd down to its children instead of creating two different ones (one for the canvas and one for the picker; and every block has a reference to the factory that created it so it can go and create output parameter blocks, which I don't much like but I'm tired). An even braver person could go through all the places where we're resetting objects then making them again and make them not reset those objects unless we're sure they've changed, because using a stateful BlockFactory makes that all a bit worse. But I think we're okay for the moment.

There's a bunch here, but one new thing to highlight: I didn't like how BlocksCatalog was generating a bunch of block definitions ahead of time for output parameters. So I took that out, and now BlockFactory has a single instantiate_block_by_name which has some smarts to generate a block right there if the name follows a particular "block_name:parameter" format.

dylanmccall · 2024-09-13T05:21:34Z

One last opinionated change here: my template editor / options list stuff came with a side-effect in our generated code: the AST to code block stuff helpfully wraps strings in quote marks, except when those strings come from an options list. We can't do that anymore since the value it sees is always just a string now :)

To address that, I added a way to specify a parameter substitution that won't be quoted: just {{parameter}} instead of {parameter}.

Also, note there is a sneaky error which the CI doesn't like, printing an incomprehensible error message as a result. If you happen to spot what that is while I'm out on Friday I'll be very happy on Monday.

dbnicholson

I haven't gotten too far, but I wanted to share some review so far since I have to run around a bit today.

dbnicholson · 2024-09-13T15:39:07Z

addons/block_code/ui/block_factory.gd

+var block_code_node: BlockCode
+var context_node: Node:
+	get:
+		return block_code_node.get_parent() if block_code_node is Node else null


Why is block_code_node here? As far as I can tell, the only reason for it is one use of context_node in Block._get_parameter_defaults. It seems easy enough to add the context node to Block.

In an earlier version of this I had BlockFactory setting defaults from an extension script itself, but I ended up moving that to Block itself so this is indeed very superfluous here. It might need to come back later if we want our set of available blocks to involve other nodes in the scene, which I had in mind here but it isn't a thing we've reached any decisions on.

dbnicholson · 2024-09-13T15:39:38Z

addons/block_code/serialization/block_script_serialization.gd

@@ -71,46 +44,46 @@ func get_categories() -> Array[BlockCategory]:
 	return []


-func generate_ast_list() -> ASTList:
+func generate_ast_list(block_factory: BlockFactory) -> ASTList:


This makes the serialization to code generation dependent on the UI again since BlockFactory depends on BlockCodeNode.

Oops! The circular dependency will be resolved with the other change removing BlockFactory.block_node, and I'll see if that lands us on block_factory being a property of BlockScript (which would certainly make some sense).

dbnicholson · 2024-09-13T16:21:56Z

addons/block_code/ui/block_factory.gd

+	_available_blocks.clear()
+	_available_blocks.append_array(_get_default_block_definitions())
+	_available_blocks.append_array(_get_inherited_block_definitions())
+	_available_blocks.append_array(_get_variable_block_definitions())


This looks like it's doing exactly the same thing that was in BlockScriptSerialization. Why was it moved here? It seems like you could continue to have all the block definition handling continue to live there. If the point is that we want to have a per-script set of block definitions, then the script object would seem to be the place to have that. What would be in BlockFactory would be BlockDefinition to Block conversion and category collection.

I'll shuffle this around a bit and see if I can flatten these things out a bit and it should reduce the diff a bunch. Mainly this happened because I was frustrated with the number of different places where we gather up block definitions, although that frustration was more with the code before #147. Looking at this at the moment, I'm extremely annoyed that _get_variable_block_definitions is accessing block_script.variables, so maaaybe BlockScript should be more involved there. I'll leave a note once when I've addressed that!

dbnicholson · 2024-09-13T16:38:54Z

addons/block_code/ui/block_factory.gd

+	_init_block_definitions()
+
+
+func instantiate_block(block_definition: BlockDefinition) -> Block:


This was a static function before, but I don't see any reason why it isn't anymore. Now that it's a method, it requires a BlockFactory instance. That appears to be the only reason for Block.block_factory.

It ended up like that so we can get at the list of available categories and set Block.color accordingly. This way we can lose Util.get_category_color, and for instance, DragManager doesn't need to copy the color property in its copy_block function. (Although actually I forgot to remove that).

But really the only reason for Block.block_factory is so the ParameterOutput component has something on which to call _block_factory.instantiate_block_by_name(). That whole scheme should be improved, but I think we'd want to dig in to DragManager to do it. For now, perhaps I can make ParameterOutput and DragManager get this information in a more special way (the current BlockCanvas could just be a static somewhere) so we don't need to have a general-purpose Block.block_factory.

Maybe the thing to do here is to keep a reference to either the current BlockScriptSerialization or BlockCodeNode instance. One or the other encompasses the block coding scene with which you'd have custom or property specific block definitions. With access to the current BlockScriptSerialization, you can query or add BlockDefinitions as needed.

instantiate_block_by_name is really nothing more than finding the BlockDefinition and creating a Block scene from it. Similarly, the available categories are already stored in the BlockScriptSerialization instance and getting the color is just a straight dictionary lookup.

addons/block_code/blocks/graphics/animationplayer_play.gd

manuq

This is working like a charm for animations! I will leave to @dbnicholson the code review. I appreciate how the last commit improves the UI for the "is action pressed/released" block, which was extra large. On the other hand, I see a bug in that one putting the action name verbatim when it should be quoted for it to work:

This then fails with error:

Parser Error: Identifier "ui_up" not declared in the current scope.

dylanmccall · 2024-09-18T23:09:06Z

This is all rebased, passing tests, and ready to review :)

dbnicholson

This is great! I left some nitpicks and comments, but nothing that would stop this from being merged now. I'm going to leave this for a couple hours to see if @manuq has any further input, and then I'll merge.

dbnicholson · 2024-09-19T15:26:23Z

addons/block_code/serialization/block_script_serialization.gd

-	_var_block_definitions.clear()
-	for block_def in BlocksCatalog.get_variable_block_definitions(variables):
-		_var_block_definitions[block_def.name] = block_def
+func instantiate_block(block_definition: BlockDefinition) -> Block:


I'd still prefer the UI parts to be entirely out of here, but I think you've shed enough tears on this refactoring. I certainly like this much better than the previous scenario.

dbnicholson · 2024-09-19T15:31:28Z

addons/block_code/ui/block_editor_context.gd

+		return block_code_node.get_parent()
+
+
+func set_block_code_node(value: BlockCode):


This could probably be the setter for block_code_node, but no big deal. If it was the setter, then I don't think you even need the internal _block_code_node.

Yeah, I'll do that :) I had it as a setter at one point and then it was doing other stuff so I felt better with it being a function, but it isn't doing other stuff at the moment.

dbnicholson · 2024-09-19T15:31:58Z

addons/block_code/ui/block_editor_context.gd

@@ -0,0 +1,38 @@
+class_name BlockEditorContext


This whole class is lovely. Nice work!

I second that!

dbnicholson · 2024-09-19T15:57:02Z

addons/block_code/ui/blocks/control_block/control_block.tscn

@@ -95,4 +95,5 @@ shift_top = 20.0
 [node name="SnapPoint" parent="VBoxContainer" instance=ExtResource("3_nhryi")]
 layout_mode = 2

+[connection signal="modified" from="." to="." method="_modified"]


This seems wrong. The modified signal triggers _modified, which emits modified... Or am I reading this all wrong?

Oh, yes, that was connected to the wrong thing! Ugh, that's a dreadful category of errors. I just remembered block.gd actually knows about its TemplateEditor now, so it can do all of the signal propagation in gdscript instead of the current scheme.

dbnicholson · 2024-09-19T15:57:37Z

addons/block_code/ui/blocks/statement_block/statement_block.gd

-	arg_name_to_param_input_dict = format_string(self, %HBoxContainer, definition.display_template, definition.defaults)
-
-
-static func format_string(parent_block: Block, attach_to: Node, string: String, _defaults: Dictionary) -> Dictionary:


So long, StatementBlock.format_string. It was nice knowing you.

dbnicholson · 2024-09-19T16:56:55Z

addons/block_code/ui/blocks/utilities/parameter_input/parameter_input.gd

+		return
+
+	if current_value is OptionData:
+		# Temporary hack: previously, the value was stored as an OptionValue


s/OptionValue/OptionData/

dbnicholson · 2024-09-19T17:17:22Z

addons/block_code/ui/blocks/utilities/template_editor/template_editor.gd

@@ -114,13 +114,11 @@ func _append_input_parameter(parameter: Dictionary, id: int):
 	parameter_input.placeholder = parameter["name"]
 	parameter_input.variant_type = parameter["type"]

-	if parameter["is_option"] and default_value is OptionData:


FWIW, is_option is never set in the current PR. I guess it must have existed in an earlier version.

addons/block_code/blocks/graphics/animationplayer_play.gd

manuq · 2024-09-19T18:11:17Z

@dylanmccall @dbnicholson I gave it a quick test, and found the following error. Adding a "change rotation by" to the "on process" leaves the _on_process empty, thus failing with syntax error.

EDIT

In the output I see:

  res://addons/block_code/code_generation/block_ast.gd:120 - Trying to assign value of type 'float' to a variable of type 'String'.
  res://addons/block_code/code_generation/block_ast.gd:120 - Trying to assign value of type 'float' to a variable of type 'String'.

This changes BlockScriptSerialization to manage its own list of available block definitions and instantiate them, replacing the existing functionality that was in CategoryFactory and ui/util.gd. The change makes it easier to understand the state of the block catalog, because it is a component of the block script itself instead of spread across different static variables. In addition, this adds a singleton block editor context, which keeps track of the current block code node and its associated block script. The context object provides enough information for different UI components to update automatically when the block script changes. https://phabricator.endlessm.com/T35564

Instead, the list of blocks by class name is generated as needed inside get_blocks_by_class.

An extension script can implement functions which refine a block's presentation depending on the current context, such as the node it is attached to. https://phabricator.endlessm.com/T35564

This extension customizes the block's "animation" options list using the list of animations in the block script's context node. https://phabricator.endlessm.com/T35564

This new component replaces StatementBlock.format_string in favour of a more declarative API. https://phabricator.endlessm.com/T35564

Instead of inferring that there is a list of options based on the current value, ParameterInput expects to be given a list of options, and get_raw_input and set_raw_input understand that the raw input is a value from the list. In addition, change TemplateEditor itself to determine the list of options for a given parameter. This option data is always from the parameter_defaults property, regardless of the block's current value. https://phabricator.endlessm.com/T35564

If the current value is not already in the options list, add it to the list. https://phabricator.endlessm.com/T35564

Instead, we infer that a parameter has a set of options by the existence of an OptionData as its default value. In addition, add a way for a code template to include a plain, unquoted string by using `{{parameter}}`. Previously this happened automatically for any parameter whose value was an OptionData. https://phabricator.endlessm.com/T35564

It is possible for some options to be very long, causing a block to appear wider than necessary. https://phabricator.endlessm.com/T35564

Instead of getting specific blocks we know to have target_node_class "", change BlocksCatalog to include those blocks in the return value from get_inherited_blocks. This is possible due to the newly simplified implementation of _get_blocks_by_class. With this change, we can completely remove the list of default block definition names from BlockScriptSerialization.

With a block code script created using a previous version of the plugin, it is possible for a node's value to still be an OptionData object at the time BlockAST.format_code_template is called. https://phabricator.endlessm.com/T35564

dylanmccall · 2024-09-19T18:30:02Z

@dylanmccall @dbnicholson I gave it a quick test, and found the following error. Adding a "change rotation by" to the "on process" leaves the _on_process empty, thus failing with syntax error.

EDIT

In the output I see:
  res://addons/block_code/code_generation/block_ast.gd:120 - Trying to assign value of type 'float' to a variable of type 'String'.
  res://addons/block_code/code_generation/block_ast.gd:120 - Trying to assign value of type 'float' to a variable of type 'String'.

Oops, that was a dumb mistake in 0bd3c26189adc755faa7bcc0d5973ef628879115. Fixed :)

manuq

All looks good from my side and the last bug I found is already fixed.

manuq · 2024-09-19T18:21:34Z

addons/block_code/ui/block_editor_context.gd

@@ -0,0 +1,38 @@
+class_name BlockEditorContext


I second that!

manuq · 2024-09-19T18:24:45Z

addons/block_code/ui/blocks/utilities/template_editor/template_editor.gd

@@ -0,0 +1,132 @@
+@tool
+class_name TemplateEditor


I really like the TemplateEditor, much better than the formatting used before.

manuq · 2024-09-19T18:29:03Z

addons/block_code/serialization/block_script_serialization.gd

@@ -176,64 +175,6 @@ func load_object_script() -> Object:
 	return null


-func _get_default_block_definitions() -> Array[BlockDefinition]:


Great code removal! Having this dynamic is much better.

manuq · 2024-09-19T18:32:31Z

addons/block_code/code_generation/block_ast.gd

+		if argument_value is OptionData:
+			# Temporary hack: previously, the value was stored as an OptionData
+			# object with a list of items and a "selected" property. If we are
+			# using an older block script where that is the case, convert the
+			# value to the value of its selected item.
+			# See also, ParameterInput._update_option_input.
+			argument_value = argument_value.items[argument_value.selected]


I don't think that we need to maintain backwards compatibility, there has been wild refactors since the last release. It doesn't harm to have this anyways.

dylanmccall force-pushed the T35564-add-block-builder branch 2 times, most recently from f6ed144 to 739c5d4 Compare September 4, 2024 01:45

manuq reviewed Sep 5, 2024

View reviewed changes

dylanmccall force-pushed the T35564-add-block-builder branch 3 times, most recently from 59f7c04 to dd230f9 Compare September 6, 2024 01:20

dylanmccall mentioned this pull request Sep 6, 2024

Decouple UI from Code Generation #147

Merged

dylanmccall force-pushed the T35564-add-block-builder branch 2 times, most recently from b553433 to 4a51075 Compare September 12, 2024 04:09

dylanmccall force-pushed the T35564-add-block-builder branch 11 times, most recently from a48e1cb to c8bb9e3 Compare September 13, 2024 04:53

dylanmccall marked this pull request as ready for review September 13, 2024 04:54

dylanmccall changed the title ~~T35564 add block builder~~ Add dynamic options lists for blocks Sep 13, 2024

dylanmccall requested review from starnight, dbnicholson and manuq September 13, 2024 04:55

dylanmccall force-pushed the T35564-add-block-builder branch from c8bb9e3 to bcf406e Compare September 13, 2024 05:04

dbnicholson reviewed Sep 13, 2024

View reviewed changes

manuq requested changes Sep 13, 2024

View reviewed changes

dylanmccall force-pushed the T35564-add-block-builder branch 4 times, most recently from 7d48c1a to 74aed6d Compare September 18, 2024 22:37

dylanmccall mentioned this pull request Sep 18, 2024

Add is_input_actioned.tres #222

Merged

dylanmccall force-pushed the T35564-add-block-builder branch from 6f7ae2d to 1a6ceec Compare September 19, 2024 05:10

This was referenced Sep 19, 2024

Block code demo scene #223

Draft

Add breakpoint block #224

Merged

dbnicholson approved these changes Sep 19, 2024

View reviewed changes

dylanmccall added 12 commits September 19, 2024 11:26

BlocksCatalog: Remove by_class_name cache

4118bae

Instead, the list of blocks by class name is generated as needed inside get_blocks_by_class.

BlockDefinition: Support extension scripts

dc99328

An extension script can implement functions which refine a block's presentation depending on the current context, such as the node it is attached to. https://phabricator.endlessm.com/T35564

animationplayer_play: Add an extension script to generate an option list

3517792

This extension customizes the block's "animation" options list using the list of animations in the block script's context node. https://phabricator.endlessm.com/T35564

Add TemplateEditor to generate UI based on a block format string

81b6456

This new component replaces StatementBlock.format_string in favour of a more declarative API. https://phabricator.endlessm.com/T35564

ParameterInput: Add current value to options list

2cfacea

If the current value is not already in the options list, add it to the list. https://phabricator.endlessm.com/T35564

ParameterInput: Change option input to fit only the selected item

4144034

It is possible for some options to be very long, causing a block to appear wider than necessary. https://phabricator.endlessm.com/T35564

BlockAST: Move some duplicated code to a function

33133a6

BlockAST: Convert OptionData to selected value

a14eed5

With a block code script created using a previous version of the plugin, it is possible for a node's value to still be an OptionData object at the time BlockAST.format_code_template is called. https://phabricator.endlessm.com/T35564

dylanmccall force-pushed the T35564-add-block-builder branch from d4f9cb2 to a14eed5 Compare September 19, 2024 18:30

manuq approved these changes Sep 19, 2024

View reviewed changes

Block: Connect template_editor modified signal automatically

e812651

dylanmccall merged commit 760a579 into main Sep 19, 2024
3 checks passed

dylanmccall deleted the T35564-add-block-builder branch September 19, 2024 18:51

dbnicholson mentioned this pull request Sep 24, 2024

Generate value blocks for AnimationPlayer animations #191

Closed

		# FIXME: We should use some kind of prefix for these things. Is this okay?
		class_name EBC_BlockExtension

		_init_block_definitions()


		func instantiate_block(block_definition: BlockDefinition) -> Block:

		return block_code_node.get_parent()


		func set_block_code_node(value: BlockCode):

		arg_name_to_param_input_dict = format_string(self, %HBoxContainer, definition.display_template, definition.defaults)


		static func format_string(parent_block: Block, attach_to: Node, string: String, _defaults: Dictionary) -> Dictionary:

		@@ -176,64 +175,6 @@ func load_object_script() -> Object:
		return null


		func _get_default_block_definitions() -> Array[BlockDefinition]:

Add dynamic options lists for blocks #206

Add dynamic options lists for blocks #206

Uh oh!

Conversation

dylanmccall commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylanmccall commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylanmccall commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbnicholson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

manuq left a comment

Choose a reason for hiding this comment

Uh oh!

dylanmccall commented Sep 18, 2024

Uh oh!

dbnicholson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

manuq commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylanmccall commented Sep 19, 2024

Uh oh!

manuq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylanmccall commented Aug 29, 2024 •

edited

Loading

dylanmccall commented Sep 12, 2024 •

edited

Loading

dylanmccall commented Sep 13, 2024 •

edited

Loading

manuq commented Sep 19, 2024 •

edited

Loading