Skip to content

Handle matrix + silhouette updates better #555

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

Merged

Conversation

adroitwhiz
Copy link
Contributor

@adroitwhiz adroitwhiz commented Feb 5, 2020

Resolves

Resolves (part of) #545
Resolves #546
Resolves scratchfoundation/scratch-gui#5647

Proposed Changes

This PR makes the following changes:

  • Add updateCPURenderAttributes method to Drawable
  • Move CPU render attribute update from RenderWebGL._touchingBounds to RenderWebGL.isTouchingColor and RenderWebGL.isTouchingDrawables
  • Add documentation notes to Drawable.isTouching, Skin.isTouchingNearest, and Skin.isTouchingLinear explaining that those functions' callers are responsible for updating the silhouette + matrix
  • Update CPU render attributes in RenderWebGL._getConvexHullPointsForDrawable

Reason for Changes

  • Add updateCPURenderAttributes method to Drawable

    • There were many places in RenderWebGL that called both Drawable.updateMatrix and Drawable.skin.updateSilhouette, and none that called just one or the other. Some checked whether the drawable's skin existed, others did not. I've consolidated calls to Drawable.updateMatrix and Drawable.skin.updateSilhouette into a single utility method, for conciseness and consistency.
    • This new consolidated method allows for future optimizations like this one not included in this PR, which in my testing improves "touching sprite" performance by 40-70%.
    • updateCPURenderAttributes does not multiply the drawable's scale by the screen-space scaling ratio, since CPU rendering always occurs at the "native" stage size.
  • Move CPU render attribute update from RenderWebGL._touchingBounds to RenderWebGL.isTouchingColor and RenderWebGL.isTouchingDrawables

    • _touchingBounds doesn't actually require the passed drawable's matrix or silhouette to be up-to-date. However, isTouchingColor and isTouchingDrawables relied on it updating those. For clarity (and to prevent things from breaking if a future coder decided to "optimize" _touchingBounds), these calls have been moved (in the form of calls to the new updateCPURenderAttributes function) to the places that actually require the drawable's matrix and silhouette to be updated. Moved to another PR
  • Add documentation notes to Drawable.isTouching, Skin.isTouchingNearest, and Skin.isTouchingLinear explaining that those functions' callers are responsible for updating the silhouette + matrix

    • This should make it clearer to future coders that they cannot use a drawable's silhouette without updating it.
  • Update CPU render attributes in RenderWebGL._getConvexHullPointsForDrawable

I have tested this manually and confirmed that it does not regress #543.

@@ -104,7 +104,7 @@ class SVGSkin extends Skin {
return mip;
}

updateSilhouette (scale = 1) {
updateSilhouette (scale = [100, 100]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

😳

(fortunately, this didn't break anything--all call sites specified a scale explicitly)

Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@fsih fsih left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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

Successfully merging this pull request may close these issues.

[Regression] say balloon goes to the wrong place first time a project is played (with some costumes) Follow up on the issue described in #398
3 participants