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

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

0 Members and 1 Guest are viewing this topic.

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Re: Shader uniform API -- part II
« Reply #15 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.
« Last Edit: August 25, 2015, 11:11:00 pm by Nexus »
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Klaim

  • Full Member
  • ***
  • Posts: 137
    • View Profile
Re: Shader uniform API -- part II
« Reply #16 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.

Gambit

  • Sr. Member
  • ****
  • Posts: 283
    • View Profile
Re: Shader uniform API -- part II
« Reply #17 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.

Klaim

  • Full Member
  • ***
  • Posts: 137
    • View Profile
Re: Shader uniform API -- part II
« Reply #18 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 (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.

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).
« Last Edit: August 29, 2015, 01:46:46 pm by Klaim »

Hapax

  • Hero Member
  • *****
  • Posts: 3383
  • My number of posts is shown in hexadecimal.
    • View Profile
    • Links
Re: Shader uniform API -- part II
« Reply #19 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.  ;)
Selba Ward -SFML drawables
Cheese Map -Drawable Layered Tile Map
Kairos -Timing Library
Grambol
 *Hapaxia Links*

Klaim

  • Full Member
  • ***
  • Posts: 137
    • View Profile
Re: Shader uniform API -- part II
« Reply #20 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?
« Last Edit: August 29, 2015, 02:41:48 pm by Klaim »

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Re: Shader uniform API -- part II
« Reply #21 on: August 29, 2015, 10:12:15 pm »
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.
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Klaim

  • Full Member
  • ***
  • Posts: 137
    • View Profile
Re: Shader uniform API -- part II
« Reply #22 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
« Last Edit: August 30, 2015, 01:16:45 am by Klaim »

Nexus

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

Jabberwocky

  • Full Member
  • ***
  • Posts: 157
    • View Profile
Re: Shader uniform API -- part II
« Reply #24 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.

select_this

  • Full Member
  • ***
  • Posts: 130
  • Current mood: just ate a pinecone
    • View Profile
    • darrenferrie.com
Re: Shader uniform API -- part II
« Reply #25 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.
Follow me on Twitter, why don'tcha? @select_this

binary1248

  • SFML Team
  • Hero Member
  • *****
  • Posts: 1405
  • I am awesome.
    • View Profile
    • The server that really shouldn't be running
Re: Shader uniform API -- part II
« Reply #26 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.
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: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Re: Shader uniform API -- part II
« Reply #27 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.
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Hiura

  • SFML Team
  • Hero Member
  • *****
  • Posts: 4321
    • View Profile
    • Email
Re: Shader uniform API -- part II
« Reply #28 on: September 04, 2015, 09:43:26 am »
This reasoning seduces me a lot! ;-)
SFML / OS X developer

Nexus

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

 

anything