3
\$\begingroup\$

I am writing a prototype for something at work. Not very many people here know much about Linux/Unix or Xlib, so I can't really ask here at work (most developers are expert Windows programmers). With this said, I have one class and a driver that I would like feedback on how well I have integrated threads, XLib and OpenGL.

The order of the files below is:

  1. glx_data.h
  2. linux_main.cpp
  3. map.h
  4. map.cpp
  5. Makefile

// 1. glx_data.h
#ifndef GLX_DATA_
#define GLX_DATA_
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <pthread.h>
#include <X11/Xlib.h>
#include <GL/glx.h>
#include <GL/gl.h>
#include <GL/glu.h>

typedef
struct glx_data
{
  Display *dpy;
  Window win;
  XVisualInfo *vi;
  Colormap cmap;
  XSetWindowAttributes swa;
  GLXContext cx;
  XEvent event;
  int dummy;
  bool double_buffer;
} GLXDATA;

#endif//GLX_DATA

linux_main.cpp

#include "glx_data.h"
#include "map.h"

static int snglBuf[] = { GLX_RGBA, GLX_RED_SIZE, 1, GLX_GREEN_SIZE, 1,
                         GLX_BLUE_SIZE, 1, GLX_DEPTH_SIZE, 12, None };

static int dblBuf[] = { GLX_RGBA, GLX_RED_SIZE, 1, GLX_GREEN_SIZE, 1, 
                        GLX_BLUE_SIZE, 1, GLX_DEPTH_SIZE, 12,
                        GLX_DOUBLEBUFFER, None };

//Global Data
static Map *gsp_Map;

static GLXDATA glxdata;

bool init_instance() {
  glxdata.dpy = XOpenDisplay(NULL);
  if (glxdata.dpy == NULL) {
    fprintf(stderr, "Failed to get XDisplay\n");
    return false;
  }
  if (!glXQueryExtension( glxdata.dpy, 
                          &glxdata.dummy, 
                          &glxdata.dummy)) {
    fprintf(stderr, "X server has no OpenGL GLX extension");
    return false;
  }
  glxdata.vi = glXChooseVisual( glxdata.dpy,
                                DefaultScreen(glxdata.dpy),
                                dblBuf);
  if (NULL == glxdata.vi) {
    glxdata.vi = glXChooseVisual( glxdata.dpy,
                                  DefaultScreen(glxdata.dpy),
                                  snglBuf
                        );
    if (NULL == glxdata.vi) {
      fprintf(stderr, "No RGB visual with depth buffer");
      return false;
    }
    glxdata.double_buffer = false;
  } else {
    glxdata.double_buffer = true;
  }
  glxdata.cmap = XCreateColormap( glxdata.dpy,
                                  RootWindow( 
                                              glxdata.dpy,
                                              (glxdata.vi)->screen
                                            ),
                                  (glxdata.vi)->visual,
                                  AllocNone);
  glxdata.swa.colormap = glxdata.cmap;
  glxdata.swa.border_pixel = 0;
  glxdata.swa.event_mask = ExposureMask | ButtonPressMask |
                           StructureNotifyMask;
  glxdata.win = XCreateWindow( glxdata.dpy,
                               RootWindow(
                                           glxdata.dpy,
                                           (glxdata.vi)->screen
                                         ),
                               0, 0,
                               300, 300,
                               0,
                               (glxdata.vi)->depth,
                               InputOutput,
                               (glxdata.vi)->visual,
                               CWBorderPixel | CWColormap |
                               CWEventMask,
                               &glxdata.swa
                             );

  XSetStandardProperties(
                         glxdata.dpy,
                         glxdata.win,
                         "glxsimple",
                         "glxsimple",
                         None,
                         NULL,
                         0,
                         NULL
                        );

  /*
   *  After window is set and glx context configured
   *  Note that we have not created the glx context yet
   */
  gsp_Map = new Map(&glxdata, 300, 300);

  /*
   * Now we can show the window and go to main message loop
   */
  XMapWindow(glxdata.dpy, glxdata.win);
  return true;
}

