Arcball and GLShader -- too protected?

[svenevs]

There are two things I constantly modify by inheriting from these classes and providing public access. Both are simple enough that it really makes no difference to me whether or not they go in the upstream. I just use them often enough that they may be good candidates.

Arcball

This one may be a good candidate to just add to the Arcball class, without needing to expose anything:

struct InterruptableArcball : public nanogui::Arcball {
    InterruptableArcball(float speedFactor = 2.0f) : Arcball(speedFactor) { }
    InterruptableArcball(const Quaternionf &quat) : Arcball(quat) { }

    void interrupt() {
        Arcball::button(mLastPos, false);
    }

public:
    EIGEN_MAKE_ALIGNED_OPERATOR_NEW
};

It's useful to be able to "close" the state without an actual mouse release. I use the Arcball to model a simple camera model for interacting with the scene, but if an event occurs that issues say a MessageDialog, or really any other widgets become in focus, I can simply call interrupt to prevent some glitchy re-interaction (since mLastPos and the new click location may be very different).

GLShader

Don't make the GLShader::Buffer or GLShader::mBuffer protected. This one is a little more specialized. Docs could be added stating plainly "you break it you buy it" -- users who choose to directly read / modify these values should be cautious and well...really need to know what they're doing.

The standard use case for me is CUDA / OpenGL interop. If I want to write to the positions of a shader or something, I need the specific GLuint associated with this attribute to map a pointer with CUDA. The class I wrote a long time ago that really doesn't need a template, but has it anyway:

class ExposedGLShader : public nanogui::GLShader {
protected:
    enum class BufferAttribs : uint8_t {
        id = 0,
        glType,
        dim,
        compSize,
        size
        // version already has a getter
    };

    template <BufferAttribs type>
    GLuint *buffer_internal(nanogui::GLShader::Buffer &buff) {
        if(type == BufferAttribs::id)            return &buff.id;
        else if(type == BufferAttribs::glType)   return &buff.glType;
        else if(type == BufferAttribs::dim)      return &buff.dim;
        else if(type == BufferAttribs::compSize) return &buff.compSize;
        else if(type == BufferAttribs::size)     return &buff.size;

        return nullptr;
    }

    template <BufferAttribs type>
    GLuint* buffer_attrib(const std::string &name, bool warn = true) {
        for(auto &pair : mBufferObjects) {
            if(pair.first == name)
                return buffer_internal<type>(pair.second);
        }

        if(warn)
            std::cerr << mName << ": warning: did not find attrib " << name << std::endl;
        return nullptr;
    }

public:
    GLuint *bufferId(const std::string &name) {
        return buffer_attrib<BufferAttribs::id>(name);
    }

    GLuint *bufferGLType(const std::string &name) {
        return buffer_attrib<BufferAttribs::glType>(name);
    }

    GLuint *bufferDim(const std::string &name) {
        return buffer_attrib<BufferAttribs::dim>(name);
    }

    GLuint *bufferCompSize(const std::string &name) {
        return buffer_attrib<BufferAttribs::compSize>(name);
    }

    GLuint *bufferSize(const std::string &name) {
        return buffer_attrib<BufferAttribs::size>(name);
    }
};

Upstream fixes would either be exposing the GLShader::Buffer class, or possibly just adding the ExposedGLShader class. Or neither.

Let me know if you think either of these are desirable.

[wjakob]

These changes both look great. Please feel free to submit a PR and I'll merge it. It would be nice if the new functions also have matching Python bindings.

[svenevs]

Sounds good. For the shader, do you want the separate class ExposedGLShader, or pass through methods added to the existing one

For Python, I'm not sure how I would return pointers. So maybe the methods return a GLuint instead? The downside is nullptr is a good trap state. Another alternative woul be returning a GLShader::Buffer &, but if it isn't available throw an exception?