Vulkan: Lineloops edge base bugfix and new tests

src/libANGLE/renderer/vulkan/VertexArrayVk.cpp

@@ -367,7 +367,6 @@ gl::Error VertexArrayVk::drawArrays(const gl::Context *context,
     }
 
     // Handle GL_LINE_LOOP drawArrays.
-    // This test may be incorrect if the draw call switches from DrawArrays/DrawElements.
     int lastVertex = drawCallParams.firstVertex() + drawCallParams.vertexCount();
     if (!mLineLoopBufferFirstIndex.valid() || !mLineLoopBufferLastIndex.valid() ||
         mLineLoopBufferFirstIndex != drawCallParams.firstVertex() ||
@@ -417,7 +416,6 @@ gl::Error VertexArrayVk::drawElements(const gl::Context *context,
 
     VkIndexType indexType = gl_vk::GetIndexType(drawCallParams.type());
 
-    // This also doesn't check if the element type changed, which should trigger translation.
     if (mDirtyLineLoopTranslation)
     {
         ANGLE_TRY(mLineLoopHelper.getIndexBufferForElementArrayBuffer(
@@ -469,6 +467,11 @@ gl::Error VertexArrayVk::onDraw(const gl::Context *context,
 
         // This forces the binding to happen if we follow a drawElement call from a drawArrays call.
         mIndexBufferDirty = true;
+
+        // If we've had a drawElements call with a line loop before, we want to make sure this is
+        // invalidated the next time drawElements is called since we use the same index buffer for
+        // both calls.
+        mDirtyLineLoopTranslation = true;
     }
 
     return gl::NoError();
@@ -507,6 +510,12 @@ gl::Error VertexArrayVk::onIndexedDraw(const gl::Context *context,
                                        gl_vk::GetIndexType(drawCallParams.type()));
         updateElementArrayBufferReadDependency(drawNode, renderer->getCurrentQueueSerial());
         mIndexBufferDirty = false;
+
+        // If we've had a drawArrays call with a line loop before, we want to make sure this is
+        // invalidated the next time drawArrays is called since we use the same index buffer for
+        // both calls.
+        mLineLoopBufferFirstIndex.reset();
+        mLineLoopBufferLastIndex.reset();
     }
 
     return gl::NoError();

src/libANGLE/renderer/vulkan/vk_helpers.cpp

@@ -303,7 +303,12 @@ Error DynamicDescriptorPool::allocateNewPool(const VkDevice &device)
 LineLoopHelper::LineLoopHelper()
     : mDynamicIndexBuffer(kLineLoopDynamicBufferUsage, kLineLoopDynamicBufferMinSize)
 {
-    mDynamicIndexBuffer.init(1);
+    // We need to use an alignment of the maximum size we're going to allocate, which is
+    // VK_INDEX_TYPE_UINT32. When we switch from a drawElement to a drawArray call, the allocations
+    // can vary in size. According to the Vulkan spec, when calling vkCmdBindIndexBuffer: 'The
+    // sum of offset and the address of the range of VkDeviceMemory object that is backing buffer,
+    // must be a multiple of the type indicated by indexType'.
+    mDynamicIndexBuffer.init(sizeof(uint32_t));
 }
 
 LineLoopHelper::~LineLoopHelper() = default;

src/tests/gl_tests/StateChangeTest.cpp

@@ -958,6 +958,135 @@ void main()
     glUnmapBuffer(GL_TRANSFORM_FEEDBACK_BUFFER);
 }
 
+// Simple state change tests for line loop drawing. There is some very specific handling of line
+// line loops in Vulkan and we need to test switching between drawElements and drawArrays calls to
+// validate every edge cases.
+class LineLoopStateChangeTest : public StateChangeTest
+{
+  protected:
+    LineLoopStateChangeTest()
+    {
+        setWindowWidth(32);
+        setWindowHeight(32);
+        setConfigRedBits(8);
+        setConfigGreenBits(8);
+        setConfigBlueBits(8);
+        setConfigAlphaBits(8);
+    }
+
+    void validateSquareAndHourglass()
+    {
+        ASSERT_GL_NO_ERROR();
+
+        int quarterWidth  = getWindowWidth() / 4;
+        int quarterHeight = getWindowHeight() / 4;
+
+        // Bottom left
+        EXPECT_PIXEL_COLOR_EQ(quarterWidth, quarterHeight, GLColor::green);
+
+        // Top left
+        EXPECT_PIXEL_COLOR_EQ(quarterWidth, (quarterHeight * 3), GLColor::green);
+
+        // Top right
+        // The last pixel isn't filled on a line loop so we check the pixel right before.
+        EXPECT_PIXEL_COLOR_EQ((quarterWidth * 3), (quarterHeight * 3) - 1, GLColor::green);
+
+        // dead center to validate the hourglass.
+        EXPECT_PIXEL_COLOR_EQ((quarterWidth * 2), quarterHeight * 2, GLColor::green);
+
+        // Verify line is closed between the 2 last vertices
+        EXPECT_PIXEL_COLOR_EQ((quarterWidth * 2), quarterHeight, GLColor::green);
+    }
+
+    GLint mPositionLocation;
+};
+
+// Draw an hourglass with a drawElements call followed by a square with drawArrays.
+TEST_P(LineLoopStateChangeTest, DrawElementsThenDrawArrays)
+{
+    ANGLE_GL_PROGRAM(program, kBasicVertexShader, kGreenFragmentShader);
+    glUseProgram(program);
+
+    // We expect to draw a square with these 4 vertices with a drawArray call.
+    auto pixelPoints =
+        std::vector<Vector2>{{8.5f, 8.5f}, {8.5f, 24.5f}, {24.5f, 8.5f}, {24.5f, 24.5f}};
+
+    auto vertices = std::vector<Vector3>();
+    for (Vector2 pixelPoint : pixelPoints)
+    {
+        vertices.emplace_back(Vector3(pixelPoint[0] * 2 / getWindowWidth() - 1,
+                                      pixelPoint[1] * 2 / getWindowHeight() - 1, 0.0f));
+    }
+
+    // If we use these indices to draw however, we should be drawing an hourglass.
+    auto indices = std::vector<GLushort>{0, 2, 1, 3};
+
+    mPositionLocation = glGetAttribLocation(program, "position");
+    ASSERT_NE(-1, mPositionLocation);
+
+    GLBuffer vertexBuffer;
+    glBindBuffer(GL_ARRAY_BUFFER, vertexBuffer);
+    glBufferData(GL_ARRAY_BUFFER, sizeof(vertices[0]) * vertices.size(), vertices.data(),
+                 GL_STATIC_DRAW);
+
+    GLBuffer indexBuffer;
+    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, indexBuffer);
+    glBufferData(GL_ELEMENT_ARRAY_BUFFER, indices.size() * sizeof(GLushort), &indices[0],
+                 GL_STATIC_DRAW);
+
+    glVertexAttribPointer(mPositionLocation, 3, GL_FLOAT, GL_FALSE, 0, nullptr);
+    glEnableVertexAttribArray(mPositionLocation);
+    glClear(GL_COLOR_BUFFER_BIT);
+    glDrawElements(GL_LINE_LOOP, 4, GL_UNSIGNED_SHORT, nullptr);  // hourglass
+    glDrawArrays(GL_LINE_LOOP, 0, 4);                             // square
+    glDisableVertexAttribArray(mPositionLocation);
+
+    validateSquareAndHourglass();
+}
+
+// Draw line loop using a drawArrays followed by an hourglass with drawElements.
+TEST_P(LineLoopStateChangeTest, DrawArraysThenDrawElements)
+{
+    ANGLE_GL_PROGRAM(program, kBasicVertexShader, kGreenFragmentShader);
+    glUseProgram(program);
+
+    // We expect to draw a square with these 4 vertices with a drawArray call.
+    auto pixelPoints =
+        std::vector<Vector2>{{8.5f, 8.5f}, {8.5f, 24.5f}, {24.5f, 8.5f}, {24.5f, 24.5f}};
+
+    auto vertices = std::vector<Vector3>();
+    for (Vector2 pixelPoint : pixelPoints)
+    {
+        vertices.emplace_back(Vector3(pixelPoint[0] * 2 / getWindowWidth() - 1,
+                                      pixelPoint[1] * 2 / getWindowHeight() - 1, 0.0f));
+    }
+
+    // If we use these indices to draw however, we should be drawing an hourglass.
+    auto indices = std::vector<GLushort>{0, 2, 1, 3};
+
+    mPositionLocation = glGetAttribLocation(program, "position");
+    ASSERT_NE(-1, mPositionLocation);
+
+    GLBuffer vertexBuffer;
+    glBindBuffer(GL_ARRAY_BUFFER, vertexBuffer);
+    glBufferData(GL_ARRAY_BUFFER, sizeof(vertices[0]) * vertices.size(), vertices.data(),
+                 GL_STATIC_DRAW);
+
+    GLBuffer indexBuffer;
+    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, indexBuffer);
+    glBufferData(GL_ELEMENT_ARRAY_BUFFER, indices.size() * sizeof(GLushort), &indices[0],
+                 GL_STATIC_DRAW);
+
+    glVertexAttribPointer(mPositionLocation, 3, GL_FLOAT, GL_FALSE, 0, nullptr);
+    glEnableVertexAttribArray(mPositionLocation);
+    glClear(GL_COLOR_BUFFER_BIT);
+    glDrawArrays(GL_LINE_LOOP, 0, 4);                             // square
+    glDrawElements(GL_LINE_LOOP, 4, GL_UNSIGNED_SHORT, nullptr);  // hourglass
+    glDisableVertexAttribArray(mPositionLocation);
+
+    validateSquareAndHourglass();
+}
+
 // Simple state change tests, primarily focused on basic object lifetime and dependency management
 // with back-ends that don't support that automatically (i.e. Vulkan).
 class SimpleStateChangeTest : public ANGLETest
@@ -1022,7 +1151,7 @@ void SimpleStateChangeTest::simpleDrawWithColor(const GLColor &color)
 
 // Test that we can do a drawElements call successfully after making a drawArrays call in the same
 // frame.
-TEST_P(SimpleStateChangeTest, DrawArraysThenDrawElement)
+TEST_P(SimpleStateChangeTest, DrawArraysThenDrawElements)
 {
     ANGLE_GL_PROGRAM(program, kBasicVertexShader, kGreenFragmentShader);
     glUseProgram(program);
@@ -1643,6 +1772,11 @@ void main() {
 }  // anonymous namespace
 
 ANGLE_INSTANTIATE_TEST(StateChangeTest, ES2_D3D9(), ES2_D3D11(), ES2_OPENGL());
+ANGLE_INSTANTIATE_TEST(LineLoopStateChangeTest,
+                       ES2_D3D9(),
+                       ES2_D3D11(),
+                       ES2_OPENGL(),
+                       ES2_VULKAN());
 ANGLE_INSTANTIATE_TEST(StateChangeRenderTest,
                        ES2_D3D9(),
                        ES2_D3D11(),