Skip to content

Conversation

@adroitwhiz
Copy link
Contributor

@adroitwhiz adroitwhiz commented Jul 18, 2019

Proposed Changes

  • Replace Drawable.getEnabledEffects() function with Drawable.enabledEffects
  • Only apply effect transform in Drawable.getLocalPosition and Drawable.sampleColor4b if drawable effects are enabled
  • Replace possiblyDefinedArgument || defaultValue pattern with if (typeof possiblyDefinedArgument === 'undefined') possiblyDefinedArgument = defaultValue in EffectTransform methods

Reason for Changes

  • Replace Drawable.getEnabledEffects() function with Drawable.enabledEffects
    • The EffectTransform.transform[Color/Point] methods, called once per pixel in touching tests, get a Drawable's enabled effects. Replacing the pseudo-getter function with a property speeds it up.
  • Only apply effect transform in Drawable.getLocalPosition and Drawable.sampleColor4b if drawable effects are enabled
  • Replace possiblyDefinedArgument || defaultValue pattern with if (typeof possiblyDefinedArgument === 'undefined') possiblyDefinedArgument = defaultValue in EffectTransform methods
    • This provides a small performance boost, possibly by removing the "truthy/falsy cast".

This gives a ~40% performance improvement to "touching color" checks on drawables with no effects enabled, and a ~15% performance improvement to ones with effects enabled, as seen in this benchmark:

Current This PR
image image

if ((localPosition[0] >= 0 && localPosition[0] < 1) && (localPosition[1] >= 0 && localPosition[1] < 1)) {
// Apply texture effect transform if the localPosition is within the drawable's space,
// and any effects are currently active.
if (drawable.enabledEffects !== 0 &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Thanks for simplifying this stuff! I have one suggestion for further simplification but let me know if you can think of a reason not to do so.

@adroitwhiz adroitwhiz force-pushed the effect-transform-optimizations branch from ec83308 to 8d1117b Compare January 7, 2020 21:13
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.

👍 thanks!

@fsih fsih merged commit a67cd48 into scratchfoundation:develop Jan 9, 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.

4 participants