Multisampling in FrameBufferSystem #6296

ivanpopelyshev · 2019-12-25T16:10:39Z

My minimal implementation for #6288

Demo: https://www.pixiplayground.com/#/edit/ZbmXFH7nt11L70Ux2jUy8

🎁 Added

Framebuffer.#multisample - number of samples that user wants. By default its 0 so shortcut if (multisample) can be used.
Extra operation blit that has to be called after renderer.render if you think that msaa is enabled on renderTexture. Operation corresponds to low-level gl.blitFramebuffer and requires extra internal framebuffer object to bind the texture.

How it works

API is secure - It won't fail in case of WegGL1 or if there's not enough samples.

Internally, colorTextures[0] for MSAA renderbuffer, so, theoretically, MRT still should work. I didn't test it though. However, colorTextures[0] still exists - that's where we blit the data for later usage.

Why not RenderBuffer object

MSAA is slow and eats extra memory.
WebGL2 doesn't exist in Safari.
I didn't see ThreeJS projects that use MSAA in production.

I do not want to add new objects and abstractions in vanilla PixiJS for a case that cant be used in production.

However, it is good as a temporary solution for demos and prototyping games. Basically, its a Toy.

Two-three hundred lines of code are not much, we just mirror WebGL2 API a bit so people can experiment how it looks.

Future:

Both PR's should be small.

RenderTextureSystem

Add high-level blit operation, that can will either use framebuffer blit, either texture copy either scaling filter, that will enable trivial WebGL1 fallback.

FilterSystem

We can enable usage of custom texture pools per object, specify that we need particular renderTexture for dealing with a container, and that texture can have multisampling.

What cant be in vanilla:

MSAA is slow. I don't believe anyone will use it in production even after those PR's.

I'm working on a real solution that can be used in production by heavy projects:

real WebGL1 antialiasing fallback
caching system with atlases
several antialiasing options for containers / graphics
solution for strange conflation in MSAA :

If this PR will be merged, I won't need to add framebuffer hacks in the plugin.

Another plugin

I hope that this thing covers @SukantPal case for filters, maybe not right now but after next FilterSystem PR.

ShukantPal

I think this gets it wrong - you shouldn't have to do a manual blit if you are rendering into a render-texture. In addition, you could add a multisample property on RenderTexture that defers to this.framebuffer.multisample.

ShukantPal

The semantics are inconsistent - a render texture won't 'blend' if clear=false in Renderer.render & multisampling is enabled.

ShukantPal

Your demo doesn't work on Firefox (MacBook Pro, Catalina 2018) (that is a WebGL 2 env). It works on Safari (WebGL 1) but there is no MSAA on that.

I think you need to fix that; have you tested on a WebGL 2 machine?

ShukantPal · 2019-12-25T17:48:08Z

Also, "4. Strange conflation with MSAA" - I think that is because you are not clearing the frame buffer with the background before rendering to it.

ivanpopelyshev · 2019-12-25T17:59:52Z

Oh, no, macs, my bane! I hate them really, I dont have one. I'll ask for a testing device on work tomorrow.

There shoulnd't be a problem with clearMode, because its an input texture that should be cleared anyway. Maybe I'll add clear there in case of forceClear just to be compliant.

Exactly, case 4 is done when something is already drawn, and the problem is that users will do it anyway. We need a way to mitigate the damage, my solution is to apply AA to all shapes befure they start combining them.

I've modified demo a bit to compare samples=4 and samples=8.

ShukantPal · 2019-12-25T18:01:23Z

@ivanpopelyshev Why are using resolution=.125 in your demo?

eXponenta · 2019-12-25T18:13:58Z

MacOS/ iOS WebGL2 support is worst.

ShukantPal · 2019-12-25T18:15:06Z

MacOS/ iOS WebGL2 support is worst.

@eXponenta The other PR works on my machine (the one with Renderbuffer classes). It is actually a problem with this PR. @ivanpopelyshev What machine are you testing on, does it have WebGL 2?

eXponenta · 2019-12-25T18:20:49Z

His machine has fully WebGL2 support.

ShukantPal · 2019-12-25T18:21:38Z

@eXponenta WebGL 2 support depends on the browser I guess, not on the machine.

eXponenta · 2019-12-25T18:22:30Z

It isn't fully true.
Because on windows there is a AGILE (Chromium) backend for Webgl to dx translation.
But on other machines (OS) webGL use OpenGL. Apple stop support openGL greater than 2.0es (WebGL 1), and WebGL2 translate to metal, with some problems.

ShukantPal · 2019-12-25T18:24:03Z

@eXponenta Well, MacBooks of course have the graphics ability to support WebGL 2. The browsers are supposed to bind the WebGL API to a OpenGL backend.