void
message_loop()
{
  while (1) {
    do {
      XNextEvent(glxdata.dpy, &glxdata.event);
      switch (glxdata.event.type) {
      case ButtonPress:
        switch (glxdata.event.xbutton.button) {
        case 1:
          gsp_Map->inc_xrotate(10);
          gsp_Map->set_update(true);
          break;
        case 2:
          gsp_Map->inc_yrotate(10);
          gsp_Map->set_update(true);
          break;
        case 3:
          gsp_Map->inc_zrotate(10);
          gsp_Map->set_update(true);
          break;
        }
        break;
      case ConfigureNotify:
        gsp_Map->set_deminsions(
                                 glxdata.event.xconfigure.width, 
                                 glxdata.event.xconfigure.height
                               );
      case Expose:
        gsp_Map->set_update(true);
        break;
      }
    } while (XPending(glxdata.dpy));
  }
  pthread_join(gsp_Map->get_pthread(), NULL);
}

int 
main(int argc, char *argv[]) {
  if (!init_instance()) {
    fprintf(stderr, "Failed to initialize application instance\n");
  }

  message_loop();

  if(gsp_Map) {
    delete gsp_Map;
  }
}

map.h

#ifndef MAP_H_
#define MAP_H_

#include "glx_data.h"

class Map
{
public:
  Map();
  Map(Map &copy);
  Map(GLXDATA *glxdata, int width, int height);
  ~Map();
  inline pthread_t &get_pthread() {return m_render_thread;}
  inline void set_update(bool update) {m_update = update;}
  inline void inc_xrotate(GLfloat deg) 
  {
    if ((m_xAngle + deg) < 360) {
      m_xAngle += deg;
    } else {
      m_xAngle = deg - (360 - m_xAngle);
    }
  }
  inline void inc_yrotate(GLfloat deg) 
  {
    if ((m_yAngle + deg) < 360) {
      m_yAngle += deg;
    } else {
      m_yAngle = deg - (360 - m_yAngle);
    }
  }
  inline void inc_zrotate(GLfloat deg) 
  {
    if ((m_zAngle + deg) < 360) {
      m_zAngle += deg;
    } else {
      m_zAngle = deg - (360 - m_zAngle);
    }
  }
  inline void set_deminsions(int width, int height) 
  {
    m_width = width; 
    m_height = height;
  }
  void render_data();
private:
  GLXDATA *m_pglxdata;
  pthread_t m_render_thread;
  bool m_render;
  bool m_update;
  bool m_update_viewport;
  int m_width;
  int m_height;
  GLfloat m_xAngle;
  GLfloat m_yAngle;
  GLfloat m_zAngle;
  static void *create_pthread(void *data) 
  {
    Map *thread_map = static_cast<Map *>(data);
    thread_map->render_data();  
    return data;
  }
};

#endif//MAP_H_

map.cpp

#include "map.h"

Map::Map()
{
}

Map::Map(Map &copy)
{
}

Map::Map(GLXDATA *glxdata, int width, int height)
{
  m_width = width;
  m_height = height;
  m_render = true;
  m_update = true;
  m_update_viewport = true;
  m_xAngle = 42.0;
  m_yAngle = 82.0;
  m_zAngle = 112.0;
  this->m_pglxdata = glxdata;
  pthread_create(&m_render_thread, NULL, Map::create_pthread, this);
}

