Graphics module: Am I going the right way?

Paul Manta
  • Graphics module: Am I going the right way? Paul Manta

    I'm trying to write the graphics module of my engine. That is, this part of the code only provides an interface through which to load images, fonts, etc and draw them on the screen. It is also a wrapper for the library I'm using (SDL in this case).

    Here are the interfaces for my Image, Font and GraphicsRenderer classes. Please tell me if I'm going the right way.

    Image

    class Image
    {
      public:
        Image();
        Image(const Image& other);
        Image(const char* file);
        ~Image();
    
        bool load(const char* file);
        void free();
        bool isLoaded() const;
    
        Image& operator=(const Image& other);
    
      private:
        friend class GraphicsRenderer;
        void* data_;
    };
    

    Font

    class Font
    {
      public:
        Font();
        Font(const Font& other);
        Font(const char* file, int ptsize);
        ~Font();
    
        void load(const char* file, int ptsize);
        void free();
        bool isLoaded() const;
    
        Font& operator=(const Font& other);
    
      private:
        friend class GraphicsRenderer;
        void* data_;
    };
    

    GrapphicsRenderer

    class GraphicsRenderer
    {
      public:
        static GraphicsRenderer* Instance();
    
        void blitImage(const Image& img, int x, int y);
        void blitText(const char* string, const Font& font, int x, int y);
        void render();
    
      protected:
        GraphicsRenderer();
        GraphicsRenderer(const GraphicsRenderer& other);
        GraphicsRenderer& operator=(const GraphicsRenderer& other);
        ~GraphicsRenderer();
    
      private:
        void* screen_;
    
        bool initialize();
        void finalize();
    };
    

    Edit: Some changes to the code and more details.

    Per some of the discussions here I decided to replace my use of void* with something like this:

    class Image
    {
      private:
        struct ImageData;
        std::shared_ptr<ImageData> data_;
    };
    

    (Obviously I'll do the same thing for the Font class.)

    I should also mention hat these are not my final, complete classes. I only show here the basic functionality (loading and rendering). Instead of telling me what functionality you think I might need to add (rotating images, skewing, scaling, etc) just concentrate on reviewing what I already have. I'll try to defend my choices it I can, or change my approach if I cannot.

  • Singleton = BAD, remove immediately. Fonts and Images should not have a free() function, that should be the job of the destructor. The GraphicsRenderer should not offer those Blit functions, you should offer object-orientated classes which will provide a position for each end result, and the actual rendering automatically managed by the GraphicsRenderer. Finally, for encapsulation, use inheritance, not PIMPL, and definitely do not use a void*, use a strongly typed opaque pointer.

    Here are some excerpts from my own design, although I've used compile-time switching and not run-time inheritance.

    class D3D9Render 
    {
    public:
        std::shared_ptr<D3D9Font> CreateFont();
    };
    class D3D9Font 
    {
    public:
        // PUBLIC INTERFACE ---------------------------------------------------
        std::unique_ptr<D3D9Text> CreateText();
    
        int Height();
        D3D9Font* Height(char newheight);
        D3D9Font(D3D9Render& ref);
    
        int Width();
        D3D9Font* Width(char newwidth);
        int Weight();
        D3D9Font* Weight(short newweight);
        bool Italic();
        D3D9Font* Italic(bool newitalic);
        string Font();
        D3D9Font* Font(string str);
        D3D9Font* CommitChanges();
    };
    
    class D3D9Text 
    {
    public:
    
        // PUBLIC INTERFACE ---------------------------------------------------
        D3D9Text* Text(string str);
        D3D9Text* PositionSizeX(short newx, short newxsize);
        D3D9Text* PositionSizeY(short newy, short newysize);
        D3D9Text* Font(std::shared_ptr<D3D9Font> ref);
        D3D9Text* Colour(unsigned int newcolour);
        string Text();
        int PositionX();
        int PositionY();
        int SizeX();
        int SizeY();
        std::shared_ptr<D3D9Font> Font();
        unsigned int Colour();
        D3D9Text* CommitChanges();
    };
    

    Here, the memory is managed for you- all ownership is managed via smart pointing, and the interface is completely object orientated.

  • First things I can spot:

    • Singletons are very very bad, usually. It's not 'Hm, do I only want one of these?' but more 'Hm, will more than one of these break the program?'.
    • void* pointers are also rather risky. Why do you need one? Bad design somewhere.
    • Font and Image seem to be closely related. Maybe you could take some of the functionality up the hierachy to a Renderable.
    • I think this is me, but any reason why you're using const char* over std::string?

  • The thing that itches most for me is the void * 'abuse'.

    Void-pointer: I don't need it. It's a way for me to limit the number of files that include SDL.h (void* data_ is just an SDL_Surface* cast to void*)

    Well, you could avoid inclusion (which I approve BTW) by forward declarating it somewhere convenient for you.

  • I'm a bit bothered by the "load" method, which breaks the Single-Responsibilty Principle (by the way, to the Communist Duck, I guess this is why he's using a const char*, because the SDL loading image function, coded in C, do not take a std::string). It shouldn't be an Image class' job to load itself, for at least two reasons :

    • If you intend to optimize your memory use, you'll want to have specialized classes for loading resources.
    • You'll handle exceptions more easily with a dedicated class.

  • On Interfaces (in general)

    So, you asked for us to review your designs for interfaces.

    You didn't give us interfaces, you gave us full class declarations. If these were interfaces, I would expect to see something like:

    virtual bool load file(const char* file) = 0;
    

    That, in C++, is an interface. I can override it in a subclass that implements functionality (in fact, I must!). If you are writing an interface, you are enforcing policy, and the above is how you do this.

    Half of the complaints about usage of void * in the other answers would've been avoided if you had just exposed the interface functions, and kept the member variables hidden (as they should be, in an interface class).

    Rawr.

    On Interfaces (yours)

    Image: Copying

    You've got a copy constructor, and an equals operator. The issue I see here is that there is no good way of preventing the user from making silly extraneous copies of images.

    For you, using SDL_surfaces, this is a big issue. Without meaning to be offensive, I'm willing to bet that you haven't considered what happens when you free an Image that is a duplicate of another image. I'm further willing to bet that you hadn't planned on handling full deep copying of the SDL_surface, and so in the aforementioned case you are likely to free an Image, and then your other copies of it will explode, killing everyone you love.

    Solution: NO COPIES. Don't do it, don't allow it. Use a factory or a C-style loader function to create new instances of an Image, and use those instead of allowing copying or equals assignments. Alternately, completely figure out how to deep-copy an SDL_image (not super hard, but annoying).

    Image: manipulation

    How do I change your images once I've loaded them? According to your interface, I don't. Still, are you sure that is a good idea? How do I find out the bit-depth of an image? Its height? Width? Color space?

    Font

    How do I draw with this font? How do I get its name? How do I prevent the copying issues I complained about above? How do I set its color? Kerning? What about Unicode support?

    Renderer: General

    So, I notice you have a couple of blit*() functions, and also a render() function. This seems to imply that you want users to be able to queue up a bunch of blitting operations, and then flush them all to screen at once using the render() call.

    That's fine; in fact, that's how my group's engine tech handles it too. :)

    The use of a singleton here is acceptable, mostly because you seem to want to be letting the renderer have complete control of drawing things. If there is only one instance of it (as there probably should be), this won't hurt anything. Not what we do, but hey, it's a matter of taste.

    There are a couple of big issues I see here, though.

    Renderer: Transformations

    You seem to be working only in 2D. That's fine. BUT...

    How do you handle things like rotating an image when you draw it? Scaling it? You need full support for something called affine transformations. This allows you to easily rotate, scale, translate, skew, and otherwise frob in a pretty fashion images.

    This needs to be supported (somehow) for both text and images.

    Renderer: Coloring and Blending

    I want to be able to blend colors onto my images, and set the colors for my text. You should expose this.

    I also want to be able to do things like blend images when blitting, so I can do things like translucent ghosts or smoke or fire.

    How to save yourself the trouble

    Use SFML. It's got better design for, well, pretty much everything over SDL. They've already done what you are trying to do here. At the very least, look at how they've spec'ed their interfaces, and how they've designed their class hierarchy.

    Also note that they address the issue of transforms I pointed out earlier in their Drawable class. And coloring. And blending.

    They've got good tutorials and documentation, so it might be worth your time to fiddle with it a bit and get a feel for what your code should be able to accomplish.

    Good luck!

