-
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?
-
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.
-
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 :D)
-
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 (https://github.com/SFML/SFML/blob/master/src/SFML/Graphics/RenderTarget.cpp#L356-L392).
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(...);
}
-
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).
-
So I ended up being rather bored this weekend, so I went ahead and implemented it. The code is available here (https://github.com/tomgalvin594/SFML) (specifically, this commit (https://github.com/tomgalvin594/SFML/commit/9805a17e139f9c96b9b6aa42dd02e89cdf536061)), and a small test program is available here (https://gist.github.com/tomgalvin594/8533203).
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 (https://github.com/tomgalvin594/SFML/blob/master/src/SFML/Graphics/BlendMode.cpp#L36-L39)). 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 (http://www.sfml-dev.org/documentation/2.1/WindowStyle_8hpp_source.php) does.
So, now that code actually, which is sometimes easier to judge than a proposal like above, any comments or suggestions?
-
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?
-
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 (https://github.com/tomgalvin594/SFML/blob/master/include/SFML/Graphics/BlendMode.hpp#L170-L190). 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)?
-
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.
-
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.
-
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 (https://gist.github.com/tomgalvin594/8533203) to work with these changes.
-
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 :D
-
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".
-
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...
-
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.
-
For reference, here are the two files in the master branch of thomas9459's fork:
BlendMode.hpp (https://github.com/tomgalvin594/SFML/blob/master/include/SFML/Graphics/BlendMode.hpp)
BlendMode.cpp (https://github.com/tomgalvin594/SFML/blob/master/src/SFML/Graphics/BlendMode.cpp)
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.
I see. An option would also be to provide the int conversions, but not document them in the public API, or clearly mark them as deprecated, to discourage users from regarding the conversion as a feature. In the worst case we postpone blend modes to SFML 3, but introducing it now is also an opportunity to get feedback about them, so that the design can potentially be improved in SFML 3.
I've searched quickly on GitHub (https://github.com/search?q=+blendnone+extension%3Acpp+extension%3Acxx&type=Code&ref=searchresults) to get a rough overview how open source projects use SFML's blend mode. As far as I see it, those that exploit the integral convertibility are either SFML forks or bindings. What concerns me more is the fact that very few people use the code
sf::BlendMode::BlendNone
even though it's not correct C++03. It would also be possible to be backward compatible with that, though...
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.
I agree. We can easily add needed functionality later, should it prove useful, but not remove it...
By the way, do you happen to know whether SFML's competitors provide a more advanced blend mode API? And do you consider the order of the 6 constructor parameters meaningful?
-
By the way, do you happen to know whether SFML's competitors provide a more advanced blend mode API?
I don't know. From a quick look, it seems that SDL 2 still only supports the 4 simple modes that SFML currently supports.
And do you consider the order of the 6 constructor parameters meaningful?
Yes. But there could be overloads / default values. There are at least two common simplifications that can be made: equation is almost always "Add", and src/dst factors are often the same for color and alpha.
-
Yes. But there could be overloads / default values. There are at least two common simplifications that can be made: equation is almost always "Add", and src/dst factors are often the same for color and alpha.
Maybe a
BlendMode(BlendFactor::Type source, BlendFactor::Type destination);
that could be extended by a BlendEquation::Type default parameter in the future?
I don't know. From a quick look, it seems that SDL 2 still only supports the 4 simple modes that SFML currently supports.
SDL (http://wiki.libsdl.org/SDL_BlendMode) supports the 4 basic modes. CrystalSpace (http://www.crystalspace3d.org/docs/online/manual/Renderer-Mixmodes.html) offers a finite set of blend modes, but many more than 4. Ogre (http://www.ogre3d.org/docs/api/1.9/OgreBlendMode_8h.html) seems to have both a simple and an expert API.
I'm more and more unsure whether it's wise to expose the whole OpenGL stuff... Another point is that even if we had the full customization, there would still be a need for a lot more predefined modes, or else everybody has to search the internet to find out how to compose common blend modes with OpenGL.
One option is also that BlendMode initially provides limited customizability: only some important constructors, no public member variables. But this would only make sense if we don't aim at providing the full set of operations, and it would still introduce complexity (maybe even more because one has to use unnamed parameters instead of member assignments).
Something we should also keep in mind is pre-multiplied alpha blending, that could come with a modification in sf::Image or sf::Texture.
This is becoming quite a bit complex for a single commit. It might be worthwhile to create a separate Git branch for this feature, so people could test it, give feedback about API simplicity and compatibility with existing code... And then we could still make a final decision (even if it would only be "we won't merge it to master, but if you really need this feature, use that branch"). What do you think?
-
Maybe a
BlendMode(BlendFactor::Type source, BlendFactor::Type destination);
that could be extended by a BlendEquation::Type default parameter in the future?
Yep.
It might be worthwhile to create a separate Git branch for this feature, so people could test it, give feedback about API simplicity and compatibility with existing code... And then we could still make a final decision (even if it would only be "we won't merge it to master, but if you really need this feature, use that branch"). What do you think?
This sounds wiser, yes ;)
-
Yes or you can do the blending in a fragment shader. :)
SFML provide render textures so you can do multipass rendering with shaders. ;)
It's very powerfull and it's what I currently do in my project.
You'll also be able to call the draw function only one time for each objects using the same texture.
-
Yes right but there are limitations with the hardware. ^^
And I really don't know in which cases the other blend modes can be usefull.
If someone have an example I'll be very interested to see it.
But normally there's an extension of GLSL which allow you to read the buffers values of your graphics card from a fragment shader but I really don't now how it works.
You haven't to do crappy work around and you can doing general blendmodes.
-
Lolilolight, stop hijacking the thread without understanding what this feature request even is about. There are clearly meaningful scenarios for blend modes, if you don't believe that, search the forum, there have been several threads about it.
It would be really nice if further posts in this thread could focus on the initial request. This concerns not only Lolilolight, but also people who want to respond to him. Thanks.
-
Ok I'll be looking for these threads, but if I've understand what you mean it's adding the posibility to do something like this with SFML ?
https://docs.unity3d.com/Documentation/Components/SL-Blend.html (https://docs.unity3d.com/Documentation/Components/SL-Blend.html)
-
Warning: Public history has been rewritten!
If you have cloned from my fork of SFML in order to try the new blending feature, you must run the following commands.
git reset --hard HEAD~4
git pull
Additionally, the commits are now located on the branch new_blending_api rather than on master. Sorry, about the history rewrite, but things will end up much cleaner this way. :D
So I have once again updated the code to reflect the discussion here. One important thing to note is that all of the blending changes have been moved to their own branch named new_blending_api. This means that any links to the code on Github should most likely be updated.
Inside the namespace sf, you can omit the sf:: prefixes.
If still applicable, where are these prefixes? If you were referring to the constant conversion functions (which have been moved to RenderTarget.cpp (https://github.com/tomgalvin594/SFML/blob/new_blending_api/src/SFML/Graphics/RenderTarget.cpp#L38-L73)), should the anonymous namespace be put in the sf namespace?
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.
Is something more than this (https://github.com/tomgalvin594/SFML/blob/new_blending_api/src/SFML/Graphics/RenderTarget.cpp#L397-L409) required for an adequate fallback?
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...
I can imagine a few scenarios where the reverse subtract could be useful, such as, for example, decreasing the amount of alpha in an render texture. If anything, min and max could be dropped as they serve little purpose from a graphical perspective and they also operate differently than the other equations (that is, they ignore the blending factors).
Maybe a
BlendMode(BlendFactor::Type source, BlendFactor::Type destination);
that could be extended by a BlendEquation::Type default parameter in the future?
Yep.
Shall I go ahead and implement this?
BlendMode(BlendFactor::Type source, BlendFactor::Type destination, BlendEquation::Type equation = BlendEquation::Add);
Finally, what is the preferred way of treating long lines in SFML? I didn't find any really long lines in other source files, so I just assumed a reasonable style with the long lines here (https://github.com/tomgalvin594/SFML/blob/new_blending_api/src/SFML/Graphics/BlendMode.cpp#L37-L44), here (https://github.com/tomgalvin594/SFML/blob/new_blending_api/src/SFML/Graphics/BlendMode.cpp#L61-L63), and here (https://github.com/tomgalvin594/SFML/blob/new_blending_api/src/SFML/Graphics/RenderTarget.cpp#L398-L400).
-
I can imagine a few scenarios where the reverse subtract could be useful, such as, for example, decreasing the amount of alpha in an render texture. If anything, min and max could be dropped as they serve little purpose from a graphical perspective and they also operate differently than the other equations (that is, they ignore the blending factors).
Could we go even further and drop the equation completely? It can still be added later if needed, and this would lower the new complexity. If I remember correctly, most of the requests were about new factors.
One remark about naming: wouldn't BlendMode::Factor and BlendMode::Equation be better, rather than BlendFactor::Type and BlendEquation::Type? In other words, why are these enums declared ouside the BlendMode class?
-
In other words, why are these enums declared ouside the BlendMode class?
I think the reason is expressivity when accessing the enumerators:
BlendFactor::SrcAlpha
orBlendMode::SrcAlpha
(in the latter, factor and equation have the same scope)
I'm a bit in a hurry now, I'll respond to your post later, Thomas.
-
This is not a problem in C++11 where you can write BlendMode::Factor::SrcAlpha if you want more expressivity.
-
Sorry, about the history rewrite, but things will end up much cleaner this way. :D
Rebasing is no problem as long as it's not in SFML master ;)
I think you could even make a further rebase and squash all the commits of the new branch to a single one (or do you want to preserve each refactoring iteration, Laurent?)
If still applicable, where are these prefixes?
Sorry, I thought they were inside sf namespace. In the anonymous namespace, it's fine like this. (Personally, I often put anonymous namespaces in the library namespace for this reason, but SFML doesn't do that).
Is something more than this required for an adequate fallback?
Actually that should solve it nicely, at least if the new arguments agree with the previous implementation. But I'm a bit worried when the user uses custom blend modes and one component is silently thrown away. Maybe there should be an error message in debug mode or an assert in this case?
Finally, what is the preferred way of treating long lines in SFML?
I don't know it either ;)
Maybe just check that the line doesn't exceed the usual line length (e.g. of Doxygen comments) too much.
Could we go even further and drop the equation completely? It can still be added later if needed, and this would lower the new complexity. If I remember correctly, most of the requests were about new factors.
I think subtractive blend mode was also requested. But I agree that we should not add all 5 equations.
This is not a problem in C++11 where you can write BlendMode::Factor::SrcAlpha if you want more expressivity.
True. For me it's fine either way. I guess the reason to put them into BlendMode is that you want the things to be grouped rather than scattered across the sf namespace?
-
or do you want to preserve each refactoring iteration, Laurent?
I don't want to preserve that, no :P
Maybe there should be an error message in debug mode or an assert in this case?
That sounds like a good idea, but SFML shouldn't spam the error output (such functions are often called every frame). Ideally there should be only one warning the first time the function is called.
I guess the reason to put them into BlendMode is that you want the things to be grouped rather than scattered across the sf namespace?
Yes.
-
Ok. Then I suggest Thomas cleans up the commits into one and applies the last suggestions, then I can create the new branch in SFML. We can still change everything then, but I think it's good if we have something practical at hand, otherwise the discussion will be endless ;)
A new branch will also help getting the attention of other users.
Reminder of the last changes:
- Move enums inside BlendMode class and rename them to Factor and Equation
- Leave only Add and Subtract equations
The error detection and other stuff can be implemented once the branch is up.
-
Once we have a clean branch I can also write the SFML.NET side of it. :)
-
There is now a new branch blendmodes (https://github.com/SFML/SFML/commits/blendmodes) based on thomas9459's commit.
I also adapted the test example to the newest API and created an updated Gist (https://gist.github.com/Bromeon/9447338).
-
We plan to merge the blendmodes branch to master for the SFML 2.2 release.
Has anybody worked with the new blend modes yet? Feel encouraged to use it and give feedback, now there's still the possibility to adapt smaller things :)
-
I've reviewed the branch, and pushed my modifications. Nothing exciting: I added an overloaded constructor as discussed before, and made a few minor modifications to the comments. I think it's ok for merging, unless other members of the team want to have a look before.
-
I checked the complete diff. Looks fine to me :).