Revamp variable system by wnbaum · Pull Request #107 · endlessm/godot-block-coding

wnbaum · 2024-07-02T20:56:10Z

This removes the old variable system we had with the VAR_DICT, and replaces it with a menu for creating typed variables that you can use as blocks.

Side note: We will need to fix the sound blocks after this is merged, since they use the VAR_DICT. I think it would be good to have some library functions or a singleton our blocks can call on for more behavior like this.

https://phabricator.endlessm.com/T35535

addons/block_code/ui/block_canvas/node_block_canvas/node_block_canvas.gd

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

addons/block_code/types/types.gd

dbnicholson · 2024-07-09T19:29:28Z

After punting on the compatibility, I still have a few questions.

Can you add the commit that manually migrates the sound blocks away from VAR_DICT? I think that's useful to see what the actual effect of this change is.
Is this blocked on https://phabricator.endlessm.com/T35536 or do you want this to go in now?
Have you discussed the UI changes with the learning team? I think there's no doubt the menu/dialog is an improvement, but it would be useful to have some design input.

wnbaum · 2024-07-09T19:48:15Z

Can you add the commit that manually migrates the sound blocks away from VAR_DICT? I think that's useful to see what the actual effect of this change is.

Sure, the sound blocks using VAR_DICT felt kinda hacky to me when it was added, so it'll be good to fix that.

Is this blocked on https://phabricator.endlessm.com/T35536 or do you want this to go in now?

Definitely not blocked, only really needed T35536 if we were going to make a migration system, which we will hold off on for now.

Have you discussed the UI changes with the learning team? I think there's no doubt the menu/dialog is an improvement, but it would be useful to have some design input.

I haven't! Will definitely get some feedback and fix the sound blocks.

wnbaum · 2024-07-11T02:08:52Z

Converted to draft for now as I work on fixing the sound blocks, which has brought up more of a problem than I expected. Will continue tomorrow, but push my working but ugly solution now. Also have to rebase.

wnbaum · 2024-07-11T21:18:09Z

Ok, after fixing some weird bugs, it seems to be working great.

New variables system is working as before.

You can now define unique local variables in block statements by suffixing them like var test__, which will give them an ID in the generated code like var test_1. This fixes the sound blocks, and also same-name for loop index variables. VAR_DICT is no more.

Edit: Just added one last fix to remove dictionary that can be replaced by type_string()

dbnicholson

Overall, I like this a lot. Thanks for changing the sound blocks in pong, too. That shows the practical implication of this kind of change.

addons/block_code/ui/block_canvas/node_block_canvas/node_block_canvas.gd

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

wnbaum · 2024-07-12T20:39:49Z

Thanks for the review @dbnicholson there's definitely some changes I should make here.

dylanmccall

I'm happy with this. A couple small suggestions for things to tweak. It bothered me that variables is a property of BlockScriptData instead of part of block_trees, but I see how it would be difficult to do the latter with our current design.

Some UX things which I can understand leaving for a follow-up, but I'll note them here for the moment. In this screenshot…

Because of the notch at the bottom, the "Set Name to …" block looks like it's closer to "Age" (below it) than to "Name" (above it).
I'm finding all this white on orange text kind of difficult to parse. Two reasons: shallow colour contrast, and it's just a lot of orange. We definitely need to have a good discussion about block design in general at some point.
You didn't touch it here, but I'm not sure if that "To String" block is doing anything for us.

dylanmccall · 2024-07-12T18:15:18Z

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

-		VAR_DICT[{name}].set_stream(load({file_path}))
-		add_child(VAR_DICT[{name}])
+		var sound__ = AudioStreamPlayer.new()
+		sound__.name = {name}


Can we prefix this name with something to reduce the opportunity for clashes?

I think that's a good idea even though with the suffixed counter it's unlikely to clash. Still, I think the common convention is that you prefix internal variables with _ or __. Furthermore, we could add in something semi-unique like bcp (for Block Code Plugin). So, __bcp_<something>.

In fact, if this variable isn't going to be exposed to the user, then there's no need to name it like the thing it's for at all. So, you just declare all the internal variables __bcp. The generator recognizes that (without tricky regex matching or trimming any parts) and suffixes the generated variable with a <counter> or _<counter>.

I think the whole point of generating the script like this is to have it exposed to the user, otherwise we could just have a library function like Lib.play_sound() that would bypass the need for local variables anyways.

That's also why I implemented a counter, instead of having a unique ID appended to each variable, because it makes the code look a lot nicer. Anyways, a prefix will definitely help here since we can also prevent the user from creating variables with our prefix.

That's fair. I think just _ or __ prefix is good enough to make conflict with a user supplied variable extremely unlikely.

dylanmccall · 2024-07-12T18:16:23Z

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

-		VAR_DICT[{name}].volume_db = {db}
-		VAR_DICT[{name}].pitch_scale = {pitch}
-		VAR_DICT[{name}].play()
+		var sound_node__ = get_node({name})


