leopard-js / leopard

Library for making Scratch-like projects with JavaScript.
https://leopardjs.com
MIT License
143 stars 27 forks source link

Extract control flow ("triggers") logic #197

Open PullJosh opened 5 months ago

PullJosh commented 5 months ago

In discussions about integrating with Patch (#195), it seems to be clear that we can improve the design of Leopard to make it more extensible and valuable for users if we extract out what we currently call the "trigger" system into a separate, cleaner, more general-purpose control flow library/package/module/something so that users of the Leopard library can choose whether or not they want to use it. (Or, one might want to mimic Scratch's control flow without using everything else that Leopard provides.)

Additionally, as I've spent more time editing Leopard projects by hand, I have found myself annoyed with the triggers system. The way we currently set up a triggers array in the sprite constructor...

this.triggers = [
  new Trigger(Trigger.GREEN_FLAG, this.whenGreenFlagClicked),
  new Trigger(Trigger.KEY_PRESSED, { key: "space" }, this.whenSpaceKeyPressed), // I am trying to write this from memory and I can't even recall if this is actually the correct syntax. The fact that I can't remember it is an indicator of the seemingly-random specific-to-us API that I'd like to replace.
]

...is very functional and reasonably concise (although we could definitely make it more concise). But it is hyper-specific to Leopard and is not really similar to the way that other JavaScript code generally handles events.


I think a more similar-to-js API would be to have some kind of "event listener" system analogous to the DOM element.addEventListener("click", handler) API. I see a few main distinctions that would set our API apart:

  1. Control flow based on generator functions. The most important feature would be to support generators so that we can support Scratch-style yield functionality. If we wanted to, we could choose to support regular and async (and async-generator) functions as well.
  2. Customizable interruption behavior. If an event fires again while a script is in progress, do we interrupt the current script? Run it twice simultaneously? Ignore the new event? This behavior should be customizable, either at the call site where the event is being fired or in the addEventListener setup.
  3. Special handling of this. The current triggers system is very careful to make sure that whatever method you pass is bound to the sprite so that this.whatever works as expected from within the called method. The DOM addEventListener API does this differently, using event.target instead, but I definitely think I prefer the this binding for Leopard's purposes.

If we're extracting this event/control flow logic into its own module (or something like that), I think it makes sense to have a generic API that can be used outside of the context of OOP (where events are attached to a particular sprite instance and so on), but that can be easily plugged into the Sprite/Stage/Project class system as needed.

Here's a proposal for the generic API:

import { EventHandler } from "@leopard/control-flow"; // or whatever

const myHandler = new EventHandler(mySprite); // Optionally, when setting up an event handler, pass an object that will be bound as `this`. In Leopard use, this would often be a sprite, but you could attach anything you want.

myHandler.addEventListener("click", function() {
  this.move(10); // this === mySprite because we chose that above
});

myHandler.dispatchEvent("click"); // There's nothing special or magical about the name "click". We could have used "asdf" here and above and gotten the same behavior. When we dispatch an event, it just looks for all the listeners that match.

myHandler.addEventListener("keyUp", function*(event) {
  if (event.key === "a") {
    for (let i = 0; i < 10; i++) {
      yield* this.sayForSecs(i, 1);
    }
  }
});

myHandler.dispatchEvent("keyUp", { key: "a" }, { duplicate: "ignore" }); // If there is already a "keyUp" event running, ignore this newly-dispatched event.
// ^ This isn't quite good enough. In Scratch, key press events for the same key will be ignored but key press events for two different keys can run simultaneously. Is there a nice-but-still-generic way to represent that behavior? I don't have a good answer.

// Note that because the "keyUp" event listener is a generator function, it can't run independently on its own. There needs to be
// some kind of frame loop that is repeatedly asking it to step.
function loop() {
  requestAnimationFrame(loop);
  myHandler.step();
}
loop();
// ^ For use in a regular Leopard project, the Project object would be responsible for calling step on each sprite's event handler

This generic API could then be integrated into a Leopard project like so:

class MySprite extends Sprite {
  constructor() {
    // The base `Sprite` class creates an internal `EventEmitter` object for each sprite instance and exposes `addEventListener()`
    // It also fires events with well-known names (like "click") for you as you would expect.
    // (This would also enable us to provide more events like "keydown", "keyup", and "keypress" instead of just one.)
    this.addEventListener("greenFlag", this.whenGreenFlagClicked);
    this.addEventListener("keyup", this.whenKeyUp);
    this.sprites.moveButton.addEventListener("click", this.whenClicked); // Listen to other sprite's events. This would remove some of the need for broadcasts.
  }

  // Maybe you like doing async things. Have fun!
  async whenGreenFlagClicked() {
    const res = await fetch("https://example.com/api/get-a-number")
      .then(res => res.json());

    this.move(res.num);
  }

  *whenKeyUp(event) {
    if (event.key === "space") {
      yield* this.jump(); // or whatever you want to do
    }
  }

  // Doesn't need to be a generator function if you don't want to be interruptible
  whenClicked() {
    this.x += 50;
  }
}

There are a couple of problems and open questions with this approach:

towerofnix commented 5 months ago

Judging by your code and discussion, I'm reading three assumed "moments" of responsibility, each of which has some contextual nuances:

  1. The moment when an event listener is added.
  2. The moment when an event is dispatched.
  3. The moment when an event listener is about to be activated.

These are all fundamentally present in your proposal, and carry inherent responsibilities, currently:

  1. Register that a handler function is to be called according to events later on. No additional information is provided at this point: only the event name and the handler function.
  2. Declare that handler functions must now be evaluated. Provide the event name and data, possibly special information as well (e.g. the desired target rather than all possible targets). Provide meta conditions that will need to be considered coming up ({duplicate: "ignore"}).
  3. Associate the event name and data with applicable targets' handler functions. Assess the meta state of its handlers ("is this handler already running?") and compare with meta conditions described just above, by the dispatch.

The steps only depend on access to the details I've described. I.e, the code dispatching an event has to know the name of the event it wants to dispatch, as well as any data to send alongside it, and have a reference to a specific target / set of targets, if desired. But it doesn't need to interview those targets for a list of the targets' handlers, or about the meta state of those handlers.

A key moment that does not exist in your proposal:

  1. The moment when an event name is registered.

That is, you don't indicate that any event name is ever, inherent to that event name, associated with different behavior. You don't suggest that a listener for "click" should always be interruptible (restartable, duplicatable) simply because its event name is "click" - instead you indicate that it's the responsibility of the call to addEventListener to specify that meta-behavior. (In your code, no event name is different from any other, and so there's no reason they need to be specially "registered" at all.)