void Map::render_data()
{
  m_pglxdata->cx = glXCreateContext( 
                                     m_pglxdata->dpy,
                                     m_pglxdata->vi,
                                     None,  
                                     True
                                   );
  if (NULL == m_pglxdata->cx) {
    fprintf(stderr, "could not create rendering context");
    return;
  }
  glXMakeCurrent(m_pglxdata->dpy, m_pglxdata->win, m_pglxdata->cx);
  static Bool displayListInited = false;
  glEnable(GL_DEPTH_TEST);
  while(m_render) {
    if(m_update_viewport) {
      glViewport(0, 0, m_width, m_height);
      m_update_viewport = false;
      m_update = true;
    }
    if(m_update) {
      glMatrixMode(GL_PROJECTION);
      glLoadIdentity();
      gluPerspective(60, 1, 1, 100);
      glMatrixMode(GL_MODELVIEW);
      glLoadIdentity();
      glTranslatef(0.0, 0.0, -3.0);
      glRotatef(m_xAngle, 0.1, 0.0, 0.0);
      glRotatef(m_yAngle, 0.0, 1.0, 0.0);
      glRotatef(m_zAngle, 0.0, 0.0, 1.0);
      if (displayListInited) {
        glCallList(1);
      } else {
        glNewList(1, GL_COMPILE_AND_EXECUTE);
          glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
          glBegin(GL_QUADS);
            // Front face
            glColor3f(0.0, 0.7, 0.1); // Green
            glVertex3f(-1.0, 1.0, 1.0);
            glVertex3f(1.0, 1.0, 1.0);
            glVertex3f(1.0, -1.0, 1.0);
            glVertex3f(-1.0, -1.0, 1.0);
            // Back face
            glColor3f(0.9, 1.0, 0.0); // Yellow
            glVertex3f(-1.0, 1.0, -1.0);
            glVertex3f(1.0, 1.0, -1.0);
            glVertex3f(1.0, -1.0, -1.0);
            glVertex3f(-1.0, -1.0, -1.0);
            // Top face
            glColor3f(0.2, 0.2, 1.0); // Blue
            glVertex3f(-1.0, 1.0, 1.0);
            glVertex3f(1.0, 1.0, 1.0);
            glVertex3f(1.0, 1.0, -1.0);
            glVertex3f(-1.0, 1.0, -1.0);
           // Bottom face
            glColor3f(0.7, 0.0, 0.1); // Red
            glVertex3f(-1.0, -1.0, 1.0);
            glVertex3f(1.0, -1.0, 1.0);
            glVertex3f(1.0, -1.0, -1.0);
            glVertex3f(-1.0, -1.0, -1.0);
          glEnd();
        glEndList();
        displayListInited = true;
      }
      if (m_pglxdata->double_buffer) {
        glXSwapBuffers(m_pglxdata->dpy, m_pglxdata->win);
      } else {
        glFlush();
      }
      m_update = false;
    }
  }
}

Map::~Map()
{
}

Makefile

all : linux_main

linux_main : map.o linux_main.o
    g++ -pthread -Wall linux_main.o map.o -o linux_main -lGL -lXext -lX11 -lGLU

linux_main.o : linux_main.cpp
    g++ -pthread -Wall -c -o linux_main.o linux_main.cpp

map.o : map.cpp
    g++ -pthread -Wall -c -o map.o map.cpp

