-
Notifications
You must be signed in to change notification settings - Fork 352
Use premultiplied alpha everywhere, take 2 #515
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
Use premultiplied alpha everywhere, take 2 #515
Conversation
|
/cc @rschamp @mzgoddard |
|
Pages 20-21 of WebGL Insights go into a bit more detail on the performance impact of |
2b8ba22 to
36e81f5
Compare
36e81f5 to
eb9f50e
Compare
eb9f50e to
10a6d87
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome - thanks so much -- and thanks for being so thorough! There are a couple things to fix up but we're really excited about this :)
d663b6c to
708f9e1
Compare
|
I hope I've addressed all feedback; thanks for the review! |
|
P.S. any idea how @mzgoddard benchmarks loading speed consistently? I may try reverting #414 in a follow-up PR per the WebGL Insights analysis I linked earlier. |
708f9e1 to
0cb97a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates!
Resolves
Resolves #513
Resolves #237
Proposed Changes
gl.UNPACK_PREMULTIPLY_ALPHA_WEBGLbefore callingtexImage2D_texturetoSkinbase class_setTexturehelper function toSkinbase classtwgl.createTexturegl.blendFunc(gl.ONE, gl.ONE_MINUS_SRC_ALPHA)for all blendingstampdraw modeRenderWebGL.sampleColor3btoSilhouette.getColormethodsSilhouettes to take both non-premultiplied and premultiplied image data by splittinggetColor4bintogetColor4bandgetPremultipliedColor4bReason for Changes
gl.UNPACK_PREMULTIPLY_ALPHA_WEBGLbefore binding texturesThis causes textures' alpha channels to be premultiplied before being sent to the GPU. This is necessary for proper blending around edges of sprites; see this article ("GPUs prefer premultiplication") for more info
_texturetoSkinbase class_setTexturehelper function toSkinbase classThese changes reduce code duplication.
_texturewas previously initialized by every implementation ofSkin, so I moved it to the base class to reduce redundancy._setTextureperforms the steps of binding the texture, setting and unsettinggl.UNPACK_PREMULTIPLY_ALPHA_WEBGL, callingtexImage2D, and updating the skin's silhouette. This would otherwise have to be done in every place that sets aSkinimplementation's texture to an image.twgl.createTextureBecause
Skintextures are non-power-of-two, no mipmaps can be generated, but for some reason twgl will attempt anyway unless explicitly told not to. This change makestwgl.createTexturethrow out tons ofglGenerateMipmapwarning messages because it is no longer passed a texture (the texture is now set outside the function that creates it).gl.blendFunc(gl.ONE, gl.ONE_MINUS_SRC_ALPHA)for all blendingThis is the proper blend function for compositing premultiplied textures.
Everything should be premultiplied.
stampdraw modeThe
stampdraw mode has not done what it's been said to do ("Draw normally except for pre-multiplied alpha") since #418. It has, in fact, done nothing differently than thedefaultdraw mode.RenderWebGL.sampleColor3btoSilhouette.getColormethodsAs described in the article I linked above, color values must be multiplied by alpha before interpolation to avoid fringing artifacts.
Silhouettes to take both non-premultiplied and premultiplied image data by splittinggetColor4bintogetColor4bandgetPremultipliedColor4bA
Silhouettecan take either data from an image, which is not premultiplied, or data from the GPU, which is premultiplied. If the data does not come premultiplied, we want to premultiply it. If it does come premultiplied, we don't want to multiply it again.I accomplished this by adding the
premultipliedgetter/setter toSilhouette. If set totrue, it indicates that data passed to the silhouette already comes premultiplied, and if set tofalseit indicates that it is not premultiplied.I added a
_getColormember toSilhouettewhich theSilhouette.colorAtfunctions now call instead ofgetColor4b. If the silhouette ispremultiplied, the member will point towards thegetPremultipliedColor4bfunction. If not, it will point towards thegetColor4bfunction.This does not appear to regress performance of
Silhouetteoperations.Unanswered Questions
It was previously determined that setting textures from
ImageDatawas faster than setting them fromHTMLCanvasElements. This may have been becauseImageDatawas not premultiplied, and the textures were not either, allowing the browser to copy the texture data to the GPU directly rather than dividing each individual pixel by its alpha.With this PR, textures are now premultiplied, like an
HTMLCanvasElement's backing store. So is it faster to set premultiplied textures from a canvas instead of grabbing theImageData?The current shader applies the "ghost" effect (which now multiplies all color values) before the "brightness" and "color" effects (which read color values). This appears to produce correct results, but is probably incorrect. However, I cannot find a way to fix it without spaghettifying the code. How should this be handled? Should the shader divide alpha values if brightness/color are enabled and multiply them again afterwards?Both in the current code and here,
extractDrawablereturns a premultiplied image. This produces a dark fringe around dragged sprites. Is this something to fix in the future?