cginternals / globjects

C++ library strictly wrapping OpenGL objects.
https://globjects.org
MIT License
539 stars 58 forks source link

Reloading shader triggers numerous changed() and does not work #369

Open X-Ryl669 opened 6 years ago

X-Ryl669 commented 6 years ago

I've a shader whose source is a File (obtained via Shader::sourceFromFile). If I want to update it, I'm calling File::reload. This does not work. The sequence of operation that this causes is this:

  1. reload call changed (once),
  2. This calls Shader::notifyChanged that calls updateSource
  3. This calls File::string and this triggers File::loadFromFileContent (so far, so good)
  4. This then calls Shader::invalidate that sets compile flags to false
  5. This calls Shader::changed and this triggers Program::invalidate
  6. This set the dirty flag

[Later on, code that use program does this:]

  1. In Program::use it's calling checkDirty (which is true)
  2. It calls Program::link that's not compiling the shaders (but re-linking old compiled shaders)

Solution: Compile shaders when the program is dirty (seems logical, not all user have a NVidia card that compile shaders when told to link them). Please notice that Program::compileAttachedShaders is protected and thus, not callable from user code. We can still call Shader::compile but this forces to keep a pointer on the shader (and that's exactly what Program is now doing, right?)

Also, I think there is too much abuse of the ChangeListener pattern here so the system is convoluted with many boolean to keep track of states. It would be so much easier if the Program was a listener of the Shader's AbstractStringSource directly, since the double listener triggering is useless in that case. The only thing that can be modified in a Shader is its source. So why should they be in the Listener callchain instead of the source itself? Typically, Shader::updateSource or invalidate is misleading, since it does not cause reloading the source's text from the AbstractStringSource.

BTW, the error return is not checked in Shader::updateSource implementation, thus any error in the shader code will get silenced is using reload feature (at ShadingLanguageIncludeImplementation_ShadingLanguageIncludeARB.cpp line 26). I guess you should check it explicitely in updateSources and throw a message upon error. Or, if the Program is compiling the shader automatically, you'll get the error at that time, that's a good workaround too.