clean :
    rm -f ./*.o linux_main
\$\endgroup\$
0

2 Answers 2

3
\$\begingroup\$

I would clean up the makefile
You don't want to do it separately for each file as you then get cut/paste errors.
You want to generalize this as much as possible (so that any change only needs to be done in one place (not in multiple locations).

PS. Not tested.

CXX        = g++
CXXFLAGS   += -Wall -pthread 

TARGET      = linux_main
SRC_FILES   = $(wildcard *.cpp)
OBJ_FILES   = $(patsubst %.cpp,%.o,$(SRC_FILES))

all: $(TARGET)

$(TARGET): $(OBJ_FILES)
    $(CXX) -o $(TARGET) $* $(CXXFLAGS) -lGL -lXext -lX11 -lGLU

clean:
    rm -f $(OBJ_FILES) $(TARGET)

The main():
In C++ we try and use objects and RAII to make the code exception safe.
Any time you have init() followed by a dest() pair it is an indication that you should have wrapped in a class and done the init() in the constructor and the dest() in the destructor.

Also note you never actually initialize gsp_Map so if it fails to initialize you end up deleting an invalid pointer.

int 
main(int argc, char *argv[]) {
  if (!init_instance()) {
    fprintf(stderr, "Failed to initialize application instance\n");
  }

  message_loop();

  if(gsp_Map) {
    delete gsp_Map;
  }
}

This would have been much neater to implement as:

int main(int argc, char *argv[])
{
     try
     {
         // Move the code for init_instance() into the Map constructor
         // Make sure the destructor tides up correctly.
         Map  mapInstance;

         // Try to reduce the amount of global accessible state.
         // It makes testing really hard to do well. Instead pass objects as
         // parameters if the function needs to use it.
         message_loop(mapInstance);
     }
     catch(...)
     {
         fprintf(stderr, "Failed to initialize application instance\n");
     }
}

In C++ code there is no need for the typedef here:

typedef
struct glx_data
{
  Display *dpy;
  Window win;
  XVisualInfo *vi;
  Colormap cmap;
  XSetWindowAttributes swa;
  GLXContext cx;
  XEvent event;
  int dummy;
  bool double_buffer;
} GLXDATA;

Each of the pointers in this structure are initialied by a function call that obviously allocates so object and returns a pointer. I don't see any code that correctly tides this up. You should wrap each of these pointers in an class that will correctly tidy itself (otherwise at each failure point you need to write code to tidy up the object correctly).

These constructors in Map are obviously not doing anything:

Map::Map()
{
}

Map::Map(Map &copy)
{
}

Since you have a constructor that does real work:

Map::Map(GLXDATA *glxdata, int width, int height)

You should delete the useless ones above they are dangerous at best (you should probably make the copy constructor and assignment operator private until you work out what is happening with all the pointers flying around your code).

The destructor is useless. Delete it and let the default compiler version do its job until you have some actual work to do here.

Map::~Map()
{
}

Yoda style testing was popular in the 80/90 but has since been discarded as untatural to read.

if (NULL == glxdata.vi)

// Most people find it easier to read the other way around

if (glxdata.vi == NULL)

There is no real advantage to the Yoda style (it is trying to protect you from accidental assignment) as all compilers will warn you if you accidentally do an assignment in a test and since you are compiling with warnings turned on and striving to have zero warning code it should not happen.

if (glxdata.vi = NULL)
              ^^^   Notice the single =

You are using fprint() rather than std::cerr (and your error messages and clean can be better handled by exceptions).

Inside glxdate them members

GLXContext cx;
XEvent     event;

don't look like they should be part of this structrue. They are not used by it and only really manipulated by functions outside the structure.

Member methods declared inside the class declaration are automatically inline. So there is no need to actually specify this:

    inline pthread_t &get_pthread() {return m_render_thread;}
    inline void set_update(bool update) {m_update = update;}

Though I will sometimes put one liners in the header file I still usually prefer to put all methods into the source file (let the compiler worry about the inline optimization you have more important things to do).

Technically this is not allowed:

pthread_create(&m_render_thread, NULL, Map::create_pthread, this);

The pthreads library is a C-Library and thus only understands the C ABI. It has no concept of the C++ ABI and thus can not call C++ functions and definitely can not call class methods. This just happens to work on some systems as you are getting lucky that that the ABI for static member methods is the same as the C-ABI.

Best to just make a C-function so that you are guaranteed for it to work. This C function can call your static member in a valid fashion.

extern "C" void* create_pthread(void* data)
{
    Map* mapObject = reinterpret_cast<Map*>(data);
    return Map::create_pthread(mapObject);
}

In general there is still too much reaching into objects and interacting with their members. Your classes should define an interface that are verbs that allow you to interact with them but they should not expose the members.

In particular the gxd_data and Map obejct are so interrelated that you should just make gxd_data a normal member of the Map (or pass it in by reference).

Edit

To try and find things I started wrapping things.
Note this is not a perfect design because I did not have a good test environment thus I was trying to wrap code without altering it too much:

glx_data.h

#ifndef GLX_DATA_
#define GLX_DATA_

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <pthread.h>

#include <stdexcept>

#include <X11/Xlib.h>
#include <GL/glx.h>
#include <GL/gl.h>
#include <GL/glu.h>


class DisplayWrapper
{
    Display*    dpy;
    public:
    DisplayWrapper()
    {
        dpy = XOpenDisplay(NULL);
        if (dpy == NULL)
        {   throw std::runtime_error("Failed to get XDisplay");
        }
        int dummy;
        if (!glXQueryExtension( dpy, &dummy, &dummy))
        {   throw std::runtime_error("X server has no OpenGL GLX extension");
        }
    }
    ~DisplayWrapper()
    {
        // Add missing code to clean up Query extension
        // TODO

        // Add missing code to clean up display here:
        // TODO
    }
    // Not a perfect wrapper but an example
    // This is a conversion operator.
    // It is bad practice to use them;
    // I am just using it here to minimize changes to the original code.
    operator Display*() {return dpy;}
};
class XVisualInfoWrapper
{
    static int dblBuf[];
    static int snglBuf[];

    XVisualInfo*    vi;
    bool            double_buffer;

    public:
    XVisualInfoWrapper(DisplayWrapper& dpy)
    {
        double_buffer = true;
        vi            = glXChooseVisual( dpy, DefaultScreen(static_cast<Display*>(dpy)), dblBuf);
        if (vi == NULL)
        {
            vi = glXChooseVisual( dpy, DefaultScreen(static_cast<Display*>(dpy)), snglBuf);
            if (vi == NULL)
            {   throw std::runtime_error("No RGB visual with depth buffer");
            }
            double_buffer = false;
        }
    }
    ~XVisualInfoWrapper()
    {
        // Add missing code to clean up VisualInfo
        // TODO
    }
    // Not a perfect wrapper but an example
    // This is a conversion operator.
    // It is bad practice to use them;
    // I am just using it here to minimize changes to the original code.
    operator XVisualInfo*() {return vi;}

    bool isDoubleBuffered() const {return double_buffer;}

};

struct ColormapWrapper
{
    Colormap            cmap;
    ColormapWrapper(DisplayWrapper& dpy, XVisualInfoWrapper& vi)
    {
        cmap = XCreateColormap( dpy,
                                RootWindow(static_cast<Display*>(dpy),static_cast<XVisualInfo*>(vi)->screen),
                                static_cast<XVisualInfo*>(vi)->visual,
                                AllocNone
                              );
    }
    ~ColormapWrapper()
    {
        // TODO Destroy the colourmap
    }
    // Not a perfect wrapper but an example
    // This is a conversion operator.
    // It is bad practice to use them;
    // I am just using it here to minimize changes to the original code.
    operator Colormap&() {return cmap;}

};

struct SetWindowAttributeWrapper: public XSetWindowAttributes
{
    SetWindowAttributeWrapper(ColormapWrapper& cmap)
    {
        colormap        = cmap;
        border_pixel    = 0;
        event_mask      = ExposureMask | ButtonPressMask | StructureNotifyMask;
    }
};

class WindowWrapper
{
    Window                      win;
    public:
    WindowWrapper(DisplayWrapper& dpy, XVisualInfoWrapper& vi, ColormapWrapper& cmap, SetWindowAttributeWrapper& swa)
    {
        win = XCreateWindow(    dpy,
                                RootWindow(static_cast<Display*>(dpy), static_cast<XVisualInfo*>(vi)->screen),
                                0, 0,
                                300, 300,
                                0,
                                static_cast<XVisualInfo*>(vi)->depth,
                                InputOutput,
                                static_cast<XVisualInfo*>(vi)->visual,
                                CWBorderPixel | CWColormap |
                                CWEventMask,
                                &swa
                            );

        XSetStandardProperties(dpy,win, "glxsimple", "glxsimple", None, NULL, 0, NULL);
    }
    ~WindowWrapper()
    {
        // TODO Correctly destroy the window
    }
    // Not a perfect wrapper but an example
    // This is a conversion operator.
    // It is bad practice to use them;
    // I am just using it here to minimize changes to the original code.
    operator Window() {return win;}
};

struct glx_data
{
    DisplayWrapper              dpy;
    XVisualInfoWrapper          vi;
    ColormapWrapper             cmap;
    SetWindowAttributeWrapper   swa;
    WindowWrapper               win;
    GLXContext cx;

    glx_data()
        : dpy()
        , vi(dpy)
        , cmap(dpy, vi)
        , swa(cmap)
        , win(dpy, vi, cmap, swa)
    {}
};

#endif//GLX_DATA

glx_data.cpp

#include "glx_data.h"

int XVisualInfoWrapper::dblBuf[]  = { GLX_RGBA, GLX_RED_SIZE, 1, GLX_GREEN_SIZE, 1, GLX_BLUE_SIZE, 1, GLX_DEPTH_SIZE, 12, GLX_DOUBLEBUFFER, None };
int XVisualInfoWrapper::snglBuf[] = { GLX_RGBA, GLX_RED_SIZE, 1, GLX_GREEN_SIZE, 1, GLX_BLUE_SIZE, 1, GLX_DEPTH_SIZE, 12, None };

Map.h

// 3. map.h
#ifndef MAP_H_
#define MAP_H_

#include "glx_data.h"

class Map
{
    glx_data glxdata;

    public:
        Map(int width, int height);
        void getEvent(XEvent& event)
        {
            XNextEvent(glxdata.dpy, &event);
        }
        bool pending()
        {
            return XPending(glxdata.dpy);
        }
        pthread_t &get_pthread() {return m_render_thread;}
        void set_update(bool update) {m_update = update;}
        void inc_xrotate(GLfloat deg) 
        {
            if ((m_xAngle + deg) < 360) {
                m_xAngle += deg;
            } else {
                m_xAngle = deg - (360 - m_xAngle);
            }
        }
        void inc_yrotate(GLfloat deg) 
        {
            if ((m_yAngle + deg) < 360) {
                m_yAngle += deg;
            } else {
                m_yAngle = deg - (360 - m_yAngle);
            }
        }
        void inc_zrotate(GLfloat deg) 
        {
            if ((m_zAngle + deg) < 360) {
                m_zAngle += deg;
            } else {
                m_zAngle = deg - (360 - m_zAngle);
            }
        }
        void set_deminsions(int width, int height) 
        {
            m_width = width;
            m_height = height;
        }
        void render_data();
    private:
        pthread_t m_render_thread;
        bool m_render;
        bool m_update;
        bool m_update_viewport;
        int m_width;
        int m_height;
        GLfloat m_xAngle;
        GLfloat m_yAngle;
        GLfloat m_zAngle;
        static void *create_pthread(void *data)
        {
            Map *thread_map = static_cast<Map *>(data);
            thread_map->render_data();
            return data;
        }
};

#endif//MAP_H_

Map.cpp

// 4. map.cpp
#include "map.h"

Map::Map(int width, int height)
{
    m_width = width;
    m_height = height;
    m_render = true;
    m_update = true;
    m_update_viewport = true;
    m_xAngle = 42.0;
    m_yAngle = 82.0;
    m_zAngle = 112.0;
    pthread_create(&m_render_thread, NULL, Map::create_pthread, this);

    XMapWindow(glxdata.dpy, glxdata.win);
}

void Map::render_data()
{
    glxdata.cx = glXCreateContext(
            glxdata.dpy,
            glxdata.vi,
            None,
            True
            );
    if (NULL == glxdata.cx) {
        fprintf(stderr, "could not create rendering context");
        return;
    }
    glXMakeCurrent(glxdata.dpy, glxdata.win, glxdata.cx);
    static Bool displayListInited = false;
    glEnable(GL_DEPTH_TEST);
    while(m_render) {
        if(m_update_viewport) {
            glViewport(0, 0, m_width, m_height);
            m_update_viewport = false;
            m_update = true;
        }
        if(m_update) {
            glMatrixMode(GL_PROJECTION);
            glLoadIdentity();
            gluPerspective(60, 1, 1, 100);
            glMatrixMode(GL_MODELVIEW);
            glLoadIdentity();
            glTranslatef(0.0, 0.0, -3.0);
            glRotatef(m_xAngle, 0.1, 0.0, 0.0);
            glRotatef(m_yAngle, 0.0, 1.0, 0.0);
            glRotatef(m_zAngle, 0.0, 0.0, 1.0);
            if (displayListInited) {
                glCallList(1);
            } else {
                glNewList(1, GL_COMPILE_AND_EXECUTE);
                glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
                glBegin(GL_QUADS);
                // Front face
                glColor3f(0.0, 0.7, 0.1); // Green
                glVertex3f(-1.0, 1.0, 1.0);
                glVertex3f(1.0, 1.0, 1.0);
                glVertex3f(1.0, -1.0, 1.0);
                glVertex3f(-1.0, -1.0, 1.0);
                // Back face
                glColor3f(0.9, 1.0, 0.0); // Yellow
                glVertex3f(-1.0, 1.0, -1.0);
                glVertex3f(1.0, 1.0, -1.0);
                glVertex3f(1.0, -1.0, -1.0);
                glVertex3f(-1.0, -1.0, -1.0);
                // Top face
                glColor3f(0.2, 0.2, 1.0); // Blue
                glVertex3f(-1.0, 1.0, 1.0);
                glVertex3f(1.0, 1.0, 1.0);
                glVertex3f(1.0, 1.0, -1.0);
                glVertex3f(-1.0, 1.0, -1.0);
                // Bottom face
                glColor3f(0.7, 0.0, 0.1); // Red
                glVertex3f(-1.0, -1.0, 1.0);
                glVertex3f(1.0, -1.0, 1.0);
                glVertex3f(1.0, -1.0, -1.0);
                glVertex3f(-1.0, -1.0, -1.0);
                glEnd();
                glEndList();
                displayListInited = true;
            }
            if (glxdata.vi.isDoubleBuffered()) {
                glXSwapBuffers(glxdata.dpy, glxdata.win);
            } else {
                glFlush();
            }
            m_update = false;
        }
    }
}

linux_main.cpp

// 2. linux_main.cpp  


#include <iostream>
#include "glx_data.h"
#include "map.h"


void message_loop(Map& gsp_Map)
{
    while (1) {
        do {
            XEvent event;
            gsp_Map.getEvent(event);
            switch (event.type) {
                case ButtonPress:
                    switch (event.xbutton.button) {
                        case 1:
                            gsp_Map.inc_xrotate(10);
                            gsp_Map.set_update(true);
                            break;
                        case 2:
                            gsp_Map.inc_yrotate(10);
                            gsp_Map.set_update(true);
                            break;
                        case 3:
                            gsp_Map.inc_zrotate(10);
                            gsp_Map.set_update(true);
                            break;
                    }
                    break;
                case ConfigureNotify:
                    gsp_Map.set_deminsions(
                            event.xconfigure.width,
                            event.xconfigure.height
                            );
                case Expose:
                    gsp_Map.set_update(true);
                    break;
            }
        } while (gsp_Map.pending());
    }
    pthread_join(gsp_Map.get_pthread(), NULL);
}

int
main(int argc, char *argv[])
{
    try
    {
        Map gsp_Map(300,300);
        message_loop(gsp_Map);
    }
    catch(std::exception const& e)
    {
        std::cerr << "Exception: Failed to initialize application instance\nBecause: " << e.what() << "\n";
    }
}
\$\endgroup\$
7
  • \$\begingroup\$ Loki thanks again for your wonderful input, I have slightly altered the code. I did less wrapping than you suggest but I think it is getting better. What are your thoughts? \$\endgroup\$ Commented Jan 3, 2012 at 20:43
  • \$\begingroup\$ Hate your main(). There is no need to call a create method. Just create an object. If it works then you just use it. If it fails it throws an exception and you print an error. \$\endgroup\$ Commented Jan 4, 2012 at 0:40
  • \$\begingroup\$ You may as well drop the use of inline it has no use. The compiler will inline the functions as its sees fit. You trying to tell the compiler what to inline will be ignored the compiler, as it is much better at it (and it knows it, and will silently ignores all your suggestions). \$\endgroup\$ Commented Jan 4, 2012 at 0:42
  • \$\begingroup\$ If the call to if (initialize_window(width, height)) I don't see any clean up code to remove the resources (Bad habbit to get into). \$\endgroup\$ Commented Jan 4, 2012 at 0:45
  • \$\begingroup\$ This call can fail: pthread_create(&m_render_thread, NULL, create_pthread, this); You should check for failure and act appropriately. \$\endgroup\$ Commented Jan 4, 2012 at 0:45
0
\$\begingroup\$
static int snglBuf[] = { GLX_RGBA, GLX_RED_SIZE, 1, GLX_GREEN_SIZE, 1,
                     GLX_BLUE_SIZE, 1, GLX_DEPTH_SIZE, 12, None };

Should that be const? I see it is used in one place, but I can’t tell if that modifies its value. If this is indeed constant, make it constexpr just to make sure it produces the table at compile time with no overhead.

static Map *gsp_Map;

The style in C++ is to put the * or & with the type, not the identifier. This is called out specifically near the beginning of Stroustrup’s first book, and is an intentional difference from C style.

static GLXDATA glxdata;

bool init_instance() {
  glxdata.dpy = XOpenDisplay(NULL);
  if (glxdata.dpy == NULL) {
    fprintf(stderr, "Failed to get XDisplay\n");
    return false;
  }

You should put the initializer right on the variable, rather than having to call the init_instance function. It could be a constructor for glxdata.

I take it that the stuff in the headers is C, not C++, and is used for C programs too. But you can derive a class from a C struct, adding member functions. This is a very handy technique.

class glxdata_t : public glx_data {
public:
    glxdata_t() { dpy = XopenDisplay(NULL);  /* etc */ }
    ~glxdata() ⋯
    other handy stuff to encapsulate
};

