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

WIP: Add named classes for final blocks #58

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

dylanmccall
Copy link
Contributor

Instead of defining blocks in category_factory.gd, define blocks ahead of time, each in their own GDScript file. Although it is not a concept in GDScript itself, we can consider block classes such as EntryBlock to be abstract block types.

By doing this, we are able to reduce duplication in the block script resource associated with each BlockCode node. In particular, the serialized data can simply refer to "PrintBlock", and all of the EntryBlock properties expected for the print block are implied.

This commit only adds ReadyBlock and PrintBlock as an example, and likely breaks everything else.

@dylanmccall dylanmccall force-pushed the block-classes branch 2 times, most recently from 1164e21 to 3024dd7 Compare June 19, 2024 22:00
Instead of defining blocks in category_factory.gd, define blocks ahead
of time, each in their own GDScript file. Although it is not a concept
in GDScript itself, we can consider block classes such as EntryBlock to
be abstract block types.

By doing this, we are able to reduce duplication in the block script
resource associated with each BlockCode node. In particular, the
serialized data can simply refer to "PrintBlock", and all of the
EntryBlock properties expected for the print block are implied.

This commit only adds ReadyBlock and PrintBlock as an example, and
likely breaks everything else.
@wjt
Copy link
Member

wjt commented Jun 24, 2024

At a high level I think this is the right direction: ignoring the Godot type system and scene/script split for a moment, I think conceptually it makes more sense for “Print block” to be a type, which gets instantiated; and the only data that should need persisting is where it is in the program, its type, and the user-supplied parameter.

The current setup where the prototype print block in the sidebar is actually an instance of the statement block type, with its label in the block editor and the template for the generated GDScript, doesn't quite feel right, particularly because those properties get persisted into the user's game once per instance of the block despite being shared between all print blocks (and so you can't adjust the generated code without recreating every print statement or manually editing the scene files).

The big block of boilerplate in _init seems a bit sad and although I suggested a code generator on Slack I think that would still make the cure worse than the disease. I think what you are trying to avoid is having to create print_block.tcsn which extends entry_block.tcsn; and you want to ensure that any properties that were set on EntryBlock are propagated onto this subclass; before then overriding the important ones.

Having experimented a little, and with the hint about abstract types in your commit message, I think we could do this a bit differently with less boilerplate. In statement_block.gd, replace the @export var block_format etc. instance variables with:

static func get_block_format() -> String:
    return ""

Then define print_block.gd as follows:

class_name PrintBlock
extends StatementBlock

static func get_block_format() -> String
    return "Print {text: STRING}"

...

(I did a quick experiment and calling get_block_format() from the superclass does call the overridden implementation from the subclass.)

Then to instantiate the print block we have 3 options:

  1. Define a scene for it – this is the simplest option, though it's a few extra clicks to define one and involves committing a lot of boilerplate to the repo that we're never going to edit. I can see why you wanted to avoid that.
  2. Override _init as you have done – now that I understand it, it's a clever trick! Once all the property-copying and -setting logic is gone I think it would boil down to:
func _init():
	var _block = load(StatementBlock.get_scene_path()).instantiate() as Node
	_block.replace_by(self, true)
	_block.queue_free()

and (apart from infinite recursion issues) I think you could actually move this to the superclass (!).

  1. In the category factory, instantiate the parent scene but replace its script:
	b = BLOCKS["statement_block"].instantiate()
        b.set_script(load("res://.../print_block.gd"))

I do not know how to persist that to disk, but if you .duplicate() an instanced scene that has had its script replaced in this way, the substitute script comes along with the duplicate as you would hope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants