Quote:Original post by MaulingMonkey

This sounds more like an argument against deep class hierarchies than reference passing -- introducing globals only hides the problem that the tree is unwieldy and opaque to understanding. You don't understand the entire system, you only understand the global -- and since anything can modify globals, you probably only think you understand it! (and what's happening to it)

Isn't that always the case?

One of biggest problems with singleton (as a pattern) is that it mixes implementation detail with design.

In a properly designed system, presence of singleton is not a problem. One way to test whether system depends on singleton is this:

struct Foo {  Foo(Bar * bar = BarSingleton::getInstance());};

If the following makes sense, then system is de-coupled to a degree that storage is not mandated.

However, in case of this particular problem, VideoScreen must be Initialize()d - which is a problem, since BarSingleton cannot be auto-constructed as required above (creating a new instance of VideoScreen leaves it in undetermined state), meaning that use of globals is obstructing a bigger problem.

Quote:-> GameApp
--> GamePlayState
---> GameMap
----> Tile
-----> Image // FINALLY gets used here

Hmmm. Warning lights flashing yet?

You've written it down yourself - you hierarchy is not suitable.

Why does image need to know who the VideoScreen is? Image is passive and doesn't do anything to it. Typically, for classes like Image you have something like:

class Image {  Image(InputStream * is) // loads from some abstract source  {    is->readThis();    is->readThat();    is->readSomeMore();    if (not_condition) throw error;  }  void draw(VideoScreen * vs, int x, int y);};

Why does tile need screen? To know the size of screen? It doesn't need that information. A Tile is dumb.

struct Tile : public Rectangle {  // singleton is perfectly fine here in the same way  // std::allocator is fine for STL containers  // but we are free to choose something else  Tile(InputStream * is, ResourceManager * rm = ResourceManager::getSingleton()) {    image = rm->loadCached(is->readTileId());    // read more data about what is contained in this tile  }  Image * image;  std::vector<Foo*> contained;};

GameMap doesn't need it either. Game map doesn't care about coordinates available for rendering. GameMap is determined by level designer.

struct GameMap {  GameMap(InputStream * is) // load from some abstract source  {    extents.x = 0;    extents.y = 0;    extents.w = is->readInt();    extents.h = is->readInt();    for (...) {      tiles.push_back(Tile(is, imageCache));    }  }  void render(VideoScreen * vs, const Rectangle & clip);  const Rectangle & getExtents() const {    return extents;  }  ResourceManager imageCache;  std::vector<Tile> tiles;  Rectangle extents;};

The above example uses typical windowing paradigm, namely view offset is part of VideoScreen (canvas or similar), and render function can query. In addition, we provide clip rectangle to limit what is drawn.

So....

