0
\$\begingroup\$

Here is my repo, pull requests are always welcome ^^

I know that OpenGL frameworks already exists (OGLPlus, for example) but I developing another one following this set of reasons:

  1. To understand how OpenGL works
  2. To develop tool (framework) which is convenient to use (at least for me)
  3. To gain practical experience designing, programming, etc.

What is wrong with SOGL now?

There are at least a few problems I'm already aware of:
1. Error-handling is ugly. For example:

(Context use asserts):

#include <context.h>
#include <GL/glew.h>
#include <GLFW/glfw3.h>
#include <cassert>
#include <iostream>

namespace SOGL
{
    static void APIENTRY glDebugOutput(GLenum source, 
                            GLenum type, 
                            GLuint id, 
                            GLenum severity, 
                            GLsizei length, 
                            const GLchar *message, 
                            const void *userParam)
{
    // ignore non-significant error/warning codes
    if(id == 131169 || id == 131185 || id == 131218 || id == 131204) return; 

    std::cout << "---------------" << std::endl;
    std::cout << "Debug message (" << id << "): " <<  message << std::endl;

    switch (source)
    {
        case GL_DEBUG_SOURCE_API:             std::cout << "Source: API"; break;
        case GL_DEBUG_SOURCE_WINDOW_SYSTEM:   std::cout << "Source: Window System"; break;
        case GL_DEBUG_SOURCE_SHADER_COMPILER: std::cout << "Source: Shader Compiler"; break;
        case GL_DEBUG_SOURCE_THIRD_PARTY:     std::cout << "Source: Third Party"; break;
        case GL_DEBUG_SOURCE_APPLICATION:     std::cout << "Source: Application"; break;
        case GL_DEBUG_SOURCE_OTHER:           std::cout << "Source: Other"; break;
    } std::cout << std::endl;

    switch (type)
    {
        case GL_DEBUG_TYPE_ERROR:               std::cout << "Type: Error"; break;
        case GL_DEBUG_TYPE_DEPRECATED_BEHAVIOR: std::cout << "Type: Deprecated Behaviour"; break;
        case GL_DEBUG_TYPE_UNDEFINED_BEHAVIOR:  std::cout << "Type: Undefined Behaviour"; break; 
        case GL_DEBUG_TYPE_PORTABILITY:         std::cout << "Type: Portability"; break;
        case GL_DEBUG_TYPE_PERFORMANCE:         std::cout << "Type: Performance"; break;
        case GL_DEBUG_TYPE_MARKER:              std::cout << "Type: Marker"; break;
        case GL_DEBUG_TYPE_PUSH_GROUP:          std::cout << "Type: Push Group"; break;
        case GL_DEBUG_TYPE_POP_GROUP:           std::cout << "Type: Pop Group"; break;
        case GL_DEBUG_TYPE_OTHER:               std::cout << "Type: Other"; break;
    } std::cout << std::endl;

    switch (severity)
    {
        case GL_DEBUG_SEVERITY_HIGH:         std::cout << "Severity: high"; break;
        case GL_DEBUG_SEVERITY_MEDIUM:       std::cout << "Severity: medium"; break;
        case GL_DEBUG_SEVERITY_LOW:          std::cout << "Severity: low"; break;
        case GL_DEBUG_SEVERITY_NOTIFICATION: std::cout << "Severity: notification"; break;
    } std::cout << std::endl;
    std::cout << std::endl;
}

    ContextSettings::ContextSettings(unsigned depth, unsigned stencil, unsigned antialiasing, 
                                     unsigned major, unsigned minor):
        depth_bits(depth), stencil_bits(stencil), antialiasing_level(antialiasing),
        major_version(major), minor_version(minor)
    {}

