T35536 decouple mid-term option #164

manuq · 2024-07-26T18:26:05Z

Hey team, here is my alternative to the UI/data decoupling. I avoided refactors and did the minimum changes to move the block properties that don't change (like label) to its own model class.

@wnbaum maybe this can be an inbetween for the AST separation you are working on?

https://phabricator.endlessm.com/T35536

wnbaum

Tested on my end, and everything is working the same, though the block scripts in the pong_game example scene should probably be resaved since the block trees are not loaded in the proper locations.

Interesting use of StringName, for the block name property, I didn't know it was much more performant for comparisons and dictionary use, which is used a lot in my PR - we'll have to switch it over.

I also like how you are instantiating the block scene based on the data model, something that I also do in my PR.

This is great and it does a lot for only a little refactoring. I still think it will be very useful to have the code generation and serialization options from my PR, since the structure (code blocks and indentation) of the script is still technically defined by the UI here.

I'm wondering though - all of the benefits implemented here are also present in my PR. If we merge this, I'll either have to replace a lot of your code with mine, or try to integrate your model into mine (which I think will prove difficult). Is it really worth it to have this intermediate step then? I'm a little unclear of the benefits given I've already implemented a lot of it.

manuq · 2024-07-29T15:19:43Z

Tested on my end, and everything is working the same, though the block scripts in the pong_game example scene should probably be resaved since the block trees are not loaded in the proper locations.

Good point, Will! I will update the Pong example.

Interesting use of StringName, for the block name property, I didn't know it was much more performant for comparisons and dictionary use, which is used a lot in my PR - we'll have to switch it over.

I also like how you are instantiating the block scene based on the data model, something that I also do in my PR.

Yes, the only small difference is that you went for a Resouce for the model, and I thought a RefCounted would be enough (after looking at BlockCategory from @dbnicholson). What would be the benefit of using resouce for the model?

This is great and it does a lot for only a little refactoring. I still think it will be very useful to have the code generation and serialization options from my PR, since the structure (code blocks and indentation) of the script is still technically defined by the UI here.

I'm wondering though - all of the benefits implemented here are also present in my PR. If we merge this, I'll either have to replace a lot of your code with mine, or try to integrate your model into mine (which I think will prove difficult). Is it really worth it to have this intermediate step then? I'm a little unclear of the benefits given I've already implemented a lot of it.

Considering that the primary goal is to decouple the model from the UI to allow more flexibility in blocks (localization, method overloading, etc), I think this is the wanted part. On top of the decoupling, you are doing interesting R&D and architectural changes which IMO would be better to compare in a separate step. That way it will be clear what are the benefits of having a parallel AST, how the sync with the UI tree is handled, if the stored resources are constantly changing or being reused, etc.

manuq · 2024-07-29T20:44:31Z

I have rebased this with main branch now. But I couldn't update the Pong demo yet because of #172 . We'll have to fix that one first.

dbnicholson · 2024-07-29T23:48:39Z

As a minor refactor I like this a lot. The model Block is a bit more of the "pure data" object while the UI Block is more clearly a UI node with SerializedBlockNode bridging them. As I mentioned over in #147, naming everything Block* is fairly confusing. It would be nice if we could settle on some nomenclature over the 3 states - UI, code and serialized.

I'm a bit torn here. I feel like the endpoint of #147 is ultimately what I want to get to as it has a clearer separation with the AST as the intermediate state between the UI and the serialization. The downside is that it's a big set of changes. While there are parts of this PR I like, I'm a little worried that it will be mostly thrown away in #147 with extra busy work.

One thing that happened here and in #147 is that the way block elements are generated was changed. While both methods are an improvement and maybe required to rework the data model, it's a lot of churn. I don't feel strongly about either method (instantiate and modify an object vs serialized resources), but it would be good to pick one way to do it.

manuq · 2024-07-30T17:12:15Z

I finally updated the Pong example. So yes let's discuss if this is wanted or not!

manuq · 2024-07-30T21:05:54Z

We have decided to go with this after adjusting a couple things so #147 can be rebased on top:

Switch from RefCounted to Resource for the blocks definition.
Rename things properly to avoid confusion.

I'll change this to draft until is ready for review.

dbnicholson · 2024-07-31T17:15:14Z

addons/block_code/blocks_catalog.gd

+const Types = preload("res://addons/block_code/types/types.gd")
+
+static var _created: bool = false
+static var _catalog: Dictionary


I think since the dictionary defaults to empty, the _created boolean is not needed. You can just check if _catalog: return.

