Shader uniform API -- part II

[Nexus]

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

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.

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.

[Klaim]

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.

[Gambit]

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.

[Klaim]

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 (not in the standard yet but voted in the coming one) (or boost string_ref ) 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.

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.

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).

[Hapax]

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. 

[Klaim]

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

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?

[Nexus]

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?
  
• 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.
• It's slightly less convenient for a few cases when std::string is not supported (additional .c_str() call).
  
• string_view/string_ref is no option because it's not standard, and it's not SFML's task to rewrite them.
• 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.

[Klaim]

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.

• 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.

• 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.

• 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.

• 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

[Nexus]

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.

[Jabberwocky]

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.

[select_this]

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.

[binary1248]

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.

[Nexus]

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.

[Hiura]

This reasoning seduces me a lot! ;-)

[Nexus]

This reasoning seduces me a lot! ;-)

Which one?
Or more clearly, what's your opinion on this topic?