New API to set shader uniforms #983

Bromeon · 2015-10-05T09:41:40Z

This pull request implements #538 and introduces a new API to set uniform variables in sf::Shader. The old setParameter() overloads are deprecated in favor of new setUniform() ones.

The sf::Glsl namespace is added to SFML. It contains type definitions for types that can be passed to Shader::setUniform(). Most types are typedefs to existing SFML types (such as sf::Vector2<T>), there are however a couple of new ones:

sf::Glsl::Mat3  // 3x3 matrix
sf::Glsl::Mat4  // 4x4 matrix
sf::Glsl::Vec4  // 4D float vector
sf::Glsl::Ivec4 // 4D int vector
sf::Glsl::Bvec4 // 4D bool vector

I provided implicit conversions from sf::Transform and sf::Color to these types where appropriate. Where necessary for ambiguity reasons, an explicit conversion must be used:

sf::Color color;
shader.setUniform("color", sf::Glsl::Vec4(color)); // pass as 4D vector
shader.setUniform("color", color); // same call, implicit conversion

sf::Transform transform;
shader.setUniform("matrix", sf::Glsl::Mat3(transform)); // pass as 3x3
shader.setUniform("matrix", sf::Glsl::Mat4(transform)); // pass as 4x4

In addition to basic GLSL types, arrays are now supported as well. They can be specified with Shader::setUniformArray() that takes a pointer to the first element and the length:

sf::Glsl::Vec2 positions[5] = ...;
shader.setUniformArray("positions", positions, 5);

Design rationale

After extensive thinking about the design (see 2 forum threads), I came to the conclusion that setUniform() overloads are more appropriate than named functions setUniformFloat(), for the following reasons:

The call is simpler and we don't force the user to type redundant information.
We allow polymorphism (a user can write a template to pass uniforms of any type, without knowing the function names).
The number of provided methods is smaller. We can work with conversions where possible. For example, we don't need to overload setUniformMat3() for sf::Glsl::Mat3, float* and sf::Transform separately, one method taking sf::Glsl::Mat3 is enough.
It is still possible to enforce type safety at compile time and at runtime. The former can be achieved with a wrapper function that explicitly requires the type to be specified.
There is no way around dedicated types anyway, otherwise we can't support GLSL arrays (or only in a very low-level way).

Try it out

A self-contained archive containing a C++ file, two GLSL shader files and an image can be downloaded temporarily from my homepage: www.bromeon.ch/sfml/feature-shader-uniforms.zip

LaurentGomila · 2015-10-05T10:14:00Z

👍 for finalizing this task.

I have two comments so far:

What about replacing all those functors in Shader.cpp with a single RAII structure similar to TextureSaver, which would save/restore the program binding with its constructor/destructor? This way you can keep the code that sets uniforms in each setUniform function, and you can keep things a little simpler.
I would create a Glsl.inl file to keep Glsl.hpp clean -- especially since all this stuff is private code.

Bromeon · 2015-10-05T14:40:15Z

Like this? 😃

Bromeon · 2015-10-05T14:49:01Z

Of course, the moment I wrote that, I realized that I could as well have outsourced the entire class definitions, since a declaration is enough for typedef...

So it should be even cleaner now.

mantognini · 2015-10-05T19:20:08Z

I haven't got time to check everything in details but I would have included the .inl at the end of the .hpp (*) file from outside both sf and priv namespaces, and added those in the .inl file so that when reading this file we know without looking at any other file in which namespace things are declared.

(*) if I remember correctly it's file to define a typedef to an incomplete type if it's full definition is available when using it so in this case it should work I believe.

Bromeon · 2015-10-05T19:51:43Z

I haven't got time to check everything in details but I would have included the .inl at the end of the .hpp (*) file from outside both sf and priv namespaces, and added those in the .inl file so that when reading this file we know without looking at any other file in which namespace things are declared.

I would have done it like this too (see Thor), but I looked at Rect.inl and adhered to that style... Laurent? ;)

(*) if I remember correctly it's file to define a typedef to an incomplete type if it's full definition is available when using it so in this case it should work I believe.

Yes, where "using" depends on how you're accessing the type -- it's a matter of whether the type is complete at that time or not. For example, you can declare pointers and references to incomplete types.

mantognini · 2015-10-05T19:56:31Z

I would have done it like this too (see Thor), but I looked at Rect.inl and adhered to that style... Laurent? ;)

Good point. Also note that in Thread.inl, the priv namespace is added there.

But I'd say we should avoid this style, at least from now on. What @SFML/sfml think?

LaurentGomila · 2015-10-06T05:39:36Z

