Uploading a sprite should check for conflicting variable IDs

[kchadha]

Expected Behavior

No two distinct variables in a given project should have the same ID.

Actual Behavior

Downloading and then uploading a sprite with local variables also duplicates variable IDs. If the sprite is uploaded into the same project it was downloaded from, this will cause problems with variable monitors (when we start exporting variable monitors in sprites).

E.g. Sprite1 has local variable foo, and we download and upload Sprite1 into the same project, creating a new sprite Sprite2 with variable foo. The variable block referring to foo in the toolbox is shared by both sprites, causing problems e.g. you can only have the monitor for one of the local variables up at any time.

[lintense]

Hi guys, I suggest you merge this issue with https://github.com/LLK/scratch-vm/issues/2429

[apple502j]

@lintense This is not #2429 . The author said they never uploaded sprites.

[lintense]

Ok, let me explain a bit:

This current bug is caused by a conflict among local variables. But https://github.com/LLK/scratch-vm/issues/2429 is actually touching the heart of the local variables issue. IMHO resolving #2429 would fix this one as a side effect.

Please read my comment on #2429 on how to reproduce both issues.

[apple502j]

There are 3 issues:

1766 - sprite upload

1375 - backpack

2429 - unknown causes

So solving #2429 does not solve these, since we do not know how #2429 is caused

[lintense]

Hi again, and thanks for your patience!

Here is my take:

1766 - No two distinct variables in a given project should have the same ID.

• Local variables from 2 distinct sprites (even if they are copies of each other) are distinct.

1375 - Resolve Variable Conflict Issues When Adding Code From the Backpack

• From what I can tell: a) 2 Global variables can be merged -> Conflict solved. b) 2 Local variable are distinct -> Conflict can be solved by giving them distinct IDs. (ie. Prefixing local variable ID with Sprite ID) c) One global and One local -> Conflict!!! The local variable could be given a distinct ID making them distinct. (ie. Prefixing local variable ID with Sprite ID) But in this case, the global variable will not be visible from the sprite with the local variable.

2429 - Local variables are shared between sprites

Actual issue being: Local variables with the same name are shadowed by the first variable created. (As if the first local variable with that name is global but only for the sprites using it - basically the same behavior as global)

• This is bad because it renders a local variable "shared" among the sprites that use it. A "normal" sprite does not expect its local variables to be global.
• This will obviously breaks the behavior of all those sprites and so, it is bad.

My point:

Fixing #2429 by prefixing local variable ID with their owner Sprite ID will render these distinct. This would obviously fix all the above cases.

In any case, the backpack is not the problem. The problem is the consolidation of the local variables between the sprites.

Hope this will help!

P.S. My suggestion of "Prefixing local variable ID with Sprite ID" is not based on my knowledge of the code base (which I have not) but to give an easy to understand idea of how it could work. P.S.2. From what I understand, #2429 seems to be caused by 2 distinct local variables having the same internal ID (probably because they have the same name).

[lintense]

Regarding your previous comment: This is not #2429 . The author said they never uploaded sprites.

Step to reproduce:

Part 1 1-Create a new project 2-Create a copy of the defaut sprite (aka Sprite2) 3-Go in Sprite1 and Create a new LOCAL variable "v" 4-Go in Sprite2 and Create a new LOCAL variable "v" Result: Both local variable are visible in the animation view and their value can be change independently (This is working perfect!)

Part 2 1-Create a new project 2-Create a new LOCAL variable "v" in the default sprite 3-Create a copy of the defaut sprite (aka Sprite2) Result: Only local variable of Sprite1 is visible in the animation view and only Sprite1 can actually update its value. (Sprite2:v is completely shadowed) Expected result: The same as in part 1.

As stated before, the backpack has nothing to do as it simply reproduce the part 2 result.

[apple502j]

While the sprite ID idea seems good, we have to understand that project.json limit exists. (On Scratch 3.0 server, 5MB is the limit - which can be easily exceeded if you have lots of scripts) Every block that references the variable (except sensing_of) stores variable ID somehow, so this is not going to work because some existing projects wouldn't save because effectively the length of ID doubles.

The core problem is, when a variable is added, sometimes they don't check conflicts - so we make issues for each cases where they didn't check conflicts, because this is how we solve the problem. (for example, uploading sprites, adding sprites from backpack, etc) #2429 describes the issue that happens (but the condition is unknown)

[apple502j]

Note: duplicating sprite does not cause ID conflict (and they should not) - see https://github.com/LLK/scratch-vm/blob/develop/src/engine/target.js#L432 and https://github.com/LLK/scratch-vm/blob/develop/src/sprites/rendered-target.js#L996

[lintense]

apple502j: While the sprite ID idea seems good... lintense: Maybe the "actual name" (Sprite ID + local var name) can be put into some lookup table that can be consulted when uploading/duplicating/creating and would then return a shorter ID. (Remember, this only has to be done for local variable hence limiting the size of such a table)

apple502j: The core problem is, when a variable is added, sometimes they don't check conflicts... lintense: Yes, this is exactly what I think!

apple502j: Note: duplicating sprite does not cause ID conflict... lintense: Just try it...

[apple502j]

@lintense I read the code and it doesn't seem like it causes ID conflict. I made a local variable, duplicated sprite, set the variable to 1 on the old sprite, and on the second sprite I checked the variable value which was 0.

Maybe the "actual name" (Sprite ID + local var name) can be put into some lookup table that can be consulted when uploading/duplicating/creating and would return a shorter ID.

You are making problems more complicated... we just need to modify uploading/duplicating/creating codes so that IDs are randomized...

[lintense]

@apple502j As long as the local variables with the same name are distinct this should be fine.

I'm not a proponent of randomized ID. Indeed, how can you insure there will be no collisions? But this is aside the point...

[apple502j]

@lintense Mathematics ensure that it's very (*100000) unlikely to get ID collision. There are 617141927185296106289002325476403184801 (87^20) possible IDs. If you are worried about random ID collision, you should be more worried about IPv6 address collision, because there are 340282366920938463463374607431768211456 (2^128) possible IPv6 addresses.

[lintense]

@apple502j So what is the status as of now? Do you have enough info to keep going? Tell me when this would be available so I can help you with quality control.

apple502j: Mathematics ensure that it's very (*100000) unlikely to get ID collision: Still... You must be aware that these randomized IDs are meant to be used in distributed system where no centralized ID provider is available (who's complicating things here...) and have mitigation measures such that only a subset of all the possibilities are available at a any given time (precisely to avoid collisions). But, as I said earlier, this is aside the point...