Add JavaScript MIPs for scaling textures larger and smaller #431

ktbee · 2019-04-01T21:34:19Z

Resolves

#374, potentially also #334

Proposed Changes

As discussed on other issues, adding mipmap functionality will be helpful for scaling images at a good resolution in Scratch. This PR:

Adds a SVGMIP as a wrapper for WebGL textures to handle creating them at a certain scale and updating them if needed
Stores JavaScript mips on SVGSkin in an array _scaledMIPs using an index offset to allow for mips that have a scale of less than one. We're using an array here for better performance instead of an object with more descriptive keys
Extends while loop in SVGSkin.getTexture to get smaller scales in addition to larger ones

Here is an example repro project. After the green flag click, the original sprite is scaled up and down while the clone remains unscaled. Both have a better resolution because they no longer share a texture at all times and use the one at its requested scale. https://scratch.mit.edu/projects/295219677/editor/

The screenshots' differences are a little tough to see at a smaller size, but it's clear when you click them and see them in a new tab. Also these were taken on a lower resolution monitor. If your screen has a hidpi, these differences are harder to see.

Production	PR Changes

Memory usage

In our previous discussion of this topic, one point against scaling mips with JavaScript instead of automatically on the GPU is that this method would use more memory. From some profiling I did with Chrome and Firefox, it seems like this test project hovers around the same amount of memory on the develop branch and this feature branch. Here are the results I found:

Chrome	`develop`	`javascript-scaled-textures`
Memory Footprint	192 MB	190 MB
GPU Memory	28.3 MB	28.5 MB

Firefox	`develop`	`javascript-scaled-textures`
Trial 1	81.96 MB (09.74%)	82.49 MB (09.79%)
Trial 2	73.19 MB (08.73%)	104.62 MB (11.86%)
Trial 3	99.08 MB (11.43%)	84.35 MB (09.85%)

Theoretically there should be a memory usage increase because there are more textures. I added the surface areas of power of 2 sized images between 2x2 and 1024x1024 and found that they would use about 33% more memory than just a single 1024x1024 image, but perhaps in the way we're using them in this branch, the browser is able to release some of that memory. From what I can see using Chrome and Firefox's tools, multiple JavaScript scaled textures hasn't had much of an impact on a tab's memory usage. I'm curious to hear more thoughts on this though!

What this hasn't solved yet

I've found this implementation of mips successfully scales sprites that have been changes by a "looks" block, but sprites that haven't been scaled by a block still don't scale well. For example, when you reduce the browser's width while using the editor, the stage scales down and the sprite's quality is reduced:

Full size stage	Smaller stage

Fullscreen mode is another example of this:

Thoughts on how to scale sprites in this scenario and also thoughts on this approach for mipmaps are much appreciated!

thisandagain · 2019-04-01T22:54:18Z

/cc @kchadha @rschamp @paulkaplan

cwillisf

This looks great! I'm so glad to have this handled in a more intelligent way :D

I understand we're going to discuss this in an upcoming, but here are a few initial thoughts:

Wrapping the fancy stuff into a new SVGMIP class makes sense to me. I'm tempted to suggest moving the whole mip chain into a separate handler class but since getTexture already encapsulates the code into a function moving it into a class would be of limited practical benefit. I guess. ;)
I'm a little surprised by the memory usage numbers but I'm glad you took the time to collect them. Your math is accurate -- the extra texture memory needed for an "infinite" MIP chain tends toward 33%.
Did you consider using OpenGL's built-in mipmap support? There might be a reason not to choose this that I haven't thought of, but it seems like it would simplify the code. I think you and @mzgoddard convinced me that we shouldn't auto-generate mipmaps but by using the level parameter of glTexImage2D we could upload the individual images all into one GL texture and let the GPU choose the mip level automatically. We could even have the GPU interpolate between mip levels but that might not lead to good results with vector images.

Sorry for the delay in responding to this; I've been focusing hard on extensionification and let my PR duties fall behind.

adroitwhiz · 2019-04-18T07:38:58Z

Did you consider using OpenGL's built-in mipmap support? There might be a reason not to choose this that I haven't thought of, but it seems like it would simplify the code.

IIRC, ~~OpenGL~~WebGL mipmaps only work with textures whose dimensions are a power of 2.

ktbee · 2019-04-18T18:32:54Z

We discussed this a bit offline, but just to follow up for anyone watching the issue:

