Fix for #59: Add if on edge, bounce

[HanClinto]

Fixes #59 to finish implementing the "if on edge, bounce" block.

There was a minor issue with the math where we needed to flip the y-axis, because Leopard and the Scratch VM use different signs for the internal representation of the y-axis.

Also ran through Prettier to clean up the formatting.

[HanClinto]

This is what a traced bounce looks like:

  *whenGreenFlagClicked() {
    this.costume = "costume1";
    this.goto(100, 0);

    while (true) {
      this.move(3);
      this.ifOnEdgeBounce();
      this.penDown = true;
      this.direction += 0.3;
      yield;
    }
  }

[towerofnix]

Thank you for your work on this :)

Here's my test of what seems to be the same script in Scratch 3.0—I figured it would be worthwhile to compare include a reference so that we can confirm that Leopard and 3.0 both function identically, right?

It's similar, but not exactly the same. It follows basically the same angles, but the distances are a little off. I wonder if there's a difference in the setup rather than the actual "if on edge, bounce" implementation - could you share the complete .sb3 you tested with so others can pass it through this PR as well, and include a reference screen of how it runs in 3.0?

I haven't had the chance to run my own project (which was only the script in the screenshot, put in the default Sprite1 Scratch Cat) with this PR, but I'd love to do a more thorough review of your pull later today if I get the chance!

/cc @apple502j You may be interested in checking this out since it builds off your own code!

[HanClinto]

Thank you for looking at this with me!

I created a new Scratch project that runs for a deterministic amount of time: https://scratch.mit.edu/projects/714711064

So in a perfect world, we will get the same result at the output. However, when running the following code, as you noted, the angles and bounce points are different.

  *whenGreenFlagClicked() {
    this.costume = "costume1";
    this.goto(100, 0);
    this.direction = 90;
    this.penColor = Color.rgb(0, 45, 255);
    this.clearPen();
    this.penDown = true;
    for (let i = 0; i < 1000; i++) {
      this.move(3);
      this.ifOnEdgeBounce();
      this.direction += 0.3;
      yield;
    }
  }

Looking at the screenshots overlaid on each other and comparing the differences, the cause seems clear:

Depending on the rotation of the sprite, we detect collision with the edge of the wall much more quickly than Scratch does. Scratch doesn't detect a hit against the wall until non-transparent pixels of the sprite touch the edge of the frame. However, in Leopard, there seems to be more of a rectangular box around the sprite, and it bounces when it's still far away from the edge of the frame.

I'm still digging into it, but I wonder if this difference is because the way we get the bounding box of a sprite is calculated from the transformation matrix, which I don't think would take into account transparent corners of a sprite being rotated? I'm not sure, since I'm a bit new to all of this.

It seems that the Scratch renderer first calculates a convex hull for each drawable within a rotated shape, and then uses that to generate a "tight" AABB around each sprite when calling getBounds():

I don't have an easy way to see the bounding boxes of elements in Scratch vs. Leopard, so I can't fully confirm that this is the root of the issues we're seeing, but I suspect this is the most significant difference.

[HanClinto]

Yup, that's the issue. I made a test program to illustrate it more: https://scratch.mit.edu/projects/714724731/editor/

  *whenGreenFlagClicked() {
    this.costume = "costume1";
    this.goto(100, 0);
    this.direction = 90;
    this.penColor = Color.rgb(255, 0, 0);
    this.clearPen();
    this.penDown = true;
    for (let i = 0; i < 100; i++) {
      this.move(10);
      this.ifOnEdgeBounce();
      yield;
    }
    this.penDown = false;
    this.goto(100, 0);
    this.direction = 0;
    this.penDown = true;
    for (let i = 0; i < 100; i++) {
      this.move(10);
      this.ifOnEdgeBounce();
      yield;
    }
    this.penDown = false;
    this.penColor = Color.rgb(161, 0, 255);
    this.goto(100, 0);
    this.direction = 45;
    this.penDown = true;
    for (let i = 0; i < 300; i++) {
      this.move(10);
      this.ifOnEdgeBounce();
      yield;
    }
    this.penDown = false;
  }

