matus-chochlik / oglplus

OGLplus is a collection of open-source, cross-platform libraries which implement an object-oriented facade over the OpenGL® (version 3 and higher) and also OpenAL® (version 1.1) and EGL (version 1.4) C-language APIs. It provides wrappers which automate resource and object management and make the use of these libraries in C++ safer and more convenient.
http://oglplus.org/
Boost Software License 1.0
491 stars 72 forks source link

Question About Optional<> Wrapper #80

Closed malamanteau closed 9 years ago

malamanteau commented 10 years ago

Hi,

I have a concern with the oglplus::Optional<> wrapper (or rather the use without) for oglplus::Uniforms.

Drivers frequently optimize out unneeded uniforms, and these optimizations are always getting smarter. Couldn't having an oglplus::Uniform WITHOUT the oglplus::Optional<> wrapper run the risk of crashing/excepting an application in a driver-optimization-dependent way?

Thanks, Greg

matus-chochlik commented 10 years ago

Hi,

this is by design, if you construct an Uniform<T> referencing an inactive uniform in a GLSL program then the construction should fail and throw. If you don't want this behaviour you can either use Optional<Uniform<T>> or use the constructor of Uniform that allows to explicitly say whether to throw if the uniform is inactive:

Program prog;
...
Uniform<GLfloat> u1(prog, "Unused", false); // <- does not throw if "Unused" is not an active uniform
Uniform<GLfloat> u2(prog, "Unused", true); // <- does throw if "Unused" is not an active uniform
malamanteau commented 10 years ago

I guess the root of my question here is -- when would you actually want for the program to throw if a uniform has been optimized out? I can't really contrive a situation when that would ever be desired (much less the expected default behavior).

It seems to me that the most common use case is going to be non-throwing uniforms, and then in rare cases you'd want to use something like an oglplus::Required<> wrapper or something like that in the event that you'd actually want an inactive uniform to throw.

matus-chochlik commented 10 years ago

Well if an uniform is optimized out (i.e. not used or if it actually is not even declared in the shaders) then the code running on the CPU referencing it is making incorrect assumptions about what's going on on the GPU which generally is IMHO an error in the program logic.

If the CPU program assumes that the currently active GPU program works for example with a ProjectionMatrix a CameraMatrix or whatever other uniforms, and the GPU program actually isn't then the application isn't written very well. There are valid situations where some of the uniforms may be unused, but so far in my experience they are the minority not the majority of uniforms in a typical program.

regnirpsj commented 10 years ago

hi matus,

shouldn't

Program prog;
...
Uniform<GLfloat> u1(prog, "Unused", false); // <- does not throw if "Unused" is not an active uniform
Uniform<GLfloat> u2(prog, "Unused", true); // <- does throw if "Unused" is not an active uniform

be rather

Program prog;
...
Uniform<GLfloat> u1(prog, "Unused", std::nothrow); // <- does not throw if "Unused" is not an active uniform
Uniform<GLfloat> u2(prog, "Unused"); // <- does throw if "Unused" is not an active uniform

that is, the declarations for the ctor should be

Uniform<T>::Uniform(Program, std::string const&, std::nothrow_t);
Uniform<T>::Uniform(Program, std::string const&);
matus-chochlik commented 10 years ago

This is a nice idea :) I'll probably add another overload for nothrow. But using the bool has one important advantage - it allows you to decide whether to throw or not at run-time, so I'll probably keep the other constructor too.

malamanteau commented 10 years ago

I guess the other reason I might advocate making Uniforms optional by default would be due to the way I usually end up having to debug shaders:

Also, my experience with (particularly NVidia) drivers has been that sometimes it will do wild optimizations that you'd never expect, and those optimizations may vary from driver to driver.

Anyway, just my 2 cents. Since both behaviors are available, I'm not particular about the default, but I did want to point out the potential risks of throwing on inactive uniforms.

matus-chochlik commented 10 years ago

I see your point, I actually end up doing the same things that you describe (using fragment shader output for debugging) very often and this is the main reason why I recently added the constructor that allows to suppress throwing by setting the bool parameter. This way you can change the behaviour of many uniforms by setting a single variable.

But, on the other hand the main reason for the current behaviour (constructors throwing by default) is actually also debugging :) If you inadvertently try to use an uniform that is inactive, then it is better to throw in the constructor because there you have access to the identifier of the uniform (or a vertex attribute for that matter). If you let it silently construct in a nil state and later try tu use it then one of the glUniform / glProgramUniform, etc. functions fails and you are left guessing which uniform / VA was causing the problem since you don't know its name anymore only that it is uninitialized.