Closed rh101 closed 2 months ago
PR #2105 shows the expected output (with 4 draw calls):
It seems that once a uniform value is updated, ProgramState::updateBatchId()
needs to be called in order to update the batch ID, and that in turn fixes the issue. This also ensures that same-value uniforms are batched together.
I think it's a bug. You should not be required to call updateBatchId()
.
The batch ID is equal to the program ID if it is using one of the built in shaders:
bool ProgramState::init(Program* program)
{
...
const auto programId = program->getProgramId();
if (programId < ProgramType::BUILTIN_COUNT)
this->_batchId = programId
...
}
The problem is when we do not use any of the built-in set of shaders; what does the batch ID get set to? Right now, it's set to a hash value. which is calculated via this:
void ProgramState::updateBatchId()
{
_batchId = XXH64(_uniformBuffers.data(), _uniformBuffers.size(), _program->getProgramId());
}
This is an expensive call, and should only be made if and only if a custom shader is being used, and the uniform data has changed. The only way to set this would be to add a call in the setUniform
method:
void ProgramState::setUniform(const backend::UniformLocation& uniformLocation, const void* data, std::size_t size)
{
if (uniformLocation.vertStage)
setVertexUniform(uniformLocation.vertStage.location, data, size, uniformLocation.vertStage.offset);
#ifdef AX_USE_METAL
if (uniformLocation.fragStage)
setFragmentUniform(uniformLocation.fragStage.location, data, size, uniformLocation.fragStage.offset);
#endif
const auto programId = program->getProgramId();
if (programId >= ProgramType::BUILTIN_COUNT)
updateBatchId();
}
In this case we're adding extra logic that should only ever apply when using custom shaders, and I'm not sure how many developers would be using their own. Would this be a worthwhile change to ProgramState::setUniform
?
Also, the change above doesn't check if the uniform data has changed, and only relies on the program using custom shaders.
@halx99 One other thing, is the hash generated by XXH64
guaranteed to be greater than the value of ProgramType::BUILTIN_COUNT
?
@halx99 One other thing, is the hash generated by
XXH64
guaranteed to be greater than the value ofProgramType::BUILTIN_COUNT
?
Yes
Builtin shaders can also have different uniforms, so there will be situations when they can't be batched either. I don't see why builtin shaders should have different logic than user shaders. And now I have suspicion that builtin shader batching is working by chance if their uniforms are ignored.
Maybe this should be solved with a dirty flag set when modifying uniforms and a possibility to trigger update manually by using ProgramState::updateBatchId()
, so you can do it after updating uniforms, while the uniform buffers are still in cache. But otherwise, it should be automatic.
Expected:
Actual:
The sprites are added in order from top left to bottom right, so they should be Red, Green, Blue, Orange. As can be seen in the actual output, the orange color is the last one set, so is being applied to all sprites. The number of GL calls is 1, which means the sprites are being batched, when they should not be. The correct GL calls should be 4.
This was working in previous versions of Axmol, and I think it may have changed after this commit : https://github.com/axmolengine/axmol/commit/2d0a63660fe082f2f797702db807d2169901a6c2
I think I have an idea on how to fix it, so should be able to get a PR up soon.