I have a slight concern with part of the implementation of backend::Program
and backend::ProgramState
.
Please correct me if I’m wrong in my understanding of how the code works.
Right now when we create a Program
, we can only do so by providing the string source to both the vertex and fragment shaders via this method:
backend::ProgramCache::getInstance()->newProgram(vertexShader, fragmentShader);
This newProgram() function does the following:
backend::Program* ProgramCache::newProgram(const std::string& vertexShader, const std::string& fragmentShader)
{
auto key = std::hash<Shader>{}(Shader(vertexShader, fragmentShader));
const auto& iter = ProgramCache::_cachedPrograms.find(key);
if (ProgramCache::_cachedPrograms.end() != iter)
{
CC_SAFE_RETAIN(iter->second);
return iter->second;
}
auto program = backend::Device::getInstance()->newProgram(vertexShader, fragmentShader);
ProgramCache::_cachedPrograms.emplace(key, program);
return program;
}
So, it creates a hash from the vertex and fragment shader strings, and uses that as the lookup key for the _cachedPrograms std::unordered_map
.
Now, here are my concerns:
-
There is no way for developers to check if a Program
already exists, since there was no way to pass in a unique identifier.
-
In order to check if a Program
exists, we need to pass in the strings for the vertex and fragment shader into the “newProgram()” function. This results in 2 issues:
a. We are required to reload the shader code each time, and while it may not be an issue if it’s always stored in RAM (although it means it’s just taking up memory for no reason),
it may be an issue if we have to hit the internal device storage (using FileUtils etc) to load it every time we need to use it.
b. The std::hash is computed over the entire code of each of the 2 shaders, so the larger the code for the shader, the longer this process takes (I’m assuming that’s how the std::hash works).
-
There is no way to create a ProgramState
from a Program
directly, as the only way to create it is to pass in the source for both shaders:
ProgramState::ProgramState(const std::string& vertexShader, const std::string& fragmentShader)
{
_program = backend::ProgramCache::getInstance()->newProgram(vertexShader, fragmentShader);
CC_SAFE_RETAIN(_program);
init();
}
This ends up forcing the loading of the shaders, then the hashing of the strings, then the key lookup, every single time we want to create a ProgramState from a Program
that we know already exists in the ProgramCache
.
If you can re-assure me that the performance impact of this way of creating the Program and ProgramState is negligible in all scenarios, then that’s fine. If testing hasn’t been done to measure the potential impact on performance because of the loading and hashing of the strings (remember there are some large and complex shaders out there), then perhaps it’s a good idea to do this testing before the final release of Cocos2d-x v4.
Personally, I wouldn’t be bothered if we passed in our own unique identifier to use as the lookup key for the Program
, rather than using a hash for this purpose. Also, just an idea, there’s the option of using both a hash and a user defined key, stored in two lists:
std::unordered_map<string (user defined ID), Program*> _customIdProgramCache;
std::unordered_map<hash, Program*> _cachedPrograms;
That way we can call something like:
backend::Program* ProgramCache::newProgram(const std::string& programId, const std::string& vertexShader, const std::string& fragmentShader)
{
const auto& iter1 = ProgramCache::_customIdProgramCache.find(programId);
if (ProgramCache::_customIdProgramCache.end() != iter1)
{
CC_SAFE_RETAIN(iter1->second);
return iter1->second;
}
auto key = std::hash<Shader>{}(Shader(vertexShader, fragmentShader));
const auto& iter2 = ProgramCache::_cachedPrograms.find(key);
if (ProgramCache::_cachedPrograms.end() != iter2)
{
ProgramCache::_customIdProgramCache.emplace(programId, iter2->second); // if it doesn't exist in the this map, then add it
CC_SAFE_RETAIN(iter2->second);
return iter2->second;
}
auto program = backend::Device::getInstance()->newProgram(vertexShader, fragmentShader);
ProgramCache::_cachedPrograms.emplace(key, program);
ProgramCache::_customIdProgramCache.emplace(programId, program);
return program;
}
My reasoning here is that the majority of the time the developer will know exactly when they’ve created and added a new Program
, and what ID belongs to it, but in certain situations a different ID may be used for exactly the same vertex and fragment shader combination, so keeping the standard hash lookup would prevent creating another instance of an equivalent Program.
Another alternative (which is probably simpler) is to just return the hash result, and add the functionality for us to be able to check if it exists, along with a way to create the ProgramState*
from a Program*
, something like this:
backend::Program* ProgramCache::newProgram(const std::string& vertexShader, const std::string& fragmentShader, std::size_t* programIdResult = nullptr)
{
auto key = std::hash<Shader>{}(Shader(vertexShader, fragmentShader));
if (programIdResult)
{
*programIdResult = key;
}
const auto& iter = ProgramCache::_cachedPrograms.find(key);
if (ProgramCache::_cachedPrograms.end() != iter)
{
CC_SAFE_RETAIN(iter->second);
return iter->second;
}
auto program = backend::Device::getInstance()->newProgram(vertexShader, fragmentShader);
ProgramCache::_cachedPrograms.emplace(key, program);
return program;
}
Anyhow, my primary concern is the potential impact of the current way of handling the shaders, so I would appreciate any feedback regarding those points.