Render issue on cocos2d-x-3.17

direct3d9 · June 19, 2018, 8:06am

After upgrading to cocos-2d-x 3.17 from 3.14.1 graphics issue (flickering artifacts) was encountered on some android devices (LG G Pad 8.3 OS Android 5.0).
Probably this issue appeared on cocos-2d-x-3.15 or cocos-2d-x-3.16.

Screenshots attached

yinjimmy · June 20, 2018, 1:59am

It seems shader issue.

direct3d9 · June 21, 2018, 11:27am

I got the LG G Pad 8.3 with Android 4.4.2 and I was able to reproduce this bug. I figured out that bug appears with DrawNode usage.

direct3d9 · June 21, 2018, 2:37pm

On LG G Pad 8.3 Configuration::getInstance()->supportsShareableVAO() returns true
Disabling using of VAO in DrawNode fixes this issue.

CrazyHappyGame · June 21, 2018, 3:07pm

Could you create issue with simple test case how to reproduce it?

direct3d9 · June 21, 2018, 4:14pm

I tested the “Node: Draw” test cases. All tests worked fine on LG G Pad 8.3. I will try to reproduce this issue in cpp-tests project.

direct3d9 · May 8, 2019, 2:46pm

I read cocos2d-x code and found some issues.

CCDrawNode.cpp:315 (DrawNode::onDraw(), DrawNode::onDrawGLLine(), DrawNode::onDrawGLPoint()):
This code updates array buffer IN ANOTHER VAO, because it executing before GL::bindVAO(_vao);

if (_dirty)
    {
        glBindBuffer(GL_ARRAY_BUFFER, _vbo);
        glBufferData(GL_ARRAY_BUFFER, sizeof(V2F_C4B_T2F)*_bufferCapacity, _buffer, GL_STREAM_DRAW);
        
        _dirty = false;
    }

When I moved this code after GL::bindVAO(_vao); flickering issues on LG G Pad 8.3 OS gone away!

CCVertexIndxBuffer.cpp:225 (void IndexBuffer::recreateVBO() const)
This is index buffer (element array buffer), but GL_ARRAY_BUFFER constant used instead GL_ELEMENT_ARRAY_BUFFER
CCMaterial.cpp:175 (bool Material::parseTechnique(Properties* techniqueProperties))

else if (strcmp(name, "renderState") == 0)
       {
         //why parseRenderState(this, space), not parseRenderState(technique, space);?
           parseRenderState(this, space);
       }

Technique pass is RenderState subclass and can have own renderState block

R101 · May 9, 2019, 4:41am

Unfortunately I don’t know enough about the workings of OpenGL, so I can’t comment, but in case they are bugs in the code, then would you please create a separate issue in github for each of those items?

kerryk · May 10, 2019, 6:14am

@zhangxm seems that this bug will interest you.

zhangxm · May 10, 2019, 7:20am

@direct3d9

i am not sure if it is a problem, because the buffer is referred by VAO, so i think it will be detected by the correct VAO. As it is described in the wiki

Note that VAOs do not copy, freeze or store the contents of the referenced buffers - if you change any of the data in the buffers referenced by an existing VAO, those changes will be seen by users of the VAO.

Yep, i think you are right. Good catch. Will you please send a pull request to fix it?
I agree with you. I don’t know why it does like this.

R101 · May 10, 2019, 12:52pm

zhangxm:

i am not sure if it is a problem, because the buffer is referred by VAO, so i think it will be detected by the correct VAO. As it is described in the wiki
Note that VAOs do not copy, freeze or store the contents of the referenced buffers - if you change any of the data in the buffers referenced by an existing VAO, those changes will be seen by users of the VAO.

After reading that line, I am interpreting it differently, specifically the part about:

...if you change any of the data in the buffers referenced by an existing VAO...

In the current implementation, the buffer is created prior to the binding of the VAO, so it’s not yet referenced by the VAO:

if (_dirty)
{
    glBindBuffer(GL_ARRAY_BUFFER, _vbo);
    glBufferData(GL_ARRAY_BUFFER, sizeof(V2F_C4B_T2F) * _bufferCapacity, _buffer, GL_STREAM_DRAW);

    _dirty = false;
}

if (Configuration::getInstance()->supportsShareableVAO())
{
    GL::bindVAO(_vao);
}
else
{
    GL::enableVertexAttribs(GL::VERTEX_ATTRIB_FLAG_POS_COLOR_TEX);

    glBindBuffer(GL_ARRAY_BUFFER, _vbo);
    // vertex
    glVertexAttribPointer(GLProgram::VERTEX_ATTRIB_POSITION, 2, GL_FLOAT, GL_FALSE, sizeof(V2F_C4B_T2F), (GLvoid *)offsetof(V2F_C4B_T2F, vertices));
    // color
    glVertexAttribPointer(GLProgram::VERTEX_ATTRIB_COLOR, 4, GL_UNSIGNED_BYTE, GL_TRUE, sizeof(V2F_C4B_T2F), (GLvoid *)offsetof(V2F_C4B_T2F, colors));
    // texcoord
    glVertexAttribPointer(GLProgram::VERTEX_ATTRIB_TEX_COORD, 2, GL_FLOAT, GL_FALSE, sizeof(V2F_C4B_T2F), (GLvoid *)offsetof(V2F_C4B_T2F, texCoords));
}

