T35591 more block definitions #199

manuq · 2024-08-16T18:00:24Z

https://phabricator.endlessm.com/T35591

Add target node class to the definition for class-specific blocks. For example, "On body entered / exited" block is specific to a node that inherits RigidBody2D. https://phabricator.endlessm.com/T35591

manuq · 2024-08-16T18:04:04Z

Here is more porting to the new block definitions. Compared with @wnbaum PR, this is using resource files, not through code like in f390ae8#diff-d355da8c634915f2c506c52aaf96eba422696b3c4b7cf0362804bd673b3a4767R431-R438

starnight · 2024-08-19T09:31:03Z

Should we transform the data type of raw_input, before assign to _line_edit.text? It shows error with Godot 4.3:

res://addons/block_code/ui/blocks/utilities/parameter_input/parameter_input.gd:65 - Invalid assignment of property or key 'text' with value of type 'float' on a base object of type 'LineEdit'.

manuq · 2024-08-19T13:56:21Z

Should we transform the data type of raw_input, before assign to _line_edit.text? It shows error with Godot 4.3:
res://addons/block_code/ui/blocks/utilities/parameter_input/parameter_input.gd:65 - Invalid assignment of property or key 'text' with value of type 'float' on a base object of type 'LineEdit'.

Yes, thanks for pointing @starnight. I'll do it. The original PR has this in 0946f37 but that commit doesn't work well in isolation.

manuq · 2024-08-19T20:36:14Z

I migrated the property blocks and the custom blocks. But now I have to change the tests.

As defined by the target class in the resource file.

dylanmccall

I read through all this and it looks good to me! It would be nice to make the InputMap stuff happen without the hack in blocks_catalog, but I don't feel like we need to do that here. (And I was going to mess around with that type of thing in #191). This already lands us a significant upgrade from the previous setup :)

dylanmccall · 2024-08-20T20:51:53Z

addons/block_code/code_generation/blocks_catalog.gd

+static func get_blocks_by_class(_class_name: String):
+	if not _class_name in _by_class_name:
+		return []
+	var block_definitions = _by_class_name[_class_name] as Dictionary
+	return block_definitions.values()


Neat! Is the aim for this to replace the setup_custom_blocks thing we have right now (like with SimpleCharacter)?

Unfortunately no. See 028fa10 how I ported the custom blocks. Do you think they should be defined in resource files and added automatically to the catalog?

Yeah, that made sense to me once I got to the later commits :) I think that would be very clever if custom blocks were defined in resource files, but I can see how that's a tangent from the purpose of this branch. I'm tempted to play with that in another branch after I've done some more pressing things. A start would be to make BlocksCatalog._BLOCKS_PATH a list and solve from there.

@dylanmccall you made me think that the ResourceLoader is already loading these resource files, so we are actually loading them twice using DirAccess. In fact, the DirAccess docs say "use ResourceLoader to access imported resources". So maybe a better idea would be:

Add a custom extension to these resource files like .block or .blockdef.

Add a ResourceFormatLoader that can handle that extension and add the block definitions to the catalog as they load.

Then custom resource files can be added to the user project without caring about the filesystem path.

If we had a custom extension it would also help later for translation since we likely need a custom translation parser that matches by file extension. That said, I think we should punt for the moment and try to get this work landed.

dylanmccall · 2024-08-20T20:59:22Z

addons/block_code/ui/picker/categories/category_factory.gd

-		b = Util.instantiate_block(block_name)
+		b = Util.instantiate_block_by_name(block_name)


While we're here (already changing most references to b), can we switch away from the single-letter variable name, maybe in a commit right before this one? I will find this nicer to read if we change b to block, especially now that there's much less else going on here :)

Yes! It was bugging me as well.

dylanmccall · 2024-08-20T21:18:57Z

addons/block_code/ui/blocks/block/block.gd

-	return serialize_props(["color", "scope"])
+	return serialize_props(["scope"])


manuq · 2024-08-21T02:09:06Z

I read through all this and it looks good to me! It would be nice to make the InputMap stuff happen without the hack in blocks_catalog, but I don't feel like we need to do that here. (And I was going to mess around with that type of thing in #191). This already lands us a significant upgrade from the previous setup :)

Thanks for taking a deep look @dylanmccall . Now it should also work and tests are passing again. This is because the last commit which is basically @wnbaum 0946f37 plus some of 073eddd (the raw_input_to_code_string(input)).

manuq · 2024-08-21T02:17:24Z

Should we transform the data type of raw_input, before assign to _line_edit.text? It shows error with Godot 4.3:
res://addons/block_code/ui/blocks/utilities/parameter_input/parameter_input.gd:65 - Invalid assignment of property or key 'text' with value of type 'float' on a base object of type 'LineEdit'.