ivanpopelyshev · 2019-12-25T20:08:04Z

The heck is going on on apple devices? We'll know tomorrow when I get said devices from test team! Stay tuned!

codecov-io · 2019-12-26T10:12:39Z

Codecov Report

Merging #6296 into dev will decrease coverage by 0.62%.
The diff coverage is 94.28%.

@@            Coverage Diff             @@
##              dev    #6296      +/-   ##
==========================================
- Coverage   77.79%   77.16%   -0.63%     
==========================================
  Files         186      183       -3     
  Lines       10219     9670     -549     
==========================================
- Hits         7950     7462     -488     
+ Misses       2269     2208      -61

Impacted Files	Coverage Δ
packages/prepare/src/BasePrepare.js	`73.43% <ø> (ø)`	⬆️
packages/text-bitmap/src/BitmapFontData.js	`100% <100%> (ø)`
packages/text-bitmap/src/BitmapText.js	`80.21% <100%> (-2.72%)`	⬇️
packages/mixin-cache-as-bitmap/src/index.js	`52.72% <100%> (ø)`	⬆️
...es/core/src/filters/spriteMask/SpriteMaskFilter.js	`95.23% <100%> (ø)`	⬆️
packages/core/src/Renderer.js	`89.61% <100%> (-1.23%)`	⬇️
packages/graphics/src/utils/buildCircle.js	`57.14% <100%> (ø)`	⬆️
packages/text-bitmap/src/formats/index.js	`100% <100%> (ø)`
packages/text-bitmap/src/formats/XMLFormat.js	`100% <100%> (ø)`
packages/text-bitmap/src/BitmapFontLoader.js	`78.43% <82.14%> (+3.43%)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 353f7db...28f4c7f. Read the comment docs.

ivanpopelyshev · 2019-12-26T11:19:47Z

Shame on me, I had a mistake in FOR cycle! Added a unit test for it. Now it should work on mac devices that somehow support webgl2

ivanpopelyshev · 2019-12-26T12:44:35Z

Need to test this thing on iOS 13.2 Safari . @SukantPal whats your device?

ShukantPal · 2019-12-26T14:34:39Z

@ivanpopelyshev It works now! (macOS Catalina, 2018 i7 MacBook Pro).

And MacBooks don't "somehow" support WebGL 2 - Firefox & Chrome do on all of 'em. It is just that Safari developers aren't implementing that API.

GoodBoyDigital

nice man! WIll need to discuss with @SukantPal which of the two PRs should be merged in.

Currently i'm leaning towards this PR as there is less Code to get us MSAA.
But @SukantPal's PR seems more flexible.. my question would be is there anything other than MSAA we can achieve with blitting and renderBuffers?

GoodBoyDigital · 2020-01-07T21:00:35Z

packages/core/src/framebuffer/FramebufferSystem.js

+     * @param {number} samples number of samples
+     * @returns {number} recommended number of samples
+     */
+    detectSamples(samples)


do we need this? Can we rather, just check values are PO2?

I really don't know what to put there. This thing works and is covered by a test. I don't know which values are available on different systems, I didn't research it.

I think we should just enforce the use of the consts?

export enum MSAA_QUALITY {
NONE = 0,
LOW = 0,
MEDIUM = 4,
HIGH = 8
}

nothing about pow2. I do not understand what do you want to put there. I made basic algorithm, it should work according to the test and specification. If user wants to specify 16 and some videocard actually allows it - fine. Btw, there's no guarantee about order and i use it. Maybe I should actually add sort() somewhere just to be sure.

it looks like there's no 16: https://webglstats.com/webgl2/parameter/SAMPLES

Maybe its my stupid time - I don't know what to write with those IF's. There will be more code in that case.

ShukantPal · 2020-01-12T16:02:49Z

@bigtimebuddy @GoodBoyDigital I don't think we should really delay this any longer. If you want, we could finalize this pull request.

I just want you to consider whether we should require users to do a blit after rendering to a multisample render-texture. I think that'll be a big source of bugs.

We might also need to consider blend modes for multisample render-textures. Renderbuffer blits don't support "blending", and this will break some expected behaviors. Instead of blitting directly onto the "texture", we might need to blit into a secondary texture and then render that onto the actual render-texture (via a shader that will blend).

ivanpopelyshev · 2020-01-12T16:10:32Z

Instead of blitting directly onto the "texture", we might need to blit into a secondary texture and then render that onto the actual render-texture (via a shader that will blend).

This PR doesn't actually forbid a blitting with shader, it doesnt include it on low-level. I use extra filter for blitting in my fork, and I think it has to be part of high-level object, alongside with WebGL1 fallback and some other things.

ShukantPal · 2020-01-12T19:11:49Z

@ivanpopelyshev I don't understand what you said. "This PR doesn't forbid blitting with a shader". How? First of all, blitting can't be done by a shader (by definition). It just copying the pixels from a render buffer onto a texture (no blending, direct copy).

"I use extra filter for blitting in my fork, and I think it has to be part of high-level object" - that doesn't make sense.

All my complaint is that we're going to have to add a line after rendering to use multisample in PixiJS -

app.renderer.render(circle, buf);
app.renderer.framebuffer.blit();

What is the problem if the blitting is handled as a part of the rendering pipeline. I don't think any user would expect to a blit after calling render. Do you have an example of a framework that uses these semantics? You said you wanted "users to know about the blit operation". Now @bigtimebuddy & @GoodBoyDigital will have to decide whether that reason is enough to separate blitting from the rendering process.

ShukantPal · 2020-01-12T19:15:20Z

@ivanpopelyshev I still don't understand why your example uses 1/8th resolution on the textures. That just nullifies the effect of multisampling.

packages/core/src/framebuffer/Framebuffer.js

ivanpopelyshev · 2020-01-13T06:08:49Z

I still don't understand why your example uses 1/8th resolution on the textures. That just nullifies the effect of multisampling.

To actually see that it works on a 4k screen.

ShukantPal · 2020-01-13T12:01:05Z

What do you see on a 4K screen? If you’re on a 4K screen, it will still just render as normal.

On my screen, it has jagged edges and doesn’t look like a circle. You’re supposed to use 1x resolution so the multisample makes the effective resolution 8. The whole point is to multisample each pixel 8 times, not multisample 8 pixels 8 times.

ivanpopelyshev · 2020-01-15T08:52:30Z

What do you see on a 4K screen? If you’re on a 4K screen, it will still just render as normal.

On my screen, it has jagged edges and doesn’t look like a circle. You’re supposed to use 1x resolution so the multisample makes the effective resolution 8. The whole point is to multisample each pixel 8 times, not multisample 8 pixels 8 times.

Pixels are half-red, that means AA is working :)

bigtimebuddy

Couple of small documentation things. Otherwise looks good. Thanks for the demo in the description @ivanpopelyshev.

bigtimebuddy · 2020-02-03T19:26:42Z

packages/constants/src/index.ts

@@ -409,3 +409,22 @@ export enum MASK_TYPES {
    STENCIL = 2,
    SPRITE = 3,
 }
+
+/**
+ * Constants for multi-sampling antialiasing


I would like really more documentation about this feature being experimental and that there's a significant performance cost paid for using it. Also, it's not super obvious, but a code example usage setting renderer.framebuffer.multisample.

linked it to Framebuffer#multisample. Don't want to ctrl+C everything.

packages/core/src/framebuffer/Framebuffer.ts

packages/core/src/framebuffer/GLFramebuffer.ts

codecov-io · 2020-02-03T19:31:00Z

Codecov Report

Merging #6296 into dev will increase coverage by 2.42%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              dev   #6296      +/-   ##
=========================================
+ Coverage   74.27%   76.7%   +2.42%     
=========================================
  Files          85      76       -9     
  Lines        4615    4142     -473     
=========================================
- Hits         3428    3177     -251     
+ Misses       1187     965     -222

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34f2401...4e536d2. Read the comment docs.

ShukantPal · 2020-02-03T23:10:29Z

@ivanpopelyshev Should we have a fallback to higher resolution if multisampling is not supported on the device? When forcing WebGL 1, the half-red pixels do not show up.

ShukantPal · 2020-02-03T23:20:13Z

@bigtimebuddy @GoodBoyDigital I approved this - but I did have concerns about requiring a blit operation after render. There is also no documentation regarding this (is there?)

…o feature-multisampling

ivanpopelyshev · 2020-02-04T09:20:26Z

@SukantPal good catch! I put it in PIXI.Framebuffer#multisample and linked from PIXI.MSAA_QUALITY

bigtimebuddy · 2020-02-04T21:45:38Z

Thanks everyone for getting this feature through!

ivanpopelyshev requested review from GoodBoyDigital, bigtimebuddy and ShukantPal December 25, 2019 16:59

ShukantPal requested changes Dec 25, 2019

View reviewed changes

ShukantPal self-requested a review December 25, 2019 17:27

ShukantPal requested changes Dec 25, 2019

View reviewed changes

ShukantPal self-requested a review December 25, 2019 17:37

ShukantPal requested changes Dec 25, 2019

View reviewed changes

GoodBoyDigital approved these changes Jan 7, 2020

View reviewed changes

bigtimebuddy added this to the v5.3.0 milestone Jan 10, 2020

ShukantPal self-requested a review January 12, 2020 19:18

ShukantPal reviewed Jan 12, 2020

View reviewed changes

packages/core/src/framebuffer/Framebuffer.js Outdated Show resolved Hide resolved

GoodBoyDigital mentioned this pull request Jan 16, 2020

Adds Renderbuffer support & multisampling for WebGL 2 #6290

Closed

6 tasks

rebased multisample framebuffer + test

dadab0c

ivanpopelyshev force-pushed the feature-multisampling branch from 28f4c7f to dadab0c Compare February 3, 2020 12:05

pixijs deleted a comment from adireddy Feb 3, 2020

Merge branch 'dev' into feature-multisampling

e617e8f

bigtimebuddy reviewed Feb 3, 2020

View reviewed changes

ShukantPal self-requested a review February 3, 2020 23:11

ShukantPal approved these changes Feb 3, 2020

View reviewed changes

ivanpopelyshev added 3 commits February 4, 2020 11:57

attending to @bigtimebuddy concerns

fdc8fe9

add a link instead of ctrl+c of a document

b2d0e84

Merge branch 'feature-multisampling' of github.com:pixijs/pixi.js int…

34f2401

…o feature-multisampling

Merge branch 'dev' into feature-multisampling

4e536d2

bigtimebuddy approved these changes Feb 4, 2020

View reviewed changes

bigtimebuddy merged commit 8d53426 into dev Feb 4, 2020

bigtimebuddy deleted the feature-multisampling branch February 4, 2020 21:45

Multisampling in FrameBufferSystem #6296

Multisampling in FrameBufferSystem #6296

Conversation

ivanpopelyshev commented Dec 25, 2019 • edited

🎁 Added

How it works

Why not RenderBuffer object

Future:

RenderTextureSystem

FilterSystem

What cant be in vanilla:

Another plugin

ShukantPal left a comment • edited

Choose a reason for hiding this comment

ShukantPal left a comment • edited

Choose a reason for hiding this comment

ShukantPal left a comment

Choose a reason for hiding this comment

ShukantPal commented Dec 25, 2019

ivanpopelyshev commented Dec 25, 2019 • edited

ShukantPal commented Dec 25, 2019

eXponenta commented Dec 25, 2019

ShukantPal commented Dec 25, 2019

eXponenta commented Dec 25, 2019

ShukantPal commented Dec 25, 2019

eXponenta commented Dec 25, 2019 • edited

ShukantPal commented Dec 25, 2019

ivanpopelyshev commented Dec 25, 2019

codecov-io commented Dec 26, 2019 • edited

Codecov Report

ivanpopelyshev commented Dec 26, 2019

ivanpopelyshev commented Dec 26, 2019

ShukantPal commented Dec 26, 2019 • edited

GoodBoyDigital left a comment

Choose a reason for hiding this comment

GoodBoyDigital Jan 7, 2020

Choose a reason for hiding this comment

ivanpopelyshev Jan 7, 2020

Choose a reason for hiding this comment

GoodBoyDigital Jan 15, 2020

Choose a reason for hiding this comment

ivanpopelyshev Jan 16, 2020 • edited

Choose a reason for hiding this comment

ivanpopelyshev Jan 16, 2020

Choose a reason for hiding this comment

ShukantPal commented Jan 12, 2020

ivanpopelyshev commented Jan 12, 2020

ShukantPal commented Jan 12, 2020

ShukantPal commented Jan 12, 2020

ivanpopelyshev commented Jan 13, 2020 • edited

ShukantPal commented Jan 13, 2020

ivanpopelyshev commented Jan 15, 2020

bigtimebuddy left a comment

Choose a reason for hiding this comment

bigtimebuddy Feb 3, 2020

Choose a reason for hiding this comment

ivanpopelyshev Feb 4, 2020

Choose a reason for hiding this comment

codecov-io commented Feb 3, 2020 • edited

Codecov Report

ShukantPal commented Feb 3, 2020

ShukantPal commented Feb 3, 2020

ivanpopelyshev commented Feb 4, 2020

bigtimebuddy commented Feb 4, 2020

ivanpopelyshev commented Dec 25, 2019 •

edited

ShukantPal left a comment •

edited

ShukantPal left a comment •

edited

ivanpopelyshev commented Dec 25, 2019 •

edited

eXponenta commented Dec 25, 2019 •

edited

codecov-io commented Dec 26, 2019 •

edited

ShukantPal commented Dec 26, 2019 •

edited

ivanpopelyshev Jan 16, 2020 •

edited

ivanpopelyshev commented Jan 13, 2020 •

edited

codecov-io commented Feb 3, 2020 •

edited