ConfettiFX / The-Forge

The Forge Cross-Platform Rendering Framework PC Windows, Steamdeck (native), Ray Tracing, macOS / iOS, Android, XBOX, PS4, PS5, Switch, Quest 2
Apache License 2.0
4.71k stars 493 forks source link

VERTEX_ATTRIB_RATE_INSTANCE ignored on macOS 10.12, iOS 10.0 #183

Closed Takarashy-URender closed 3 years ago

Takarashy-URender commented 3 years ago

I found an interesting bug with the MetalRednerer.mm @ 3149-3165 I ported an Vulcan Instancing example and whatever I did the instance vertex attributes were not properly updating. After digging in The-Forge code, I found out that even though I'm not using any tessellation and my vertex attribute is set to VERTEX_ATTRIB_RATE_INSTANCE, the if/else (MetalRednerer.mm @ 3158) is never executed.

For the fix, it's obvious the else is skipped since we enter the @available section on Mac10.12 or iOS10, so I have extracted the latter if/else into a function to avoid duplication of code, since it has to be checked in either case. Here the proposed solution:

// Extracted function

void setVertexBufferLayoutStepFunction(MTLRenderPipelineDescriptor* renderPipelineDesc, uint32_t bufferIndex, VertexAttribRate rate)
{
    ASSERT(renderPipelineDesc);

    if(rate == VERTEX_ATTRIB_RATE_INSTANCE)
    {
        renderPipelineDesc.vertexDescriptor.layouts[bufferIndex].stepFunction = MTLVertexStepFunctionPerInstance;
    }
    else
    {
        renderPipelineDesc.vertexDescriptor.layouts[bufferIndex].stepFunction = MTLVertexStepFunctionPerVertex;
    }
}

// Changed code in void addGraphicsPipelineImpl(Renderer pRenderer, const GraphicsPipelineDesc pDesc, Pipeline** ppPipeline)

#if defined(ENABLE_TESSELLATION)
            if (@available(macOS 10.12, iOS 10.0, *))
            {
                if (pPipeline->pShader->mtlVertexShader.patchType != MTLPatchTypeNone)
                {
                    renderPipelineDesc.vertexDescriptor.layouts[bufferIndex].stepFunction = MTLVertexStepFunctionPerPatchControlPoint;
                }
                else
                {
                    setVertexBufferLayoutStepFunction(renderPipelineDesc, bufferIndex, attrib->mRate);
                }
            }
            else
#endif
            setVertexBufferLayoutStepFunction(renderPipelineDesc, bufferIndex, attrib->mRate);
manas-kulkarni commented 3 years ago

Good catch. We will fix it in the next release. Thanks

Takarashy-URender commented 3 years ago

Great thanks a lot 👍

wolfgangfengel commented 3 years ago

This should be resolved in this release.