Also note that in Thread.inl, the priv namespace is added there.

In Thread.inl, the namespace priv surrounds local private stuff, not the class definition.

I would have done it like this too (see Thor), but I looked at Rect.inl and adhered to that style... Laurent?

Do you really need to ask if you should do like in all other files? ;)

Regarding the modifications: looks perfect now 👍

Bromeon · 2015-10-06T07:40:06Z

Do you really need to ask if you should do like in all other files? ;)

No, I already did it like in all other files ;) the question is rather:

But I'd say we should avoid this style, at least from now on. What does @SFML/sfml think?

But it's a different question, so let's discuss it elsewhere and not pollute this pull request any further....

Regarding the modifications: looks perfect now 👍

Okay, cool :)

By the way, for those who haven't seen it yet, I attached a small self-contained example to my initial post, so everybody can test it.

binary1248 · 2015-10-08T18:11:08Z

include/SFML/Graphics/Glsl.inl

+/// <algorithm> and MSVC's annoying 4996 warning in header
+///
+////////////////////////////////////////////////////////////
+void copyMatrix(const float* source, std::size_t elements, float* dest);


Is there a reason for this parameter ordering? I find following the same ordering as std::memcpy to be more "intuitive" and probably easier for people to remember.

The function is identical to C++11 std::copy_n<float*>(). But maybe you're right, it's also more consistent with initializeMatrix() above...

Bromeon · 2015-10-12T07:28:05Z

I applied the suggestions and some other changes.

Within UniformBinder, I left out ensureGlContext(), because as Laurent pointed out, it has already been called at that point. I don't like "better safe than sorry" reasoning, we need to be clear about our logic. I emphasized this with a comment.
Renamed the internal functions to copyMatrix(). All overloads start with the source parameter, as that's more idiomatic C++ (STL algorithms).
Added a few lines of code to define all setUniform(), setUniformArray() and setParameter() overloads for OpenGL ES, instead of hundreds of lines of empty implementations.
Squashed the commits.

LaurentGomila · 2015-10-12T08:09:08Z

Within UniformBinder, I left out ensureGlContext(), because as Laurent pointed out, it has already been called at that point.

It still needs to be called (it was there before your modifications, it was for a good reason ;)), in case setUniform is called in a thread that has no active GL context. That's how all GL resources work: they call ensureGlContext() in every function that makes an OpenGL call, even if their constructor already did it, because things could happen in different threads.

Added a few lines of code to define all setUniform(), setUniformArray() and setParameter() overloads for OpenGL ES, instead of hundreds of lines of empty implementations.

Although it reduces the number of lines to type for you, it clutters the public header. I prefer hiding such details, even if it involves typing more useless code for us. It's not as if it was going to change often.

Bromeon · 2015-10-12T08:29:11Z

It still needs to be called

Okay, I misunderstood you. Thanks for the explanation! 😀

Although it reduces the number of lines to type for you, it clutters the public header. I prefer hiding such details, even if it involves typing more useless code for us.

It makes me sad how we have all kinds of powerful abstractions in C++, but resort to silly boilerplate code 😟 are those 15 lines in the header a real problem? I'm not worried about typing (my IDE is powerful enough), but about code readability. Having a .cpp file of which the half are just empty dummy implementations is awful... But I can change it of course, if keeping things out of the header is the priority.

LaurentGomila · 2015-10-12T08:48:22Z

It makes me sad how we have all kinds of powerful abstractions in C++, but resort to silly boilerplate code

Don't forget that it is not true code, just a quick and dirty placeholder until we switch to OpenGL ES 2 and have a proper implementation of shaders for all platforms.

are those 15 lines in the header a real problem?

I like keeping public headers clean.

code readability

Nobody's supposed to "read" this code, it's empty.

Having a .cpp file of which the half are just empty dummy implementations is awful...

Would you feel better if we created a separate cpp file for the empty implementation, and leave the original Shader.cpp clean?

Bromeon · 2015-10-12T12:25:08Z

I like keeping public headers clean.

I don't see headers so much as the public interface, because they're in fact not. The documentation should be the primary source to learn about the API, headers are more tailored to developers/"power users" than average users. And as a developer, it may be more intuitive to see the same thing handled with less code, but it depends of course on the person.

Still, I totally understand your point, and generally I agree that it's a good idea to let .hpp files give a clean and concise overview of the class.

Would you feel better if we created a separate cpp file for the empty implementation, and leave the original Shader.cpp clean?

I thought about that too, but I don't think it's worth the special case for the configuration script. Handling it in code would require headers, and that's probably only more confusing. I'll just add the empty functions to the bottom of Shader.cpp 😉

