[Google Blockly] Add customized input/output connectors in Sprite Lab by fisher-alice · Pull Request #51066 · code-dot-org/code-dot-org

fisher-alice · 2023-04-01T00:36:12Z

This PR customizes the input/output connector shapes in Google Blockly labs (such as Poetry) and adds constants for the Google Blockly renderers: geras, thrasos, and zelos and the default renderer which is now thrasos.

Currently in CDO Blockly labs, output blocks of certain types have customized connector shapes:

Sprite -> triangular shape
Behavior -> round shape
location -> rectangular shape

This PR customizes the output connector shapes of these type blocks and the input connector of blocks that accept these blocks in Google Blockly labs.

Google provides a nice Codelab tutorial on how to build a custom renderer and includes sections on how to 'Override constants' and 'Change connection shapes'.

Initially, when I added the cdoConstantsProvider class, it extended the Google Blockly's geras.ConstantProvider class since geras was our standard renderer. Then within cdoRenderer.js, I overrode the makeConstants_ function. Since geras uses highlights, the next step would have been to extend the Highlighter and HighlightConstantProvider classes associated with the geras renderer to fix the highlight paths.

After consulting with @maribethb about this, she suggested using the thrasos renderer which is the modern version of geras but doesn't have the highlights. She said that geras was designed to be identical to the "legacy" rendering code (pre-2019 when the new renderer system was added) and thrasos was designed to be the replacement. The Google Blockly team now explicitly recommends that new code use thrasos. I discussed this change with @mikeharv. He pointed out that thrasos is already used for Music Lab and for several reasons laid out in his already merged PR, it made sense to use thrasos as our default renderer.

So now, cdoConstantsProvider class extends the Google Blockly's blockRendering.ConstantProvider class. I change the name of the cdoRenderer class to cdoRendererGeras since geras is no longer our default renderer. Then I override the makeConstants_ function in cdoRendererThrasos to return our customized constants provider.

In cdoConstantsProvider.js, I override the shapeFor and init functions. The Codelab tutorial provides an example for a rectangular connection which I use for 'location' blocks. But I also add the functions to draw the svg paths for a rounded connection and a triangular connection. I used svgPaths utility functions provided by mainline Blockly.
In the shapeFor function, the input and output connector shapes are customized according to the blockTypeShapeMap and if not customized, then the standard 'puzzle' connector shape is used.

I also add constants in blockly/constants.js for the 3 renderers and the default renderer.

For the following videos, I use a Poetry lab which is a Google Blockly lab, and then copy and paste Sprite lab blocks with customized connector shapes. In order to do this, I use the 'blocklyVersion=CDO' in the Poetry lab to be able to paste these Sprite lab blocks and then go back to mainline Blockly in the Poetry lab.

BEFORE UPDATE:

production-poetry.mp4

As a comparison, here is a video of the blocks in Sprite Lab (CDO Blockly) currently in production. Note that there are no insertion markers ('shadow' of output or next block when close to compatible block).

sprite-lab-production.mp4

AFTER UPDATE:

after-update-fix-bug.mp4

There remained a bug that @maribethb helped me figure out - The output connector insertion markers were not rendered correctly. For example, on the 'location (200, 200)' block, the insertion marker's output connector shape was incorrectly a puzzle shape:

I was using the block style to map which shape to use for the output connector. However, an insertion marker is a block whose style does not match the block it is a marker for since markers are a different color. Thus, I now use connection.type to determine the shape of the output connector as I do for the input connector which also simplifies the code. I was thinking about a block that accepts more than one type as input such as the 'move [sprite] pixels [number] toward [location]' block which accepts a sprite, a number and a location type. But at each specific connector, the type accepted will always be the same!

Links

jira ticket - [Google Blockly] Create block socket shapes for Sprite Lab

Testing story

I tested locally.

Follow-up work

Deployment strategy

Privacy

Security

Caching

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

…-lab-socket-shapes

…ndererGeras

fisher-alice · 2023-04-08T23:26:13Z


 export const NOTIFICATION_ALERT_TYPE = 'notification';

-export const BlocklyVersion = {


Moved this constant to blockly/constants.js.

maribethb · 2023-04-14T20:57:26Z

+      [outputBlockStyleTypes.BEHAVIOR_TYPE]: this.ROUND_INPUT_OUTPUT,
+      [outputBlockStyleTypes.LOCATION_TYPE]: this.RECT_INPUT_OUTPUT
+    };
+    const blockStyleName = connection.getSourceBlock().styleName_;


I think the bug with the insertion markers is happening because insertion markers don't get the normal style name for the block they're a marker of. They are a different color so they don't have the same style. I did some testing and for insertion marker blocks, this comes up as auto_#000000 or something similar instead of sprite_blocks etc. This causes this lookup to fail and you fall back to the puzzle shape.