I like that, I think! Will probably touch on this some more while responding to your points.

An "extra" moment that you are also disregarding, but is actually present in DOM listeners:

  1. The moment when a listener is evaluated.

DOM provides event objects which have methods like preventDefault or stopPropagation or stopImmediatePropagation. These provide event listeners a way to conditionally respond, almost completely dynamically, with whatever meta-information the interface lets them. The main caveat is that it has to happen "immediately" — you can't do event.stopPropagation() after await new Promise(res => setTimeout(res, 1000)), because by that point it's already gone ahead and propagated!

You've proposed dodging this altogether by simply letting event be or reflect exactly the contents of the dispatched message. No "magic" behavior at all. There are real advantages to that simplicity, but I do want to highlight a moment that exists in DOM and not in your proposal, so limits where we have room to introduce dynamics / behavioral nuances.

The big one: I don't have a good way [of] handling duplicate events that may or may not be considered duplicates depending on the event data being sent.

Relevant reference code to this point:

myHandler.dispatchEvent("keyUp", { key: "a" }, { duplicate: "ignore" });
  // If there is already a "keyUp" event running, ignore this newly-dispatched event.

// ^ This isn't quite good enough. In Scratch, key press events for the same key
// will be ignored but key press events for two different keys can run simultaneously.
// Is there a nice-but-still-generic way to represent that behavior?
// I don't have a good answer.

I want to point out that there is no such thing in Scratch (excluding extremely obscure bugs and in-editor cases) as two copies of the same script running at once. There's no such thing as running a "duplicate" of the handler for the event. Instead, event handlers (scripts with hat blocks) either can or cannot be interrupted and restarted.

So I'm going to rewrite this dispatch like this instead, for clarity:

