Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
X-Ryl669 opened this issue May 16, 2018 · 0 comments
Open

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

X-Ryl669 opened this issue May 16, 2018 · 0 comments
Labels

Comments

@X-Ryl669
Copy link

X-Ryl669 commented May 16, 2018

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.

@cgcostume cgcostume added the bug label Oct 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants