Handle subpixel viewboxes for SVG skins #594

adroitwhiz · 2020-05-05T16:32:52Z

Depends on #745

Resolves

Resolves #143
Resolves #233
Resolves #427
Resolves #533
Resolves #639

Proposed Changes

~~Clean up the SVGSkin code a bit~~ Done in Always use SVG mipmaps that match or exceed the skins' screen-space size #649
Set the minimum SVG texture scale to 1/8 its normal size
- This is mentioned in Subpixel SVG costume handling: a writeup #550--the "native size" of the rendered sprite must be a multiple of 1/the smallest scale, so this means that all SVG skins' dimensions will be rounded up to a multiple of 8. Setting this lower (to, for instance, 1/32 scale) would require all SVG skins' dimensions to be rounded up to a multiple of 32. I think 1/8 is a good tradeoff between skins not looking pixelated at small sizes and minimizing the amount of "slack space" around the edges.
Use the adjusted bounds + center from the algorithm in Subpixel SVG costume handling: a writeup #550 on SVG skins.
Pass "logical bounds" into the shader so it can calculate "logical coordinates" under which distortion effects are applied.
- SVG skins' bounds are now expanded, but we want things like the whirl and fisheye effects' center to still be positioned in the middle of the original, non-expanded bounds. We also don't want the pixelate or mosaic effects to go beyond the non-expanded bounds. To do this, we tell the shader what the original bounds are.
Separate skins' size and rotation center into a "native size" + "native rotation center" and "quad size" + "quad rotation center'.
- The "native size" and "native rotation center" refer to the un-expanded bounds. They are used to calculate skins' AABBs and when the VM wants to get a skin's size. They do not depend on details like the amount of "slack space" present around the adjusted texture, and as such are used for fencing sprites.
- The "quad size" and "quad rotation center" refer to the expanded bounds. They are used for everything else, and are so named because they refer to the size of the actual rendered quadrilateral. I'm not sure what else to call them-- "texture size" seems misleading. Any ideas?

Reason for Changes

As I talked about in #550, these changes seem to be the best way to fix the jittering problem that vector costumes have at the moment. These do add some complexity but I'm not sure how to avoid it, so I could use your help in that area.

Test Coverage

Tests ~~are blocked on #537 and~~ still need some discussion on what should be tested

src/SVGSkin.js

src/shaders/sprite.vert

src/Skin.js

src/Drawable.js

fsih · 2020-07-17T17:54:10Z

src/Drawable.js

@@ -102,6 +102,9 @@ class Drawable {
        this._inverseTransformDirty = true;
        this._visible = true;

+        this._aabbDirty = true;


transformDirty means that one or more of rotationTransformDirty, skinScaleDirty, rotationCenterDirty.
transformDirty implies aabbDirty, but aabbDirty doesn't imply transformDirty

src/Drawable.js

src/EffectTransform.js

adroitwhiz · 2021-03-15T03:49:29Z

@fsih @cwillisf This should be ready now that scratch-svg-renderer is all split up and we can do the viewbox logic here! I've also tried my best to address the review comments. Also note that this depends on #745.

I remember you not liking the names quadSize and quadRotationCenter--any ideas what I could use instead? textureSize/textureRotationCenter seems misleading because that seems to refer to the texture's actual dimensions, which isn't what quadSize and rotationCenter refer to.

Skins' "native size" is the size that their bounds are supposed to be. Their "quad size" is the size the quadrilateral is really rendered at.

_setSubpixelViewbox ensures nonzero image dimensions at all rendered scales.

...and just return nativeRotationCenter in getSkinRotationCenter to do so.

This was a case of premature optimization that needs to go for the next commit's optimization to work.

This is an optimization from PR scratchfoundation#470 that was taken out because of other unrelated changes in that PR which kept breaking things. We can put it back in now, and it's especially useful here to avoid going through a nested getter when accessing quadSize.

Now we only mess around with "logical bounds" in the shader if effects which distort the sprite are enabled. This also avoids passing the extra v_logicalCoord varying.

This results in a tad bit of duplicated code, but considering that we have 4 different code paths (colorAtNearest, colorAtLinear, isTouchingNearest, and isTouchingLinear) for sampling a silhouette, that's to be expected. This more closely matches the GPU pipeline, in which color and position calculations are more intertwined. This replaces the hacky fix in scratchfoundation#424 with a solution that matches the GPU: instead of not transforming points outside the skin bounds, just return transparency/false early.

This provides a slight but noticeable performance improvement, and gains back some of what was lost in the previous commit.

adroitwhiz added pr - changes requested needs-discussion labels May 5, 2020

adroitwhiz requested review from fsih and cwillisf May 5, 2020 16:32

adroitwhiz assigned fsih, cwillisf and adroitwhiz May 5, 2020

adroitwhiz force-pushed the subpixel-v3 branch from 8317671 to 7eadb5f Compare May 12, 2020 20:49

adroitwhiz force-pushed the subpixel-v3 branch 2 times, most recently from 6d843b0 to 7eadb5f Compare June 8, 2020 02:25

adroitwhiz mentioned this pull request Jun 17, 2020

Should vector skins/costumes always be rendered with a resolution greater than their screen-space dimensions? #620

Closed

adroitwhiz force-pushed the subpixel-v3 branch from 7eadb5f to 95c1e1d Compare July 14, 2020 20:10