    Context::Context(const ContextSettings& settings)
    {
        assert(glfwInit());

        static int at_exit = -1;

        if(at_exit != 0)
            at_exit = std::atexit([] { glfwTerminate(); });

        set_settings(settings);
        assert(m_handle = glfwCreateWindow(1, 1, "", nullptr, nullptr));
        bind();

        glewExperimental = true;
        assert(glewInit() == GLEW_OK);

        glEnable(GL_DEPTH_TEST);
        glEnable(GL_DEBUG_OUTPUT);
        glEnable(GL_DEBUG_OUTPUT_SYNCHRONOUS); 
        glDebugMessageCallback(glDebugOutput, nullptr);
        glDebugMessageControl(GL_DONT_CARE, GL_DONT_CARE, GL_DONT_CARE, 0, nullptr, GL_TRUE);
    }

    Context::Context(unsigned depth, unsigned stencil, unsigned antialiasing, 
                     unsigned major, unsigned minor) :
        Context(ContextSettings(depth, stencil, antialiasing, major, minor))
    {}

    Context::~Context()
    {
        glfwDestroyWindow(m_handle);
    }

    Context::Context(Context&& o)
    {
        m_handle = o.m_handle;
        o.m_handle = 0;
    }

    Context::Context(Context& o)
    {
        set_settings(o.settings());

        assert(m_handle = glfwCreateWindow(1, 1, "", nullptr, o.m_handle));
        bind();

        glewExperimental = true;
        assert(glewInit() == GLEW_OK);
    }

    const ContextSettings& Context::settings()
    {
        return m_settings;
    }

    void Context::draw_arrays(const PrimitiveType& mode, int first, int count)
    {
        glDrawArrays(remap(mode), first, count);
    }

    void Context::bind()
    {
        glfwMakeContextCurrent(m_handle);
    }

    void Context::unbind()
    {
        glfwMakeContextCurrent(nullptr);
    }

    bool Context::is_active()
    {
        return glfwGetCurrentContext() == m_handle;
    }

    Context::operator GLFWwindow*()
    {
        return m_handle;
    }

    void Context::set_settings(const ContextSettings& settings)
    {
        m_settings = settings;

        glfwWindowHint(GLFW_VISIBLE, false);
        glfwWindowHint(GLFW_RESIZABLE, false);
        glfwWindowHint(GLFW_DEPTH_BITS, m_settings.depth_bits);
        glfwWindowHint(GLFW_STENCIL_BITS, m_settings.stencil_bits);
        glfwWindowHint(GLFW_SAMPLES, m_settings.antialiasing_level);
        glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, m_settings.major_version);
        glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, m_settings.minor_version);
        glfwWindowHint(GLFW_OPENGL_DEBUG_CONTEXT, true);
        glfwWindowHint(GLFW_OPENGL_FORWARD_COMPAT, GL_TRUE);
        glfwWindowHint(GLFW_OPENGL_PROFILE, GLFW_OPENGL_CORE_PROFILE);
    }
};

(Textures can't handle errors at all):

#include "texture.h"
#include <GL/glew.h>
#include <cassert>

#define STB_IMAGE_IMPLEMENTATION
#include <stb_image.h>

namespace SOGL
{
    Texture::Texture(TextureTarget target): m_target(target)
    {
        glCreateTextures(remap(m_target), 1, &m_id);
        glTextureParameteri(m_id, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
        glTextureParameteri(m_id, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
    }

    Texture::Texture(const char* filename): m_target(TextureTarget::Texture2D)
    {
        int w, h, n;

        unsigned char* data = stbi_load(filename, &w, &h, &n, 3);
        assert(data != nullptr);

        glCreateTextures(remap(m_target), 1, &m_id);
        glTextureImage2DEXT(m_id, remap(m_target), 0, GL_RGB, 
                            w, h, 0, GL_RGB, GL_UNSIGNED_BYTE, data);

        //glTexParameterf(target, pname, param);
        //glTextureParameterf(texture, pname, param);
        glTextureParameteri(m_id, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
        glTextureParameteri(m_id, GL_TEXTURE_MIN_FILTER, GL_LINEAR_MIPMAP_LINEAR);
        make_mipmaps();

        stbi_image_free(data);
    }

    Texture::~Texture()
    {
        glDeleteTextures(1, &m_id);
    }

    Texture::Texture(Texture&& o)
    {
        m_id = o.m_id;

        o.m_id = 0;
    }

    void Texture::allocate(int w, int h, int level, TextureFormat format, 
                           TextureInternalFormat internal_format, DataType type)
    {
        glTextureImage2DEXT(m_id, remap(m_target), level, remap(internal_format), 
                            w, h, 0, remap(format), remap(type), nullptr);
    }

    void Texture::allocate(int w, int h, int d, int level, TextureFormat format, TextureInternalFormat internal_format, DataType type)
    {
        glTextureImage3DEXT(m_id, remap(m_target), level, remap(internal_format), 
                            w, h, d, 0, remap(format), remap(type), nullptr);
    }

    void Texture::make_mipmaps()
    {
        glGenerateTextureMipmap(m_id);
    }

    TextureTarget Texture::target()
    {
        return m_target;
    }

    void Texture::bind(unsigned binding)
    {
        glActiveTexture(GL_TEXTURE0 + binding);
        glBindTexture(remap(m_target), m_id);
    }

    void Texture::unbind(unsigned binding)
    {
        glActiveTexture(GL_TEXTURE0 + binding);
        glBindTexture(remap(m_target), 0);
    }

    unsigned Texture::id()
    {
        return m_id;
    }

    Texture::operator unsigned()
    {
        return id();
    }
};

(VBO and other OpenGL resources don't have error-checking mechanisms):

#include "vertex_buffer.h"
#include <GL/glew.h>

namespace SOGL
{
    VertexBuffer::VertexBuffer()
    {
        glCreateBuffers(1, &m_id);
    }

    VertexBuffer::~VertexBuffer()
    {
        glDeleteBuffers(1, &m_id);
    }

    VertexBuffer::VertexBuffer(VertexBuffer&& o)
    {
        m_id = o.m_id;

        o.m_id =  0;
    }

    void VertexBuffer::allocate(unsigned count, BufferUsage usage)
    {
        glNamedBufferData(m_id, count, nullptr, remap(usage));
    }

    void VertexBuffer::allocate(const void * data, unsigned count, BufferUsage usage)
    {
        glNamedBufferData(m_id, count, data, remap(usage));
    }

    void VertexBuffer::bind(BufferTarget target)
    {
        glBindBuffer(remap(target), m_id);
    }

    void VertexBuffer::unbind(BufferTarget target)
    {
        glBindBuffer(remap(target), 0);
    }

    unsigned VertexBuffer::id()
    {
        return m_id;
    }

    VertexBuffer::operator unsigned()
    {
        return id();
    }
};

(Contexts also can't cache errors from OpenGL Debug Output. Instead they are are simply displayed in the terminal):

    static void APIENTRY glDebugOutput(GLenum source, 
                            GLenum type, 
                            GLuint id, 
                            GLenum severity, 
                            GLsizei length, 
                            const GLchar *message, 
                            const void *userParam)
{
    // ignore non-significant error/warning codes
    if(id == 131169 || id == 131185 || id == 131218 || id == 131204) return; 

    std::cout << "---------------" << std::endl;
    std::cout << "Debug message (" << id << "): " <<  message << std::endl;

    switch (source)
    {
        case GL_DEBUG_SOURCE_API:             std::cout << "Source: API"; break;
        case GL_DEBUG_SOURCE_WINDOW_SYSTEM:   std::cout << "Source: Window System"; break;
        case GL_DEBUG_SOURCE_SHADER_COMPILER: std::cout << "Source: Shader Compiler"; break;
        case GL_DEBUG_SOURCE_THIRD_PARTY:     std::cout << "Source: Third Party"; break;
        case GL_DEBUG_SOURCE_APPLICATION:     std::cout << "Source: Application"; break;
        case GL_DEBUG_SOURCE_OTHER:           std::cout << "Source: Other"; break;
    } std::cout << std::endl;

    switch (type)
    {
        case GL_DEBUG_TYPE_ERROR:               std::cout << "Type: Error"; break;
        case GL_DEBUG_TYPE_DEPRECATED_BEHAVIOR: std::cout << "Type: Deprecated Behaviour"; break;
        case GL_DEBUG_TYPE_UNDEFINED_BEHAVIOR:  std::cout << "Type: Undefined Behaviour"; break; 
        case GL_DEBUG_TYPE_PORTABILITY:         std::cout << "Type: Portability"; break;
        case GL_DEBUG_TYPE_PERFORMANCE:         std::cout << "Type: Performance"; break;
        case GL_DEBUG_TYPE_MARKER:              std::cout << "Type: Marker"; break;
        case GL_DEBUG_TYPE_PUSH_GROUP:          std::cout << "Type: Push Group"; break;
        case GL_DEBUG_TYPE_POP_GROUP:           std::cout << "Type: Pop Group"; break;
        case GL_DEBUG_TYPE_OTHER:               std::cout << "Type: Other"; break;
    } std::cout << std::endl;

    switch (severity)
    {
        case GL_DEBUG_SEVERITY_HIGH:         std::cout << "Severity: high"; break;
        case GL_DEBUG_SEVERITY_MEDIUM:       std::cout << "Severity: medium"; break;
        case GL_DEBUG_SEVERITY_LOW:          std::cout << "Severity: low"; break;
        case GL_DEBUG_SEVERITY_NOTIFICATION: std::cout << "Severity: notification"; break;
    } std::cout << std::endl;
    std::cout << std::endl;
}

There are a lot of different ways to potentially handle errors, and I don't know which I should choose.
2. Not all functionality of OpenGL is encapsulated
3. I do not know how to supply resources to examples. Here is the CMakeLists.txt from my examples folder:

MACRO(SUBDIRLIST result curdir)
    FILE(GLOB children RELATIVE ${curdir} ${curdir}/*)
    SET(dirlist "")
    FOREACH(child ${children})
    IF(IS_DIRECTORY ${curdir}/${child})
        LIST(APPEND dirlist ${child})
    ENDIF()
    ENDFOREACH()
    SET(${result} ${dirlist})
ENDMACRO()

SUBDIRLIST(EXAMPLES ${CMAKE_CURRENT_SOURCE_DIR})

FOREACH(EXAMPLE ${EXAMPLES})
    add_subdirectory(${EXAMPLE})
ENDFOREACH()

Root CMakeLists:

cmake_minimum_required(VERSION 3.0.2)
project(SOGL)

option(BUILD_EXAMPLES "Build examples" ON)

function(static_compile p)
    target_compile_options(${p}
        PUBLIC "/MT$<$<STREQUAL:$<CONFIGURATION>,Debug>:d>"
    )
endfunction()

macro ( mark_as_internal _var )
  set ( ${_var} ${${_var}} CACHE INTERNAL "hide this!" FORCE )
endmacro( mark_as_internal _var ) 

set(CMAKE_CXX_FLAGS_RELEASE "/MT -O2" CACHE INTERNAL "" FORCE)
set(CMAKE_CXX_FLAGS_DEBUG "/MTd" CACHE INTERNAL "" FORCE)

set(BUILD_UTILS OFF CACHE INTERNAL "" FORCE)
set(GLFW_INSTALL OFF CACHE INTERNAL "" FORCE)
set(GLFW_BUILD_DOCS OFF CACHE INTERNAL "" FORCE)
set(GLFW_BUILD_TESTS OFF CACHE INTERNAL "" FORCE)
set(BUILD_SHARED_LIBS OFF CACHE INTERNAL "" FORCE)
set(GLFW_BUILD_EXAMPLES OFF CACHE INTERNAL "" FORCE)
set(USE_MSVC_RUNTIME_LIBRARY_DLL OFF CACHE INTERNAL "" FORCE)

add_subdirectory(lib/glfw EXCLUDE_FROM_ALL)
include_directories(lib/glfw/include)
static_compile(glfw)

add_subdirectory(lib/glew/build/cmake EXCLUDE_FROM_ALL)
include_directories(lib/glew/include)
static_compile(glew_s)


include_directories(lib/glm/glm)

include_directories(lib/stb)
include_directories(include)

mark_as_internal(GLEW_OSMESA)
mark_as_internal(GLEW_REGAL)
mark_as_internal(GLFW_DOCUMENT_INTERNALS)
mark_as_internal(GLFW_USE_HYBRID_HPG)
mark_as_internal(GLFW_VULKAN_STATIC)

file(GLOB_RECURSE SOURCE_LIST "src/*.cpp" "include/*.h")
add_library(SOGL STATIC ${SOURCE_LIST})

find_package(OpenGL REQUIRED)
target_link_libraries(SOGL glew_s glfw ${OPENGL_LIBRARIES})

if(BUILD_EXAMPLES)
    add_subdirectory(examples)
endif()
\$\endgroup\$
  • \$\begingroup\$ Hey there, as-is this question is off-topic because you don't include the code to be reviewed in the question itself, only a link. Please include all necessary code, or factor out a chunk that should be reviewed independently. \$\endgroup\$ – Dannnno Sep 7 '17 at 20:43
  • \$\begingroup\$ Sorry, I'll fix it now \$\endgroup\$ – nuke_bird Sep 7 '17 at 20:53
  • \$\begingroup\$ @Dannnno I add some pieces of code. It's a bit unusual way to format the code, I probably do it wrong \$\endgroup\$ – nuke_bird Sep 7 '17 at 21:08
  • \$\begingroup\$ I've edited your question to fix the formatting and some grammar. There still are some issues - most of your code samples are too small to actually work with. Actual functions or classes are the bare minimum for what should be included. \$\endgroup\$ – Dannnno Sep 7 '17 at 21:16
  • \$\begingroup\$ @Dannnno thanks you a lot for editing my post. I re-formatted the text and added more code \$\endgroup\$ – nuke_bird Sep 7 '17 at 21:35
1
\$\begingroup\$
  1. You have a number of assert(do something that returns a bool); These will get stripped out of the executable when building in Release mode.

So instead of

assert(glfwInit());

You should write:

auto init_success = glfwInit()
assert(init_success);
  1. if(id == 131169 || id == 131185 || id == 131218 || id == 131204) This is pretty dodgy. At the very least add a comment listing what each of these are.

  2. I don't like your implicit cast operations from your wrapper types to the native GL handles. If you are going to use wrappers, commit. There will always be a few edge cases where some functionality needs the native handle, but that's what friend is for.

  3. regarding glDebugOutput(). Libraries shouldn't just write to cout. You would be better served to build a string and invoke a callback that is application-defined.

  4. I would make the calls to glNamedBufferData() in the VertexBuffer constructors.

  5. regarding Texture(const char*). You do not want your GL thread to be ever waiting on I/O. It's better to receive a buffer, and delegate the file loading to someone else.

  6. Use the GL* types (instead of unsigned) They are there for a reason.

\$\endgroup\$
  • \$\begingroup\$ 1. Should I write custom assert function? 7. It is because header files don't include gl.h and etc. By the way, GLuint == unsigned, so what the problem? \$\endgroup\$ – nuke_bird Sep 7 '17 at 23:26
  • \$\begingroup\$ @nuke_bird GLuint == unsigned on your system, not necessarily on everyone's. Why would you not include khronos' headers? See my ammended answer for the assert \$\endgroup\$ – Frank Sep 7 '17 at 23:32
  • \$\begingroup\$ first thing that I'm trying to achieve - encapsulate all basic OpenGL primitives (and headers, too). If someone need make direct OpenGL call - he must include khronos headers by himself. I must to do something like static_assert(std::is_same<unsigned, GLuint>::value, "GLuint != unsigned"); ? oh, now I understood what do you talk about asserts. Yes, It make sense \$\endgroup\$ – nuke_bird Sep 7 '17 at 23:43
  • \$\begingroup\$ I think your approach is somewhat misguided. You should be either opaque or not. This half-opaque, half-transparent interface just seems half-commited. \$\endgroup\$ – Frank Sep 8 '17 at 0:34
  • \$\begingroup\$ Yeah, I'm completely agree with you... I'm more inclined towards an opaque style. So I'll just remove direct access to resource identifiers Thank you for your advices. When I finish refactoring of code, can I drop you a notification for additional code review? \$\endgroup\$ – nuke_bird Sep 8 '17 at 0:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.