VertexArray being deleted by std::move

[jherico]

I'm seeing an odd error with a project I'm working on. I have the following code

      using namespace oglplus;
      shapes::Plane planeShape(Vec3f(1, 0, 0), Vec3f(0, 1, 0));
      const std::initializer_list<const GLchar*> names = { "Position", "TexCoord" };
      shapes::ShapeWrapper * planePtr = new shapes::ShapeWrapper(names, planeShape);
      VertexArray vao = planePtr->VAOForProgram(*shader);
      qDebug() << "Done";

This compiles and runs fine, but when I attempt to use the vao, it generates an error because the VAO id is invalid. I've tracked it down to the std::move operation here: https://github.com/matus-chochlik/oglplus/blob/develop/implement/oglplus/shapes/wrapper.ipp#L21

When this move executes, for some reason the VAO ID itself is deleted. I have breakpoints on that line, the qDebug() line above, and this line: https://github.com/matus-chochlik/oglplus/blob/develop/include/oglplus/vertex_array.hpp#L52 where the delete occurs.

At some point between the std::move and the debug line, the VAO is deleted by a call to glDeleteVertexArrays. However, the vao object I get back still has the identifier, so when you try to use it you get GL_INVALID_OPERATION from the glBindVertexArray call and oglplus throws an exception.

I'm seeing this in both master and develop, but not in fd50019b073ba2edcdc2d0a5f7494e745c5d50a9 (which another project I'm using is on) so it looks like something broke between the current code and then. I'll try to do a binary search to isolate the commit later tonight.

[matus-chochlik]

Hi,

could You try latest master or develop? A bug in the GL object wrapper manifesting in this manner has been recently fixed.

[jherico]

I just did a fetch, and I'm seeing the same behavior on aa4a21ce0c56ea59d97f39d11f5ac8ba5302f391 and 394978f930a2799910d6e81ba603aa178f89f3bf

[jherico]

Bisect says this is the bad commit, investigating: https://github.com/matus-chochlik/oglplus/commit/b4126ab83c854364e0dc2f2d826452e04184dc1e

[jherico]

Related to this I think: http://stackoverflow.com/questions/4819936/why-no-default-move-assignment-move-constructor

[matus-chochlik]

Could You pull and test the latest commits from develop? I've tried the changes with your example and with the following code and both seem to work OK, here.

VertexArray vao1;
VertexArray vao2 = std::move(vao1);

std::cout << GetName(vao1) << std::endl;
std::cout << GetName(vao2) << std::endl;

[jherico]

Sorry for the delay, I had to stop using oglplus on the project and take a different route, and hadn't gotten back to it.

on 0726da36b1306efadc0d46ec573013dc87750653 I'm seeing the following behavior:

This works, but it kind of defeats the purpose of using a ShapeWrapper.

    using namespace oglplus;
    ShapeWrapperPtr plane = ShapeWrapperPtr(
      new shapes::ShapeWrapper({ "Position", "TexCoord" }, shapes::Plane())
    );
    VertexArray vao = plane->VAOForProgram(*planeProgram);
    std::cout << GetName(vao) << std::endl;
    vao.Bind();

This throws an exception when I call plane->Use() because the VAO has already been destroyed

    using namespace oglplus;
    ShapeWrapperPtr plane = ShapeWrapperPtr(
      new shapes::ShapeWrapper({ "Position", "TexCoord" }, shapes::Plane())
    );
    plane->UseInProgram(*planeProgram);
    plane->Use();

So does this

    using namespace oglplus;
    ShapeWrapperPtr plane = ShapeWrapperPtr(
      new shapes::ShapeWrapper({ "Position", "TexCoord" }, shapes::Plane(), *planeProgram)
    );
    plane->Use();

[jherico]

More specifically, I've discovered that it looks like success or failure depends on whether you're using the move constructor or the move assignment operator:

    // Uses move constructor
    VertexArray vao = plane->VAOForProgram(*planeProgram);
    std::cout << GetName(vao) << std::endl;
    // Assert SUCCEEDS
    assert(glIsVertexArray(GetName(vao)));

    VertexArray vao2;
    // uses move assignment operator
    vao2 = plane->VAOForProgram(*planeProgram);
    std::cout << GetName(vao2) << std::endl;
    // Assert FAILS
    assert(glIsVertexArray(GetName(vao2)));

[matus-chochlik]

Yes, there was a potential problem with move-assignment in Object. I've committed a fix to develop, and tested it on both g++ and clang++. Surprisingly on g++ the results were the same even before the fix, it seems that the tighter rules for implicit move construction/assignment were not yet implemented on the older versions.

Could you test that and let me know if that works for your apps?

[matus-chochlik]

Well, eventually I've decided that I'll add explicit definitions of all copy/move constructors and assignment operators to all specializations of ObjectOps, ObjZeroOps and ObjCommonOps. It's quite a lot of repetitive code but IMO in the end its the best solution to this issue.