Remove _layerOrder and just store sprites in-order #210
Conversation
|
Nice! Glad to see compatibility improvements and simpler code. I've been meeting with some of the good folks working on Patch, a "Python Scratch" that serves a similar purpose as Leopard but for Python rather than JavaScript. They are updating their codebase to use Leopard as their underlying VM rather than their forked version of scratch-vm. (scratch-vm was quite messy to integrate with and Leopard is much simpler.) If they want to provide a "live" editor experience like Scratch, where sprites perpetually "exist" even when no scripts are running, and the instances appear on the stage immediately when creating or modifying sprites in the editor, it would be nice if they could add/remove sprites to/from a running project. We currently disallow that, mostly as a historical decision. As far as I recall, the reasoning for disallowing it was basically we don't know if the behavior will work correctly and nobody would ever want to do that anyway. I'm wondering, now that you've just been digging into this code, if you have any sense of how difficult it would be to enable adding/deleting sprites (which are not necessarily clones) while a project is running. |
|
I don't think it would be that difficult. In fact, this PR adds EDIT: Although |
| Object.freeze(sprites); // Prevent adding/removing sprites while project is running | ||
| this.targets = [ | ||
| stage, | ||
| ...Object.values(this.sprites as Record<string, Sprite>), |
There was a problem hiding this comment.
Why the as phrase here? Can the sprites parameter just be typed to this within the constructor itself? Does the Leopard editor actually interact with Leopard's TypeScript definitions, and would typing Project constructor params cause problems there, or nah? (Do any of the other public classes ex. Sprite have any typed parameters yet?)
| for (const target of targets) { | ||
| // Iterate over targets in top-down order, as Scratch does | ||
| for (let i = this.targets.length - 1; i >= 0; i--) { | ||
| const target = this.targets[i]; |
There was a problem hiding this comment.
We looked this over in Discord—the old behavior was broken!
Simple layer sequence test
clones have ghost effect and are numbered left to right
(so it's 1, 2, 3, original, for both sprites)
Scratch:
giga clone 3, giga clone 2, giga clone 1, giga sprite, cat sprite, cat clone 1, cat clone 2, cat clone 3, stage (matches rendered layer order)
Leopard (before this PR):
cat clone 3, cat clone 2, cat clone 1, cat sprite, giga sprite, giga clone 1, giga clone 2, giga clone 3, stage
So this is a (very) substantial behavior change, but brings closer to Scratch compat in an important way. Ostensibly related to #163 / #212 but those issues don't really touch on how layers have to do with script execution. And Leopard did attempt to execute scripts in a reliable order (spritesAndClones did sort by layer order)... it was just the wrong order (executing bottom to top, then stage, instead of top to bottom, then stage).
src/Project.ts
Outdated
| this.filterSprites((sprite) => { | ||
| if (!sprite.isOriginal) return false; | ||
|
|
||
| for (const sprite of this.spritesAndStage) { | ||
| sprite.effects.clear(); | ||
| sprite.audioEffects.clear(); | ||
| } | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
Kinda dull, can we separate clearing the effects to happen after filterSprites + does this still affect the stage?
this.filterSprites(sprite => sprite.isOriginal);
this.forEachTarget(target => {
target.effects.clear();
target.audioEffects.clear();
});
| public addSprite(sprite: Sprite, behind?: Sprite): void { | ||
| if (behind) { | ||
| const currentIndex = this.targets.indexOf(behind); | ||
| this.targets.splice(currentIndex, 0, sprite); | ||
| } else { | ||
| this.targets.push(sprite); | ||
| } | ||
| } |
There was a problem hiding this comment.
removeSprite below doesn't remove a sprite if it's already not in the list, but this function will double-up adding a sprite. This feels like a bad idea, even if addSprite is made not actually public atm lol
This matches how Scratch behaves, and means we don't have to mess around with _layerOrder anymore.
This matches how Scratch does it.
adroitwhiz
force-pushed
the
sprite-order
branch
from
9b508f9 to
c3f93c1
Compare
3 participants
Depends on #207.
This gets rid of the
cloneshierarchy and_layerOrderproperty, and stores every rendered target (sprite, stage, or clone) in one array kept in layer order. Clones now refer directly to the "original" sprite instead of their parent.While this does require some more management of that array, it means we don't have to:
_layerOrderpropertyStarting triggers is also done in top-down order now (reverse layer order), which matches how Scratch does it.