binary1248 · 2015-10-12T12:33:03Z

Sooner or later, the backend implementations using the underlying GL objects will probably have to be split anyway since supporting multiple specifications (GL vs GLES) and multiple versions of the same specification (2.1 vs e.g. 4.5 which supports DSA) within a single file will become a complete mess. This is also one of the reasons why I favoured keeping UniformBinder isolated in the .cpp file: when using DSA it becomes completely unnecessary.

Bromeon · 2015-10-12T22:27:39Z

@binary1248 Okay, good to know. I think we should keep such things in a separate future PR though, there's also some optimization potential.

I applied the changes as discussed above: reintroduced ensureGlContext() and all the empty definitions for OpenGL ES.

Something to keep in mind: the API currently provides an implicit conversion from sf::Color to sf::Glsl::Vec4. While being convenient, this disables future conversions from colors to other GLSL types, most notably sf::Glsl::Ivec4 (or a potential sf::Glsl::Uvec4 that may be supported one day). Even though multiple conversions can coexist, the call setUniform(string, color) will be ambiguous, breaking code that relies on the implicit conversion.

It would be possible to add explicit conversions from sf::Color to other GLSL types. This however would not allow things like

sf::Glsl::Ivec4 colors[3] =
{
    sf::Color::Yellow,
    sf::Color::Cyan,
    sf::Color::Red,
};

So the question really boils down to whether the syntax

shader.setUniform("color", sf::Glsl::Vec4(color));

is such a big issue, that we desperately need

shader.setUniform("color", color);

at the cost of future limitations as outlined above.

To demonstrate the difference, I added another commit that implements implicit conversions from sf::Color to both sf::Glsl::Vec4 and sf::Glsl::Ivec4. I don't think it's inherently bad if the user explicitly specifies whether he needs a conversion to float or integer 4D vectors. It would also be more consistent to sf::Transform and sf::Glsl::Mat3/4:

sf::Color color;
shader.setUniform("color", sf::Glsl::Vec4(color));
shader.setUniform("color", sf::Glsl::Ivec4(color));

sf::Transform transform;
shader.setUniform("transform", sf::Glsl::Mat3(transform));
shader.setUniform("transform", sf::Glsl::Mat4(transform));

What do you think? Or is it unrealistic that people use other GLSL types than vec4 for colors?

binary1248 · 2015-10-12T23:21:02Z

Or is it unrealistic that people use other GLSL types than vec4 for colors?

In an sfml-graphics only context (not sf::Context 😉), it is probably unrealistic since users would not care about how colour data flows through the shader stages. They just accept the fact that colours are vec4s, even more so since SFML still supports legacy GL which provides gl_Color which is a vec4 itself. Just look at the example shaders.

If we are to stay somewhat future-proof, we should probably allow for other types as well. Already now, shaders can be used in a limited way as GL wrappers although I don't know whether people would actually bother using sf::Color in that case. In really advanced use cases, reducing the size of the colour attribute if you know your destination framebuffer only supports e.g. a 32-bit RGBA format (like SFML does with its FBO RenderTextures) might provide some sort of performance improvement.

Bromeon · 2015-10-14T09:25:01Z

Okay. So should I merge the two commits?
Other feedback?

binary1248 · 2015-10-17T19:58:38Z

So should I merge the two commits?

Sure.

I guess this looks fine now. Like I said, there is optimization potential, but I'll leave that for another time in the (hopefully not to distant) future. 😉

👍

Bromeon · 2015-10-21T08:01:00Z

