From 8c231b6990f10906051b94b286f69975118e02c1 Mon Sep 17 00:00:00 2001 From: Stephen White Date: Sat, 17 Mar 2018 13:10:03 -0400 Subject: [PATCH] Fix a DrawElements bug in the Metal backend. The indexOffset of the draw was not being used. It must be included in the indexBufferOffset. Renamed indexBufferOffset -> indexBufferBaseOffset. Add a DrawElements test which exercises zero and non-zero index offsets. --- src/backend/metal/CommandBufferMTL.mm | 7 +- src/backend/opengl/CommandBufferGL.cpp | 8 +- src/tests/CMakeLists.txt | 1 + src/tests/end2end/DrawElementsTests.cpp | 137 ++++++++++++++++++++++++ 4 files changed, 146 insertions(+), 7 deletions(-) create mode 100644 src/tests/end2end/DrawElementsTests.cpp diff --git a/src/backend/metal/CommandBufferMTL.mm b/src/backend/metal/CommandBufferMTL.mm index 20608df53c..81d36b2660 100644 --- a/src/backend/metal/CommandBufferMTL.mm +++ b/src/backend/metal/CommandBufferMTL.mm @@ -167,7 +167,7 @@ namespace backend { namespace metal { ComputePipeline* lastComputePipeline = nullptr; RenderPipeline* lastRenderPipeline = nullptr; id indexBuffer = nil; - uint32_t indexBufferOffset = 0; + uint32_t indexBufferBaseOffset = 0; CurrentEncoders encoders; encoders.device = mDevice; @@ -304,6 +304,7 @@ namespace backend { namespace metal { case Command::DrawElements: { DrawElementsCmd* draw = mCommands.NextCommand(); + size_t formatSize = IndexFormatSize(lastRenderPipeline->GetIndexFormat()); ASSERT(encoders.render); [encoders.render @@ -311,7 +312,7 @@ namespace backend { namespace metal { indexCount:draw->indexCount indexType:lastRenderPipeline->GetMTLIndexType() indexBuffer:indexBuffer - indexBufferOffset:indexBufferOffset + indexBufferOffset:indexBufferBaseOffset + draw->firstIndex * formatSize instanceCount:draw->instanceCount baseVertex:0 baseInstance:draw->firstInstance]; @@ -526,7 +527,7 @@ namespace backend { namespace metal { SetIndexBufferCmd* cmd = mCommands.NextCommand(); auto b = ToBackend(cmd->buffer.Get()); indexBuffer = b->GetMTLBuffer(); - indexBufferOffset = cmd->offset; + indexBufferBaseOffset = cmd->offset; } break; case Command::SetVertexBuffers: { diff --git a/src/backend/opengl/CommandBufferGL.cpp b/src/backend/opengl/CommandBufferGL.cpp index 14cf7ec6df..73466d53a8 100644 --- a/src/backend/opengl/CommandBufferGL.cpp +++ b/src/backend/opengl/CommandBufferGL.cpp @@ -243,7 +243,7 @@ namespace backend { namespace opengl { PipelineBase* lastPipeline = nullptr; PipelineGL* lastGLPipeline = nullptr; RenderPipeline* lastRenderPipeline = nullptr; - uint32_t indexBufferOffset = 0; + uint32_t indexBufferBaseOffset = 0; PersistentPipelineState persistentPipelineState; persistentPipelineState.SetDefaultState(); @@ -505,7 +505,7 @@ namespace backend { namespace opengl { lastRenderPipeline->GetGLPrimitiveTopology(), draw->indexCount, formatType, reinterpret_cast(draw->firstIndex * formatSize + - indexBufferOffset), + indexBufferBaseOffset), draw->instanceCount, draw->firstInstance); } else { // This branch is only needed on OpenGL < 4.2 @@ -513,7 +513,7 @@ namespace backend { namespace opengl { lastRenderPipeline->GetGLPrimitiveTopology(), draw->indexCount, formatType, reinterpret_cast(draw->firstIndex * formatSize + - indexBufferOffset), + indexBufferBaseOffset), draw->instanceCount); } } break; @@ -635,7 +635,7 @@ namespace backend { namespace opengl { case Command::SetIndexBuffer: { SetIndexBufferCmd* cmd = mCommands.NextCommand(); - indexBufferOffset = cmd->offset; + indexBufferBaseOffset = cmd->offset; inputBuffers.OnSetIndexBuffer(cmd->buffer.Get()); } break; diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt index d61bb51737..72206e1a5f 100644 --- a/src/tests/CMakeLists.txt +++ b/src/tests/CMakeLists.txt @@ -75,6 +75,7 @@ add_executable(nxt_end2end_tests ${END2END_TESTS_DIR}/BufferTests.cpp ${END2END_TESTS_DIR}/BlendStateTests.cpp ${END2END_TESTS_DIR}/CopyTests.cpp + ${END2END_TESTS_DIR}/DrawElementsTests.cpp ${END2END_TESTS_DIR}/DepthStencilStateTests.cpp ${END2END_TESTS_DIR}/IndexFormatTests.cpp ${END2END_TESTS_DIR}/InputStateTests.cpp diff --git a/src/tests/end2end/DrawElementsTests.cpp b/src/tests/end2end/DrawElementsTests.cpp new file mode 100644 index 0000000000..5e2d5b727a --- /dev/null +++ b/src/tests/end2end/DrawElementsTests.cpp @@ -0,0 +1,137 @@ +// Copyright 2018 The NXT Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "tests/NXTTest.h" + +#include "utils/NXTHelpers.h" + +constexpr uint32_t kRTSize = 4; + +class DrawElementsTest : public NXTTest { + protected: + void SetUp() override { + NXTTest::SetUp(); + + renderpass = device.CreateRenderPassBuilder() + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + .AttachmentSetColorLoadOp(0, nxt::LoadOp::Clear) + .SetSubpassCount(1) + .SubpassSetColorAttachment(0, 0, 0) + .GetResult(); + + renderTarget = device.CreateTextureBuilder() + .SetDimension(nxt::TextureDimension::e2D) + .SetExtent(kRTSize, kRTSize, 1) + .SetFormat(nxt::TextureFormat::R8G8B8A8Unorm) + .SetMipLevels(1) + .SetAllowedUsage(nxt::TextureUsageBit::OutputAttachment | nxt::TextureUsageBit::TransferSrc) + .SetInitialUsage(nxt::TextureUsageBit::OutputAttachment) + .GetResult(); + + renderTargetView = renderTarget.CreateTextureViewBuilder().GetResult(); + + framebuffer = device.CreateFramebufferBuilder() + .SetRenderPass(renderpass) + .SetAttachment(0, renderTargetView) + .SetDimensions(kRTSize, kRTSize) + .GetResult(); + + nxt::InputState inputState = device.CreateInputStateBuilder() + .SetInput(0, 4 * sizeof(float), nxt::InputStepMode::Vertex) + .SetAttribute(0, 0, nxt::VertexFormat::FloatR32G32B32A32, 0) + .GetResult(); + + nxt::ShaderModule vsModule = utils::CreateShaderModule(device, nxt::ShaderStage::Vertex, R"( + #version 450 + layout(location = 0) in vec4 pos; + void main() { + gl_Position = pos; + })" + ); + + nxt::ShaderModule fsModule = utils::CreateShaderModule(device, nxt::ShaderStage::Fragment, R"( + #version 450 + layout(location = 0) out vec4 fragColor; + void main() { + fragColor = vec4(0.0, 1.0, 0.0, 1.0); + })" + ); + + pipeline = device.CreateRenderPipelineBuilder() + .SetSubpass(renderpass, 0) + .SetPrimitiveTopology(nxt::PrimitiveTopology::TriangleStrip) + .SetStage(nxt::ShaderStage::Vertex, vsModule, "main") + .SetStage(nxt::ShaderStage::Fragment, fsModule, "main") + .SetIndexFormat(nxt::IndexFormat::Uint32) + .SetInputState(inputState) + .GetResult(); + + vertexBuffer = utils::CreateFrozenBufferFromData(device, nxt::BufferUsageBit::Vertex, { + -1.0f, -1.0f, 0.0f, 1.0f, + 1.0f, 1.0f, 0.0f, 1.0f, + -1.0f, 1.0f, 0.0f, 1.0f, + 1.0f, -1.0f, 0.0f, 1.0f + }); + indexBuffer = utils::CreateFrozenBufferFromData(device, nxt::BufferUsageBit::Index, { + 0, 1, 2, 0, 3, 1 + }); + } + + nxt::RenderPass renderpass; + nxt::Texture renderTarget; + nxt::TextureView renderTargetView; + nxt::Framebuffer framebuffer; + nxt::RenderPipeline pipeline; + nxt::Buffer vertexBuffer; + nxt::Buffer indexBuffer; + + void Test(uint32_t indexCount, uint32_t instanceCount, uint32_t firstIndex, + uint32_t firstInstance, RGBA8 bottomLeftExpected, RGBA8 topRightExpected) { + uint32_t zeroOffset = 0; + nxt::CommandBuffer commands = device.CreateCommandBufferBuilder() + .BeginRenderPass(renderpass, framebuffer) + .BeginRenderSubpass() + .SetRenderPipeline(pipeline) + .SetVertexBuffers(0, 1, &vertexBuffer, &zeroOffset) + .SetIndexBuffer(indexBuffer, 0) + .DrawElements(indexCount, instanceCount, firstIndex, firstInstance) + .EndRenderSubpass() + .EndRenderPass() + .GetResult(); + + queue.Submit(1, &commands); + + EXPECT_PIXEL_RGBA8_EQ(bottomLeftExpected, renderTarget, 1, 3); + EXPECT_PIXEL_RGBA8_EQ(topRightExpected, renderTarget, 3, 1); + } +}; + +// The most basic DrawElements triangle draw. +TEST_P(DrawElementsTest, Uint32) { + + RGBA8 filled(0, 255, 0, 255); + RGBA8 notFilled(0, 0, 0, 0); + + // Test a draw with no indices. + Test(0, 0, 0, 0, notFilled, notFilled); + // Test a draw with only the first 3 indices (bottom left triangle) + Test(3, 1, 0, 0, filled, notFilled); + // Test a draw with only the last 3 indices (top right triangle) + Test(3, 1, 3, 0, notFilled, filled); + // Test a draw with all 6 indices (both triangles). + Test(6, 1, 0, 0, filled, filled); +} + +NXT_INSTANTIATE_TEST(DrawElementsTest, MetalBackend, OpenGLBackend, D3D12Backend, VulkanBackend)