Be sure you use the real name of the struct, not the all-caps typedef, when you derive from it.

I assume XopenDisplay is a C function, so I kept it as-is. In C++, you do not use the NULL macro.

Given that the work is done automatically, you can then (just) report an error as part of your set-up.

if (NULL == glxdata.vi) {

Don’t use NULL macro in C++. Don’t compare pointers (or pointer-like objects) against nullptr explicitly; rather, use their truth value (an operator bool in smart pointer types)

if (glxdata.vi)

but as noted earlier, this should all be in the constructor, so it’s just vi not glxdata.vi.

  fprintf(stderr, "No RGB visual with depth buffer");

You are using C library output? I suppose this whole function was just copied from example code that was for C. Even in C, fprintf with a string that does not contain any replacement sequences is just a pure waste of time.

std::cerr << "No RGB vis⋯";

(In real code, this would go to a logging module, or just use that string for the exception text)

gsp_Map = new Map(&glxdata, 300, 300);

Core Guideline C.149 — no naked new or delete.

You should probably make this a unique_ptr as a drop-in replacement without otherwise changing the architecture.

inline pthread_t &get_pthread() {return m_render_thread;}

This is a pure accessor; it should be const. It’s already been noted that the inline keyword is not needed here. I won’t repeat the earlier observations on the overall class design.

  inline void inc_xrotate(GLfloat deg) 
  {
    if ((m_xAngle + deg) < 360) {
      m_xAngle += deg;
    } else {
      m_xAngle = deg - (360 - m_xAngle);
    }
  }

Did you notice that there are three functions with exactly the same contents except for the name of one variable?

At the very least, use a helper function to condition the variable:

    m_xAngle= keep_in_circle (m_xAngle, deg);

(though I don’t see why you don’t just modulo 360 after adding)
But better for here and other uses in general would be to get rid of three separately named variables for x, y, and z and use an array of 3 values so they can be indexed by a loop.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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