Tags
c++ software-engineering sdl
Related questions and answers
  • /////////////////////////////////////////////////////////////////////////////////////////////////////////////// class Sprite { private: string name; char symbol; float shield; int location[2]; bool alive; public: ///////////////////// Get and SET all the privates...}; // for the old/new screen command // Some nCurses setup int r = 0, c = 0; // current row and column (upper-left is (0,0)) const int nrows = 56, // number of rows in window ncols = 79... set_location(int X, int Y) { location[0] = X; location[1] = Y;}; bool Alive() {return alive;}; void SetLife(bool f) {alive= f;}; //////////////////////////////// Move

  • in advance of first use. Here is a bit a sketch of the classes I am using: typedef unsigned int ResourceId; // Resource is an abstract data type. class Resource { Resource(); virtual... // of Resource. Note it only hands give a const reference to the Resource, as // it is read only. template <class T> class ResourceGuard { public: ResourceGuard(T *_resource): resource...)); } // Generally, this will automatically load/unload data, and is called // once per frame. It's also where the caching scheme comes into play. void update(); }; The trouble

  • I'm trying to write my level classes by having a base class that each level class inherits from...The base class uses pure virtual functions. My base class is only going to be used as a vector that'll have the inherited level classes pushed onto it...This is what my code looks like at the moment, I've tried various things and get the same result (segmentation fault). //level.h class Level... supposed to include in the inherited class structure, but this is what I have at the moment... //level1.h class Level1: public Level { public: Level1(); ~Level1(); void load(); void

  • I'm trying to load MD2 models (stolen from Cube) and my loader seems to be loading the models fine, but I can't say the same for the drawing of the model. Here's my code: typedef float vec3_t[3...; float current_time, old_time, interpol; int type, current_frame, next_frame; }; struct md2_glcmd_t { float s; float t; int index; }; #define MD2_IDENT (('2'<<24) + ('P'<<16) + ('D'<<8) + 'I') #define MD2_VERSION 8 class MD2 : public VertexModel { public: MD2(const std::string& Path); ~MD2(); void Draw(timestep_t Time); private

  • " #include <vector> template <typename BaseObject> class CGameObjectFactory { public: // cleanup and release registered object data types bool ReleaseClassTypes...Dear all, this is going to be tough: I have created a game object factory that generates objects of my wish. However, I get memory leaks which I can not fix. Memory leaks are generated by return new Object(); in the bottom part of the code sample. static BaseObject * CreateObjectFunc() { return new Object(); } How and where to delete the pointers? I wrote bool ReleaseClassType(). Despite

  • with different types of objects in the same scene. I was also thinking about making a MeshNode class and then make a Mesh object that contains them, but then I have some conflict on where to store some data... don't want to find out in the end that I have to rewrite everything again, as a lot of other objects will be working with these data. Sorry if it's a too subjective matter, but I need some insight. I'm...well... I'm building the animation system of my game engine (the skeletal and skinned animation stuff), and I came to a point where I added so much functionality in the frame and node structures

  • it is the pad that is causing the problem. Maybe the bullet is being drawn out of the screen already. But I am not sure since I cannot debug maybe somebody can see the flaw in my code. main.cpp class Missile { private: static const double angle = (3.14159265358979323846 / 2); public: bool Alive; static const int V = 5; double X; double Y; void Init(bool alive, int x, int y) { Alive = alive; X = x; Y = y; } void Update() { Y -= V; } void Kill

  • my classes and still have them do what I want. Here's a few examples of a dependency chain: I have a status effect class. The class has a number of methods (Apply/Unapply, Tick, etc.) to apply... related to the graphics library I'm using, but is more of a conceptual thing. In C#, I coupled graphics in with alot of my classes which i know is a terrible idea. Wanting to do it decoupled... as drawable and updatable and I get a stack of screens handling my battle graphics. While I haven't been able to get the screen manager to work perfectly yet, I think I can with some time. My

  • onscreen, in the shape of the terrain rectangle, but there are no regular lines etc. Here's the code I use for rendering: void ShaderProgram::Draw() { using namespace AntiMatter; if( ! m...I'm writing a generic ShaderProgram class that compiles a set of Shader objects, passes args to the shader (like vertex position, vertex normal, tex coords etc), then links the shader components into a shader program, for use with glDrawArrays. My vertex data already exists in a VertexBufferObject that uses the following data structure to create a vertex buffer: class CustomVertex { public