Support for separate blend states per render target on OpenGL

[minimalism]

BlendState.IndependentBlendEnable was not implemented on OpenGL platforms.
This fix unfortunately does not account for _lastBlendState only storing the previous state of index 0.

[minimalism]

I'm worried about a potential issue that could arise:
The functions GL.BlendEquationSeparate and GL.BlendFuncSeparate read the blend equation and functions from _targetBlendState[0] and sets this on all four buffers.

The PlatformApplyState function in BlendState.OpenGL.cs has a comparison with _lastBlendState that aims to reduce unnecessary OpenGL calls, which needs to take into account whether the previous blend state had Independent Blending enabled when it was applied.
If not, it could incorrectly believe the new blend state is identical to the previous even though it isn't.

I'm not entirely sure how best to address this. I still feel this is an improvement from the previous version, which could not apply different blend states to separate render targets.

[Jjagg]

This is great @minimalism! :)

I think the caching problem is solved if you set the blendstate of all targets when _independentBlendEnable is not set. Does that seem right?

[minimalism]

You mean set _targetBlendState 0 through 3 in the BlendState class? That could be a solution though I'm not sure how this would interact with the DirectX BlendState class. And it would then maybe be kind to generate an exception if the user tries to access the this[int index] member while _independentBlendEnable is false, to avoid the situation where someone sets up their blend state in this order:

blendState[0].AlphaBlendFunction = BlendFunction.ReverseSubtract; 
blendState[1].AlphaBlendFunction = BlendFunction.Add;
blendState.IndependentBlendEnable = true;

Here I would expect to still have the ReverseSubtract AlphaBlendFunction on target blend state 0, but would actually have Add on all four.

I think setting device._lastBlendState._independentBlendEnable in PlatformApplyState and taking its value into consideration in that same function would avoid the ordering issue above, but would also be slightly more complex. Hopefully I'll have some time to check this solution out this week.

[Jjagg]

You mean set _targetBlendState 0 through 3 in the BlendState class

Yes.

I'm not sure how this would interact with the DirectX BlendState class

There is no interaction. The caching only happens for the OpenGL backend and the code you're changing only exists in OpenGL builds.

Here I would expect to still have the ReverseSubtract AlphaBlendFunction on target blend state 0, but would actually have Add on all four.

The blend state is only applied (and the last blend state stored) when PlatformApplyState is called. That doesn't happen in your example. When it is called after your code, the cached value for all targets would still be whatever blendState[0] was before any of your code (assuming IndependentBlendEnable was false and it was applied), causing no redundant state setting.

[minimalism]

True, I didn't think about waiting until the PlatformApplyState call :) I think that is a good solution.

[minimalism]

There were also some other bugs (obvious in hindsight) in my PR that I fixed

[Jjagg]

@minimalism It seems the Blend*Separatei functions are only core since GL 4.0.
glBlendFuncSeparate: https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/glBlendFuncSeparate.xhtml
glBlendEquationSeparate: https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/glBlendEquationSeparate.xhtml

We'd need to add a flag to GraphicsCapabilities to indicate if the GraphicsDevice supports setting separate blend states to targets. I haven't checked if there are any extensions we can use yet.

[Jjagg]

There were also some other bugs (obvious in hindsight) in my PR that I fixed

Awesome :) I'm curious what those are.

[minimalism]

The GL.ColorMask and GL.BlendFuncSeparate calls were both inside the independentBlendState == true branch and outside the conditional.
GL.ColorMask is now only outside the conditional (because its behavior does not vary based on the value of independentBlendState, even though glColorMaski does exist it's not within my scope here) while GL.BlendFuncSeparate is inside but has the correct behavior in both cases.

[Jjagg]

@minimalism Do you know how to tackle the GraphicsCapabilities part of this?

[minimalism]

Looking at the sites you linked, for DesktopGL I would just compare glMajorVersion <= 4. For the other platforms, I don't know

[Jjagg]

There's also an extension for lower gl versions: https://www.khronos.org/registry/OpenGL/extensions/ARB/ARB_draw_buffers_blend.txt

For DirectX you can set the flag to true since separate blending is supported in all feature levels we support.

For mobile and web set it to false, I don't think GL ES supports the indexed variants for blending.

[minimalism]

Not sure about that failing Mac test. Does the test device have independent blend state support? The other platforms certainly appear to. I don't have the appropriate hardware to test this myself.

[Jjagg]

Mac GL drivers are notoriously bad. We can just ignore the test on the Mac build bot by changing this line

#if !XNA

to

// The Mac build bot GL driver does not support independent blend states
#if !XNA && !MONOMAC

This is almost good to go, after the above fix can you revert the submodule change from 7b044f0?
You can do that by cd'ing into the ThirdParty/Dependencies repo and checking out bf99ec34cc1f83787d19806df186fd331e55310d, then adding and committing the change from the main repo.
Alternatively you can revert your commit.

[minimalism]

Well let's see if that did it, I don't know how the submodule commit slipped in to begin with.

[Jjagg]

Oops, I think the Mac build bot runs the Linux test project for some reason. I edited it, hope it passes now. @minimalism You'd best do a pull before you make any local changes now.

[Jjagg]

Looking good! Would be good if someone can give this a test. I'll hold off for a bit, but if no one has the time I'll merge this anyway because the changes to the working codepath are minor and are well tested.

Though it would be nice to get a unit test in for this. @minimalism If you're not up for that I'll open an issue for it.

[minimalism]

You're probably right.. I think it would be good if someone with an older device could test the ARB_draw_buffers_blend extensions because that code has literally never been run. Unit tests wouldn't hurt either, would be nice to get this wrapped up, but I can't today.

[minimalism]

Oops, made some commits on my own branch that weren't intended for this PR. Reverted, and will put my future questionable commits on a separate branch

[tomspilman]

@Jjagg - Might want to do a "Squash and Merge" on this one once it is ready.

[minimalism]

Yeah word. I guess we're waiting for some test coverage to come along

[Jjagg]

I'm gonna merge this one. Thanks @minimalism, this is a great PR. Sorry for the long wait!