I trace three patterns -- a vertical bounce, a horizontal bounce, and then a diagonal bounce (45 degrees). In Scratch, you'll notice that the diagonal bounces extend almost to the same extents that the vertical / horizontals do.

In this PR, the diagonals bounce far sooner than the verticals / horizontals.

I'm not entirely sure how to go about getting a tighter bounding box from the renderer in getBoundingBox(), but I think that's what we'll need to do if we're to match behavior 100%.

[HanClinto]

So I tested this with the improvements from #119 and it works a LOT better... but it's still not identical.

vs.

Some of this looks like it might be a difference of keeping the sprite within the "fence" -- I'm still trying to figure out where all of the differences might be coming from, but given how much better this is performing now, this might be good enough for first-pass.

After #119 gets merged, I'll do a final rebase / test on this PR and we'll be able to take it from there, but I'm pretty optimistic about how much better this is behaving now.

[HanClinto]

Hah. Silly mistake on my part. I switched to the tight bounding box for ifOnEdgeBounce but forgot to do the same for keepInFence. Once I did that, the results are MUCH closer:

Overlaid:

Still not pixel-perfect, but I'm feeling happy enough with this that I'm comfortable merging it.

@adroitwhiz are there any caching / performance concerns that I need to be aware of when calling getTightBoundingBox?

[HanClinto]

@PullJosh Another thing to keep in mind is that 100% compatibility with Scratch may not be what we want in this case, given that the reference implementation isn't without its faults either.

[PullJosh]

@HanClinto I'm not actually clear on what the fault is in that example project. Is it the fact that the sprite can still be inside an edge after bouncing?

Edit: Oh, I see. Sometimes touching "edge" reports the false when it should be true.

[HanClinto]

@PullJosh Yeah, that and the fact that on some edges it stops at the edge of a pixel, and on the others it goes halfway through the pixel before stopping. It's related to this PR here from Adroitwhiz on the scratch-render project. I guess bringing it up here is a bit of a non-sequitor, but I just mean all that to say that the ifOnEdge bounce behavior isn't entirely clean on the Scratch 3.0 side, and it appears that there is some inconsistency in the behavior between Scratch 2.0 and 3.0, as well as internal inconsistency with Scratch 3.0 itself, etc.

But that's a bit of a non-sequitor, since matching Scratch 3.0 behavior is still the goal of our project (warts and all), and this PR doesn't (yet) match behavior of Scratch 3.0 pixel-perfect.

I guess I'm wondering -- is this good-enough yet? Or should we keep striving for better accuracy before merging it in?

[PullJosh]

I guess I'm wondering -- is this good-enough yet? Or should we keep striving for better accuracy before merging it in?

I am personally a big fan of "good enough" followed by incremental improvements later. To my tastes, this PR is ready to be merged despite not perfectly matching Scratch 3.0's behavior.

But if @adroitwhiz or @towerofnix disagree, I trust their judgement.

[PullJosh]

Once this is merged, let's create a new release (minor version bump) so that people can use the new touching "edge" and if on edge, bounce features.

[PullJosh]

@adroitwhiz Is this something that we can merge?

[adroitwhiz]

I think the review comments should be addressed, and it needs to be rebased manually

[HanClinto]

Implemented both of @towerofnix's proposed changes and rebased to latest master in 5186824. Am happy to update this again if we want to wait to pull this in until after the Typescript PR is merged in -- I have no problem rebasing my work after that point again. Whichever people prefer, but at least you won't be continuing to wait on me to update this PR. Sorry for the delay on this one!

[towerofnix]

Heya! Nice to see you again!

No problem at all about the delay. The rest of us have been on pause since around July anyway, only reviving the repos in the last couple of weeks. How about we wait til the TypeScript PR is merged and come back to this? It'll be easier to work on incremental additions like this then! Thanks also for addressing my review comments!

[towerofnix]

@adroitwhiz got the Typescript changes merged into this — it should be good to go and is tracked in #188 now! Thank you again, @HanClinto!