Fix for missing initialization and memory leak in recent 0.6.0.1 support merge.

[ChrisDenham]

This fixes a couple of leaks of texture and depth buffer objects, and also a crash in OculusDevice destructor when/if attempting to destroy texture buffer before realization of device.

[ChrisDenham]

oops, didn't mean to include changes to the gitignore files (so please ignore the gitignores, lol)

[bjornblissing]

Well I guess that only the relevant commit is: 2298de0ae8e6bff129d8318d0ed23148488a0c51

Since all other seem to handle when master was supporting the 0.5 SDK.

I have looked at the above commit. But I am a bit reluctant to merge this. I am thinking about making the OculusTextureBuffer and OculusDepthBuffer inherit from osg::Referenced. And creating a OculusMirrorTexture class as well, wrapping the behavior for the ovrGLTexture.

Thus making use of reference counters and then moving the clean up code inside each destructor instead.

[ChrisDenham]

Ok, fair enough, sounds like a good plan. Anything that removes the need for explicit initialization and cleanup seems like a good idea to me. :-)

[ChrisDenham]

Thanks for 6885e52ba13bddb8e3eff18a4a0673e3d2acd41a it fixes the leak but looks like it still may need the checks in destructor before calling destroy because the device could be destroyed before is has realised: e.g.:

OculusDevice::~OculusDevice()
{
    // Delete mirror texture
    if (m_mirrorTexture.valid()) {
        m_mirrorTexture->destroy();
    }

    // Delete texture and depth buffers
    for (int i = 0; i < 2; i++) {
        if (m_textureBuffer[i].valid()) {
            m_textureBuffer[i]->destroy();
        }
        if (m_depthBuffer[i].valid()) {
            m_depthBuffer[i]->destroy();
        }
    }

    ovrHmd_Destroy(m_hmdDevice);
    ovr_Shutdown();
}

[bjornblissing]

Of course it is good practice to check points being valid before using them. BUT is it really an actual issue in this case?

If the initialization of fails, the current implementation will cause more major errors any way. It certainly does not handle all possible error cases.

[ChrisDenham]

Not a big issue, but the very simple example of where it would crash is where you create the OculusDevice class and then delete it before you have even attempted to initialise it by running the viewer. It just seemed that you ought to always be able to delete an object immediately after creating it. I just happened to notice the issue while i was tracking down the leak.

[ChrisDenham]

Also, I did wonder if the calls to destroy each texture/buffer could just go in there respective destructors, so they would be automatically freed on ref count = 0 ? Or would that be too late?

[bjornblissing]

No, that would be to late. Actually even running things in OculusDevice destructor is too late. The destruction should happen before the destruction of the valid OpenGL context. Otherwise you cannot run the required glDeleteFramebuffers call for the mirror texture.

If you look at the following line: https://github.com/bjornblissing/osgoculusviewer/blob/6885e52ba13bddb8e3eff18a4a0673e3d2acd41a/src/oculusdevice.cpp#L151

You can see that I have prepared for running this command with a valid context. But I still have not found a way to detect when the viewer closes and still have a valid OpenGL context open. Currently when the destructor for OculusDevice runs, the context is already closed.

[ChrisDenham]

Ah, ok. Well, I guess my point still stands though: The code:

{
        osg::ref_ptr<OculusDevice> oculusDevice = new OculusDevice(...blah blah);
}

should not crash, however silly the user might seem for creating the device object and then not using/initializing it. I think it is sometimes useful to be able to test objects in isolation.

[bjornblissing]

Well, I agree with your principle. But there would require me to add a lot of error handling for fail cases that are pretty rare. So it is not the highest on my priority list.

[ChrisDenham]

OK, fair enough. Thanks anyway for sharing your work.