Adressed in commit 8d18e33

manuq · 2024-08-21T02:44:50Z

@starnight @dylanmccall @dbnicholson I think this is ready for a final review. Compared with the original PR #147:

This uses resource files for the per-class blocks like Area2D "on body entered / exited". For this, a new field @export var target_node_class: String was added to the BlockDefinition.
Like the original PR, defines custom blocks in each "Simple" class script.
The "is input actioned" block is defined in code too because the options need to be fed according to the running project. Maybe as a follow-up we can have a resource file and change the options dynamically?
The "get / set variable" blocks are defined in code too. And since these are per BlockScriptSerialization, I'm not adding them to the catalog. Because the whole point of a catalog is to have a unique name for the block that can be instantiated. But since I'm not adding them to the catalog, they end up serialized in the old way. I need to work on this as a follow-up. At least they still work, so there are no regressions.
The old serialization is not removed yet because of the above item.
The last remaining thing serialized is "scope". I need to look carefully to the original PR to see if this was addressed or not.

starnight · 2024-08-21T09:57:01Z

Thank you @manuq! It is a big code refactoring.

However, I notice if I add BlockCode under SimpleCharater and SimpleScoring node, it shows error:

Cannot get class 'SimpleCharacter'.
Cannot get class 'SimpleScoring'.

manuq · 2024-08-21T10:08:49Z

Thank you @manuq! It is a big code refactoring.

However, I notice if I add BlockCode under SimpleCharater and SimpleScoring node, it shows error:
Cannot get class 'SimpleCharacter'.
Cannot get class 'SimpleScoring'.

I see it now, thanks. It seems that the block catalog is trying to add their blocks too early. If I switch to another BlockCode and then back to the SimpleCharacter one, I see the custom blocks.

manuq · 2024-08-21T12:14:43Z

Thank you @manuq! It is a big code refactoring.
However, I notice if I add BlockCode under SimpleCharater and SimpleScoring node, it shows error:
Cannot get class 'SimpleCharacter'.
Cannot get class 'SimpleScoring'.
I see it now, thanks. It seems that the block catalog is trying to add their blocks too early. If I switch to another BlockCode and then back to the SimpleCharacter one, I see the custom blocks.

This is now fixed with the 2 last fixups. And the picker logic for custom blocks is much cleaner now! Because the _get_subclasses(_class_name) utility now handles custom classes. For example for SimpleCharacter it will get:

["SimpleCharacter", "CharacterBody2D", "PhysicsBody2D", "CollisionObject2D", "Node2D", "CanvasItem", "Node", "Object"]

This would welcome a unit test.

manuq · 2024-08-21T13:29:34Z

I just redid the Pong example with the new blocks in the PR. While doing it, I've found that there is a hack adding suffix "__nil__" in some parameter inputs in the original PR. I don't quite understand why that was needed in the original PR, but removing it makes things work again. So I appended another fixup.

dbnicholson

All looks good to me besides some nitpicks. Feel free to squash and merge when you're ready!

dbnicholson · 2024-08-22T19:38:53Z

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

 		_:
 			return "%s" % input
+	return ""


I think this is unnecessary since the _ case will catch everything.

I thought the same when looking at @wnbaum branch. But if I remove this line, the syntax parser complains for the function saying "Not all code paths return a value.". So I understand why he added it.

dbnicholson · 2024-08-22T19:46:20Z

addons/block_code/ui/util.gd

@@ -50,7 +50,7 @@ static func instantiate_block_by_name(block_name: StringName) -> Block:
 	return instantiate_block(block_definition)


-static func _get_subclasses(_class_name: String) -> Array[String]:
+static func _get_builtin_subclasses(_class_name: String) -> Array[String]:


Nitpick. These aren't subclasses (classes that inherit this class), they're parent or inherited classes (what this class inherits from).

Oh of course! Good catch, I'll rename the function.

For this, the previous instantiate_block is renamed to instantiate_block_by_name.

And remove them from the category factory. Also add a default value for speed in the characterbody2d_move block. https://phabricator.endlessm.com/T35591

And change the body type from NODE_PATH to OBJECT, as in previous definitions. https://phabricator.endlessm.com/T35591

https://phabricator.endlessm.com/T35591

Port the property setter/changer/getter blocks to the new block definition. These are not loaded from resource files, they are defined in code. https://phabricator.endlessm.com/T35591

Add a way in the blocks catalog to add custom blocks. Port SimpleScoring and SimpleCharacter to the new block definitions. Change the picker logic to instantiate these blocks. https://phabricator.endlessm.com/T35591

For easier debugging.