Addressed! Thanks.

manuq · 2024-07-31T19:05:23Z

Making this draft until we release.

The block definition must have a unique name and a type. It will be used to instantiate Block scenes. https://phabricator.endlessm.com/T35536

The catalog is where block definitions are loaded. For now, only "On Ready" and "Print" block definitions are ported. https://phabricator.endlessm.com/T35536

Given a block unique name, instantiate a Block scene from the block definition in the catalog. https://phabricator.endlessm.com/T35536

For now only "On Ready" and "Print" blocks are ported. https://phabricator.endlessm.com/T35536

Change type to StringName, to match the block model. And remove the empty string default because it's mandatory. https://phabricator.endlessm.com/T35536

Name, position, and children blocks are the only properties needed to be stored. The rest can be derived from the definition. And leave a TODO note for removing the serialization once the port is complete. https://phabricator.endlessm.com/T35536

Position is not part of the serialized properties anymore. https://phabricator.endlessm.com/T35536

And fallback to the previous behavior if the block isn't ported to the catalog yet. The scene position is set to the resource position. https://phabricator.endlessm.com/T35536

For Block, StatementBlock and EntryBlock classes. Only if the block is in the block catalog. Otherwise keep the previous behavior. The exceptions are: - Color: it should be derived from the category, but needs refactoring - Scope: may be handled in a different way

To the new format.

manuq · 2024-08-01T18:02:37Z

I just rebased this with main, ready for review. The renames on top are in #178

wnbaum

Everything is looking great here!

One thing you could do is load the blocks in the blocks catalog from files since they are resources now, but this is also done in my PR. Also, I feel like it would make sense to port over some methods from CategoryFactory to the blocks catalog script, since something like get_builtin_blocks() would belong better in something like that.

Anyways, this is all stuff I can do in my PR. The data model is a lot closer to where we want it with a small amount of changes. Let me know if you want to make those changes here but I think this is ready to merge @manuq.

wnbaum · 2024-08-02T16:31:21Z

addons/block_code/ui/block_canvas/serialized_block_tree_node.gd

@@ -1,10 +1,16 @@
 class_name SerializedBlockTreeNode
 extends Resource

-@export var serialized_block: SerializedBlock
+@export var name: StringName
+@export var position: Vector2


Another thing here: Why does every SerializedBlockTreeNode have its position serialized? Only the top level block really needs its position saved. Anyways, it still works fine, with nested blocks having position (0,0), and I wouldn't change it here unless you really want to because it allows you to make very minimal changes to BlockCanvas.

Another thing here: Why does every SerializedBlockTreeNode have its position serialized? Only the top level block really needs its position saved. Anyways, it still works fine, with nested blocks having position (0,0), and I wouldn't change it here unless you really want to because it allows you to make very minimal changes to BlockCanvas.

Yes, I thought about this. For a snapped/attached block, as soon as the user detaches it by dragging it to the canvas, it will need a position. So I thought that it is better to keep the position as property to all the saved blocks, and just ignore it if they are attached. What do you think?

Yep, that's totally fine. It makes the code a little simpler in this case!

manuq · 2024-08-02T19:09:25Z

Everything is looking great here!

One thing you could do is load the blocks in the blocks catalog from files since they are resources now, but this is also done in my PR. Also, I feel like it would make sense to port over some methods from CategoryFactory to the blocks catalog script, since something like get_builtin_blocks() would belong better in something like that.

Anyways, this is all stuff I can do in my PR. The data model is a lot closer to where we want it with a small amount of changes. Let me know if you want to make those changes here but I think this is ready to merge @manuq.

Yeah, feel free to move the definition to separate .tres files in your PR, as you have the code snippet to do it already. And yes, several methods should be moved from the UI side (CategoryFactory) to the blocks catalog. You can move them as you need to reach the code generation decoupling. I don't want to block your work any more!

wnbaum · 2024-08-02T19:20:21Z

Yeah, feel free to move the definition to separate .tres files in your PR, as you have the code snippet to do it already. And yes, several methods should be moved from the UI side (CategoryFactory) to the blocks catalog. You can move them as you need to reach the code generation decoupling. I don't want to block your work any more!

Sounds good! Seems like you accomplished what we wanted to do here, so I'm going to merge!

manuq · 2024-08-05T17:02:29Z

@wnbaum I forgot to tell you that this one had a fixup commit that needed to be squashed before merging. Thanks for the review & merge.

manuq requested review from wjt, dylanmccall, starnight, dbnicholson and wnbaum July 26, 2024 18:26