@adroitwhiz has a good point, for WebGL 1, automatic mipmaps only work for images with dimensions that are a power of two. We'd need to use an OpenGL extension or upgrade our WebGL version to be able to automatically generate mipmaps for non-power of two images.

ktbee · 2019-04-18T19:05:16Z

Also @cwillisf @kchadha, I turned off twgl's automatic mipmap creation option in this PR and it is all set to be reviewed!

cwillisf

👍code looks good but tests are failing... marking as "needs work" for the tests

ktbee · 2019-05-22T20:55:24Z

Hooray for tests! They helped me catch a couple bugs, which I fixed with the following changes:

Use this._texture.getTexture() instead of just this._texture in SVGSkin's dispose function.
Updated the silhouette for sprites with a scale of 1 to make sure it's set initially even if the sprite isn't scaled.

@cwillisf let me know what you think!

src/SVGSkin.js

adroitwhiz · 2019-06-03T13:34:25Z

src/SVGSkin.js

+                this._silhouette.update(textureData);
+            }
+
+            if (scale === 1) {


This seems a bit flaky-- it looks like it assumes createMIP will first be called with scale 1, which may not be the case. Could this be moved elsewhere, perhaps to setSVG?

Hmm, it's not meant to assume createMIP will be called with a scale 1. The intention instead is if it gets called with a scale of 1, use the textureData to set the _maxTextureScale and update the silhouette. We need it to be in the callback in createMIP instead of setSVG because we need the textureData that is passed to the callback in SVGMIP.draw.

It seems like _maxTextureScale could be uninitialized (stuck at 1) if createMIP is never called with a scale of exactly 1. Is it possible to get in that situation, like if you load a project where a sprite's scale is >1 or something like that? It's important for _maxTextureScale to be as high as reasonable to avoid reintroducing this issue: scratchfoundation/scratch-gui#4211

After looking through the code some more I think this sequence would cause some sort of bad behavior, though I'm not sure exactly what:

Have a vector costume on a sprite (createMIP will be called with scale 1)

Use a looks block to set size to something big (createMIP will be called with scale >1)

Open that costume in the paint editor

Make a change to the costume which changes its dimensions significantly (setSVG will be called, dirtying most MIPs, but createMIP won't be called with scale 1, so _maxTextureScale will be based on the old dimensions)

I did this sequence of steps and this one weird trick happened (prod vs with this pr patched)

This thing I saw may have been scratchfoundation/scratch-gui#5216 (comment)

cwillisf

I reviewed this with help from @fsih and @kchadha; overall it seems like a good approach but we had a few concerns about complexity / readability and one or two specific concerns about potential bugs. Happy to discuss in more detail if there's something I/we missed!

cwillisf · 2019-09-24T19:56:06Z

src/SVGMIP.js

+        this._callback = callback;
+        this.dirty = false;
+
+        this.draw();


I think setting dirty to true by default and skipping this.draw() here might allow this to skip drawing sometimes, or at worst wouldn't hurt.

It doesn't have to be for every MIP. But we should at least call draw in setSVG on the 1.0 scale MIP. This "primes" the SVGRenderer by loading an Image instance and drawing it to a canvas. The loading part can need a lot of time, so we should load the svg during the initial project load to avoid potentially large draw times when you switch to a SVG costume that hasn't been drawn before.

cwillisf · 2019-09-24T19:59:13Z

src/SVGSkin.js


        /** @type {Number} */
-        this._maxTextureScale = 0;
+        this._largestTextureScale = 1;


I think it's confusing to have _maxTextureScale and _largestTextureScale in the same class. Maybe rename one or both to clarify the difference?

Combined with another comment, we could get rid of _largestTextureScale. It is needed if we keep already created MIPs. If we dump all MIPs on setSVG calls, we don't need _largestTextureScale.

cwillisf · 2019-09-24T20:07:34Z

src/SVGSkin.js

-            this._renderer.gl.deleteTexture(this._texture);
+            for (const mip of this._scaledMIPs) {
+                if (mip) {
+                    this._renderer.gl.deleteTexture(mip.getTexture());


It might make sense to move this into a dispose method on SVGMIP.

cwillisf · 2019-09-24T20:27:17Z

src/SVGSkin.js

+                this._silhouette.update(textureData);
+            }
+
+            if (scale === 1) {


It seems like _maxTextureScale could be uninitialized (stuck at 1) if createMIP is never called with a scale of exactly 1. Is it possible to get in that situation, like if you load a project where a sprite's scale is >1 or something like that? It's important for _maxTextureScale to be as high as reasonable to avoid reintroducing this issue: scratchfoundation/scratch-gui#4211

After looking through the code some more I think this sequence would cause some sort of bad behavior, though I'm not sure exactly what:

Have a vector costume on a sprite (createMIP will be called with scale 1)

Use a looks block to set size to something big (createMIP will be called with scale >1)

Open that costume in the paint editor

Make a change to the costume which changes its dimensions significantly (setSVG will be called, dirtying most MIPs, but createMIP won't be called with scale 1, so _maxTextureScale will be based on the old dimensions)

cwillisf · 2019-09-24T20:28:51Z

src/SVGSkin.js

+                for (testScale; maxDimension * testScale <= MAX_TEXTURE_DIMENSION; testScale *= 2) {
+                    this._maxTextureScale = testScale;
+                }
+                this._silhouette.update(textureData);


I think it could be confusing to have a silhouette update in this branch as well as the if (scale > this._largestTextureScale) branch. Also, what if this callback gets called with scale > 1 first, then scale === 1 later -- it seems like we'd end up with an undesirable(?) update to a smaller silhouette.

src/SVGSkin.js

cwillisf · 2019-09-24T21:55:14Z

src/SVGSkin.js

-                this._texture = twgl.createTexture(gl, textureOptions);
-                this._silhouette.update(textureData);
-            }
+        this._svgRenderer.loadString(svgData);


Instead of using a dirty flag on the SVGMIP instances, why not just dump all the SVGMIPs here? That might take some extra time (deleting WebGL textures) but unless it's a LOT of time I'd rather have simpler code => less chance of breaking it later :)

We could:

Call dispose on each current MIPs.

Set the array length to 0.

Create the 1.0 scale MIP at INDEX_OFFSET.

Force the 1.0 MIP to draw.

mzgoddard · 2019-10-02T20:00:47Z

src/SVGSkin.js

-            this.setRotationCenter.apply(this, rotationCenter);
-            this.emit(Skin.Events.WasAltered);
-        });
+        if (typeof rotationCenter === 'undefined') rotationCenter = this.calculateRotationCenter();


In a different change we realized that setting the rotationCenter before the image has drawn will cause a "flash" of the old image at the wrong scale. We need to setRotationCenter after drawing.

One way we might do that is move the rotationCenter set logic in createMIP?

src/SVGSkin.js

mzgoddard · 2019-10-24T19:48:11Z

src/SVGSkin.js

+        const textureCallback = textureData => {
+            // Check if we have the largest MIP
+            // eslint-disable-next-line no-use-before-define
+            if (!this._scaledMIPs.length || this._scaledMIPs[this._scaledMIPs.length - 1]._scale <= scale) {


I think we can make this a little simpler.

One way would be if you pass another argument to createMIP. Pass the scaled index and we can this._scaledMIPs.length - 1 === scaleIndex to test for this.

mzgoddard · 2019-10-24T19:50:13Z

src/SVGSkin.js

-                super.setEmptyImageData();
-                return;
-            }
+    resetMIPs (mip, rotationCenter) {


I think this method is pretty slick.

…s at a different scale and updating them Check scale for largest MIP

BryceLTaylor · 2019-11-19T21:57:05Z

Test Plan:

Description: We will now take vector images and render them at several different sizes in memory to make scaling images look better.

To test:

clone the sprite so you can see it and the original and try all of the below (make changes to both)
Take a vector image and scale it to different sizes
Make sure it looks right when making it bigger
Make sure it looks right when making it smaller
Start with a huge sprite and make it very small
Start with a tiny sprite and make it tinier
Start with a very small sprite and make it very big
Edit a sprite while it is in different sizes
Make the bounds of the sprite larger or smaller (erase parts)
add graphics effects at different sizes
rotate the sprite at different sizes

It's important to get Browser coverage for this issue

cwillisf

👍

ktbee added pr - needs review compatibility labels Apr 1, 2019

ktbee requested review from mzgoddard, kchadha, fsih and cwillisf April 1, 2019 21:34

thisandagain assigned cwillisf Apr 1, 2019

thisandagain added this to the April 2019 milestone Apr 1, 2019

ktbee changed the title ~~Add SVGMIP as a wrapper for WebGL textures to handle creating texture…~~ Add JavaScript MIPs for scaling textures larger and smaller Apr 2, 2019

cwillisf reviewed Apr 18, 2019

View reviewed changes

cwillisf mentioned this pull request Apr 18, 2019

Low quality on vector costumes in smaller stage mode #437

Closed

ktbee force-pushed the javascript-scaled-textures branch from 9b039ce to 5e625b8 Compare April 18, 2019 18:50

ktbee marked this pull request as ready for review April 18, 2019 19:04

cwillisf approved these changes Apr 30, 2019

View reviewed changes

cwillisf added pr - changes requested and removed pr - needs review labels Apr 30, 2019

ktbee force-pushed the javascript-scaled-textures branch from e20f682 to b00e0c4 Compare May 13, 2019 17:42

ktbee force-pushed the javascript-scaled-textures branch 2 times, most recently from 1cffc87 to 8851b8c Compare May 22, 2019 19:51

ktbee added pr - needs review and removed pr - changes requested labels May 22, 2019

adroitwhiz reviewed May 23, 2019

View reviewed changes

src/SVGSkin.js Outdated Show resolved Hide resolved

ktbee force-pushed the javascript-scaled-textures branch 2 times, most recently from 8f1d47c to 231cae1 Compare May 23, 2019 18:42

adroitwhiz reviewed Jun 3, 2019

View reviewed changes

cwillisf requested changes Sep 24, 2019

View reviewed changes

cwillisf added pr - changes requested and removed pr - needs review labels Sep 24, 2019

cwillisf assigned ktbee and unassigned cwillisf Sep 24, 2019

cwillisf modified the milestones: April 2019, October 2019 Sep 24, 2019

mzgoddard reviewed Oct 2, 2019

View reviewed changes

ktbee force-pushed the javascript-scaled-textures branch from 231cae1 to 895467d Compare October 4, 2019 20:36

ktbee force-pushed the javascript-scaled-textures branch from 895467d to c520e41 Compare October 21, 2019 19:07

mzgoddard reviewed Oct 24, 2019

View reviewed changes

src/SVGSkin.js Outdated Show resolved Hide resolved

mzgoddard reviewed Oct 24, 2019

View reviewed changes

adroitwhiz mentioned this pull request Oct 26, 2019

Don't multiply SVG size by device pixel ratio scratchfoundation/scratch-svg-renderer#84

Merged

Add SVGMIP as a wrapper for WebGL textures to handle creating texture…

f172526

…s at a different scale and updating them Check scale for largest MIP

ktbee force-pushed the javascript-scaled-textures branch from c520e41 to f172526 Compare October 28, 2019 17:53

ktbee requested a review from cwillisf October 28, 2019 17:54

ktbee added pr - needs review and removed pr - changes requested labels Oct 28, 2019

fsih mentioned this pull request Nov 21, 2019

Vector sprites appear jagged after exiting full-screen mode #526

Closed

adroitwhiz mentioned this pull request Nov 21, 2019

Synchronous SVG mipmaps #527

Merged

cwillisf approved these changes Nov 26, 2019

View reviewed changes

cwillisf added pr - good to merge and removed pr - needs review labels Nov 26, 2019

cwillisf modified the milestones: October 2019, November 2019 Nov 26, 2019

cwillisf merged commit b1274d5 into scratchfoundation:develop Nov 26, 2019

ktbee deleted the javascript-scaled-textures branch December 3, 2019 21:18

Add JavaScript MIPs for scaling textures larger and smaller #431

Add JavaScript MIPs for scaling textures larger and smaller #431

Uh oh!

Conversation

ktbee commented Apr 1, 2019

Resolves

Proposed Changes

Memory usage

What this hasn't solved yet

Uh oh!

thisandagain commented Apr 1, 2019

Uh oh!

cwillisf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adroitwhiz commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktbee commented Apr 18, 2019

Uh oh!

ktbee commented Apr 18, 2019

Uh oh!

cwillisf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktbee commented May 22, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fsih Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fsih Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwillisf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzgoddard Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryceLTaylor commented Nov 19, 2019

Uh oh!

cwillisf left a comment

Choose a reason for hiding this comment

Uh oh!

cwillisf left a comment •

edited

Loading

adroitwhiz commented Apr 18, 2019 •

edited

Loading

cwillisf left a comment •

edited

Loading

fsih Sep 24, 2019 •

edited

Loading

fsih Sep 26, 2019 •

edited

Loading

mzgoddard Oct 2, 2019 •

edited

Loading