Scratch-vm: Dropped sprite names that are numbers do not work

[ericrosenbaum]

Expected Behavior

It should be possible to drop a variable that reports a number onto an input that expects a sprite name in order to select a sprite with a name that is a number.

This bug affects any block that accepts a sprite name as an input:

• Go to
• Glide to
• Point towards
• Create clone of
• Distance to
• X position of

For example, here the bananas are a sprite named "3" and dropping blocks reporting the number 3 onto the "distance to" block works correctly in Scratch 2.0:

I saw this issue while trying out the "chaos game" project, which currently does not work at all in 3.0, because it uses both "point toward" and "distance to" with dropped numeric values to refer to sprites:
https://scratch.mit.edu/projects/243588792/
https://llk.github.io/scratch-gui/develop/#243588792

Actual Behavior

In 3.0, the same blocks report 10,000:

Steps to Reproduce

• Try stacks as in screenshots above
• Note that the "chaos game" project does not work: https://llk.github.io/scratch-gui/develop/#243588792

[ericrosenbaum]

I'm not sure whether the runtime's getSpriteTargetByName function should do a cast to string itself, or if each block implementation should do the cast. We generally do casting like this in the block implementations, but obviously that is error prone already, where we miss cases (e.g. the current bug). cc @cwillisf

[thisandagain]

/cc @kchadha @ktbee

[ktbee]

Thought I'd note something interesting I found while repro-ing: when you don't use the "round" block, the Scratch Cat sprite is able to say the correct distance to the sprite with the numeric name. The "round" block might be part of the culprit here.

[joker314]

@kchadha Yeah, the "round" block returns a JavaScript Number, whereas the raw variable block returns a JavaScript String. We still need to have the distinction because things like the "switch to costume" blocks take the type of the data into account if there is a costume with a name that is a number.

In Scratch 2.0, it can be seen that double equality (i.e. with type coercion) is used at the level of the function which gets a sprite by name: https://github.com/LLK/scratch-flash/blob/develop/src/scratch/ScratchStage.as#L148

I would therefore argue that we _should_ be able to safely do the same in Scratch 3.0 without breaking things @ericrosenbaum

[kchadha]

@ktbee, I think @joker314 meant to tag you in that response.

In regards to @ericrosenbaum's question, I looked at all the places where getSpriteTargetByName is used in the VM and it is used solely in block functions with the exception of isTouchingSprite which in turn is used by block functions. Thus, I think it's reasonable check if the input has type number in that function and convert it to a string to look up the target. That would make writing new block functions which reference sprite names less error prone. I would still double check that the switch costume to and switch backdrop to blocks still work correctly as @joker314 mentioned.

See the tests @joker314 added in #1517 for more details on how those blocks should behave.

[ktbee]

Thank you for taking a look @joker314 @kchadha! I also dug into this a bit and have a better sense of @ericrosenbaum's question now. Initially I was thinking the same thing as @kchadha, that we should just always cast the sprite name to a string if we need to in getSpriteTargetByName to avoid this issue in the future and to avoid repeating similar logic in multiple blocks. However, we may want to continue this practice of putting the string casting in blocks for now (in this issue, in the "distance to" block) because @mzgoddard let me know that he has plans to tackle this string casting elsewhere in the VM as part of his performance optimizations and take this logic out of the blocks entirely.

@kchadha for the time being, do you have a preference between fixing this one block or casting to a string in getSpriteTargetByName? If we do the latter, should I also remove the other places we cast to a string in blocks (making sure we don't break @joker314's tests)?

[kchadha]

@ktbee, I'm fine with fixing it in the original blocks if there is going to be an optimization for the casting. Maybe file an issue for that so that @mzgoddard's work can reference that and this discussion can be tracked.

For fixing it in the original blocks, it would be good to make sure that it's fixed in all of the places that @ericrosenbaum's original description mentions:

* Go to
* Glide to
* Point towards
* Create clone of
* Distance to
* X position of

[kchadha]

@ktbee I'm fine with either approach. If you go with the former, it would be good to track the idea of needing to cast in one place in an issue (so that this discussion can be tracked and @mzgoddard can reference the issue when that's been fixed).

If you decide to fix it in the individual block definitions, it would need to get fixed for the following blocks mentioned in @ericrosenbaum's PR description (and any others that have a pluggable input that could possibly reference a sprite name):

This bug affects any block that accepts a sprite name as an input:

• Go to
• Glide to
• Point towards
• Create clone of
• Distance to
• X position of

[ktbee]

Sounds good @kchadha! And thank you for (re)enumerating the blocks that should have the string casting added, I had missed that in @ericrosenbaum's original description. I'll file the optimization issue for @mzgoddard and add string casting to those blocks.

[ktbee]

Resolved by https://github.com/LLK/scratch-vm/pull/2038