Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "touching edge" and tight bounding box functionality #119

Merged
merged 8 commits into from
Jul 18, 2022

Conversation

adroitwhiz
Copy link
Collaborator

This is necessary for implementing #114.

There are a few changes to the renderer I made here:

  • Replace the SkinCache class with a WeakMap of Costumes to Skins
    • The reason that the SkinCache existed was to associate renderer-specific Skin data (which holds a costume's WebGL texture and other rendering stuff) with corresponding Costumes. We don't want to hold onto that data if those costumes are deleted, which is why I created a deletion mechanism. However, we can just use a WeakMap instead.
  • Replace _setSkinUniforms with _renderSkin
    • This is mostly incidental, and was done because I noticed a WebGL warning in the console when we tried to render skins without a texture. By unifying the setting of uniforms (preparing to render a skin) and actually rendering the skin, we can return early from rendering if no texture exists. Previously, we were only returning from the "prepare to render" part, and rendering unconditionally.
  • Cache sprite-specific data in Drawable class
    • This is the big change that makes the rest of this possible. Much as we have Skins associated with Costumes, we now have Drawables associated with Sprites. The transformation matrix is now stored here, and only recalculated when a sprite's transform-related properties change. Ideally, the sprite would signal this itself by setting a flag whenever its position, size, or rotation change, but I'm not sure about whether we want that sort of cross-cutting here, so I just added diff checking for all the properties that cause the transformation matrix to be recalculated.
    • This is important because tight bounding boxes need convex hulls. While a transform matrix isn't that expensive to recalculate every frame, convex hulls would definitely be, so we need a way to cache them.
  • Implement tight bounding box and "touching edge"
    • Add getImageData method to skins
      • This gives us a way to get the texture data for a skin at a given scale, CPU-side. This will be important for calculating the convex hull.
    • Add effectTransformPoint function, which replicates the whirl, fisheye, pixelate, and mosaic effects in JavaScript
      • These effects change the convex hull of a sprite, so when we calculate the convex hull, we need to take them into account. Unfortunately, this means reimplementing their shader code in JavaScript, as Scratch's rendering engine does.
    • Add convex hull calculation
    • Add a function to get the tight bounding box of a sprite, and use that to implement "touching edge"
      • This function basically transforms each convex hull point by the sprite's current transformation matrix, then creates a bounding box from all those points.

There's still no "touching edge" support for the sb-edit Leopard code generator--that needs to be implemented separately.

This fixes a WebGL warning when we tried to render skins that had no
texture. Now, we return before trying to render them.
Unfortunately, handling this properly involves a lot of edge cases.
We have to implement the bounding box correctly even for sprites with
distortion effects, and the pixels' area must be accounted for.
Copy link
Member

@towerofnix towerofnix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really clean! Thanks a million for putting this thorough implementation together, I definitely didn't expect anything like it so quickly :)

I have a couple comments but they're small and mostly requesting some further details / exploration. In general this pull looks good to me. Thank you!

src/Renderer.js Outdated Show resolved Hide resolved
src/Renderer.js Show resolved Hide resolved
src/renderer/VectorSkin.js Show resolved Hide resolved
const EPSILON = 1e-3;

// Transform a texture-space point using the effects defined on the given drawable.
const effectTransformPoint = (drawable, src, dst) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simulating effect transformations in JavaScript is really interesting to me, even though the details are a bit beyond my comprehension :P

I think it'd be interesting to look over the discussion/summary in scratchfoundation/scratch-render#196 and try testing the example project in Leopard with these changes and "if on edge, bounce" implemented.

@adroitwhiz adroitwhiz requested a review from towerofnix July 17, 2022 21:48
Now that we're caching the same Rectangle object, we need to copy it
before snapping it to int bounds.
src/Renderer.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@PullJosh PullJosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error I noticed: Changing a sprite's size does not cause its appearance to change until some other update (like moving or rotating) makes Leopard "notice" that something is different.

@PullJosh
Copy link
Collaborator

I should say, by the way, that this update is absolutely amazing and way above my pay grade. Thank you for the amazing work @adroitwhiz! 🤩

@adroitwhiz adroitwhiz requested a review from PullJosh July 18, 2022 00:07
@adroitwhiz
Copy link
Collaborator Author

Whoops, fixed those issues. I'm definitely in support of converting stuff over to TypeScript now that we have a build step.

Copy link
Member

@towerofnix towerofnix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a while of updating my local setup but I got Leopard working locally and tested this out with a new example project—it looks like it works great :)

Copy link
Collaborator

@PullJosh PullJosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A thorough review of all the code is beyond my abilities, but the behavior looks good in my test project, and @towerofnix signed off on the behavior in their own test project, so I am content.

@adroitwhiz adroitwhiz merged commit 1e4e89e into leopard-js:master Jul 18, 2022
@HanClinto
Copy link
Contributor

BTW, I tested these changes with the ifOnEdgeBounce implementation from #114 and it works very well -- see my most recent post in that PR for screenshots of improved performance. Excellent work!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants