Clearly document the implications of deferred GL context creation

[lum-supakorn]

I'm trying to get the viewer example to work with the example window setup code (mainly to enable MSAA for modeling viewing).

By default, viewer sets up the window like this:

ViewerExample::ViewerExample(const Arguments& arguments):
            Platform::Application{arguments, Configuration{}
                .setTitle("Magnum Viewer Example")
                .setWindowFlags(Configuration::WindowFlag::Resizable)}
        { ... }

But when the window was setup like this (took from other examples):

ViewerExample::ViewerExample(const Arguments& arguments)
    : Platform::Application{arguments, NoCreate} {
    /* Setup window */
    {
        const Vector2 dpiScaling = this->dpiScaling({});
        Configuration conf;
        conf.setTitle("Magnum Model Viewer Example")
            .setSize(conf.size(), dpiScaling)
            .setWindowFlags(Configuration::WindowFlag::Resizable);
        GLConfiguration glConf;
        glConf.setSampleCount(dpiScaling.max() < 2.0f ? 8 : 2);
        if (!tryCreate(conf, glConf)) {
            create(conf, glConf.setSampleCount(0));
        }
    }
    ...
    }

the code compiled but when executed I got Segmentation fault. I'm not quite sure if this is the only way to enable MSAA or if this is directly related to the viewer example but so far this is the only example that has this issue.

Magnum (2020.06) and all other dependencies were compiled as CMake subprojects. I'm working on Ubuntu 20.04 x86_64.

Thank you in advance for your investigation.

[lum-supakorn]

Update: I worked around this issue by creating GLConfiguration inside Platform::Application constructor:

ViewerExample::ViewerExample(const Arguments& arguments)
    : Platform::Application{
          arguments,
          Configuration{}
              .setTitle("Magnum Viewer Example")
              .setWindowFlags(Configuration::WindowFlag::Resizable),
          GLConfiguration{}.setSampleCount(16)} { ... }

But I still haven't figured out what's wrong with the above window setup snippet.

[lum-supakorn]

Update: Upon further investigation, it seems like the problem somehow stems from Shaders::PhongGL. Adding the following lines (from viewer) to mouseinteraction (where window is setup with the problematic code) causes Segmentation fault:

    Shaders::PhongGL _coloredShader,
        _texturedShader{Shaders::PhongGL::Flag::DiffuseTexture};

[mosra]

Hi!

If the GL context isn't created directly in the Platform::Application constructor call but later in create() / tryCreate(), class members will get constructed earlier than the GL context is ready and can cause this exact behavior, or sometimes the GL::Context: no current context assertion.

To work around that, the simplest is to use the NoCreate constructors for all GL objects and do their "proper" initialization later when the context is ready. For example, taking your code from above:

    Shaders::PhongGL _coloredShader{NoCreate},
        _texturedShader{NoCreate};

and then in the actual constructor:

ViewerExample::ViewerExample(const Arguments& arguments)
    : Platform::Application{arguments, NoCreate} {
        ...
        create(...);

        // now the context is ready, initialize all GL-dependent members 
        _coloredShader = Shaders::PhongGL{};
        _texturedShader = Shaders::PhongGL{Shaders::PhongGL::Flag::DiffuseTexture};
    }

I can't find a place where this would be clearly mentioned in the documentation — that isn't great, will fix that.

[lum-supakorn]

Wonderful! May I ask a small follow-up question? Why is the window setup code inside an inner block scope?

[mosra]

I'm doing it just to limit the scope of the local variables like conf, dpiScaling and glConf, because it makes no sense to use them after the context is created. Apart from that, the block serves no functional purpose in this case, it's just a code style I use sometimes -- application constructors tend to get rather long over time and this way I can "isolate" setup blocks from each other, reuse the same variable name multiple times, and avoid issues where a variable would get accidentally reused later with some stale value.

Another example of the same style is for example here -- instead of having to use found1, found2 etc. and then having it prone to copypaste errors where I accidentally use found2 instead of found3, it's the same name always: https://github.com/mosra/corrade/blob/42795f836a4ae0ab5c6379a4a32a7cc875e38372/src/Corrade/Containers/Test/StringViewTest.cpp#L1228-L1270

Relevant info on cppreference.

[mosra]

I can't find a place where this would be clearly mentioned in the documentation — that isn't great, will fix that.

This has been done in mosra/magnum@2bd933d3ef20c934cd49b80bcacadb4606a8b6e7, closing this issue as resolved.