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

Cache and don't re-create transformed hull points #445

Merged

Conversation

adroitwhiz
Copy link
Contributor

@adroitwhiz adroitwhiz commented Apr 23, 2019

Resolves

One Two of the concerns in #365

Proposed Changes

  • Create Rectangles per-Drawable for holding get(Fast)Bounds results
  • Store transformed convex hull points in an array initialized when un-transformed convex hull points are initialized
  • Add "transformed convex hull dirty" flag

Reason for Changes

  • Create Rectangles per-Drawable for holding get(Fast)Bounds results

Drawable.getBounds, Drawable.getBoundsForBubble, Drawable.getFastBounds, and Drawable.getAABB take an optional result parameter (a Rectangle). If given, it will be set to the drawable's bounds.

If a result parameter was not passed, however, those functions would create an entirely new Rectangle on each call. To make things worse, they're called from several places in the codebase without a result being passed.

Now, each Drawable instantiates its own Rectangles to hold the bounds results only once: when the Drawable itself is created. This should prevent unnecessary creation of Rectangles.

  • Store transformed convex hull points in an array initialized when un-transformed convex hull points are initialized

Previously, calling _getTransformedHullPoints would result in an entirely new array being created and filled up with twgl.v3 objects each time.

Now, the array is created and filled with twgl.v3 objects only when the convex hull itself changes. _getTransformedHullPoints now modifies these twgl.v3 objects in-place. This change should greatly reduce the number of unnecessary objects created.

  • Add "transformed convex hull dirty" flag

Previously, calling _getTransformedHullPoints meant always recalculating the positions of transformed convex hull points. Now, they are only recalculated when the transform has actually changed.

These changes greatly reduce the allocations made by convex-hull functions:

"Vector scaling performance test 2" (makes heavy use of "if on edge, bounce")

Before After
image image
image image

const projection = twgl.m4.ortho(-1, 1, -1, 1, -1, 1);
const skinSize = this.skin.size;
const halfXPixel = 1 / skinSize[0] / 2;
const halfYPixel = 1 / skinSize[1] / 2;
const tm = twgl.m4.multiply(this._uniforms.u_modelMatrix, projection);
const transformedHullPoints = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you verified that callers never modify the contents of the array returned by this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_getTransformedHullPoints is called by getBounds and getBoundsForBubble.

getBounds passes it to Rectangle.initFromPointsAABB which does not modify the array.

getBoundsForBubble calls map and filter on the array, both of which create new arrays instead of modifying in-place.

@fsih fsih removed their assignment Jun 25, 2020
@BryceLTaylor BryceLTaylor modified the milestones: May 2019, Overdue Jul 13, 2020
@fsih fsih assigned adroitwhiz and unassigned cwillisf Jul 23, 2020
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.

👍

@cwillisf cwillisf merged commit 4ebea93 into scratchfoundation:develop Oct 8, 2020
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.

None yet

7 participants