  VideoScreen * vs = new OpenGLVS(); // or DXVS() or SDLVS() or WhateverVS()  // file, memory, internet, pak, ... whatever source  InputStream * is = getReader("levels/level01.map");  GameMap map(is);  vs->setViewOffset(50,20);  // similar to singleton  // unless we really need it, we just use default clip  // and redraw everything, but we have the choice of replacing it   // to optimize the rendering  map.render(vs, vs->getExtents());

So you end up with classes which are independent of each other, yet interoperate in a clear manner.

Note that to create a Tile or Image, you do not need to know anything about GameMap or Game or VideoScreen or anything else. That's not what Image or Tile do - they are dumb and passive and do one thing and one thing only.

Quote:Original post by Rockoon1
This sort of reference spaghetti can be almost as evil as GoTo, but probably not so much in this case. Globals can also be evil, but also not so much in this case.

Spaghetti? By your definition, any code that has more than one function is spaghetti code then? Because the function could be called from any number of places. What do you propose then, put everything inside main()?

By your definition, everything else becomes spaghetti code.

Global variables are not evil in them selves. What is evil is allowing everyone to change a global.

The reason globals are frowned upon is not because everything can see them, it is because everyone can change them. This makes for a debug nightmare. If anyone can change the global, then if an error pops up you have no idea where it came from.

There are two ways to make globals not have that issue. The first is to initialize the global on start up and not allow it to be changed. This makes sure that if there is a problem with the global that it has to be in the constructor or the code calling the constructor. The other way is to pass an instance to the global and that is the only thing that is allowed to change the global. That way all changes go through a single class and instantiated object.

So remember, globals are not bad, only allowing anyone to change them is.

theTroll

In my game code, I've found that in every instance, the cleanest solution I could come up with involved no globals/singletons whatsoever. Globals just take bad design and make it require less syntax. Try removing every global variable in your program and replacing it with arguments. If that makes your code "ugly" or "complicated", all you've really done is taken the lipstick off the pig. Good designs will have small numbers of arguments to methods/constructors and no globals.

When I eventually realized that, I ended up spending a lot more time thinking about code than I used to, but the end result was much cleaner. Look at everything in terms of what it logically should "need" to function, even if, at the current time, that doesn't "work" in your current code base. For instance, looking at your code example:

// I would prefer to do thispixelRenderer->DrawAt(x,y,colour);// as opposed to thispixelRenderer->DrawAt(screen, x, y, colour);// or even thispixelRenderer->setScreen(screen);pixelRenderer->DrawAt(x,y,colour);

Like I said earlier, global variables just reduce the amount of syntax. Looking at it logically, the first DrawAt in essence already receives the screen as an argument (as does every other function in your program). The downside is that there is no way for the caller to change the "argument" to refer to a different screen. The second version of DrawAt is exactly the same, only you are able to pass a specific screen to it. In this case, the second DrawAt is far more flexible, and at the same time gets rid of globals. See? Better (in this case, more flexible) design doesn't have globals in it.

The third design is really not much better than globals. Just like global variables, class variables are kind of implied arguments to all member functions. This can be a good thing, as in a lot of cases you have sets of functions that do all take similar arguments (and this should be the only motivation for a class, IMO). In this case however, does every single function in pixelRenderer need the screen? Probably not. But even if they did, looking at it from another direction: Does a pixelRenderer need a screen to exist? (which, IMO, should be the only motivation for a class variable) The answer should also be no.

As for the readability, yes its a bit more typing. But thats the amount of typing it takes to express your design in C++. Global variables just hide syntax and pollute the design with unnecessary arguments. It boils down to the same thing as goto and control structures. One is shorter, one expresses your design using constructs of the language properly.

Quote:Original post by Antheus
Why does tile need screen? To know the size of screen?

With the hierarchy mentioned, the tile needs the screen so that its image will know what the screen is.

Quote:GameMap doesn't need it either. Game map doesn't care about coordinates available for rendering.

But images, and therefore tiles do, and maps are composed of tiles therefore GameMap needs to know what the VideoScreen is. Only the images care about the "extents", they care about the drawing methods (i.e. DrawImage()) that are provided by the VideoScreen interface.

Quote:

struct GameMap {  GameMap(InputStream * is) // load from some abstract source  {    extents.x = 0;    extents.y = 0;    extents.w = is->readInt();    extents.h = is->readInt();    for (...) {      tiles.push_back(Tile(is, imageCache));    }  }  void render(VideoScreen * vs, const Rectangle & clip);  const Rectangle & getExtents() const {    return extents;  }  ResourceManager imageCache;  std::vector<Tile> tiles;  Rectangle extents;};

The above example uses typical windowing paradigm, namely view offset is part of VideoScreen (canvas or similar), and render function can query. In addition, we provide clip rectangle to limit what is drawn.

So....