That we're adding a node to the scene and then just getting it like this really does hint at other approaches to blocks like this one (what's stopping us from telling developers to "add an AudioStreamPlayer in the scene and use a block to call play() on it"). But that's for another day ;)

addons/block_code/ui/picker/categories/variable_category/create_variable_button.gd

addons/block_code/ui/picker/categories/variable_category/create_variable_button.tscn

Instead of accessing variables by VAR_DICT, use GDScript properties. Add a new "variables" property to block script data resource. Add a menu for creating variable of specific type. Validate new variable name and check for conflicts with existing variables.

Allows us to declare local variables in block scripts with unique names. Uses new static functions in InstructionTree to handle this. For example, "var __temp = 0" in a block statement will become "var temp_1 = 0" in the generated code.

wnbaum · 2024-07-16T00:04:44Z

Ok, I made a bunch of requested changes. Ready for review!

Local variable ID counter is reset at the beginning of each entry block
Variables now use the raw name the user puts in and are checked for validity/conflicts with existing variables and possible local variables
Local variables are now prefixed with __ rather than suffixed
To String Block removed
Spacer added between groups of variable blocks in the picker for clarity
Variable create dialog scene separated from variable create button scene

dbnicholson

Admittedly I kinda glossed over the last set of changes, but it looks like you addressed the things that I commented on. Time to land this plane!

wnbaum requested review from dbnicholson, dylanmccall and manuq July 2, 2024 20:56

wnbaum force-pushed the variables-revamp branch from 0d5bb1a to 93fd0c0 Compare July 3, 2024 18:17

dbnicholson reviewed Jul 3, 2024

View reviewed changes

addons/block_code/ui/block_canvas/node_block_canvas/node_block_canvas.gd Outdated Show resolved Hide resolved

addons/block_code/ui/picker/categories/category_factory.gd Show resolved Hide resolved

manuq reviewed Jul 5, 2024

View reviewed changes

addons/block_code/types/types.gd Outdated Show resolved Hide resolved

dbnicholson mentioned this pull request Jul 10, 2024

Add version to block script data #122

Merged

wnbaum marked this pull request as draft July 11, 2024 02:07

wnbaum force-pushed the variables-revamp branch 2 times, most recently from 9363cee to e7af2df Compare July 11, 2024 21:15

wnbaum marked this pull request as ready for review July 11, 2024 21:18

wnbaum force-pushed the variables-revamp branch from e7af2df to 81f4637 Compare July 11, 2024 21:22

wnbaum requested a review from dbnicholson July 11, 2024 21:27

dbnicholson requested changes Jul 12, 2024

View reviewed changes

dylanmccall reviewed Jul 12, 2024

View reviewed changes

dbnicholson mentioned this pull request Jul 15, 2024

Test serialization #132

Merged

wnbaum added 4 commits July 15, 2024 19:53

Revamp variable system

16537da

Instead of accessing variables by VAR_DICT, use GDScript properties. Add a new "variables" property to block script data resource. Add a menu for creating variable of specific type. Validate new variable name and check for conflicts with existing variables.

Add type string to generated code

725e945

Unique local variables

e4f1e35

Allows us to declare local variables in block scripts with unique names. Uses new static functions in InstructionTree to handle this. For example, "var __temp = 0" in a block statement will become "var temp_1 = 0" in the generated code.

Fix old sound blocks. Use local variables instead of VAR_DICT

aaf9f2d

wnbaum force-pushed the variables-revamp branch from 81f4637 to aaf9f2d Compare July 15, 2024 23:57

wnbaum requested review from dbnicholson and dylanmccall July 16, 2024 00:05

dylanmccall approved these changes Jul 16, 2024

View reviewed changes

dbnicholson approved these changes Jul 16, 2024

View reviewed changes

dbnicholson merged commit 2bbe016 into main Jul 16, 2024

dbnicholson deleted the variables-revamp branch July 16, 2024 04:37

Conversation

wnbaum commented Jul 2, 2024 • edited by wjt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dbnicholson commented Jul 9, 2024

Uh oh!

wnbaum commented Jul 9, 2024

Uh oh!

wnbaum commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wnbaum commented Jul 11, 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!

Uh oh!

Uh oh!

Uh oh!

wnbaum commented Jul 12, 2024

Uh oh!

dylanmccall left a comment • 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!

dylanmccall Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

dbnicholson Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

wnbaum Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

dbnicholson Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

dylanmccall Jul 12, 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!

Uh oh!

Uh oh!

wnbaum commented Jul 16, 2024

Uh oh!

dbnicholson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wnbaum commented Jul 2, 2024 •

edited by wjt

Loading

wnbaum commented Jul 11, 2024 •

edited

Loading

wnbaum commented Jul 11, 2024 •

edited

Loading

dylanmccall left a comment •

edited

Loading

dylanmccall Jul 12, 2024 •

edited

Loading