manuq force-pushed the T35536-decouple-alternative branch from 909d361 to cf32dc9 Compare July 26, 2024 18:55

wnbaum approved these changes Jul 26, 2024

View reviewed changes

manuq force-pushed the T35536-decouple-alternative branch from cf32dc9 to a563cdc Compare July 29, 2024 20:19

manuq force-pushed the T35536-decouple-alternative branch 2 times, most recently from 8956f34 to 6353070 Compare July 30, 2024 17:07

manuq mentioned this pull request Jul 30, 2024

Block Canvas: Reuse saved block scene resource #176

Merged

manuq force-pushed the T35536-decouple-alternative branch from 6353070 to 2c9de36 Compare July 30, 2024 21:03

manuq marked this pull request as draft July 30, 2024 21:06

manuq force-pushed the T35536-decouple-alternative branch from 2c9de36 to f7a5961 Compare July 31, 2024 16:35

manuq marked this pull request as ready for review July 31, 2024 16:38

manuq mentioned this pull request Jul 31, 2024

T35536 renames #178

Merged

dbnicholson reviewed Jul 31, 2024

View reviewed changes

manuq marked this pull request as draft July 31, 2024 19:05

manuq force-pushed the T35536-decouple-alternative branch from f7a5961 to 2d81768 Compare August 1, 2024 00:25

manuq added 5 commits August 1, 2024 14:21

Add block definition resource

9d62ccd

The block definition must have a unique name and a type. It will be used to instantiate Block scenes. https://phabricator.endlessm.com/T35536

Add Blocks Catalog

410fd7d

The catalog is where block definitions are loaded. For now, only "On Ready" and "Print" block definitions are ported. https://phabricator.endlessm.com/T35536

Util: Instantiate Block scene from catalog

87ed823

Given a block unique name, instantiate a Block scene from the block definition in the catalog. https://phabricator.endlessm.com/T35536

CategoryFactory: Instantiate blocks from definition

690136c

For now only "On Ready" and "Print" blocks are ported. https://phabricator.endlessm.com/T35536

Block scene: Change block_name property

edb88a6

Change type to StringName, to match the block model. And remove the empty string default because it's mandatory. https://phabricator.endlessm.com/T35536

manuq added 7 commits August 1, 2024 14:21

Block resource: Add name and position

5a772b7

Name, position, and children blocks are the only properties needed to be stored. The rest can be derived from the definition. And leave a TODO note for removing the serialization once the port is complete. https://phabricator.endlessm.com/T35536

Block resource: Update position from Block scene

6e17904

Position is not part of the serialized properties anymore. https://phabricator.endlessm.com/T35536

BlockCanvas: Use block definitions when loading

97cd48f

And fallback to the previous behavior if the block isn't ported to the catalog yet. The scene position is set to the resource position. https://phabricator.endlessm.com/T35536

Update template Block resource

46f1db2

To the new format.

Update Pong game example

ca1c8eb

fixup! Add Blocks Catalog

56c2031

manuq force-pushed the T35536-decouple-alternative branch from 2d81768 to 56c2031 Compare August 1, 2024 17:22

manuq marked this pull request as ready for review August 1, 2024 18:01

wnbaum approved these changes Aug 2, 2024

View reviewed changes

wnbaum merged commit a5d5b65 into main Aug 2, 2024
2 checks passed

wnbaum deleted the T35536-decouple-alternative branch August 2, 2024 19:20

T35536 decouple mid-term option #164

T35536 decouple mid-term option #164

Uh oh!

Conversation

manuq commented Jul 26, 2024

Uh oh!

wnbaum left a comment

Choose a reason for hiding this comment

Uh oh!

manuq commented Jul 29, 2024

Uh oh!

manuq commented Jul 29, 2024

Uh oh!

dbnicholson commented Jul 29, 2024

Uh oh!

manuq commented Jul 30, 2024

Uh oh!

manuq commented Jul 30, 2024

Uh oh!

dbnicholson Jul 31, 2024

Choose a reason for hiding this comment

Uh oh!

manuq Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

manuq commented Jul 31, 2024

Uh oh!

manuq commented Aug 1, 2024

Uh oh!

wnbaum left a comment

Choose a reason for hiding this comment

Uh oh!

wnbaum Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

manuq Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

wnbaum Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

manuq commented Aug 2, 2024

Uh oh!

wnbaum commented Aug 2, 2024

Uh oh!

Uh oh!

manuq commented Aug 5, 2024

Uh oh!

Uh oh!