And split obtaining the inputmap action names to a inner function. https://phabricator.endlessm.com/T35591

Using the new block definition. These are the only blocks that are not stored in the catalog. Instead, they are dynamically generated when the BlockScriptSerialization changes. Also: Move constant for the builtin category properties to the constants.gd script. Also: Finally remove the preloaded scene blocks from the category_factory.gd script. The single place where those scenes are now instantiated is the ui/util.gd script. https://phabricator.endlessm.com/T35591

Stop persisting the color in the serialization. https://phabricator.endlessm.com/T35591

Co-authored-by: Manuel Quiñones <[email protected]>

To avoid single character names.

Block definition: Add target node class

ec266e4

Add target node class to the definition for class-specific blocks. For example, "On body entered / exited" block is specific to a node that inherits RigidBody2D. https://phabricator.endlessm.com/T35591

manuq requested review from dbnicholson, starnight and dylanmccall August 16, 2024 18:00

Block catalog: Add getter for blocks by class

4349a7a

As defined by the target class in the resource file.

manuq force-pushed the T35591-more-block-definitions branch from 703e5d1 to 16eac33 Compare August 20, 2024 20:05

dylanmccall approved these changes Aug 20, 2024

View reviewed changes

dbnicholson approved these changes Aug 22, 2024

View reviewed changes

manuq added 11 commits August 23, 2024 08:36

UI util: Instantiate all blocks by class

855c3dd

For this, the previous instantiate_block is renamed to instantiate_block_by_name.

Add CharacterBody2D block definitions

203b1de

And remove them from the category factory. Also add a default value for speed in the characterbody2d_move block. https://phabricator.endlessm.com/T35591

Add Area2D block definitions

4a1e52a

And change the body type from NODE_PATH to OBJECT, as in previous definitions. https://phabricator.endlessm.com/T35591

Add AnimationPlayer block definitions

2fbb3cb

https://phabricator.endlessm.com/T35591

Add AudioStreamPlayer block definitions

9514bbb

https://phabricator.endlessm.com/T35591

Add RigidBody2D block definitions

575e7c8

https://phabricator.endlessm.com/T35591

Blocks catalog: Add property blocks

dd1e0e9

Port the property setter/changer/getter blocks to the new block definition. These are not loaded from resource files, they are defined in code. https://phabricator.endlessm.com/T35591

Simple nodes: Port custom blocks

768de50

Add a way in the blocks catalog to add custom blocks. Port SimpleScoring and SimpleCharacter to the new block definitions. Change the picker logic to instantiate these blocks. https://phabricator.endlessm.com/T35591

Block definition: Add _to_string() method

4c6acb1

For easier debugging.

Blocks catalog: Port "is action pressed / released" block

e70ac87

And split obtaining the inputmap action names to a inner function. https://phabricator.endlessm.com/T35591

manuq and others added 4 commits August 23, 2024 08:36

Derive block color from category

9a0da76

Stop persisting the color in the serialization. https://phabricator.endlessm.com/T35591

Serialize user input as Variants instead of Strings

c037bfd

Co-authored-by: Manuel Quiñones <[email protected]>

Category factory: rename variable "b" to "block"

2d13c92

To avoid single character names.

Redo the Pong example

9dec121

manuq force-pushed the T35591-more-block-definitions branch from 64efb30 to 9dec121 Compare August 23, 2024 11:38

manuq merged commit aa80f0c into main Aug 23, 2024
3 checks passed

manuq deleted the T35591-more-block-definitions branch August 23, 2024 11:45

		b = Util.instantiate_block(block_name)
		b = Util.instantiate_block_by_name(block_name)

		return serialize_props(["color", "scope"])
		return serialize_props(["scope"])

T35591 more block definitions #199

T35591 more block definitions #199

Uh oh!

Conversation

manuq commented Aug 16, 2024

Uh oh!

manuq commented Aug 16, 2024

Uh oh!

starnight commented Aug 19, 2024

Uh oh!

manuq commented Aug 19, 2024

Uh oh!

manuq commented Aug 19, 2024

Uh oh!

dylanmccall 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

manuq commented Aug 21, 2024

Uh oh!

manuq commented Aug 21, 2024

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

manuq commented Aug 21, 2024

Uh oh!

manuq commented Aug 21, 2024

Uh oh!

manuq commented Aug 21, 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!

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

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!

Uh oh!

dylanmccall Aug 21, 2024 •

edited

Loading

dbnicholson Aug 22, 2024 •

edited

Loading

manuq commented Aug 21, 2024 •

edited

Loading

starnight commented Aug 21, 2024 •

edited

Loading

manuq commented Aug 21, 2024 •

edited

Loading

manuq Aug 23, 2024 •

edited

Loading