An Implementation for Generic Blend Modes

[thomas9459]

The topic of adding more blend modes has come up several times before, and there is even an open issue in the tracker for it (https://github.com/SFML/SFML/issues/298). Unfortunately, there does not seem to be any progress for over a year on the topic. I recently could have used such a feature myself for creating special effects and believe that this feature would benefit many others as well. I would be more than willing to implement the code if Laurent approves a design.

Here is my design proposal:

• Change sf::BlendMode to a class with the following public members: sourceFactor, destFactor, and equation.
• Change the current enumerators of sf::BlendMode to static members of sf::BlendMode of type sf::BlendMode (similar to sf::Transform::Identitiy).
• Overload the == operator in sf::BlendMode to test if the modes are the same (useful for the cache when drawing).
• Rewrite RenderTarget::applyBlendMode(BlendMode mode) to simply set the OpenGL blend functions to sourceFactor and destFactor.

Issues:

• Because this proposal changes the type of sf::BlendMode, should it wait until SFML 3.0 to be implemented?
• Should the OpenGL constants (GL_ZERO, GL_ONE, GL_SRC_ALPHA, etc.) be used, or should similar constants be defined in the sf namespace?
• How, if at all, should the separate blending function for alpha be handled?

These changes should (if I understand correctly) keep the following code valid,

void Laser::draw(sf::RenderTarget &target, sf::RenderStates states) const {
    states.blendMode = sf::BlendMode::BlendAdd;
    target.draw(m_sprite, states);
}

while allowing the following as well.

void Laser::draw(sf::RenderTarget &target, sf::RenderStates states) const {
    states.blendMode = sf::BlendMode::BlendAdd;
    // The texture uses premultiplied alpha
    states.blendMode.srcFactor = GL_ONE; // or sf::BlendFactor::One
    target.draw(m_sprite, states);
}

What do you guys think? Any suggestions and/or improvements?

[eXpl0it3r]

Since I don't know how such an implementation would work OpenGL wise, I can't say anything on the design, I just wanted to point out:

• Because this proposal changes the type of sf::BlendMode, should it wait until SFML 3.0 to be implemented?
• Should the OpenGL constants (GL_ZERO, GL_ONE, GL_SRC_ALPHA, etc.) be used, or should similar constants be defined in the sf namespace?

If this is an API breaking change (i.e. you can't use old SFML 2.0 code with the new version), it could not be released under the 2.x versioning, so yes it would have to wait till SFML 3.x.
SFML should not be using constants from other libraries as public API, but instead declare them within in SFML.

[Foaly]

I'd love to see this implemented! I've always wanted to use the subtractive blend mode in SFML.
As far as I can tell the design looks good (although I am not an expert in OpenGL), but we'll wait what Laurent says.
Like exploit3r said if the change breakes backwards compatibility it would have to wait till SFML 3, but the way I understand your proposal it doesn't. (which would be even better )

[Nexus]

Hm, I'm not sure if it's enough for all blend modes to have only source and dest factor with glBlendFunc(); sometimes you probably need glBlendFuncSeparate(). See also RenderTarget.cpp.

I also thought about this a while ago, because we definitely need more blend modes in SFML (for example screen). To maintain compatibility to a wide extent, you can do the following (keep in mind that enum distributes the enumerators to the surrounding scope):

namespace sf
{
    class BlendMode { ... };

    const BlendMode BlendAdd(...);
    const BlendMode BlendMultiply(...);
    const BlendMode BlendAlpha(...);
    const BlendMode BlendNone(...);
}

[thomas9459]

To maintain compatibility to a wide extent, you can do the following (keep in mind that enum distributes the enumerators to the surrounding scope):

I completely forgot about that; my code is full of sf::BlendMode::Blend*. 
A quick search on GitHub leads me to believe that no one uses the verbose method, so Nexus's suggestion of using const BlendMode objects seems to be the best solution to provide compatibility.

Concerning glBlendFuncSeparate(), would it be better to simply add srcFactorAlpha, dstFactorAlpha, and equationAlpha? Also, how would you handle the case where it is not supported? Unless SFML still supports versions before OpenGL 1.4 / OpenGL ES 2.0, the test for the extension doesn't seem necessary (unless I am missing something).

[thomas9459]

So I ended up being rather bored this weekend, so I went ahead and implemented it. The code is available here (specifically, this commit), and a small test program is available here.

This implementation strays from the original design proposed above, most notable using setters and the addition of the apply() function. One of the issues that I found, however, is that the lengths of many of the identifiers are very long. This becomes especially relevant when using the full constructor (see these lines). This is mainly due to sf::BlendFactor and sf::BlendEquation not actually being enumerations, but rather namespaces that contain unnamed enum, in order to enforce scoping, like sf::Style does.

So, now that code actually, which is sometimes easier to judge than a proposal like above, any comments or suggestions?

[Nexus]

Thanks a lot for the great effort. From what I've seen so far, the code looks very good and is well documented. I have not tested it yet, so this will be mainly about the API.

The functions equationToGlConstant() and factorToGlConstant() should be private (or even better: global in the .cpp file), apply() should cover the use cases in the SFML API.

I'm not sure if we need all the encapsulation... A simple struct with public members is easier to handle. I doubt that we need to restrict read access or do sophisticated work in the setter methods, so that the encapsulation would pay off.

The members of the class should be of the corresponding enum types for type safety, and not Uint32.

Concerning the constructors, the user can have using namespace or namespace aliases to get rid of the long scope names. A 6-parameter constructor will be enough initially, it's still possible to add more constructors later. Providing constructors that take default parameters has two problems: Default values may not be obvious, and you can call the constructor with a number of parameters that is not meaningful (e.g. 4, specifying alpha src factor but not alpha dst factor). Named constructor idiom (i.e. static factory functions) are also a possibility in the long term, to make certain parameter combinations more expressive.

About the default constructor, it would mainly add a little convenience. SFML itself always initializes blend modes explicitly, and up to now every user had to do it (because enum variables are uninitialized otherwise). However, a default constructor is meaningful from the perspective that SFML always uses alpha blending unless specified otherwise.

There are some formal mistakes and typos:

• "Dstination" instead of "destination" (lowercase)
• "The sf::BendEquation to be converted"
• sf::BlendMode blendMode(); -- omit the parentheses for default construction
  
• You sometimes use tabs, replace them with 4 spaces
• The CMakeLists.txt file seems to contain whitespace changes... are those CR/LF? Did you make it more or less consistent? :)

What do you (and everybody else) think?

[thomas9459]

The functions equationToGlConstant() and factorToGlConstant() should be private (or even better: global in the .cpp file), apply() should cover the use cases in the SFML API.

They are already private. I guess they are actually better suited for file scope, though.

I'm not sure if we need all the encapsulation... A simple struct with public members is easier to handle. I doubt that we need to restrict read access or do sophisticated work in the setter methods, so that the encapsulation would pay off.

I always thought that if you have member functions, then you should make it into a class, although it really does seem like overkill in this case.

The members of the class should be of the corresponding enum types for type safety, and not Uint32.

I was using the same trick that sf::Style used to enforce scoping and avoid polluting the namespace. There appears to be no way enforce scoping and type safety in C+03 (if I'm wrong, please correct me). I can hardly wait until SFML moves to C++11 so we can get enum class, among other things.

Concerning the constructors, the user can have using namespace or namespace aliases to get rid of the long scope names. A 6-parameter constructor will be enough initially, it's still possible to add more constructors later. Providing constructors that take default parameters has two problems: Default values may not be obvious, and you can call the constructor with a number of parameters that is not meaningful (e.g. 4, specifying alpha src factor but not alpha dst factor). Named constructor idiom (i.e. static factory functions) are also a possibility in the long term, to make certain parameter combinations more expressive.

About the default constructor, it would mainly add a little convenience. SFML itself always initializes blend modes explicitly, and up to now every user had to do it (because enum variables are uninitialized otherwise). However, a default constructor is meaningful from the perspective that SFML always uses alpha blending unless specified otherwise.

I'll work on doing that.

• You sometimes use tabs, replace them with 4 spaces

Sorry, my editor's default is tabs (as that is what I use for my projects), and sometimes I forget to change it.  :-[

• The CMakeLists.txt file seems to contain whitespace changes... are those CR/LF? Did you make it more or less consistent? :)

I don't remember making those changes on purpose, but they appear to make it more consistent (they used to end in LF, now they end in CRLF). It appears that this is gedit's default behavior to change line endings to be consistent.

Well, I just realized that I definitely should have used a feature branch for this rather than committing straight to main. Any advice on how to fix this from those well versed in git (I already pushed to my GitHub SFML fork)?

[wintertime]

I think you can just checkout the master of your fork and then, assuming your fork is origin:
git checkout -b newbranchname
git push --set-upstream newbranchname
Then use that branch when you do the pullrequest on github.

If you like to you could also --after you secured your commits in that new branch (otherwise you can loose your commits and can try restoring your commits from the reflog)-- move your master back by checking it out, using reset --hard and push -f it to github, but thats easier to screw up so you better read the git help first.

[Nexus]

I guess they are actually better suited for file scope, though.

Yes, you can create an anonymous namespace at the beginning of the .cpp file. If you're not sure about it, maybe have a look at how other files in SFML did it.

I was using the same trick that sf::Style used to enforce scoping and avoid polluting the namespace.

I mean that you should save the member variables as enum types, not integers. This has nothing to do with scoping, you can still leave the enums in a namespace. sf::Style variables need to be integers because there are multiple flags which can be combined with operator|; this is not the case here.

Well, I just realized that I definitely should have used a feature branch for this rather than committing straight to main. Any advice on how to fix this from those well versed in git (I already pushed to my GitHub SFML fork)?

There's no need to create a branch for this; merging the pull request to SFML will involve a separate branch (your master) besides master anyway. I could also create a branch locally to customize the history, but I don't think it's necessary.

[thomas9459]

I finally got some time to work on this, and I just pushed the code to Github. Does that look more like a proper SFML API, Nexus?

The only thing that concerns me is the type sf::BlendFactor::BlendFactor and sf::BlendEquation::BlendEquation. In your post you make is seam like they can somehow be shortened to f::BlendFactor and sf::BlendEquation:: respectively, but I don't know how to do that without remove the namespaces.

Also, I found quite a few tabs in other files, so I went ahead and changed those to spaces.

EDIT: Updated the test program to work with these changes.

[Nexus]

The main things look good so far. Some points:

• operator== and operator!= don't need to be friend anymore.
• The enum is declared correctly with its scope. But I'd name the types themselves rather BlendEquation::Type and BlendFunction::Type rather than BlendEquation::BlendEquation and BlendFunction::BlendFunction.
• The constructor signature is quite long, you could introduce a line break after 3 parameters (both .hpp and .cpp files).
• Inside the namespace sf, you can omit the sf:: prefixes.
• The .cpp file must include the OpenGL.hpp header.
• The member variables can be aligned to the same column (at their declaration in the header).
• There are some unneded empty lines between member function definitions in the .cpp file.

There are also some implications to consider (not specifically for you):

• SFML itself needs to be adapted in some places, because the switch won't work anymore with the class.
• The new API drops support for graphics cards without glBlendFuncSeparate(). If necessary, I can introduce code that falls back to glBlendFunc() for the existing blend modes.

Also, I found quite a few tabs in other files, so I went ahead and changed those to spaces.

Thanks. If you do a pull request, it would be good to keep that commit separate, so we don't mix unrelated issues.

Furthermore, Laurent hasn't lost a single word about this whole new API... In the end he doesn't like it and we can throw everything away

[Laurent]

I think it's mostly ok.

The enum is declared correctly with its scope. What I'm wondering is, if the type itself should be named BlendEquation::Type or BlendFunction::Type rather than BlendEquation::BlendEquation and BlendFunction::BlendFunction?

Yep, that would look better.

Moreover, apply() should not be there. Let's keep all implementation details in RenderTarget.cpp and the BlendMode structure clean. People who use direct OpenGL calls will not use sf::BlendMode anyway, so I don't see any reason to leave a public apply() function, other than possibly breaking the implementation / state cache / whatever.

Don't forget to align parenthesis in constructor initializer list.

Two points are annoying:

1. It does break API compatibility since sf::BlendMode is now a structure and no longer an enum. In "regular" use cases it won't make any difference, but we could imagine scenarios like serialization (direct cast to int) that would no longer work with the new implementation. Is this acceptable though?

2. We must have a fallback for systems without glBlendFuncSeparate.

On a more personal note, I think that this new API brings complexity. Instead of 4 simple blend modes, we now have factors and equations, with a lot more possibilities. Sure for those who are used to the current API, things can remain as simple as before. But for newcomers, that will have a look at the documentation, won't they be lost with all these parameters?

By the way, are all the new constants really necessary? I didn't look deeply at them, so make sure they are all really relevant, and not just added "because now we can".

[Nexus]

Thanks for taking the time to look at it.

1. It does break API compatibility since sf::BlendMode is now a structure and no longer an enum. In "regular" use cases it won't make any difference, but we could imagine scenarios like serialization (direct cast to int) that would no longer work with the new implementation. Is this acceptable though?

Yes, also the mentioned switch statement is such a case. My thought was that the user would mostly not dispatch on different blend modes himself, but rather pass them to SFML and store them in variables.

If you deem it crucial to allow switch or serialization for backwards compatibility, it would be possible to introduce implicit conversions to int and explicit ones from int for the existing blend modes. But I don't like to introduce such a hack unless absolutely necessary.

2. We must have a fallback for systems without glBlendFuncSeparate.

Would it suffice if it were supported for the existing blend modes? Then we could adapt the SFML rendering such that if the GLEW extension is not supported, it will recognize those cases and use the old glBlendFunc().

But in general, it's not possible to map the new API with 6 parameters to the simplified one.

On a more personal note, I think that this new API brings complexity. Instead of 4 simple blend modes, we now have factors and equations, with a lot more possibilities. Sure for those who are used to the current API, things can remain as simple as before. But for newcomers, that will have a look at the documentation, won't they be lost with all these parameters?

It does indeed add complexity, but if you want to make blending more flexible, there's hardly a way around it. Going only half-way (support glBlendFunc() but not glBlendFuncSeparate() and glBlendEquation()) is a bad idea, because it still doesn't satisfy a lot of use cases. If we only provided predefined blend modes, there would have to be a lot to cover many cases, and then users would again be lost because they don't know which of them are the most common ones.

One thing we should certainly do is emphasize in the tutorials and documentation that the predefined blend mode constants will be enough for most cases, and the rest only exists for customization.

The alternative would be a separate API (conceptually similar to the separation of high-level and low-level rendering API in SFML), but I think it would add even more complexity.

By the way, are all the new constants really necessary? I didn't look deeply at them, so make sure they are all really relevant, and not just added "because now we can".

I'll investigate that. The question is whether you only want to provide the minimum number of enumerators that is necessary to implement the current blend modes, or at least a few more common ones? Where should we draw the border? It will be more or less arbitrary...

[Laurent]

If you deem it crucial to allow switch or serialization for backwards compatibility, it would be possible to introduce implicit conversions to int and explicit ones from int for the existing blend modes. But I don't like to introduce such a hack unless absolutely necessary.

I don't want such hacks neither. But one of my goal for SFML 2 was to provide backward compatibility. Broken cases are rare, but they can exist, and if a game compiles / works with SFML 2.1 but not SFML 2.2 then it's clearly a problem.

Would it suffice if it were supported for the existing blend modes? Then we could adapt the SFML rendering such that if the GLEW extension is not supported, it will recognize those cases and use the old glBlendFunc().

I guess that's the best we can do

The question is whether you only want to provide the minimum number of enumerators that is necessary to implement the current blend modes, or at least a few more common ones? Where should we draw the border? It will be more or less arbitrary...

I'm fine with factors. What I'm more worried about are the equations: are min/max and inverse subtract really useful? I would like to see concrete use cases for them.