Add update drawable methods for each property #469
Conversation
| // TODO: vm's requests to drawableID that do not have a Drawable object. | ||
| if (!drawable) return; | ||
| drawable.updateEffect(effectName, value); | ||
| } |
There was a problem hiding this comment.
These are 9 methods which all do basically the same thing but on a different property. It would probably be more DRY to add a getDrawable method to RenderWebGL and then have the VM call the respective update methods directly on the drawable, though that would have the downside of coupling the Drawable's API and class structure to the VM somewhat.
There was a problem hiding this comment.
Hmm. That's a fair thought.
I think making such a change in this PR will be out of scope. We might be repeating ourselves some, but we are not changing the API in a large way.
that would have the downside of coupling the Drawable's API and class structure to the VM
This would be a pretty big change. RenderWebGL is essentially designed as a foreign function interface in case we need it to become one.
Hmm. I think this code is DRY. As each method calls a different property, they are not doing the same thing. There is the drawable safety check but that's unavoidable. It's the nature of checking that a variable has value before operating on it. It doesn't look dry, but I think that's just the limits of the JS syntax.
src/RenderWebGL.js
Outdated
| */ | ||
| updateDrawableSkinId (drawableID, skinId) { | ||
| const drawable = this._allDrawables[drawableID]; | ||
| // TODO: vm's requests to drawableID that do not have a Drawable object. |
There was a problem hiding this comment.
Can we make this todo comment (and the associated ones below) clearer in terms of what actually needs to be done?
There was a problem hiding this comment.
Alternatively, we should file an issue either here or in VM and note it in these todo comments as well as the one in the now deprecated updateDrawableProperties function.
There was a problem hiding this comment.
Sure.
For clarity: The text of this current comment is an abbreviation of the comment in updateDrawable. https://github.com/LLK/scratch-render/pull/469/files#diff-0e66131b35a2396bf8b007193ee7bc77R1437-R1442
if (!drawable) {
/**
* @todo fix whatever's wrong in the VM which causes this, then add a warning or throw here.
* Right now this happens so much on some projects that a warning or exception here can hang the browser.
*/
return;
This is a very old issue. And not one these new updateDrawable* methods are introducing.
Resolves
Make VM and Render faster.
Proposed Changes
Reason for Changes
The current updateDrawableProperties and updateProperties spend more time looking for what the changes are in the given properties than actually making changes to Drawable.
This is because most updateDrawableProperties calls from RenderedTarget change 1 or 2 properties. So of the 13 properties (6 base properties and 7 effect properties) 1 or 2 change. The remaining 12 or 11 checks for if something is in properties are false.
If we use an active api in place of the current passive api, we can inform renderer with Drawable changes much faster.
I think it may be best to deprecate existing methods to use one common path for changing drawable values. An additional benefit of this will be decreasing our temporary objects. Each update call currently creates at least one object. We can at a minimum decrease that by one.
Test Coverage
There does not currently seem to be a clear path to test these changes. RenderWebGL does not have a unit testing environment.
I could create new integration scratch file tests. Direction on this would be greatly appreciated.
Benchmark Data
I've opted to include % of function time results from chrome inspector for 14844969 and 130041250. These motion block heavy projects provide a good way to see the relative performance inside these functions.
14844969
changeX changes the x position of a sprite. This version does not include the other changes that will improve getFencedPosition. Using the new position calls releases a lot of time to be instead spent in getFencedPosition.
Changing skins is a unique case with the update calls. Relative to the other update calls changing the skin and rotation center do a lot more work. This will be improved separately from this change.
This table also illustrates well the time spent in updateProperties not performing any changes for skin changes. switchCostume doesn't modify effects, position, direction, scale or visibility. So that time is spent just checking if blocks and looping over the effects.
130041250
Like the changeX in the other project, the time is able to shift to time in getFenced. The improvement was to the point that the profiler did not sample any calls in the new function.
Since turnLeft and setDirection do not need to check the fenced position, this call goes from almost all time spent in update calls to no time.
An additional thread is clear here like switchCostume, there is spent in some work emitting events from RenderedTarget, that we can improve in a later PR.