Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: Shader uniform API -- part II  (Read 11403 times)

0 Members and 1 Guest are viewing this topic.

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6226
  • Thor Developer
    • View Profile
    • Bromeon
Shader uniform API -- part II
« on: May 13, 2015, 11:13:49 am »
In part I, 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 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
(click to show/hide)

The second option would be to provide named functions.
Commit 7d1d4da
(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.
« Last Edit: May 13, 2015, 11:58:51 am by Nexus »
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development: first SFML book

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6226
  • Thor Developer
    • View Profile
    • Bromeon
Re: Shader uniform API -- part II
« Reply #1 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).
« Last Edit: May 13, 2015, 11:56:35 am by Nexus »
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development: first SFML book

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32451
    • View Profile
    • SFML's website
    • Email
Re: Shader uniform API -- part II
« Reply #2 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).
Laurent Gomila - SFML developer

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6226
  • Thor Developer
    • View Profile
    • Bromeon
Re: Shader uniform API -- part II
« Reply #3 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 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?
« Last Edit: May 13, 2015, 03:36:38 pm by Nexus »
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development: first SFML book

binary1248

  • SFML Team
  • Hero Member
  • *****
  • Posts: 1392
  • I am awesome.
    • View Profile
    • The server that really shouldn't be running
Re: Shader uniform API -- part II
« Reply #4 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 ;). 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 (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.
SFGUI # SFNUL # GLS # Wyrm <- Why do I waste my time on such a useless project? Because I am awesome (first meaning).

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6226
  • Thor Developer
    • View Profile
    • Bromeon
Re: Shader uniform API -- part II
« Reply #5 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:
  • In contrary to sf::Vector3, they are really only used in one specific place, sf::Shader::setUniform(). And there, they are only used as a type-tag, they have no other function than choosing the correct overload.
  • The coexistence of sf::Transform, sf::Matrix3 and sf::Matrix4 will certainly lead to confusion. This could be prevented by nesting Matrix3 and Matrix4 types into sf::Shader. They could still be moved to sf namespace at a later stage, leaving a typedef in sf::Shader for backwards compatibility.
  • Obviously, people would request more features for matrix types. If SFML provides multiple matrix types in the API, then the answer "we only provide it as type-dispatchers for sf::Shader" is a really bad one. No one will understand why there is not even basic functionality like matrix multiplication or matrix addition. And as soon as you implement something, you have to add more "for consistency reasons". In fact, this is even a bigger problem for a potential sf::Vector4, because you'd need to overload the whole set of arithmetic operators "for consistency", even though hardly anybody will ever use them. We're drifting too much into a Math library with such design choices... Even if we provide several operations, it will never be enough for a decent linear algebra toolkit, and we have to draw an arbitrary border.
  • If you provide such types in the sf namespace, they must have value semantics. Matrix3 must store 16 values, it can't hold a pointer to them. This is a problem you don't have with sf::Shader::setMat3(), you can directly pass your pointer. Without it, you have a useless copy from your data structure to the intermediate SFML type, which has no other purpose than overload resolution.
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, 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) :)
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development: first SFML book

zmertens

  • Jr. Member
  • **
  • Posts: 80
    • View Profile
Re: Shader uniform API -- part II
« Reply #6 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.
The truth will set you free but first it will piss you off.

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6226
  • Thor Developer
    • View Profile
    • Bromeon
Re: Shader uniform API -- part II
« Reply #7 on: June 21, 2015, 02:35:32 pm »
Further opinions on this topic? So far, the main arguments are:

Expressive function names (setUniformFloat etc.)
  • Can avoid subtle type errors, see GameObject example
  • Makes the mapping from C++ to GLSL clear. Allows overloads like setUniformMat3(sf::Transform) and setUniformMat3(float*)
  • We don't need dedicated types for matrices and 4D vector (which lead to further questions: scope, value/pointer semantics, features?)
Overloads (setUniform, setUniformArray)
  • Type inference follows modern C++
  • Allows generic programming, see GlslStruct example
  • Type safety can still be checked at runtime
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...).
« Last Edit: June 21, 2015, 02:39:03 pm by Nexus »
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development: first SFML book

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6226
  • Thor Developer
    • View Profile
    • Bromeon
Re: Shader uniform API -- part II
« Reply #8 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 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 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.
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development: first SFML book

eXpl0it3r

  • SFML Team
  • Hero Member
  • *****
  • Posts: 9597
    • View Profile
    • development blog
    • Email
Re: Shader uniform API -- part II
« Reply #9 on: August 24, 2015, 11:44:43 am »
FYI: Not exactly sure why but the Debian CI build failed.
« Last Edit: August 24, 2015, 01:18:24 pm by eXpl0it3r »
Official FAQ: https://www.sfml-dev.org/faq.php
Nightly Builds: https://www.nightlybuilds.ch/
——————————————————————
Dev Blog: https://dev.my-gate.net/
Thor: http://www.bromeon.ch/libraries/thor/

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6226
  • Thor Developer
    • View Profile
    • Bromeon
Re: Shader uniform API -- part II
« Reply #10 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?
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development: first SFML book

Hapax

  • Hero Member
  • *****
  • Posts: 3017
  • My number of posts is shown in hexadecimal.
    • View Profile
Re: Shader uniform API -- part II
« Reply #11 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.
Selba Ward - SFML drawables
Kairos - Timing Library
Rectangular Boundary Collision - Rectangular SAT Collision

@Hapaxiation - Hapaxia on Twitter

dabbertorres

  • Hero Member
  • *****
  • Posts: 506
    • View Profile
    • website/blog
Re: Shader uniform API -- part II
« Reply #12 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.
« Last Edit: August 24, 2015, 07:25:12 pm by dabbertorres »

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6226
  • Thor Developer
    • View Profile
    • Bromeon
Re: Shader uniform API -- part II
« Reply #13 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:
  • SFML doesn't provide any typedefs for many GLSL equivalents. For example bvec2 needs to be written as sf::Vector2<bool>.
  • We need to handle 4D vectors and matrices somehow. I don't consider sf::Vector4 and sf::Matrix class templates a good idea, for reasons extensively outlined in the original thread. So we would be left with sf::Shader::Vector4 or sf::Glsl::Vector4. And here, I thought a little bit of consistency would be good, i.e. sf::Glsl::Vec2 and sf::Glsl::Vec4 instead of sf::Vector2f and sf::Glsl::Vector4<float>.
  • A dedicated Glsl namespace would make clear that the types are exactly their GLSL equivalents and have no use outside of shaders.
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.
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development: first SFML book

dabbertorres

  • Hero Member
  • *****
  • Posts: 506
    • View Profile
    • website/blog
Re: Shader uniform API -- part II
« Reply #14 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.

 

anything