SFML community forums

General => General discussions => Topic started by: Nexus on May 13, 2015, 11:13:49 am

Title: Shader uniform API -- part II
Post by: Nexus on May 13, 2015, 11:13:49 am
In part I (http://en.sfml-dev.org/forums/index.php?topic=14802.0), we discussed possible APIs to provide more functions to set GLSL uniform variables from SFML, which is currently done by several sf::Shader::setParameter() overloads. I continued working on that issue and came up with two possible implementations. Both are available in the feature/shader_uniforms (https://github.com/SFML/SFML/tree/feature/shader_uniforms) branch on GitHub. It's a proof-of-concept rather than a finished implementation.

The first option is to simply provide more overloads of setParameter().
Commit 8af7af4 (https://github.com/SFML/SFML/commit/8af7af4b952dad503411cc3d49353e6e58a15ebc)
(click to show/hide)

The second option would be to provide named functions.
Commit 7d1d4da (https://github.com/SFML/SFML/commit/7d1d4da363b57ac82458cb8c1f869d667053c37b)
(click to show/hide)


Advantages of expressive API (setFloat etc.)

1. It avoids subtle type errors arising from implicit conversions (e.g., you can pass an int if you actually mean unsigned int, calling the wrong OpenGL function).

2. The call is expressive about the GLSL type being set. This is the strongest argument, as it makes clear how different C++ types like sf::Transform, sf::Color, sf::Vector2f are mapped to their GLSL counterparts mat4, vec4, vec2. Furthermore, it allows multiple mappings, for example sf::Transform can be passed as either mat3 or mat4.

3. We do not need to introduce new types like sf::Shader::Matrix3, as the function name would contain the name. However, this applies only to single values, not arrays. For example, it's not possible to pass an array of 4-dimensional vectors (unless types are introduced, or we assume a contiguous XYZWXYZW... memory layout).


Advantages of existing API (setParameter overloads)

1. sf::Shader::setParameter() need not be deprecated. In my opinion however, "setParameter" is not an ideal name anyway, "setUniform" would be much clearer. Everybody using shaders knows GLSL and in that context, "uniform" is a very clear and descriptive term.

2. Overloads of the same function allow for generic programming. And this is not an academic argument, it would allow a high-level representation of GLSL structs on C++ side:
GlslStruct st("light");
st.member("position", x, y);
st.member("intensity", 30.f);
st.apply(shader);
This is trivial to implement with setParameter overloads (I have already done it); but very tedious with setFloat, setVec2 etc. You essentially need to duplicate all the combinations.

It should be noted that setParameter overloads do not really simplify a lot on developer side. In SFML, we can't use templates, not even for GLSL arrays, because every C++ type has to be dispatched to the right OpenGL function that sets the uniform variable. However, as noted above, the SFML user can benefit from templates.
Title: Re: Shader uniform API -- part II
Post by: Nexus on May 13, 2015, 11:29:52 am
There are further points to consider when we provide more types.

1. Will SFML ever allow setting other GLSL variables, namely vertex attributes? This would affect the naming.


2. For the second API, there are different naming options: setX, setUniformX, uniformX. I think it would not be bad if "uniform" is contained within the function name, for expressivity. This would also solve point 1.


3. Which types do we provide? There are tons of combinations. As usual, I'd suggest starting with the most useful ones, we can still extend the API when necessary. We should anticipate the addition of new types in the design though.


4. The documentation needs serious reconsideration. It's pointless to duplicate a verbose description for every possible setter. Even more if it's done the current way:
////////////////////////////////////////////////////////////
/// \brief Change a 3-components vector parameter of the shader
///
/// \a name is the name of the variable to change in the shader.
/// The corresponding parameter in the shader must be a 3x1 vector
/// (vec3 GLSL type).
///
/// Example:
/// \code
/// uniform vec3 myparam; // this is the variable in the shader
/// \endcode
/// \code
/// shader.setParameter("myparam", 5.2f, 6.0f, -8.1f);
/// \endcode
///
/// \param name Name of the parameter in the shader
/// \param x    First component of the value to assign
/// \param y    Second component of the value to assign
/// \param z    Third component of the value to assign
///
////////////////////////////////////////////////////////////
void setVec3(const std::string& name, float x, float y, float z);
The parameter description is not very useful: "first component of the value to assign" is very generic; nothing that couldn't be inferred from the signature. The method description would not be necessary with expressive function names, either. A code example for each of dozens of methods is also not necessary; I'd rather put a few ones directly to the sf::Shader description.

To be honest, I think a Doxygen \brief documentation per method + some extended examples in sf::Shader would be totally enough. Once a user has set an uniform, he sees the obvious pattern that those methods all follow.


5. As pointed out by binary1248 in the other thread, the implementation can be optimized, if SFML does not call OpenGL functions immediately, but store the uniforms and process everything at once before binding the shader. This is very well possible with my current implementation: I used functors to reduce the code duplication; they can be stored and invoked later (C++11: std::function, C++03: type-erased class with virtual functions).
Title: Re: Shader uniform API -- part II
Post by: Laurent on May 13, 2015, 12:14:32 pm
Quote
To be honest, I think a Doxygen \brief documentation per method + some extended examples in sf::Shader would be totally enough. Once a user has set an uniform, he sees the obvious pattern that those methods all follow.
The thing is, parameters appear in the documentation, so they have to be documented, even if the comment doesn't add any relevant information. The generated doc would be valid without it, but we'd end up with warnings during the generation process, and an inconsistent doc (why are all other parameters of the API documented and not those?).

Other than that, option 2 looks good, with expressive function names ("uniform" included).
Title: Re: Shader uniform API -- part II
Post by: Nexus on May 13, 2015, 03:29:35 pm
The Doxygen warning is only triggered when some but not all parameters are documented -- not if none is documented. We can write down the \params, but if they're not helpful, we'd better omit them directly. Otherwise it's that enforced Javadoc mentality :P
/**
 * Return string representation
 *
 * Converts the object into a string
 *
 * @return String representation of the object
 */

String toString()
Yeah, 8 LOC 8)

In Thor, I usually don't write a detailed documentation if it makes no sense. See here (http://www.bromeon.ch/libraries/thor/v2.0/doc/classthor_1_1_arrow.html) for example. And people seem to be happy with it ;)

Edit: I think it could be useful, but only because of the first parameter (the uniform variable's name). What we might do is combining the remaining parameters:
////////////////////////////////////////////////////////////
/// \brief Specify value for vec3 uniform
///
/// \param name  Name of the uniform variable in GLSL
/// \param x,y,z Values of the vec3 components
///
////////////////////////////////////////////////////////////




About naming:
Shader::setUniformFloat or Shader::uniformFloat?
Shader::setUniformMatrix3Array or Shader::uniformMatrix3Array?
Title: Re: Shader uniform API -- part II
Post by: binary1248 on May 15, 2015, 04:37:41 am
I prefer having a single uniform (no pun intended :P) name for all methods than explicit names that serve no other purpose than to remind people of the signature of the method they are calling. In C, there is no choice so they have to do it like that there, but if you ask me, in a C++ API, if you end up appending type information to the function name you are already doing something wrong.

It's not about being expressive, it's about catering to people who simply have no idea what they are doing or just lack the discipline/good habits to make sure that such mistakes don't happen too often. If we already imply that one has to have GLSL knowledge, I don't think it is reasonable to expect people who are put off by mistakes which are the result of bad programming practice to be using sf::Shader. sf::Shader is something advanced users will end up using, and we should be able to expect them to "make things work" on their own, even with the not-so-expressive API.

Furthermore, it has been planned for what... 2 years now, that SFML will transition to C++11 and beyond at some point. Appending type information to these methods is a step in the complete opposite direction. It will completely eliminate the benefit of type inference, if you ask me the single most valued feature of C++11, which in my experience speeds up prototyping/development by an order of magnitude. Think about it, you have a data structure whose design is not yet finalized. It has to set some/many shader uniforms before it is drawn. Initially you didn't expect to need floating point precision for some of its variables but it turns out that using integer values leads to subtle rendering errors. You go ahead and change the variable type in the structure definition and adjust the GLSL, and thanks to C++11 everything else will "just work", but wait... those 30 setIvec3() calls... damn it.

As stated, I prefer having a single name for all methods, but that name doesn't have to stay setParameter. Like Nexus already said, setParameter doesn't really mean much, especially to experienced OpenGL programmers who would have understood/expected setUniform for a function that... you know... sets uniforms. Furthermore, in the distant future, SFML might also support setting "other kinds of parameters" inside GLSL shaders which need not necessarily be uniforms, so making it clear now that uniforms are being set is important.

Advantages of expressive API (setFloat etc.)

1. It avoids subtle type errors arising from implicit conversions (e.g., you can pass an int if you actually mean unsigned int, calling the wrong OpenGL function).
Like I said, this only works out on paper, and can only happen if the call is not ambiguous to begin with (single conversion direction, e.g. calling with a bool and int won't work). In the hands of the users it really won't matter. Not to mention, in later versions of OpenGL, support for unsigned int was added as well, so if we wanted, we could even consider adding an unsigned int overload that checks whether the entry point is available and warns and falls back to another variant if it isn't.

Also, if all we care about is "type safety" between user code and their GLSL, we can always use glGetActiveUniform() to check if the passed type and the actual GLSL type even match and warn if they don't.

2. The call is expressive about the GLSL type being set. This is the strongest argument, as it makes clear how different C++ types like sf::Transform, sf::Color, sf::Vector2f are mapped to their GLSL counterparts mat4, vec4, vec2. Furthermore, it allows multiple mappings, for example sf::Transform can be passed as either mat3 or mat4.
I don't think this is a valid argument. It is kind of obvious that sf::Vector2f will map to vec2 and anybody who claims they know a bit of GLSL will know that colours are almost always represented as vec4. I agree that sf::Transform is ambiguous, simply due to the fact that people have no idea what it is and what its semantics are. Even after looking through the SFML source, people won't easily know that it is a 4x4 matrix, but in reality is only used as a 3x3 matrix but has to be 4x4 because SFML uses glLoadMatrixf(). To solve this problem, I suggest deprecating the sf::Transform overload and replacing it with sf::Matrix3 and sf::Matrix4 overloads which can be returned from sf::Transform when needed.

3. We do not need to introduce new types like sf::Shader::Matrix3, as the function name would contain the name. However, this applies only to single values, not arrays. For example, it's not possible to pass an array of 4-dimensional vectors (unless types are introduced, or we assume a contiguous XYZWXYZW... memory layout).
I don't see why we couldn't just introduce sf::Vector4. sf::Vector3 is already barely used (only by audio and sensor), yet it is a "first class citizen" within the API. This would also be nice to have with sf::Matrix3 and sf::Matrix4, we would only define the absolute basics of matrix manipulation and leave everything else up to the user to do.

Also, I don't see why we need to differentiate the naming of the methods that set arrays (setParameterArray vs setParameter). If the GLSL parameter type is an array, then that entire array is the parameter. We aren't setting an array of parameters, just a single one, which is why I find having the same name as the other non-array methods makes more sense. Instead of passing a single pointer and a size, in the spirit of C++11, we could already go ahead and take first and last parameters instead:
setParameter(const std::string& name, const Vector2f* vectorArray, std::size_t length)
vs
setParameter(const std::string& name, const Vector2f* first, const Vector2f* last)
Maybe at some point, we will make it even more general, using templates which allow for any STL container to be passed. OpenGL never said every array will always have a static size (https://www.opengl.org/wiki/Interface_Block_%28GLSL%29#Shader_storage_blocks) ;). In any case, modifying a sub-range of the GLSL array is already allowed, so passing containers that you know will never be larger than the array can already be implemented as is.

1. Will SFML ever allow setting other GLSL variables, namely vertex attributes? This would affect the naming.
You'll have to restate this question. Technically you don't "set" vertex attributes you bind them. This is because they vary (hence the old, somewhat related, qualifier varying) within a single execution of the shader, and the only way you can influence their value is through the data source to which the attribute is bound. If we went ahead and implemented a way to bind attributes, the method should be called something like bindAttribute() which I find the most natural.

3. Which types do we provide? There are tons of combinations. As usual, I'd suggest starting with the most useful ones, we can still extend the API when necessary. We should anticipate the addition of new types in the design though.
I wouldn't say tons. Looking at the 4.4 reference card (https://www.khronos.org/files/opengl44-quick-reference-card.pdf) (page #8), we can see that there aren't that many transparent GLSL types. The "catch up game" that GPUs had to play over the last years is over, at this point in time, newer cards have more or less native support for the same data types as are available in C++. If we leave out double precision floats (for now) and all the non-square matrices (which if you ask me are only used in very very specific scenarios anyway), there aren't that many to add.
Title: Re: Shader uniform API -- part II
Post by: Nexus on May 15, 2015, 02:35:18 pm
Think about it, you have a data structure whose design is not yet finalized. It has to set some/many shader uniforms before it is drawn. Initially you didn't expect to need floating point precision for some of its variables but it turns out that using integer values leads to subtle rendering errors. You go ahead and change the variable type in the structure definition and adjust the GLSL, and thanks to C++11 everything else will "just work", but wait... those 30 setIvec3() calls... damn it.
Good point. Type inference is really helpful, and some situations like the GLSL struct in C++ (see my previous post) are only enabled with it.

However, your example is also possible in the other direction: somebody changes a type somewhere, without considering that through several layers of compile-time polymorphism, a different setUniform() overload is called, leading to a runtime error. You don't even need templates for that. What I have in mind:
class GameObject()
{
     int getHealth() const;
};

// somewhere else
GameObject& obj = ...;
shader.setUniform("health", obj.getHealth());
Now, GameObject is refactored, its method returns now float. BAM, the GLSL code is not prepared. With setUniformInt(), it would have continued to work -- and you get a warning at compile time notifying you of the conversion. Much better than a runtime error aka "an internal OpenGL call failed": before you get the idea to even read the cerr output in the console, you wonder for a decent amount of time why your shader suddenly doesn't work, as you haven't even touched its code ;)

Not to mention, in later versions of OpenGL, support for unsigned int was added as well, so if we wanted, we could even consider adding an unsigned int overload that checks whether the entry point is available and warns and falls back to another variant if it isn't.
This is what I wanted to do, but since there was no entry point for unsigned ints on my system, I left it for the moment.

Also, if all we care about is "type safety" between user code and their GLSL, we can always use glGetActiveUniform() to check if the passed type and the actual GLSL type even match and warn if they don't.
That's a really good idea. I think a check like this could be enabled in Debug mode.

I don't see why we couldn't just introduce sf::Vector4. sf::Vector3 is already barely used (only by audio and sensor), yet it is a "first class citizen" within the API. This would also be nice to have with sf::Matrix3 and sf::Matrix4, we would only define the absolute basics of matrix manipulation and leave everything else up to the user to do.
Having a dedicated API for matrices and 4D vectors comes with multiple problems:
Also, I don't see why we need to differentiate the naming of the methods that set arrays (setParameterArray vs setParameter). If the GLSL parameter type is an array, then that entire array is the parameter. We aren't setting an array of parameters, just a single one, which is why I find having the same name as the other non-array methods makes more sense.
The idea was to draw the border between GLSL basic types and GLSL arrays. These are not just different types, they're different categories of types. And since the setUniform overload for arrays would have a different parameter count, you can't even use it for type inference... Qt does this too (http://doc.qt.io/qt-4.8/qglshaderprogram.html), for example.

Instead of passing a single pointer and a size, in the spirit of C++11, we could already go ahead and take first and last parameters instead:
setParameter(const std::string& name, const Vector2f* vectorArray, std::size_t length)
vs
setParameter(const std::string& name, const Vector2f* first, const Vector2f* last)
Maybe at some point, we will make it even more general, using templates which allow for any STL container to be passed.
I also thought about this -- the problem is just, SFML uses everywhere the (pointer, length) convention. There's no apparent reason why sf::Shader should behave differently...

But it might be possible to provide an adapter template, either on user side or in SFML:
template <typename InputIterator>
void Shader::setUniform(const std::string& name, InputIterator begin, InputIterator end)
{
   std::vector<...> contiguous(begin, end);
   setUniform(name, contiguous.data(), contiguous.size());
}

When you provide a (begin, end) overload for each specific type, you'd have to adapt each overload on its own to support iterators, because otherwise you get an ambiguous call when passing two pointers. Oh... this reminds me why a setUniformArray is a good idea: the above template would eat everything with 2 values of the same type, including setUniform(string, float, float) :)
Title: Re: Shader uniform API -- part II
Post by: zmertens on May 16, 2015, 11:38:28 pm
Some of these topics go over my head, but I will agree on changing the
setParameter(...)
call to a
setUniform(...)
call or something similar due to the reasons already mentioned by Nexus and Binary. It does feel more explicit and conveys what is being modified better I think.
Title: Re: Shader uniform API -- part II
Post by: Nexus on June 21, 2015, 02:35:32 pm
Further opinions on this topic? So far, the main arguments are:

Expressive function names (setUniformFloat etc.)
Overloads (setUniform, setUniformArray)
I have the impression that the first option is currently more straightforward, because function names are expressive and we don't have to debate about the additional SFML types. If we want the best of both worlds, it's still possible to use the expressive API and later provide an additional setUniform() overload that dispatches to those methods. If not in SFML, this can be added un-intrusively as void setUnifom(sf::Shader&, Params...).
Title: Re: Shader uniform API -- part II
Post by: Nexus on August 23, 2015, 11:45:09 pm
I worked further on this and implemented most of the needed functions. You can see the progress in the feature/shader_uniforms (https://github.com/SFML/SFML/tree/feature/shader_uniforms) branch.

At the moment, the current incomplete API looks as follows:
void setUniformFloat(const std::string& name, float x);
void setUniformVec2(const std::string& name, const Vec2& vector);
void setUniformVec3(const std::string& name, const Vec3& vector);
void setUniformVec4(const std::string& name, const Vec4& vector);
void setUniformVec4(const std::string& name, const Color& color);

void setUniformInt(const std::string& name, int x);
void setUniformIvec2(const std::string& name, const Ivec2& vector);
void setUniformIvec3(const std::string& name, const Ivec3& vector);
void setUniformIvec4(const std::string& name, const Ivec4& vector);

void setUniformBool(const std::string& name, bool x);
void setUniformBvec2(const std::string& name, const Bvec2& vector);
void setUniformBvec3(const std::string& name, const Bvec3& vector);
void setUniformBvec4(const std::string& name, const Bvec4& vector);

void setUniformMat3(const std::string& name, const float* pointer);
void setUniformMat3(const std::string& name, const Mat3& matrix);
void setUniformMat4(const std::string& name, const float* pointer);
void setUniformMat4(const std::string& name, const Mat4& matrix);
void setUniformMat4(const std::string& name, const sf::Transform& transform);

void setUniformFloatArray(const std::string& name, const float* valueArray, std::size_t length);
void setUniformVec2Array(const std::string& name, const Vector2f* vectorArray, std::size_t length);
void setUniformVec3Array(const std::string& name, const Vector3f* vectorArray, std::size_t length);
void setUniformVec4Array(const std::string& name, const Vec4* vectorArray, std::size_t length);
void setUniformMat3Array(const std::string& name, const Mat3* matrixArray, std::size_t length);
void setUniformMat4Array(const std::string& name, const Mat4* matrixArray, std::size_t length);

void setUniformSampler2D(const std::string& name, const Texture& texture);
void setUniformSampler2D(const std::string& name, CurrentTextureType);


New types
Vec4, Mat3 and Mat4 are new types nested in the sf::Shader class. I thought about providing a dedicated namespace sf::Glsl for these types. This would not bloat the sf::Shader scope too much, and it is immediately obvious what the types stand for. This would look as follows:
(click to show/hide)

Now, why do we even need those types? I really tried to find a way around them, but as soon as we want to provide support for GLSL arrays, it's impossible.


GLSL arrays
As you see above, arrays are currently passed with this signature:
void setUniformTArray(const std::string& name, const T* array, std::size_t length);

With a Glsl namespace, it would also be possible to write Glsl::Array<T> type and a single template void setUniformArray(const std::string& name, const Glsl::Array<T>& array). But this would again raise questions for value/pointer semantics and unnecessary copies, as it is already the case for vectors and matrices.

I also considered a generic Shader::setUniform() interface that doesn't contain the GLSL type in the method name. However, as the shader code needs to be adapted whenever the type changes, the use cases are limited. On the contrary, I see quite some potential (http://en.sfml-dev.org/forums/index.php?topic=18146.msg130703#msg130703) for type errors that cannot be caught at compile time.

There are indeed some opportunities for polymorphism on C++ side, for example a class that can write an entire GLSL struct. Without deeper thought, it also seems that the whole array functions could be easily implemented with templates, but the reality looks a bit more complicated. Since different OpenGL functions are called depending on the types, an internal dispatch is still necessary. And because OpenGL must remain contained in the .cpp file, but templates are implemented in the header, we still need an indirection layer.

Eventually, this boils down to the same as providing an additional generic setUniformArray() method, which selects the concrete setUniformTArray() method depending on the type. If this is really a desired feature, we could think about providing something like this in the future, also for basic GLSL types (not arrays), but I wouldn't add it now already.


Number of overloads
In order to reduce the total number of functions, I omitted the overloads that accept every parameter separately. With C++11, people can use the {x, y} syntax anyway. That is, instead of providing
setUniformVec3(float x, float y, float z);
we just have
setUniformVec3(const sf::Glsl::Vec3& v);
which is exactly the same as:
setUniformVec3(const sf::Vector3f& v);

I have used the typedefs in the signature for consistency with Vec4. Maybe it's a better idea to use sf::Vector2 and sf::Vector3 for clarity, but we can also mention this in the documentation.
Title: Re: Shader uniform API -- part II
Post by: eXpl0it3r on August 24, 2015, 11:44:43 am
FYI: Not exactly sure why but the Debian CI build failed (http://sfml-dev.org:8080/job/sfml-debian-64-gcc/267/console).
Title: Re: Shader uniform API -- part II
Post by: Nexus on August 24, 2015, 01:16:17 pm
Yes, I've only used a single compiler. I'll polish things once the API is fine :)

Any feedback regarding the points in my last post?
Title: Re: Shader uniform API -- part II
Post by: Hapax on August 24, 2015, 03:13:43 pm
I'm not versed enough in OpenGL or GLSL to be anywhere near knowledgable in this area but your design looks clean and readable.
Title: Re: Shader uniform API -- part II
Post by: dabbertorres on August 24, 2015, 07:19:38 pm
Quote from: Nexus
Number of overloads
I like getting rid of the vector component overloads (x, y, z) and just kept vector overloads.

I agree that the argument naming should be sf::Vector2*, sf::Vector3*, and etc. I see where the Vec# abbreviations come from, but this is a part of SFML, I think the naming for Vector types should be consistent across all of SFML. If someone is going to be using shaders/glsl, they'll know that glsl Vec# types correlate just fine with SFML Vector# types.

Also:
Quote from: Nexus
void setUniformVec2Array(const std::string& name, const Vector2f* vectorArray, std::size_t length);
void setUniformVec3Array(const std::string& name, const Vector3f* vectorArray, std::size_t length);
if you stick with *vec# naming, you may want to change the vectorArray type to reflect that. :)

Quote from: Nexus
GLSL arrays
I need to think about this some more, but I think a template/generic function could work quite well here.
Declare a generic version, and specialize it for each supported type. This wouldn't require indirection, and it would satisfy OpenGL remaining in the .cpp.
If desired, then the generic version could #error out to let the user know they picked an incorrect type (if the default error is deemed not clear enough).

// .hpp
template<typename T>
void setUniform(const std::string& name, const T& value)
{
    #error "Invalid type in sf::Shader::setUniform..." etc
}

// .cpp
template<>
void setUniform<float>(const std::string& name, float value)
{
    ...
}

... and so on
 

This was supported in C++03, right? I forget which template stuff was added in C++11 sometimes. If it isn't, maybe that would have to be done in SFML 3.
Title: Re: Shader uniform API -- part II
Post by: Nexus on August 24, 2015, 08:19:47 pm
I agree that the argument naming should be sf::Vector2*, sf::Vector3*, and etc. I see where the Vec# abbreviations come from, but this is a part of SFML, I think the naming for Vector types should be consistent across all of SFML. If someone is going to be using shaders/glsl, they'll know that glsl Vec# types correlate just fine with SFML Vector# types.
That's true, I don't like this inconsistency too much, either. My reasoning was:
But I'm still open for alternatives. Maybe not providing convenient typedefs is a good idea, as it doesn't come with the danger that people are faced with multiple vector types in SFML without knowing that they are the same.

I need to think about this some more, but I think a template/generic function could work quite well here.
Declare a generic version, and specialize it for each supported type. This wouldn't require indirection, and it would satisfy OpenGL remaining in the .cpp.
Yes, but then we can as well provide overloaded functions. The problem is not providing polymorphism, but the circumstance that templates cannot cover different cases here. Writing a template just to specialize it for every type is not generic programming.

If desired, then the generic version could #error out to let the user know they picked an incorrect type (if the default error is deemed not clear enough).
#error is a preprocessor directive, but static assert or SFINAE could be used. But we can also just let the compiler do overload resolution and output errors at ambiguous/not-matching calls. Overload resolution is even more flexible, as it allows implicit conversions.
Title: Re: Shader uniform API -- part II
Post by: dabbertorres on August 24, 2015, 10:30:55 pm
Quote from: Nexus
Argument type naming
Hmm.

The boolean typedef is easily resolved if needed, though sf::Vector2<bool> isn't a big deal I think.

4D vectors and matrices... You've got a point there. I'm not sure how I feel about this as much anymore. I'm starting to lean in the direction you went. Though I find Vec2f, Vec3b, Vec4i, etc a bit more readable. Also, it's at least a little more consistent with the sf::Vector2<T> naming scheme. Putting them in a sf::glsl namespace is a good idea too.

SFML3 with C++11 might allow for some nice ways to work with this.
template<typename T>
using Vec4<T> = std::array<T, 4>;

using Vec4f = Vec4<float>;
 
It does look slightly nicer. Though, if it's using C++11 (and 14?), hopefully uniform buffer objects could be used and then something like:
template<typename... Uniforms>
class Shader
{
    ...
    set(Uniforms... uniforms);
}
 
could be plausible. And would be nice to use. I think.

Quote from: Nexus
generics/overloads
True. I just find the setUniform<float>(5.3f) syntax a bit nicer than several different function names. :)
As well though, just because templates allow for generic programming doesn't mean you have to strictly adhere to generic programming.

Quote from: Nexus
Error stuff
Yeah. Though static_assert is C++11, isn't it? And yes, I know SFINAE would work as well, but, with all my experience with templates, the error messages produced are often... large. And difficult for someone not used to them. Since SFML doesn't make use of a large amount of templates, I figured it might be nice to send a bit nicer of an error message to the user.
Title: Re: Shader uniform API -- part II
Post by: Nexus on August 25, 2015, 11:00:01 pm
I'm starting to lean in the direction you went. Though I find Vec2f, Vec3b, Vec4i, etc a bit more readable.
Yes, but this would combine the disadvantage of being inconsistent to SFML's existing vector types with the disadvantage of not representing GLSL types. I don't think introducing a third convention is a good idea :P

Quote from: dabbertorres
SFML3 with C++11 might allow for some nice ways to work with this.
template<typename T>
using Vec4<T> = std::array<T, 4>;

using Vec4f = Vec4<float>;
Then we have to access members using [0], [1], [2], [3] instead of x, y, z, w, which is again inconsistent. Writing a Vector4 type is really trivial, I would deliberately not provide any mathematical operations.

Quote from: dabbertorres
True. I just find the setUniform<float>(5.3f) syntax a bit nicer than several different function names. :)
Yes, but is
setUniform<sf::Glsl::Matrix4>(name, values)
also nicer than
setUniformMat4(name, values)
?

There's quite a few reasons that favor setUniform overloads (not specializations), foremost genericity and type inference. But it forces the user to always use the dedicated SFML types. One one hand, this makes calls more verbose and adds unneeded copies (unless we use pointer semantics):
shader.setUniform(sf::Glsl::Mat4(name, values));
// instead of
shader.setUniformMat4(name, values);

Also, it adds another layer to conversions when there are multiple options, that is:
sf::Transform transform;

shader.setUniform(sf::Glsl::Mat3(transform));
shader.setUniform(sf::Glsl::Mat4(transform));
// instead of
shader.setUniformMat3(transform);
shader.setUniformMat4(transform);

But maybe we can live with that. We just need to get further in this discussion, the same arguments have already been around for months ;)

By the way, we cannot call the namespace glsl because it conflicts with the naming convention (sf::Shapes, sf::Style). However, the user can easily write
namespace glsl = sf::Glsl;
which also shortens the expressions.
Title: Re: Shader uniform API -- part II
Post by: Klaim on August 29, 2015, 12:15:16 am
At the moment, the current incomplete API looks as follows:
void setUniformFloat(const std::string& name, float x);
void setUniformVec2(const std::string& name, const Vec2& vector);
void setUniformVec3(const std::string& name, const Vec3& vector);
void setUniformVec4(const std::string& name, const Vec4& vector);
void setUniformVec4(const std::string& name, const Color& color);

void setUniformInt(const std::string& name, int x);
void setUniformIvec2(const std::string& name, const Ivec2& vector);
void setUniformIvec3(const std::string& name, const Ivec3& vector);
void setUniformIvec4(const std::string& name, const Ivec4& vector);

void setUniformBool(const std::string& name, bool x);
void setUniformBvec2(const std::string& name, const Bvec2& vector);
void setUniformBvec3(const std::string& name, const Bvec3& vector);
void setUniformBvec4(const std::string& name, const Bvec4& vector);

void setUniformMat3(const std::string& name, const float* pointer);
void setUniformMat3(const std::string& name, const Mat3& matrix);
void setUniformMat4(const std::string& name, const float* pointer);
void setUniformMat4(const std::string& name, const Mat4& matrix);
void setUniformMat4(const std::string& name, const sf::Transform& transform);

void setUniformFloatArray(const std::string& name, const float* valueArray, std::size_t length);
void setUniformVec2Array(const std::string& name, const Vector2f* vectorArray, std::size_t length);
void setUniformVec3Array(const std::string& name, const Vector3f* vectorArray, std::size_t length);
void setUniformVec4Array(const std::string& name, const Vec4* vectorArray, std::size_t length);
void setUniformMat3Array(const std::string& name, const Mat3* matrixArray, std::size_t length);
void setUniformMat4Array(const std::string& name, const Mat4* matrixArray, std::size_t length);

void setUniformSampler2D(const std::string& name, const Texture& texture);
void setUniformSampler2D(const std::string& name, CurrentTextureType);


Do you really need to use std::strings? or would it be ok to use const char* or something like string_ref/string_view (from other libraries - does sfml have something like that already?)? I'm fine with strings of course, but it looks like there might be a lot of allocations potentially going on just for some reading code.
Title: Re: Shader uniform API -- part II
Post by: Gambit on August 29, 2015, 04:30:20 am
SFML is a C++ library and while const char* vs std::string is personal preference I believe that std::string is considered the prefered choice of the two. The overloads that a const std::string& (Reference) instead of a copy if thats what you mean by allocations. On the other hand, SFML comes with its own string implementation named sf::String which is of course non-standard.
Title: Re: Shader uniform API -- part II
Post by: Klaim on August 29, 2015, 01:44:12 pm
SFML is a C++ library and while const char* vs std::string is personal preference I believe that std::string is considered the prefered choice of the two.

I know I know, all of that, of course.   ::)

I participate to the C++ community so I'm not for const char *. I'm pointing a potential performance issue which is very well known in the community. The usual solution is to have something like std::string_view (http://en.cppreference.com/w/cpp/experimental/basic_string_view) (not in the standard yet but voted in the coming one) (or boost string_ref (http://www.boost.org/doc/libs/1_59_0/libs/utility/doc/html/string_ref.html) ) etc. which avoid a potential std::string construction in some hidden cases which are legion string-manipulation code.

That's a minor issue of course and could be fixed later if performance shows.

Quote
The overloads that a const std::string& (Reference) instead of a copy if thats what you mean by allocations.

If you have a const char* originally and pass it to this function, a std::string is constructed to match the const reference std::string required, then it is destroyed. Depending on the size of the string content and the STL implementation, you do pay more than if you just wanted the function to read your original const char* (in particular if the small-buffer-optimization can't be used, the string will allocate and deallocate, which you want to avoid most of the time in a critical loop).
That's the usual performance concern with this kind of interface. (again not deadly important here, I don't mind a lot, just pointing the thing)

The std::string_view (again not yet in the standard but a variant is available for example in boost) would take a const char* or a std::string and never ever allocate anything in the background and work like a const refrence without the need to make implicit conversions.

Quote
On the other hand, SFML comes with its own string implementation named sf::String which is of course non-standard.

I didn't consider this but I don't think that solves anything.

Anyway I don't have enough experience with shader to know if it there will be performance concerns here. It just feel to me that the values will be passed often (once per frame?) and if someone do

    setUniformFloat( "my_value_have_a_long_name", 42.0f );

each frame, they are paying something each frame which they might think is not a lot. To escape this issue one would make sure to store these names in exactly const std::string (not a sfml::String nor any other type to avoid the copy construction).

I don't see any class that looks like a string_ref/_view in the SFML api so right now I guess it's not important and maybe later if such tool is provided then an optimization pass would be done on the API (with 100%compatibility I believe).
Title: Re: Shader uniform API -- part II
Post by: Hapax on August 29, 2015, 01:52:37 pm
If you have a const char* originally and pass it to this function, a std::string is constructed to match the const reference std::string required, then it is destroyed. Depending on the size of the string content and the STL implementation, you do pay more than if you just wanted the function to read your original const char* (in particular if the small-buffer-optimization can't be used, the string will allocate and deallocate, which you want to avoid most of the time in a critical loop).
If you're using std::string instead of char* to begin with, this would avoid this scenario.  ;)
Title: Re: Shader uniform API -- part II
Post by: Klaim on August 29, 2015, 02:38:53 pm
If you have a const char* originally and pass it to this function, a std::string is constructed to match the const reference std::string required, then it is destroyed. Depending on the size of the string content and the STL implementation, you do pay more than if you just wanted the function to read your original const char* (in particular if the small-buffer-optimization can't be used, the string will allocate and deallocate, which you want to avoid most of the time in a critical loop).
If you're using std::string instead of char* to begin with, this would avoid this scenario.  ;)

Of course. I said so in the later paragraph. If you look at the example too, there is a common case where you think you use std::string but you don't ;)

Quote from: klaim
Anyway I don't have enough experience with shader to know if it there will be performance concerns here. It just feel to me that the values will be passed often (once per frame?) and if someone do

    setUniformFloat( "my_value_have_a_long_name", 42.0f );

each frame, they are paying something each frame which they might think is not a lot. To escape this issue one would make sure to store these names in exactly const std::string (not a sfml::String nor any other type to avoid the copy construction).

Also, even if you use static const std::string for fixed names, you also pay the cost of std::string for a compile-time value. So you still pay for something (allocation). And if your string values are stored in a big string buffer, then you have to build std::strings for that.
It really depend on what kidn of code this is about of course, so it might not be important here, I'm not sure, but so far I thought this kind of api was called a lot.

Though again it's a minor concern for now I guess?
Title: Re: Shader uniform API -- part II
Post by: Nexus on August 29, 2015, 10:12:15 pm
My thoughts about Klaim's suggestion:
Title: Re: Shader uniform API -- part II
Post by: Klaim on August 30, 2015, 01:12:49 am
My thoughts about Klaim's suggestion:
  • The arguments are string literals in more than 99% of the cases, so const char* would suffice.
  • const char* would indicate that the user is expected to pass string literals -- a good or bad thing?
Is it a convention specific to SFML? Otherwise as a user seeing const char* (after finding why it's not a std::string), I would just suspect the implementation to either read or copy the content of the string and not expect the lifetime of the passed string to be required to live longer than the call (except documented otherwise but even standard exception message are like that). Also, this forces null-terminated strings which is not the case with std::string and string_view/ref which are "ranges" that can contain several null characters.
The passed pointer could be a litteral or something coming from a big text buffer, it wouldn't make a difference (as long as it's null terminated). I'm guessing that part of what is reasonable depends on the opengl api too.
All these points should be considered I guess.

To clarify: I don't tend to consider const char* to ask specifically for sttring litterals so I'm not sure if that's how such an API will be considered by other users.

Quote
  • The allocation is probably negligible compared to the OpenGL calls. This may change when SFML batches uniforms one day, instead of passing them to OpenGL immediately.
Interesting.

Quote
  • It's slightly less convenient for a few cases when std::string is not supported (additional .c_str() call).
Yeah, this can make the user code noisy if the user decided to store the names in strings. It's related to your string litteral question I guess.

Quote
  • string_view/string_ref is no option because it's not standard, and it's not SFML's task to rewrite them.
About that, I was thinking that there are a lot of precedents for alternative (often simpler to understand for newbies) types not present initially in the standard but provided early by SFML, like time related types and thread stuffs (though the doc clarify that you might want to use the standard if you can). So I'm not sure if having a very simple implementation wouldn't be ok, though you have to take the decision of course :) (or maybe experience maintaining SFML shows that it's not a good idea already).
Boost, chromium/google, llvm/clang, and some other libraries each have their own version which is why it was proposed and quickly accepted in the coming standard (they didn't have the same interface though), and all these implementations seem very simple. So maybe it's easy to provide.
But yeah maintenance might be problematic in the future. Maybe it should not be considered.

Quote
  • API consistency: SFML uses mostly std::string or sf::String in its API.
  • When we start with const char*, we can still switch to std::string later without breaking the API. It would however introduce an additional allocation in the cases where std::string was used.
  • This current redesign of the uniform methods is an opportunity to change things like that. It cannot be done again until the advent of SFML 3.

OK nice
Title: Re: Shader uniform API -- part II
Post by: Nexus on August 30, 2015, 09:24:00 am
Also, this forces null-terminated strings which is not the case with std::string and string_view/ref which are "ranges" that can contain several null characters.
Yes, but in this specific case we forward the string as a C string to OpenGL, internal null characters are not allowed anyway.

To clarify: I don't tend to consider const char* to ask specifically for sttring litterals so I'm not sure if that's how such an API will be considered by other users.
What we should consider, and this is why I mentioned this point and the one about API consistency: users will ask themselves why char* is used here, and not std::string or sf::String like everywhere else. We'd better have a good answer ;)

About that, I was thinking that there are a lot of precedents for alternative (often simpler to understand for newbies) types not present initially in the standard but provided early by SFML, like time related types and thread stuffs
Threads and time is crucial functionality required for the workings of SFML, which is simply not available in C++98. We had no choice. At least threads will go when C++11 is used. String views on the other hand are a pure optimization -- since it's unlikely that SFML users waste their performance with string conversions, I don't like to extend the API further. We already have a custom string type with sf::String, even that is controversial.

I'd also be interested in further opinions on this.
Title: Re: Shader uniform API -- part II
Post by: Jabberwocky on August 30, 2015, 08:52:33 pm
I'd also be interested in further opinions on this.

+1 vote for const char*

Any potential confusion can be solved by a short comment like:  "a const char* argument is used rather than a const std::string& to avoid the extra constructor cost for the common use case where a string literal is provided as an argument".  Give an example if necessary.

But it's a minor detail, so I'm not too worried either way.  If you do go with a const std::string reference, anyone concerned with the optimization can always just declare some std::string constants somewhere in their program to hold their parameter names in.
Title: Re: Shader uniform API -- part II
Post by: select_this on September 03, 2015, 04:03:48 pm
Speaking from personal experience, in a literal-string-heavy application, there is a noticeable difference in having that extra allocation; but we're talking web servers, HTML / JSON generation etc. here with all the string manipulation that entails, so way above the amount of string processing that SFML's shader API is going to have to deal with.
Title: Re: Shader uniform API -- part II
Post by: binary1248 on September 03, 2015, 11:45:22 pm
The standard library added const std::string& overloads to functions that previously only took const char*s in C++11. I guess to make everybody happy. Don't know about doubling the number of methods here just because of that performance gain that would only happen if the sf::Shader implementation was changed a bit as well. The map that holds the cached uniform locations currently uses std::strings as keys.
Title: Re: Shader uniform API -- part II
Post by: Nexus on September 04, 2015, 08:53:24 am
Good points. Storing const char* in a map without custom predicate is not possible...

I think as soon as it makes code more complex, we need an actually proven performance gain, not just assumptions. Doubling the number of methods (of which there are already dozens) is hardly a good solution.
Title: Re: Shader uniform API -- part II
Post by: Hiura on September 04, 2015, 09:43:26 am
This reasoning seduces me a lot! ;-)
Title: Re: Shader uniform API -- part II
Post by: Nexus on September 04, 2015, 09:50:52 am
This reasoning seduces me a lot! ;-)
Which one? :)
Or more clearly, what's your opinion on this topic?
Title: Re: Shader uniform API -- part II
Post by: Hiura on September 04, 2015, 10:57:38 am
Generally speaking for the std::string approach (instead of the C approach), but mostly for the "do we have a proof of performance boost with a more complex code?".
Title: Re: Shader uniform API -- part II
Post by: Klaim on September 09, 2015, 10:53:51 pm
Generally speaking for the std::string approach (instead of the C approach), but mostly for the "do we have a proof of performance boost with a more complex code?".

Same here, that's why I was asking more experienced shader users if they actually do call these kind of apis in critical loops.

I think the overload solution might work well indeed. The standard committee voted string_view in the coming standard to help library implementor later, but as already discussed it's not available here.

I didn't know the values were stored in a map... I though the values were only sent to opengl.
Title: Re: Shader uniform API -- part II
Post by: Nexus on September 21, 2015, 02:22:06 pm
[Link to feature/shader_uniforms (https://github.com/SFML/SFML/tree/feature/shader_uniforms) branch]

I added the sf::Glsl namespace in <SFML/Graphics/Glsl.hpp> (https://github.com/SFML/SFML/blob/feature/shader_uniforms/include/SFML/Graphics/Glsl.hpp).

Furthermore, the matrix types Mat3 and Mat4 now use value semantics. As we still have the direct float* overloads for Shader::setUniformMat3/4(), there is no unnecessary copying.

Once more, I considered using setUniform() overloads, as it would allow genericity... However I still think it makes some things confusing. For example, it's not obvious that users have to pass
shader.setUniform(sf::Glsl::Mat3(transform));
shader.setUniform(sf::Glsl::Mat4(transform));
// instead of
shader.setUniformMat3(transform);
shader.setUniformMat4(transform);

With the current way, they can specify what type they need in GLSL (e.g. mat3), and they see all the possible overloads of that function (float*; Glsl::Mat3; sf::Transform). But I still don't like it 100%.
Title: Re: Shader uniform API -- part II
Post by: Nexus on October 05, 2015, 11:44:17 am
I reflected again a lot about possible APIs and their advantages and drawbacks, and I'm happy to announce that I've finally reached a point where it's ready :)

Feel free to test and give feedback to pull request #983 (https://github.com/SFML/SFML/pull/983).
Title: Re: Shader uniform API -- part II
Post by: Suslik on January 31, 2016, 05:01:03 am
So I want to pass a generic 4x4 matrix as a uniform to sf::Shader without
Code: [Select]
shader.setParameter("matrixColumn0", data[0][0], data[0][1], data[0][2], data[0][3]);
shader.setParameter("matrixColumn1", data[1][0], data[1][1], data[1][2], data[1][3]);
shader.setParameter("matrixColumn2", data[2][0], data[2][1], data[2][2], data[2][3]);
shader.setParameter("matrixColumn3", data[3][0], data[3][1], data[3][2], data[3][3]);
Do I have any options that do not include SFML git version manual recompilation?

Update:
I did recompile current git-default version that contained sf::Shader::SetUniform(...) function but when they are used in actual project, they cause linking errors due to functions like this(Glsl.h, line 40) not being labelled as library interface(it means they are not exported in .lib/.dll) :
Code: [Select]
void copyMatrix(const float* source, std::size_t elements, float* dest);

Adding __declspec(dllexport) like this:
Code: [Select]
void SFML_GRAPHICS_API copyMatrix(const float* source, std::size_t elements, float* dest);
solves the problem.

There's also some serious incompatibility going regarding z-buffer that is turned on by default on Windows in SFML 2.3.1 and turned off on linux(at least on drivers I tested). In SFML 2.3.2 it's turned off by default under both OS'es. I had to set sf::ContextSettings::depthBits manually to enable it.
Title: Re: Shader uniform API -- part II
Post by: Laurent on January 31, 2016, 09:13:18 am
Quote
I did recompile current git-default version that contained sf::Shader::SetUniform(...) function but when they are used in actual project, they cause linking errors due to functions like this(Glsl.h, line 40) not being labelled as library interface
This is already fixed, check recent commits/PRs.

Quote
There's also some serious incompatibility going regarding z-buffer that is turned on by default on Windows in SFML 2.3.1 and turned off on linux(at least on drivers I tested). In SFML 2.3.2 it's turned off by default under both OS'es. I had to set sf::ContextSettings::depthBits manually to enable it.
Default context settings most likely depend on the graphics driver. Don't assume anything about them, especially since it's not documented. If you want something, enable it explicitly.