Now, as @direct3d9 mentioned, changing the order in which this occurs is actually having an impact on the output, since it fixes the flickering issue for him. That can only mean that the order is significant, so it matters. OpenGL sample code relating to VAO always has the exact same order, where glBindVertexArray() is always called before glBindBuffer(), so perhaps there is something to this.

This would be the corrected order in DrawNode::onDraw():

if (Configuration::getInstance()->supportsShareableVAO())
{
    GL::bindVAO(_vao);
    if (_dirty)
    {
        glBindBuffer(GL_ARRAY_BUFFER, _vbo);
        glBufferData(GL_ARRAY_BUFFER, sizeof(V2F_C4B_T2F) * _bufferCapacity, _buffer, GL_STREAM_DRAW);

        _dirty = false;
    }
}
else
{
    if (_dirty)
    {
        glBindBuffer(GL_ARRAY_BUFFER, _vbo);
        glBufferData(GL_ARRAY_BUFFER, sizeof(V2F_C4B_T2F) * _bufferCapacity, _buffer, GL_STREAM_DRAW);

        _dirty = false;
    }

    GL::enableVertexAttribs(GL::VERTEX_ATTRIB_FLAG_POS_COLOR_TEX);

    glBindBuffer(GL_ARRAY_BUFFER, _vbo);
    // vertex
    glVertexAttribPointer(GLProgram::VERTEX_ATTRIB_POSITION, 2, GL_FLOAT, GL_FALSE, sizeof(V2F_C4B_T2F), (GLvoid *)offsetof(V2F_C4B_T2F, vertices));
    // color
    glVertexAttribPointer(GLProgram::VERTEX_ATTRIB_COLOR, 4, GL_UNSIGNED_BYTE, GL_TRUE, sizeof(V2F_C4B_T2F), (GLvoid *)offsetof(V2F_C4B_T2F, colors));
    // texcoord
    glVertexAttribPointer(GLProgram::VERTEX_ATTRIB_TEX_COORD, 2, GL_FLOAT, GL_FALSE, sizeof(V2F_C4B_T2F), (GLvoid *)offsetof(V2F_C4B_T2F, texCoords));
}

direct3d9 · May 13, 2019, 9:32am

This flickering articfacts are presents ONLY if shareable VAO is enabled. Changing buffer object contents BEFORE calling of GL::bindVAO(_vao) may changes buffers in ANOTHER VAO, that bound earlier.

zhangxm · May 14, 2019, 2:08am

buffer is bound to VAO in DrawNode::setupBuffer(). I think the flickering is because some opengl driver doesn’t implement correctly. And move the codes after binding VAO will not break other things, so i think it is ok to move updating buffer data after binding VAO.

@direct3d9 i will send PR to fix them if you will not send one.

direct3d9 · May 14, 2019, 11:03am

buffer initially bound to VAO in DrawNode::setupBuffer(), but rebound into another VAO in DrawNode::onDraw(), DrawNode:: onDrawGLLine() and DrawNode:: onDrawGLPoint(), because another VAO may be current before DrawNode::onDraw() execution.

Please send 3 PRs:
1 - move buffer updating code after GL::bindVAO(_vao)

2 - Fix buffer type in CCVertexIndxBuffer.cpp:225 (void IndexBuffer::recreateVBO() const)
This is index buffer (element array buffer), but GL_ARRAY_BUFFER constant used instead GL_ELEMENT_ARRAY_BUFFER. (see above)

3 - Fix parseRenderState() for technique (see above)

zhangxm · May 17, 2019, 2:10am

VAO will bind VBO when glVertexAtrribPointer is invoked, only bind VBO(except GL_ELEMENT_ARRAY_BUFFER) will not change VAO’s state. You can refer to the url for detail explanation. If it has problem, then why it only has problem on LG Pad? And how did you move the codes? We should update VBO in both using VAO or not.

I will send PRs for other 2 issues.

zhangxm · May 17, 2019, 2:27am

Fixed in PR.

kerryk · May 17, 2019, 10:33am

These fixes, (and any fixes in general) are done in the opengl version or the metal version? or metal will get them when its fully merged?

zhangxm · May 22, 2019, 1:22am

Some fixes will be sent to metal support branch, some are not, because they do not exist in metal branch.