Take the 2-minute tour ×
Game Development Stack Exchange is a question and answer site for professional and independent game developers. It's 100% free, no registration required.

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.

share|improve this question
    
Question: What sort of engine is this? What is the expected scope of the project? I'm a little worried that I might be giving poor advice based on a misconception of what you intended your project to do. –  ChrisE Mar 21 '11 at 16:39
add comment

5 Answers

up vote 1 down vote accepted

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.

share|improve this answer
    
After some of the comments I decided that I will define a private struct FontData and have a std::shared_ptr<FontData> as a private member instead of the void*. This means I can avoid forward declaration too, which means there's no mention of the library in the header file. :) –  Paul Manta Mar 20 '11 at 21:24
    
Ok, nice solution. –  jv42 Mar 21 '11 at 9:03
add comment

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!

share|improve this answer
1  
There are other stylistic decisions that you've been taken to task over (load() methods, void*, use of singletons, etc.) that, frankly, are completely overshadowed by the lack of functionality your proposed interface exposes. You shouldn't waste time worrying over what software development tropes you should be using when your software engineering here is so goddamn broken it makes my eyes bleed. –  ChrisE Mar 21 '11 at 16:14
1  
You're off to a good start, you just need some more experience so you can learn to spot what functionality you are missing. Check out SFML, make some fun little apps, and then come back to this when you've got a clearer idea what capabilities you need to expose. Don't worry how ugly the plumbing is until you've got everything you need otherwise. –  ChrisE Mar 21 '11 at 16:16
1  
Interface: Yes, it is an interface. It's the API kind of interface, not the virtual class type of interface. Void-pointer: Already sorted out. See my comments on jv42's answer. | Image copying: You make that assumption based on what? | Manipulation: A lot of details of the classes I haven't yet implemented or I considered only a distraction to the question. Also, you seem to be asking be for things like image rotation, getting the name of the font, etc. I'll implement those when and if the need arises, not now. [contd.] –  Paul Manta Mar 21 '11 at 21:06
1  
[cont.] SFML: Indeed, I just discovered this library and I like it a lot more. | Bottom line: What I've shown here is only the core (loading, and rendering). Other details I considered unimportant to show. Also, I won't start implementing any features (rotation, skewing, scaling) unless I later find I need them. It'll be easy to do that anyway. –  Paul Manta Mar 21 '11 at 21:09
1  
+1 for SFML. It'll get rid of most of this interface stuff with its API. :P –  The Communist Duck Mar 21 '11 at 21:24
show 9 more comments

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.
share|improve this answer
    
Load: No, it should be an Image class' job to load itself, the same way it's std::ifstream's job to load itself. This is not the place to do mem management: it will be done in a separate ResourceManager class. | Strings: It's not hard converting std::string to char*, so that's not the reason. the reason is simply that std::string would provide no advantage. –  Paul Manta Mar 21 '11 at 8:01
add comment

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.

share|improve this answer
2  
Singleton: Singletons aren't bad if they're used right. Basically, the renderer hasn't any state to speak of so singleton is okay here (it's more complex than that, though). | Free: Deallocation is handled in the destructor. The free() function is there for the same reason the ifstream::close() function exists. | Blit: I'm trying to separate the data from the mechanism that uses the data. | Sprites: This is just the graphics module. Sprites are to be handled in the higher levels of the engine, in a manner similar to what you described. [contd.] –  Paul Manta Mar 20 '11 at 17:43
    
[cont.] Inheritance: I don't understand how I could use inheritance here. | Void-pointer: Why shouldn't it be used? I thought it's a good way to limit the number of files that have access to the 3rd-party library. data_ is just an SDL_Surface* cast to void*. Whenever I use it I cast it back to SDL_Surface*. | Blit [contd.]: Another way of thinking about this is, the image doesn't draw itself &mdash; the renderer draws it. The image simply exists. –  Paul Manta Mar 20 '11 at 17:47
    
Thanks for showing me std::shared_ptr, though. Thanks definitely going to come in handy. –  Paul Manta Mar 20 '11 at 18:12
1  
Free: depending the system, destroying the object and its associated resources (such as a texture) may need to happen in different threads, hence the need for a free() function; also, one could be using a custom allocation scheme that does not call the destructor. –  Sam Hocevar Mar 20 '11 at 18:14
1  
@DeadMG I'm also confused on how to provide encapsulation with inheritance. I always figured the best way to hide private members would be sticking to the strongly typed opaque pointer. Could you elaborate on your inheritance suggestion? –  michael.bartnett Mar 20 '11 at 19:44
show 1 more comment

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?
share|improve this answer
    
Good spot on the const char*, I hadn't noticed that. –  DeadMG Mar 20 '11 at 17:39
    
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*). | Strings: Do I have any reason not to use them? I don't do any manipulations, I just need a string. I'm free to use std::string outside of these classes, if I so desire. | Singletons: Yes, more than one GR will break my code. GR initializes SDL. Having it a singleton I don't have to worry about when SDL gets initialized or exited. –  Paul Manta Mar 20 '11 at 17:58
    
The advantage of less files including SDL.h is pretty much zero. Somehow, I wouldn't have a renderer intialize SDL. I would abstract that to the main engine, or something else. What if you wanted other parts of SDL, except graphics? –  The Communist Duck Mar 20 '11 at 18:20
    
Other components: I would initialize them separately, in the components that need those parts. | Initialization: The main engine (GameManager, usually) shouldn't care about details such as what library I'm using. In my current setup I don't have to worry about initialization one bit, everything happens automatically (and is ensured to happen exactly when I need it to happen, not later, not sooner). | Header: Yes, there's no real technical advantage. I do find the code to be neater when the underlying library isn't exposed to me in the class interface, however. –  Paul Manta Mar 20 '11 at 18:30
1  
More of a thing that the only points I can see wrong with the design you have reasons for. I still stand by that singletons are not needed here, and that void pointers should be replaced with SDL_Surface ones. :P –  The Communist Duck Mar 20 '11 at 19:03
show 3 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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