Decouple block type from block variant

[manuq]

And use Variant.Type enum for the Value block variant.

This means we stick to GDScript variants for the values represented by Value blocks:

• A constant like "viewport width"
• A property like "rotation"
• A parameter like A in "A + B"
• The resulting value of "A + B"

The slots that can contain Value blocks also have a matching variant type now.

Unfortunately there is no way to convert the native Variant.Type enum to string, so still we need dictionaries to go back and forth the string formatting.

Previously there was a custom NODE type, although unused. This is now replaced by NODE_PATH, which is the same thing that Godot does in the PackedScene resource when exporting a property of type Node.

Also: improve readability of drag manager and use a constant for the minimum slot distance.

[wnbaum]

Great! Much better than what we had before, thanks for working together on this!

Again, my only concern is that having the type NodePath instead of Node doesn't really make sense. When we say something is of Node type, we mean that it has some specified properties and methods. The properties and methods of Node are completely different to the ones in the NodePath type, which is really just a String path that points to a node (of any class) relative to the root. But for now, I suppose TYPE_NODE_PATH can correspond to anything that is a Node.

I'd love to hear some other opinions on this too!! Is it worth integrating Godot's type system with ours? Or should we define the types ourselves (i.e. with Strings)?

[starnight]

For the original custom NODE, is it Godot's NODE: Base class for all scene objects?

[manuq]

Great! Much better than what we had before, thanks for working together on this!

Again, my only concern is that having the type NodePath instead of Node doesn't really make sense. When we say something is of Node type, we mean that it has some specified properties and methods. The properties and methods of Node are completely different to the ones in the NodePath type, which is really just a String path that points to a node (of any class) relative to the root. But for now, I suppose TYPE_NODE_PATH can correspond to anything that is a Node.

You can easily get a node from a node path.

Think it in another way: how are we going to persist a node in a resource? Try creating a new resource and then add @export var foo: Node. You will get an error saying:

res://test_game/my_resource.gd:3 - Parse Error: Node export is only supported in Node-derived classes, but the current class inherits "Resource".

I'd love to hear some other opinions on this too!! Is it worth integrating Godot's type system with ours? Or should we define the types ourselves (i.e. with Strings)?

Yes! Other opinions welcome!

[wjt]

I think this makes sense. It is sad to have to enumerate all the built-in variant types by hand. It is weird that Variant.Type does not behave as a dict (which has a find_key method) while user-defined enums do.

[wjt]

I don't think you mean to include the literal "snap_point"snap string in the message here?

[wjt]

Suggested change

	push_error("Warning: a snap point does not reference it's parent block.")
	push_error("Warning: a snap point does not reference its parent block.")

But can we be more specific about which snap point is problematic?

[wjt]

The "HACK" comment here refers to "NONE" but you're changing it to NIL here.

[manuq]

I think this makes sense. It is sad to have to enumerate all the built-in variant types by hand. It is weird that Variant.Type does not behave as a dict (which has a find_key method) while user-defined enums do.

Yeah, I don't think we need to do this mapping at all in the future. We should embrace "the resource way" and get rid of this custom serializing.

[manuq]

I'm merging this as it's a big refactor. Sorry everyone, I can help to resolve conflicts in your work in progress.

[wnbaum]

Think it in another way: how are we going to persist a node in a resource? Try creating a new resource and then add @export var foo: Node. You will get an error saying:

res://test_game/my_resource.gd:3 - Parse Error: Node export is only supported in Node-derived classes, but the current class inherits "Resource".

@manuq Well we wouldn't really be persisting a Node itself, just it's type right? Is the type not just a way of specifying what a variant is? That's why I suggested Strings, since we would be able to have more than just the types set in Variant.Type. I think there is more to Godot's type system than just Variant.Type. For example, if I do var camera: Camera2D, I am specifying that yes, my variant is an Object, but it's also specifically a subclass of Node2D < CanvasItem < Node. Ideally we would have a way to incorporate this into our block type system.

[manuq]

@manuq Well we wouldn't really be persisting a Node itself, just it's type right?

I think yes, we should be persisting the node path and gettng the node at runtime. In the same way as PackedScene does.

Is the type not just a way of specifying what a variant is? That's why I suggested Strings, since we would be able to have more than just the types set in Variant.Type.

I don't know what kind of blocks are you thinking, but for sure the BlockType enum can be extended to have "BlockType.ELEFANT" for example. The BlockType.VALUE is meant for representing Variants. Maybe this will be more clear once we stop doing our own serialization/deserialization and use Resources all around.

I think there is more to Godot's type system than just Variant.Type. For example, if I do var camera: Camera2D, I am specifying that yes, my variant is an Object, but it's also specifically a subclass of Node2D < CanvasItem < Node. Ideally we would have a way to incorporate this into our block type system.

Yes, for sure, but not for persisting. Variants can be serialized to text, be serialized as binary and stored to disk, transferred via network, etc.

[wnbaum]

I guess I'm just not understanding why we need our VALUEs to be of one of the Variant.Types, or even serializable at all. For example, in the _on_body_entered(body) method, it returns a body which is a Node. This node and its path changes all the time depending on what happens during runtime - we never persist that specific node to a block script. So the type of the value in this case should be Node, and therefore it should only be able to snap into a slot of type Node. In my eyes, the only reason to add typing is to guarantee things can snap together correctly, without generating errors. The only time we DO persist things is in the ParameterInput, where the user's raw text input gets saved. There shouldn't be any raw input for a Node anyways, the user cannot instantiate a new node in the parameter input.

So I guess I'm saying I see no benefit to having Variant.Type as the type of our variant_type. We are only using variant_type as a specifier (for snapping). Maybe it could make ParameterInput slightly cleaner, but it's already been implemented. And since there are limited options in using Variant.Type as a specifier (doesn't use subclassing or class names at all), we should just turn variant_type into a String. That way we could define blocks like Set velocity {physics_body: PhysicsBody2D} that only takes in rigid bodies and character bodies. In the current system we could only do Set velocity {object: OBJECT} which would generate an error for nodes or even snapped objects that don't have the right property.

For now I think it's okay if we just keep every Node input of type OBJECT, that way we aren't limited in our block scripts. However, this could cause runtime errors and I do think eventually it will be important to have inputs with class types.