Incorrect output when using custom shaders about axmol

[rh101]

PR #2105 shows the expected output (with 4 draw calls):

[rh101]

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.

[smilediver]

I think it's a bug. You should not be required to call updateBatchId().

[rh101]

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]

@halx99 One other thing, is the hash generated by XXH64 guaranteed to be greater than the value of ProgramType::BUILTIN_COUNT?

Yes

[smilediver]

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.