You should probably use the same connection.check_ value instead of trying to use the category of the block. For example, when debugging this I used text and number blocks since I don't have sprite and behavior blocks. But the text_count block outputs a number despite being in the text block category. If you went off the category of the blocks instead, you would have that block with a misleading shape. By making sure you are always basing it off the connection checks, you can make sure both sides of a connection will have the matching shape (barring edge cases like a block accepting both text and numbers, but I'll assume that can't apply to a block accepting both sprites and behaviors).

I hope this helps, sorry it took me a while to figure out the cause of this!

Thanks so much! Yes - this makes sense. I was making things more complicated than they had to be!
I was thinking about the 'move [sprite] pixels [number] toward [location]' block which accepts a sprite, a number and a location. But at each specific connector, the type accepted will always be the same.

maribethb · 2023-04-14T20:58:36Z

+     */
+    function makeMainPath(up) {
+      return Blockly.utils.svgPaths.line([
+        Blockly.utils.svgPaths.point(-width, (-1 * up * height) / 2),


btw I wonder if this should be GoogleBlockly here and elsewhere? I'm not sure exactly what Blockly refers to since you import GoogleBlockly in this file. This is just a random drive-by comment so feel free to ignore if it's not relevant.

Blockly is a global variable we assign here. And your comments are always welcome!

I think that means that Blockly and GoogleBlockly are kind of just different instances of same thing?

Earlier, I tried replacing GoogleBlockly with Blockly but got the following errors:

I think that perhaps we need both instances - one that is customized by our wrapper and one that is mainline?

@fisher-alice I think that both of these will have the same utilities so it maybe doesn't matter which we use... but if it's possible to use GoogleBlockly.utils without breaking anything, that feels a little tighter/cleaner.
Currently, this just relies on that googleblockly.js script; it's not something we're likely to change but I guess it still counts as a dependency.

That makes sense. Will do -thanks! Thanks @maribethb for the initial suggestion!

Missed your earlier reply/screenshot. I can see that we might need to use a separate instance here (from global/window). If it breaks things, no need to change!

Sorry late to the party here -- my immediate reaction is that I didn't think we should be referencing google blockly directly in our code -- we generally use the wrapper, which feels like the right practice to me. We don't currently reference GoogleBlockly directly anywhere else (unless we need it to subclass google blockly classes, as you are doing here), as far as I can tell?

That said, thinking about it more, I do think that using it directly in these "addons" that are only referenced in the wrapper does feel less circular than trying to use the wrapper (which we do elsewhere, and maybe should not, eg here) -- since we only are using this code in the wrapper (via the different renderers). Does that make sense to folks? Happy to chat offline if what I'm saying isn't super clear.

I don't have a strong opinion using GoogleBlockly vs the wrapper in the 'addons', but I do think we should be consistent, and you bring up a good point about how the wrapper is being used in other `addons'.
@mikeharv , thoughts?

After discussing offline, the group decided to reference GoogleBlockly considering comments above and also the eventual goal of using solely mainline and deprecating CDOBlockly.

mikeharv

Fantastic work on this, Alice! A few suggestions, mostly for comments. Since we're also slightly impacting Music Lab (in a good way), I also tagged that team so they'd have a chance to weigh in on Thrasos being a shared default across labs.

mikeharv · 2023-04-17T15:28:27Z

+    options.renderer = Renderers.ZELOS;
  } else if (experiments.isEnabled('geras')) {
-    options.renderer = 'cdo_renderer';
+    options.renderer = Renderers.GERAS;


Thanks for cleaning these up with constants!

mikeharv · 2023-04-17T15:39:23Z

+        -width * 1.5 + ', 0 ',
+        -width * 1.5 + ', ' + -1 * up * height + ' ',
+        '0, ' + -1 * up * height + ' '
+      ]);


Is there a link we could add with documentation for how these path values can be interpreted?

I added the link to the curve definition from mainline blockly which includes a link to MDN doc. Lmk if that is sufficient - thanks!

mikeharv · 2023-04-17T15:41:36Z

-    'cdo_renderer',
-    CdoRenderer,
+    Renderers.GERAS,
+    CdoRendererGeras,


Thanks for renaming!

mikeharv · 2023-04-17T15:45:43Z

-        ? 'cdo_renderer_zelos'
-        : 'cdo_renderer_thrasos',
+        ? Renderers.ZELOS
+        : Renderers.DEFAULT,


Calling out for Music Lab team (@breville @sanchitmalhotra126) that we recently updated our other labs to also use Thrasos as the default renderer. Thrasos is likely going to remain a firm requirement for Sprite Lab due to Alice's awesome work in this PR.

mikeharv · 2023-04-17T15:50:00Z

+     */
+    function makeMainPath(up) {
+      return Blockly.utils.svgPaths.line([
+        Blockly.utils.svgPaths.point(-width, (-1 * up * height) / 2),


@fisher-alice I think that both of these will have the same utilities so it maybe doesn't matter which we use... but if it's possible to use GoogleBlockly.utils without breaking anything, that feels a little tighter/cleaner.
Currently, this just relies on that googleblockly.js script; it's not something we're likely to change but I guess it still counts as a dependency.

mikeharv · 2023-04-17T15:52:07Z

+  GERAS: 'cdo_renderer_geras',
+  THRASOS: 'cdo_renderer_thrasos',
+  ZELOS: 'cdo_renderer_zelos',
+  DEFAULT: 'cdo_renderer_thrasos'


@breville @sanchitmalhotra126 see also ^

It's possible for Music Lab to still use Thrasos without implicitly agreeing that Music Lab is using whatever our default happens to be. Let us know if you have any concerns!

mikeharv

🎉

sanchitmalhotra126

Looks fine to me from a Music Lab perspective!

sanchitmalhotra126 · 2023-04-17T19:38:56Z

+    var pathUp = makeMainPath(1);
+    var pathDown = makeMainPath(-1);


nit: do these need to be vars?

Mainline uses var but I can update to be more consistent with our codebase.

fisher-alice added 15 commits March 30, 2023 10:09

add cdoConstantsProvider and override shapeFor

018f7ec

add triangle input/output path

636b108

add roundConnector

3581cef

Merge branch 'staging' into alice/sprite-lab-socket-shapes

488ccad

add styles shape map and customize input/output connectors via style

ff58f79

fix map constant

cdc9420

fix input connectors

fdd4ceb

add curve function

514d2fc

add constants

bd1f463

small tweaks

24a5eae

Merge branch 'staging' into alice/sprite-lab-socket-shapes

c5dd9ea

Merge branch 'staging' into alice/sprite-lab-socket-shapes

8481a25

use mainline def of curve

5ad49e3

fix lint

39ba3d3

Merge branch 'staging' into alice/sprite-lab-socket-shapes

df9b759

fisher-alice marked this pull request as ready for review April 6, 2023 16:18

fisher-alice requested review from a team and mikeharv April 6, 2023 16:18

fisher-alice marked this pull request as draft April 6, 2023 18:59

Mike Harvey added 2 commits April 6, 2023 16:03

use thrasos as default renderer

1fca9a4

update renderer experiment options

946fc54

mikeharv mentioned this pull request Apr 6, 2023

[Google Blockly] use thrasos as default renderer #51179

Merged

8 tasks

fisher-alice added 5 commits April 6, 2023 16:05

Merge branch 'mike/use-thrasos-as-default-renderer' into alice/sprite…

530e77d

…-lab-socket-shapes

add customized input/output connector shapes to thrasos, rename cdoRe…

212af72

…ndererGeras

merge with staging

a3d2c01

add renderer constants

7dbbf7f

move BlocklyVersion constant to blockly/constants

f8ab891

fisher-alice commented Apr 8, 2023

View reviewed changes

fisher-alice marked this pull request as ready for review April 9, 2023 00:16

maribethb reviewed Apr 14, 2023

View reviewed changes

fisher-alice added 2 commits April 14, 2023 17:47

simplify input/output connector types in shapeFor

ba4336d

rename map

7def1a1

mikeharv reviewed Apr 17, 2023

View reviewed changes

fisher-alice added 4 commits April 17, 2023 11:00

use GoogleBlockly utilities instead of Blockly

ff0f108

add comment about connection.check_

747f01d

add PUZZLE_TAB comment

c0d69b7

add defn of curve

3389371

fisher-alice requested review from breville, mikeharv and sanchitmalhotra126 April 17, 2023 19:18

mikeharv approved these changes Apr 17, 2023

View reviewed changes

sanchitmalhotra126 approved these changes Apr 17, 2023

View reviewed changes

change var to const

83a4533

fisher-alice merged commit 1441113 into staging Apr 18, 2023

fisher-alice deleted the alice/sprite-lab-socket-shapes branch April 18, 2023 21:03


		export const NOTIFICATION_ALERT_TYPE = 'notification';

		export const BlocklyVersion = {

		var pathUp = makeMainPath(1);
		var pathDown = makeMainPath(-1);

Uh oh!

Conversation

fisher-alice commented Apr 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Links

Testing story

Follow-up work

Deployment strategy

Privacy

Security

Caching

PR Checklist:

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!

maribethb Apr 14, 2023 • 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!

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!

bencodeorg Apr 17, 2023 • 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!

mikeharv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeharv left a comment

Choose a reason for hiding this comment

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fisher-alice commented Apr 1, 2023 •

edited

Loading

maribethb Apr 14, 2023 •

edited

Loading

bencodeorg Apr 17, 2023 •

edited

Loading