  VideoScreen * vs = new OpenGLVS(); // or DXVS() or SDLVS() or WhateverVS()  // file, memory, internet, pak, ... whatever source  InputStream * is = getReader("levels/level01.map");  GameMap map(is);  vs->setViewOffset(50,20);  // similar to singleton  // unless we really need it, we just use default clip  // and redraw everything, but we have the choice of replacing it   // to optimize the rendering  map.render(vs, vs->getExtents());

So you end up with classes which are independent of each other, yet interoperate in a clear manner.

I'm not sure I understand this. I don't think it's very clear at all. Where is the rendering code in this? I can't see where the Image might actually be drawn. Can you clarify on this, please?

Quote:
Note that to create a Tile or Image, you do not need to know anything about GameMap or Game or VideoScreen or anything else. That's not what Image or Tile do - they are dumb and passive and do one thing and one thing only.

But how does the Image know what to draw on?

I've "shallowed out" my class hierarchy in my current project. The design I think I'll go with has a single GameApp object that has a stack of game state classes and a "RenderManager" class. Each state class will have access to Renderer classes, and upon initialization a state will be passed a reference to the RenderManager, at which point it will add the appropriate Renderers to the Rendering manager. Thus, the game state itself doesn't have to care about rendering at all; it only changes the game world and interprets input. This "game world" object is then passed to the RenderManager's rendering function. So, the class hierarchy would look something like this:

GameApp-> RenderManager---> TileRenderer---> UnitRenderer---> etc...-> GameState---> GameWorld (gets passed to each of the renderers when they are added to the RenderManager)

So, the only objects that would need to know about the VideoScreen would be the GameApp (the VS would be a member variable), the RenderManager, and the rendering calls of each of the Renderers.

Is a desirable design, or should I attempt to flatten out my hierarchy even further?

[Edited by - Oberon_Command on October 12, 2008 2:17:06 PM]

Quote:Original post by Oberon_Command

Quote:Original post by Antheus
Why does tile need screen? To know the size of screen?

With the hierarchy mentioned, the tile needs the screen so that its image will know what the screen is.

Quote:GameMap doesn't need it either. Game map doesn't care about coordinates available for rendering.

But images, and therefore tiles do, and maps are composed of tiles therefore GameMap needs to know what the VideoScreen is. Only the images care about the "extents", they care about the drawing methods (i.e. DrawImage()) that are provided by the VideoScreen interface.

Quote:

struct GameMap {  GameMap(InputStream * is) // load from some abstract source  {    extents.x = 0;    extents.y = 0;    extents.w = is->readInt();    extents.h = is->readInt();    for (...) {      tiles.push_back(Tile(is, imageCache));    }  }  void render(VideoScreen * vs, const Rectangle & clip);  const Rectangle & getExtents() const {    return extents;  }  ResourceManager imageCache;  std::vector<Tile> tiles;  Rectangle extents;};

The above example uses typical windowing paradigm, namely view offset is part of VideoScreen (canvas or similar), and render function can query. In addition, we provide clip rectangle to limit what is drawn.

So....

  VideoScreen * vs = new OpenGLVS(); // or DXVS() or SDLVS() or WhateverVS()  // file, memory, internet, pak, ... whatever source  InputStream * is = getReader("levels/level01.map");  GameMap map(is);  vs->setViewOffset(50,20);  // similar to singleton  // unless we really need it, we just use default clip  // and redraw everything, but we have the choice of replacing it   // to optimize the rendering  map.render(vs, vs->getExtents());

So you end up with classes which are independent of each other, yet interoperate in a clear manner.

I'm not sure I understand this. I don't think it's very clear at all. Where is the rendering code in this? I can't see where the Image might actually be drawn. Can you clarify on this, please?

Quote:
Note that to create a Tile or Image, you do not need to know anything about GameMap or Game or VideoScreen or anything else. That's not what Image or Tile do - they are dumb and passive and do one thing and one thing only.

But how does the Image know what to draw on?

Assuming I understand Antheus' example correctly, the rendering is done in Image::draw(), and the image knows what to draw on because it receives a pointer to a VideoScreen object as an argument to draw().

Does that answer your questions at all?

[Oops, looks like you edited your post since I posted this.]

Quote:Original post by jykAssuming I understand Antheus' example correctly, the rendering is done in Image::draw(), and the image knows what to draw on because it receives a pointer to a VideoScreen object as an argument to draw().

Does that answer your questions at all?

Well, that much I guessed, since it's stated somewhere above. What I can't quite see is where Image::draw() is actually getting the pointer to the VideoScreen instance from.

Quote:Well, that much I guessed, since it's stated somewhere above. What I can't quite see is where Image::draw() is actually getting the pointer to the VideoScreen instance from.

In Antheus' example, the VideoScreen pointer is created in main(), and is then passed to GameMap::render(). Although Antheus did not supply a definition for GameMap::render(), it presumably looks something like this (I'll leave out culling/clipping for simplicity):

void render(VideoScreen * vs, const Rectangle & clip){    for (size_t i = 0; i < tiles.size(); ++i) {        tiles.image->draw(vs, ...);    }}

That's a very simple example of course (you could use iterators, for_each and boost::bind - whatever you prefer), but it should show how the video screen makes its way from the main() function to Image::draw().