cginternals / cginternals/globjects

Reloading shader triggers numerous changed() and does not work

Open
#369 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
C++
Stars
563
Forks
59
PR merge metrics
No merged PRs in 30d

Description

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.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.