myHandler.dispatchEvent("keyUp", { key: "a" }, { interrupt: "skip" });
// possible "interrupt" property options:
//  - "restart" (discard current evaluation state, start over)
//  - "skip"    (retain current evaluation state, drop/discard event)
//  - "queue"   (retain current evaluation state, start it again afterwards)
//    [there's no such thing as "queue" in real Scratch, just providing
//     as an example of possible behavior here]

We still need to address deciding which handlers to restart, though. Of course, at the end of the day, the problem is that listeners aren't describing what they are doing and so we, Leopard (at the moment "when an event listener is about to be activated") have no idea whether this listener is suitable for restarting or not.

By ruling out the moment "when a listener is evaluated" (no event magic), we actually only have one possible moment for expression when the listener can possibly be described: when the listener is added!

Let's think about that third step, the current one we're hypothetically evaluating, in more detail.

  1. We want to associate the data provided by the first step (when the listener was added) with the data we provided by the middle step (when the event was dispatched).
  2. We can see the meta state of a listener by keeping track of what its return value is from the last time we called it.
    • If it's undefined, we haven't run the function yet, so obviously it's inactive. Or maybe it was set back to undefined after the handler ended, in which case it's also inactive.
    • If it's a promise, this function is active, but it cannot be interrupted, since you can't interrupt a promise.
    • If it's a generator, this function is active, and it can be interrupted, since you can just discard the generator and never return control back to it. (The browser garbage collects it sometime soon, etc.)
  3. We have no idea about the actual internal behavior of the (running) event handler, though.
    • We don't know which part of the event it's responding to because that's defined internally!
  4. Even though we really do have data about the event to be dispatched, and of course we can remember which events we sent previously... we don't know what's going on inside the handler function. We only know if it's active or inactive.

OK. Let's think about Scratch scripts now.

when key "a" pressed
say "A is for: I'm *A* happy camper!" for 4 seconds

when key "any" pressed
start sound pop
think "I have no idea what key was pressed... Oh me oh my..." for 3 seconds

Scratch has hat blocks that don't actually provide context to the scripts beneath them. Really, all a hat block is responsible for is telling the runtime information about this "event handler" (the blocks beneath it) — giving enough information for the runtime to decide whether or not to run the script!

I think the example above is actually a useful one to try out in Scratch, yourself. Go put those blocks together!

Did you see that when you pressed any key, it started the "pop" script, when you followed that up by pressing some other key, it didn't start the script all over again? Even though an event with different "details" is what caused the hat block to trigger the first time, you still had to wait for that response to finish before any new events you sent would go through.


OK, OK, secretly there are actually two ways forward here. I'm going to reel back a little because the first one is so simple it might sting a little, and it'll be nice to have out of the way, first:

If you want to, otherwise unconditionally, not interrupt a running handler, the only information needed is whether or not the handler is currently running, and we don't need to change anything to be able to observe that.

All we need to do is keep track of whether or not each handler (for the relevant event name) is currently running. If a keyUp handler is currently running, and your new keyUp event has been described to "don't interrupt running event handlers please" (interrupt: "skip"), then we will simply skip over that keyUp handler. Nothing more to it!

The second approach is to go for something perhaps a little bit closer to Scratch's blocks. You might represent the scripts above this way, in Leopard:

this.addEventListener("keyPressed", { key: "a" }, function*() {
  yield* this.sayAndWait("A is for: I'm *A* happy camper!", 4);
});

this.addEventListener("keyPressed", function*() {
  yield* this.startSound(this.sounds.pop /*or whatever */);
  yield* this.thinkAndWait("I have no idea what key was pressed... Oh me oh my...", 3);
});

Say you press the A key, now.

Well, this way you still need to keep track of which scripts are running, because otherwise "interrupting" doesn't even mean anything...

I wanted to bring this up as an alternate syntax of dealing with event listeners, but I don't like it for a couple reasons:

  1. It's not JavaScript-y. JavaScript event handlers respond, as part of the listener function, to data on the event, they don't declare which conditions need to be met beforehand (in almost all cases, certainly when you're only working with addEventListener and not arcane magic).
  2. It's no fun. Handling event data with your own grubby code paws means using real deal if and if (event.key === "a") and /* Oo! I don't want to detect any specific key, I don't need ANY condition this time!!! */ code. That's great for teaching coding in general. And...
  3. It's Leopard-specific. We obviously have to make certain admissions about what will be specific to Leopard because it's actually specific to Scratch and we are reflecting Scratch, but generally, it's best to avoid as much as possible introducing any "strange" interfaces - ones that aren't totally typical of normal JavaScript. (Maybe other languages declare the data they want event listeners to respond to up-front - hey, Scratch does, for example! - but JavaScript doesn't, and we are helping to teach JavaScript.)

I think the simpler solution (necessary anyway for reflecting Scratch behavior) is better: just know which ones are running, and avoid interrupting those if the dispatch was provided interrupt: "skip".

I haven't worked out the details of what this would look like with clones. I'm hoping that because each EventEmitter can be given a sprite instance to bind to, it won't be too bad.

You know, it didn't click until right now that clones don't re-run the Sprite subclass constructor anyway... I understand that there's good reason for that but it does feel a little strange...

But yes, as long as the listeners are never arrow functions, they can be rebound and it shouldn't be very hard to have whatever part of code is actually responsible for evaluating those handlers, to call them bound to the applicable clone, rather than the applicable sprite.

Note that you can't bind a function twice:

f = function() { return this }
obj1 = {a: 'b'}
obj2 = {c: 'd'}
console.log(f.bind(obj1).call(obj2)) // {a: 'b'}
console.log(f.call(obj2)) // {c: 'd'}

So we would have to never literally use .bind(), and instead just provide the appropriate target as this via handler.call() (or handler.apply()) at right-after-dispatch-time.

There isn't a good way to listen to all of the events in a Leopard project. For example, it might be nice to be able to have one sprite listen to every single click in the entire project. Maybe there's some kind of event bubbling that makes sense, where events are fired on a particular sprite but bubble up to the Project and you can listen to events on the whole project if you want?

Hmm. I mean, there are actually very few events that have to do with the current sprite. Those ones are simply meaningless if you take them out of the context of that sprite. Does "when this sprite click (but psst I mean anywhere in the project)" mean "when any sprite is clicked" or does it mean "when any sprite or the stage is clicked" or does it mean "when the got dang project is clicked"?

When-key-pressed isn't dispatched on individual targets, it's dispatched on the project and then the event is disseminated to targets. No target "owns" the event, it's just responding like a "when I receive" block. (Fun fact: in super early versions of Scratch, the green flag was literally coded as a broadcast!)

The problem is that this is backwards to how JavaScript treats propagation. In JavaScript, if you want to detect a key anywhere on the page, you have to add that event right on the document body or the window. You can't add it onto yourself (some random element), because that element won't receive the event unless it's focused!

In Leopard we are adding events right onto ourselves, and the project is disseminating the events to us. It's bubbling down instead of up. It's still bubbling, so it's a similar behavior, but um... the direction is more like an anvil than a bubble, you know...

I don't know if there is a good way to get around this. In JavaScript if you have a component that does not live forever but does listen to things outside of it, it needs both a constructor and a tear-down function, and JavaScript literally just doesn't support this by default - you or a framework takes responsibility for calling that teardown function. As they say, there is a bit of ✨ magic ✨ that Leopard should probably be taking care of for anyone coming from Scratch, but it's inevitably going to be a) hacky and fragile or b) inconsistent with "normal" JavaScript behavior.

I'm inclined towards the latter of those two evils. Accordingly, I think we should avoid using terminology that is carefully tied to typical event behavior of JavaScript, because we just can't reflect it perfectly if we want to take care of responsibility that wouldn't typically be ours.

IMO adding event handlers on this, like you've done, is quite reasonable, and if we are doing that then all we need is a separate name for an event that is distinct from one scoped to this sprite. this.addEventListener("clickAnywhere"), etc.

Maybe inside each receiving sprite you do something like this.project.addEventListener("myCoolEvent", mySpriteMethod) and you send out a broadcast by doing this.project.dispatchEvent("myCoolEvent"), but then you have to be careful to make sure that the listener method is somehow being bound to the correct sprite–because by default it would be bound to the project.

I don't think binding is a problem at all! Like we discussed above, dissemination is down into sprites. Whatever is running the receivers understands which target owns the receiver, because the target added the receiver to itself.

this.addEventListener("myCoolEvent", this.mySpriteMethod);

project.dispatchEvent("myCoolEvent", {foo: "bar"});
// this runs myCoolEvent handlers on all targets by iterating over
// those targets, and of course it knows which target it's
// currently iterated onto, and runs the handler bound to
// that target!

IMO all events dispatched onto project are disseminated onto all targets. That is just what project.dispatchEvent means. It's the behavior that is called when you press a key (project.dispatchEvent("keyDown", {key: "a"})).

Target-specific events are as simple as theRelevantTarget.dispatchEvent("click") and dissemination is down, not up, so this does not propagate to the project or disseminate to other sprites.

Or, alternatively, do we have a dedicated "broadcast" method that just provides the broadcast name to the listener?

IMO this is the wrong approach. You couldn't possibly have two receivers for different broadcasts running at the same time unless you exposed information about the event handler itself, which I described being against earlier.

I don't think broadcast dissemination has any special problem though. You just add the event listener onto yourself, dispatch on the project, project disseminates event to all targets, all target handlers are run with appropriate "this" binding.

towerofnix commented 5 months ago

Sorry most of that rambling is probably pretty poorly structured LOL. Here's a TL;DR!

Overall I love the proposal. It's slick and clean and looks both easy and fun to use. It's related to JavaScript code patterns instead of being soaked in Leopard-isms — something we've agreed, with consensus, is a really good thing, before! https://github.com/leopard-js/leopard/pull/177#issuecomment-1474905810

We definitely don't claim to hold the same opinions as whatever we wrote back then though LOL. Not that they wouldn't have any merit - it's just that, in general, avoiding code particular to Leoaprd and using language that's closer to JavaScript is mostly a good idea.

However, we don't want to go overboard with "looking like" normal JavaScript. Because, normal JavaScript isn't coded the way that Scratch is — constructors and teardowns just for dealing with event listeners are normal in JS and total nonsense in Scratch (and maybe other game-oriented programming languages, too!). If we are going to hand-wave certain idiosyncrasies of JavaScript away, we need to be careful in what JS behavior is tied to those, and not act like we're representing those, either.

This is why I'm in favor of a more simplistic approach where dispatching on the project always disseminates the event to all targets, and we just use alternate event names for events, like "click", that are contextualized to a specific sprite if you think about them one way, but not if you think about them another.

We wrote a lot of technical details but in general aren't too worried about running into technical issues - not with clones, not with broadcasts, not any of that. The level of simplicity actually puts Leopard projects in an easier place to get implemented without a ton of pain! I think it aligns well with Scratch and that basically all of the ✨ magic ✨ that Leopard wants to be responsible for - it can be responsible for, without much difficulty. As long as code interfaces with Leopard the way Leopard needs: it's impossible to restart promises so please use generators if you want interruptible async, it's impossible to rebind an already-bound function so please don't use arrow functions, just pass in a normal non-bound function, perhaps defined on your Sprite subclass declaration.

PullJosh commented 5 months ago

I was struggling to keep track of all these details in my head, so I created a proof of concept event listener system. https://codepen.io/PullJosh/pen/eYazxax?editors=0010

It supports event listener callbacks that are functions, async functions, generators, and async generators. When you dispatch an event, you can choose to restart currently-running iterators or skip those that are currently running (using interrupt: "restart" or interrupt: "skip").

I also created a boolean option called immediate. If you dispatch the event with immediate: true, the callback will be called immediately. If you set immediate: false, the callback will be queued up and will wait to be started until the next frame.

I tried to set up the demo environment to be as easy to play with as possible. Please give it a try and check if the behavior all seems correct. (I'll be honest–the code is kinda ugly. But I think the behavior might be correct.)

PullJosh commented 5 months ago

By the way, I've always been a reckless driver when it comes to testing code, but this little bit of logic in the CodePen above feels like precisely the kind of code that would benefit most from automated unit testing.

It feels like a good sign that extracting the code out like this puts it in prime position to be easily testable, independent of the rest of Leopard's behavior.

towerofnix commented 5 months ago

Wow, this is impressive. We admittedly jumped into the example before reading your comments above properly through. I was expecting to ask if you were being aware of Scratch not actually executing scripts as soon as the corresponding event occurs (i.e, "priming" hat blocks to be executed during the next frame), but then I saw you have an "Immediate mode" checkbox and it's on by default (for the demo). Nice!

"Immediate mode", I believe, doesn't exist in Scratch except for custom blocks, which aren't internally handled the same way (but do exhibit running a different script instantaneously, like "immediate mode" does). My impression is that with "immediate mode" enabled all the behavior lines up with what I'd expect, which is awesome. But I don't have a "canon" behavior to compare "immediate mode" against, so for the sake of review I'll be focusing on behavior with "immediate mode" disabled.

We'd encourage you to make a clone/copy of this pen before looking into fixes, so that the original is kept around and you/we can compare the implementations.

Table of cases:

Case ID First frame Second frame Third frame Working?
Case A and skip x2 none none yes
Case B and skip and skip none nope
Case C and restart x2 none none nope
Case D and restart and skip none nope
Case E and restart and restart none yes
Case F and skip and restart none yes
Case G (and either) none and skip yes
Case H (and either) none and restart yes

Working (Case A):

Two same-frame clicks of Click Sprite1 (and skip) does what I expect it to: execution begins on the next frame, with one "Sprite1 clicked part I" message, and the next frame a single "part II" message, too.

  1. Frame 1
    • Click Sprite1 (and skip)
    • 🆗 As expected, no message
    • Click Sprite1 (and skip)
    • (I believe this is equivalent to pressing Double click Sprite1 (and restart). I will not be using the "double click" options, but only since I find clicking the action twice to be more clear.)
    • 🆗 As expected, no message
  2. Step frame: Frame 2
    • 🆗 "Sprite1 clicked part I"
  3. Step frame: Frame 3
    • 🆗 "Sprite1 clicked part II"

Not working (Case B):

Following Click Sprite1 (and skip) on frame 1 with Click Sprite1 (and skip) on frame 2 does not appear to be behaving correctly.

  1. Frame 1
    • Click Sprite1 (and skip)
    • 🆗 As expected, no message
  2. Step frame: Frame 2
    • 🆗 As expected, "part I"
    • Click Sprite1 (and skip)
    • 🆗 As expected, no (additional) message
  3. Step frame: Frame 3
    • ❌ No message! Expected "part II"
  4. Step frame: Frame 4
    • ❌ Unexpected "part II"
  5. Step frame: Frame 5+
    • 🆗 As expected, no message

Click Sprite1 (and skip) should not have an effect at all if the script is ongoing. I would expect the event to simply be discarded; "part II" on frame 3 and nothing on frames 4 and beyond.

Not working (Case C):

Two same-frame clicks of Click Sprite1 (and restart) appears to double execution for the first frame, and not double execution for the second frame. This is strange.

  1. Frame 1
    • Click Sprite1 (and restart)
    • 🆗 As expected, no message
    • Click Sprite1 (and restart)
    • 🆗 As expected, no message
  2. Step frame: Frame 2
    • 🆗 As expected, "part I"
    • ❌ Unexpected "part I" (doubled!)
  3. Step frame: Frame 3
    • 🆗 As expected, "part II"
  4. Step frame: Frame 4+
    • 🆗 As expected, no message

I'd expect it to be just the same as clicking Sprite1 once, because there isn't an active script to restart during frame 1. (I've only "primed" it for executing on the next frame, I have not begun execution.)

Not working (Case D):

Following Click Sprite1 (and restart) on frame 1 with Click Sprite1 (and skip) on frame 2 does not appear to be behaving correctly.

  1. Frame 1
    • Click Sprite1 (and restart)
    • 🆗 As expected, no message
  2. Step frame: Frame 2
    • 🆗 As expected, "part I"
    • Click Sprite1 (and skip)
    • 🆗 As expected, no (additional) message
  3. Step frame: Frame 3
    • ❌ No message! Expected "part II"
  4. Step frame: Frame 4
    • ❌ Unexpected "part II"
  5. Step frame: Frame 5+
    • 🆗 As expected, no message

This is exactly the same behavior as Case B. Click Sprite1 (and skip) while the script is active should have no effect.

Working (Case E):

Following Click Sprite1 (and restart) on frame 1 with Click Sprite1 (and restart) on frame 2 is working as expected. Nice!

  1. Frame 1
    • Click Sprite1 (and restart)
    • 🆗 As expected, no message
  2. Step frame: Frame 2
    • 🆗 As expected, "part I"
    • Click Sprite1 (and restart)
    • 🆗 As expected, no (additional) message
  3. Step frame: Frame 3
    • 🆗 As expected, "part I"
  4. Step frame: Frame 4
    • 🆗 As expected, "part II"
  5. Step frame: Frame 5
    • 🆗 As expected, no message

Working (Case F):

Following Click Sprite1 (and skip) on frame 1 with Click Sprite1 (and restart) on frame 2 is working as expected. Nice!

  1. Frame 1
    • Click Sprite1 (and skip)
    • 🆗 As expected, no message
  2. Step frame: Frame 2
    • 🆗 As expected, "part I"
    • Click Sprite1 (and restart)
    • 🆗 As expected, no (additional) message
  3. Step frame: Frame 3
    • 🆗 As expected, "part I"
  4. Step frame: Frame 4
    • 🆗 As expected, "part II"
  5. Step frame: Frame 5
    • 🆗 As expected, no message

This is exactly the same behavior as Case E.

Working (Case G):

Click Sprite1 (and skip) on frame 1, then Step frame twice, and Click Sprite1 (and skip) on frame 3, is working as expected. (This also works if the first click was Click Sprite1 (and restart) and the second was still Click Sprite1 (and skip).)

  1. Step frame: Frame 1
    • Click Sprite1 (and skip)
    • 🆗 As expected, no message
  2. Step frame: Frame 2
    • 🆗 As expected, "part I"
  3. Step frame: Frame 3
    • 🆗 As expected, "part II"
    • Click Sprite1 (and skip)
  4. Step frame: Frame 4
    • 🆗 As expected, "part I"
  5. Step frame: Frame 5
    • 🆗 As expected, "part II"
  6. Step frame: Frame 6
    • 🆗 As expected, no message

Working (Case H):

Click Sprite1 (and skip) on frame 1, then Step frame twice, and Click Sprite1 (and restart) on frame 3, is working as expected. (This also works if the first click was Click Sprite1 (and restart) and the second was still Click Sprite1 (and restart).)

  1. Step frame: Frame 1
    • Click Sprite1 (and skip)
    • 🆗 As expected, no message
  2. Step frame: Frame 2
    • 🆗 As expected, "part I"
  3. Step frame: Frame 3
    • 🆗 As expected, "part II"
    • Click Sprite1 (and restart)
  4. Step frame: Frame 4
    • 🆗 As expected, "part I"
  5. Step frame: Frame 5
    • 🆗 As expected, "part II"
  6. Step frame: Frame 6
    • 🆗 As expected, no message

This is exactly the same behavior as Case G.


It feels like a good sign that extracting the code out like this puts it in prime position to be easily testable, independent of the rest of Leopard's behavior.

Totally! Hopefully following through these manual tests gives you an impression of what behavior we'd expect. Totally encouraged to ask if you're confused or wondering about anything. Automated tests will definitely assist making sure a working implementation keeps working.

It looks like there are more examples coded to check out (whenClickedAnywhere, asyncOnce, asyncLoop), and they could help figure out what's going on, and more nitty-gritty behavior expectations. We haven't checked them out yet - for now hopefully the failing cases give useful direction.

Thanks a bunch for putting this together!! We haven't taken a look at the code either, except for the example Leopard project (definition for Sprite1) and the demo environment (to make sure we understood what each button was doing). But we're excited to check it out soon, and we'd be happy to look closely if you have trouble identifying what's going wrong in your code to make the unexpected behavior.

PullJosh commented 5 months ago

Adding a quick flag in the sand here to point out that decorators are not currently supported in JavaScript but they have been in TypeScript for years. If decorators ever come to JS it would be lovely to have.

class MySprite extends Sprite {
  constructor() {
    // ...

    // No longer needed!
    // this.addEventListener("greenFlag", this.whenGreenFlagClicked);
  }

  // Yay for decorators 🎉
  @greenFlag
  *whenGreenFlagClicked() {
    // ...
  }
}

(@towerofnix I'll investigate your response more when I can)

towerofnix commented 5 months ago

They're workin' on it! Stage 3, so waiting on progress from implementers (which is always moving, slower or quicker). https://github.com/tc39/proposal-decorators