Now that the corresponding PR has been merged, I introduced the SFML_DEPRECATED macro, and used SFML_DOXYGEN (from #961) to hide implementation details.

I also improved the documentation in several parts, and provided a usage example.

LaurentGomila · 2015-10-21T08:18:11Z

src/SFML/Graphics/Shader.cpp

+void Shader::setUniform(const std::string& name, const Glsl::Vec2& v)
+{
+    UniformBinder binder(*this, name);
+	if (binder.location != -1)


Tab spotted ;)

Same in all other overloads.

Fixed. I wonder why EditorConfig didn't prevent them... Maybe the VS plugin isn't perfect.

Implements a new design for the shader uniform API. * Added Shader::setUniform() and Shader::setUniformArray() overloads for the following types: -> scalars: float, int, bool -> vectors: 2D, 3D, 4D -> matrices: 3x3, 4x4 -> arrays of basic types -> samplers (sf::Texture) -> conversions for SFML types (sf::Transform, sf::Color) * Added sf::Glsl namespace with GLSL-equivalent types * Deprecated Shader::setParameter() overloads Other related changes: * Refactored sf::Shader internals to avoid code duplication * Improved documentation * Added SFML_DEPRECATED macro to Doxyfile * Defined _SCL_SECURE_NO_WARNINGS to disable std::copy() warnings on MSVC

eXpl0it3r · 2015-10-25T21:42:30Z

Anything left? Final reviews?

binary1248 · 2015-10-28T01:48:04Z

Looks good to me. I can't spot anything that I would want to comment on right now, but from what I can tell, this should serve as a good starting point for future work anyway. 👍

eXpl0it3r · 2015-10-29T08:50:40Z

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Bromeon added feature m:sfml-graphics labels Oct 5, 2015

Bromeon added this to the 2.4 milestone Oct 5, 2015

binary1248 reviewed Oct 8, 2015
View reviewed changes

Bromeon force-pushed the feature/shader_uniforms branch from df1c2f0 to 4025ac6 Compare October 12, 2015 07:24

Bromeon force-pushed the feature/shader_uniforms branch from 4025ac6 to 6e21ef6 Compare October 12, 2015 22:24

binary1248 assigned Bromeon Oct 14, 2015

Bromeon force-pushed the feature/shader_uniforms branch 2 times, most recently from af68756 to 98faba0 Compare October 21, 2015 07:53

LaurentGomila reviewed Oct 21, 2015
View reviewed changes

Bromeon force-pushed the feature/shader_uniforms branch from 98faba0 to 9c5c750 Compare October 21, 2015 08:29

eXpl0it3r merged commit 9c5c750 into master Nov 1, 2015

eXpl0it3r deleted the feature/shader_uniforms branch November 1, 2015 23:19

LaurentGomila mentioned this pull request Nov 2, 2015

Support more types in sf::Shader::setParameter #538

Closed

mantognini added the s:accepted label Aug 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New API to set shader uniforms #983

New API to set shader uniforms #983

Bromeon commented Oct 5, 2015

LaurentGomila commented Oct 5, 2015

Bromeon commented Oct 5, 2015

Bromeon commented Oct 5, 2015

mantognini commented Oct 5, 2015

Bromeon commented Oct 5, 2015

mantognini commented Oct 5, 2015

LaurentGomila commented Oct 6, 2015

Bromeon commented Oct 6, 2015

binary1248 Oct 8, 2015

Bromeon Oct 8, 2015

Bromeon commented Oct 12, 2015

LaurentGomila commented Oct 12, 2015

Bromeon commented Oct 12, 2015

LaurentGomila commented Oct 12, 2015

Bromeon commented Oct 12, 2015

binary1248 commented Oct 12, 2015

Bromeon commented Oct 12, 2015

binary1248 commented Oct 12, 2015

Bromeon commented Oct 14, 2015

binary1248 commented Oct 17, 2015

Bromeon commented Oct 21, 2015

LaurentGomila Oct 21, 2015

Bromeon Oct 21, 2015

eXpl0it3r commented Oct 25, 2015

binary1248 commented Oct 28, 2015

eXpl0it3r commented Oct 29, 2015

New API to set shader uniforms #983

New API to set shader uniforms #983

Conversation

Bromeon commented Oct 5, 2015

Design rationale

Try it out

LaurentGomila commented Oct 5, 2015

Bromeon commented Oct 5, 2015

Bromeon commented Oct 5, 2015

mantognini commented Oct 5, 2015

Bromeon commented Oct 5, 2015

mantognini commented Oct 5, 2015

LaurentGomila commented Oct 6, 2015

Bromeon commented Oct 6, 2015

binary1248 Oct 8, 2015

Choose a reason for hiding this comment

Bromeon Oct 8, 2015

Choose a reason for hiding this comment

Bromeon commented Oct 12, 2015

LaurentGomila commented Oct 12, 2015

Bromeon commented Oct 12, 2015

LaurentGomila commented Oct 12, 2015

Bromeon commented Oct 12, 2015

binary1248 commented Oct 12, 2015

Bromeon commented Oct 12, 2015

binary1248 commented Oct 12, 2015

Bromeon commented Oct 14, 2015

binary1248 commented Oct 17, 2015

Bromeon commented Oct 21, 2015

LaurentGomila Oct 21, 2015

Choose a reason for hiding this comment

Bromeon Oct 21, 2015

Choose a reason for hiding this comment

eXpl0it3r commented Oct 25, 2015

binary1248 commented Oct 28, 2015

eXpl0it3r commented Oct 29, 2015