Skip to content

Add new draw and loadSVG methods to the SvgRenderer #107

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
merged 2 commits into from
Jan 3, 2020

Conversation

adroitwhiz
Copy link
Contributor

@adroitwhiz adroitwhiz commented Nov 1, 2019

Resolves

A step towards resolving scratchfoundation/scratch-render#518

Proposed Changes

This PR adds two public methods and one private method to SvgRenderer:

  • loadSVG, which takes in an SVG string, loads it with fromString, creates an <img> that can be rendered to a canvas, then calls the passed callback
  • draw (not _draw), which synchronously draws a previously loaded SVG to the SvgRenderer.canvas
  • _createSVGImage, which handles creation of the <img> itself

It also:

  • Adds the loaded getter property, which returns whether or not the <img> has finished loading and can be rendered.
  • Deprecates fromString and _draw. I checked scratch-gui, scratch-paint, and scratch-vm, and none of them use these methods--it's only scratch-render that uses them, and with Synchronous SVG mipmaps scratch-render#527 merged, it will stop using them.

Reason for Changes

Currently, the SvgRenderer's methods conflate:

  • The asynchronous operation of loading a new SVG as an <img> element
  • The synchronous operation of drawing an already loaded SVG to a <canvas> at an arbitrary scale

This can result in unpredictable behavior. For instance, the _draw method executes asynchronously the first time it is called after a new SVG has been loaded, as it has not yet created an <img> from the SVG. However, subsequent calls will be synchronous, as the <img> has been created and can be synchronously drawn to a <canvas>.

These changes make the synchronous/asynchronous split explicit with new methods that are either always synchronous or always asynchronous.

@adroitwhiz
Copy link
Contributor Author

I did some weird Git stuff to squash commits-- the review feedback is addressed in "Reuse _cachedImage"

*/
_draw (scale, onFinish) {
// Convert the SVG text to an Image, and then draw it to the canvas.
if (this._cachedImage) {
this._drawFromImage(scale, onFinish);
if (this._cachedImage === null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one should stay as this._cachedImage === null to avoid creating duplicate images

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.

LG! Thanks!

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

3 participants