Public Group

This topic is 2781 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

I have a grid class and a few child classes of it, one of which is called map. I had a goal of something I wanted them to do, and succesfully pulled it off, but im not happy with the way it's been done, I just kinda trial and errored through it until I got something that worked, and now I'd like to know why this works and my other method does not. The function below moves all the tiles in the grid one blocks width to the left.
virtual void MoveLeft(list<list<T>> *tileh) { list<list<T>>::iterator indexh; list<T>::iterator indexw; for(indexh = tileh->begin(); indexh != tileh->end();indexh++) { for(indexw = indexh->begin(); indexw != indexh->end(); indexw++) { indexw->Position.left -= indexw->Position.right; } } }
it is called within the Map class in the maps moveleft function with the maps tileh as a parameter. The one thing I don't quite understand is, since the tileh is part of grid.h, Why do I need to point to the map's tileh? Why can't I just do something like this?
virtual void MoveLeft() { list<list<T>>::iterator indexh; list<T>::iterator indexw; for(indexh = tileh.begin(); indexh != tileh.end();indexh++) { for(indexw = indexh->begin(); indexw != indexh->end(); indexw++) { indexw->Position.left -= indexw->Position.right; } } and whilst I'm asking about this, if I made a function something like this list<T> gettiles() { return tileh; } in the grid file, would it return the grids tileh, or the maps tileh if the function was called by a map class? down below is the full files of the grid.h and map.h. I left out map.cpp as I don't think it shows anything of importance. }
 //grid.cpp #pragma once #include "Tile.h" #include <list> #include "Sprite.h" #include "WindowCreater.h" #include "Directx.h" #include "Direct3Dev.h" #define LeftClickParameters WindowCreator *gw, Mouse* mouse template <typename T> struct Grid { SPRITE *sprite; Direct3Dev *d3dev; WindowCreator *win; list<T> tilew; list<list<T>> tileh; int width, hieght; int w, h; int x, y; //constructer Grid(int mwidth, int mhieght,SPRITE *sprites, WindowCreator *gw, Direct3Dev *d3d) { win=gw; d3dev=d3d; width = mwidth; hieght = mhieght; w = sprites->width; h = sprites->height; sprite = sprites; } //methods //void savefile(); //void openfile(); // Virtual methods //virtual list<T> ReadTiles() = 0; //template functions virtual void Render(list<list<T>> *tileh) { d3dev->SetSwapChain(win); d3dev->StartRender(win); list<list<T>>::iterator indexh; list<T>::iterator indexw; for(indexh = tileh->begin(); indexh != tileh->end();indexh++) { for(indexw = indexh->begin(); indexw != indexh->end(); indexw++) { d3dev->DrawSprite(&indexw->spriteStore,indexw->spriteStore.frameStore,indexw->Position.left,indexw->Position.top,1); } } d3dev->EndRender(win); } virtual void MoveLeft() =0; virtual void MoveLeft(list<list<T>> *tileh) { list<list<T>>::iterator indexh; list<T>::iterator indexw; for(indexh = tileh->begin(); indexh != tileh->end();indexh++) { for(indexw = indexh->begin(); indexw != indexh->end(); indexw++) { indexw->Position.left -= indexw->Position.right; } } } virtual void MoveRight() = 0; virtual void MoveRight(list<list<T>> *tileh) { list<list<T>>::iterator indexh; list<T>::iterator indexw; for(indexh = tileh->begin(); indexh != tileh->end();indexh++) { for(indexw = indexh->begin(); indexw != indexh->end(); indexw++) { indexw->Position.left += indexw->Position.right; } } } virtual void MoveUp() =0; virtual void MoveUp(list<list<T>> *tileh) { list<list<T>>::iterator indexh; list<T>::iterator indexw; for(indexh = tileh->begin(); indexh != tileh->end();indexh++) { for(indexw = indexh->begin(); indexw != indexh->end(); indexw++) { indexw->Position.top -= indexw->Position.bottom; } } } virtual void MoveDown() =0; virtual void MoveDown(list<list<T>> *tileh) { list<list<T>>::iterator indexh; list<T>::iterator indexw; for(indexh = tileh->begin(); indexh != tileh->end();indexh++) { for(indexw = indexh->begin(); indexw != indexh->end(); indexw++) { indexw->Position.top += indexw->Position.bottom; } } } //virtual void addrow() =0; virtual void addrow() { list<T>::iterator indexw; for(indexw = tileh.back().begin(); indexw != tileh.back().end(); indexw++) { T temp; temp.Position.left = indexw->Position.left; temp.Position.top = indexw->Position.top+h; temp.Position.right=w; temp.Position.bottom=h; temp.getSprite(*sprite,8,8); temp.setFrame(0,0); tilew.push_back(T(temp)); } hieght++; tileh.push_back(tilew); tileh.clear(); } //virtual void deleterow() = 0; virtual void deleterow() { if(hieght>1) { tileh.pop_back(); hieght--; } } //virtual void addcolumb() =0; virtual void addcolumb() { list<list<T>>::iterator indexh; int c=0; for(indexh = tileh.begin(); indexh != tileh.end();indexh++,c++) { T temp; temp.Position.left = indexh->back().Position.left+w; temp.Position.top = indexh->back().Position.top; temp.Position.right=w; temp.Position.bottom=h; temp.getSprite(*sprite,8,8); temp.setFrame(0,0); indexh->push_back(temp); } width++; } //virtual void deletecolumb() =0; virtual void deletecolumb() { if(width>1) { list<list<T>>::iterator indexh = tileh.begin(); for(indexh = tileh.begin(); indexh != tileh.end();indexh++) { indexh->pop_back(); } width--; } } list<T> ReadTiles() { list<T> tiles; list<list<T>>::iterator indexh; list<T>::iterator indexw; for(indexh = tileh.begin(); indexh != tileh.end();indexh++) { for(indexw = indexh->begin(); indexw != indexh->end(); indexw++) { tiles.push_back(*indexw); } } return tiles; } }; 
 #pragma once #include "Grid.h" #include "MapTile.h" struct Map : Grid<MapTile> { list<list<MapTile>> tileh; list<MapTile> tilew; //constructer Map(int width, int hieght,SPRITE *sprites, WindowCreator *gw, Direct3Dev *d3d) : Grid<MapTile>(width, hieght, sprites, gw, d3d) { for(int i=0; i < hieght; i++) { for(int c=0; c < width; c++) { MapTile temp; temp.Position.left = w*c; temp.Position.top = h*i; temp.Position.right=w; temp.Position.bottom=h; temp.getSprite(*sprite,hieght,width); temp.spriteStore.frameStore=0; temp.SpName = "green.bmp"; tilew.push_back(temp); } tileh.push_back(tilew ); tilew.clear(); } } //methods void MoveLeft(); void MoveRight(); void MoveUp(); void MoveDown(); void Render(); void Brush(SPRITE *newSprite,POINT MousePosition); }; 

##### Share on other sites
Why do you have another list<list<MapTile> > tileh' inside the Map class block? If it inherits from Grid<MapTile> it should already have it...

##### Share on other sites
You should rethink your design. The classes should try to obey the Single Responsibility Principle. I suspect that composition, rather than inheritance, would be more appropriate here.

A start on separating the logic here might be:

• Grid<T>. Responsible for maintaining an W x H grid of T
• GridIterator<T>. Responsible for allowing a client to move a "cursor" around a Grid in two dimensions
• A Map is simply a Grid<MapTile> (i.e. typedef)
• Building a Map is done by a Map "Factory" (could be a class or a simple function)
• A MapRenderer takes a Map reference and draws it.

Note that this design requires no inheritance.

##### Share on other sites

Why do you have another list<list<MapTile> > tileh' inside the Map class block? If it inherits from Grid<MapTile> it should already have it...

Wow... I had that there to test something I was doing earlier... Deleting it fixed everything. Thanks * infinity!

You should rethink your design. The classes should try to obey the Single Responsibility Principle. I suspect that composition, rather than inheritance, would be more appropriate here.

A start on separating the logic here might be:

• Grid<T>. Responsible for maintaining an W x H grid of T
• GridIterator<T>. Responsible for allowing a client to move a "cursor" around a Grid in two dimensions
• A Map is simply a Grid<MapTile> (i.e. typedef)
• Building a Map is done by a Map "Factory" (could be a class or a simple function)
• A MapRenderer takes a Map reference and draws it.

Note that this design requires no inheritance.

You may be right, I'm just having a bit of touble thinking this through as I've never heard of this single responsibility principle and just read a bit about it for the first time.

So Grid<T> I get that, make a class storing the width and height of a grid, now above I have a bunch of functions that move the grid around and make it bigger and smaller,
these would be contained within this grid class?

GridIterator, I have no intention of having a cursor you can move around within the map, so I'll be skiping that one I think, unless I'm missinterpretting something there.

This I don't quite understand, what I'm thinking would work is like, a Map class that contains a Grid object or soemthing? but I don't quite think that's what your saying.

also the map factory, I don't understand what it is, I think I have a general Idea of what it's supposed to do, but some enlightenment would be nice.

The MapRenderer, is this like, a class or function that's sole purpose is to Render the map? Doesn't sound very polymorphic to me, and I'm a big fan of polymorphism. If I'm getting this right could you explain the advantages to using this over some other methods? If I'm getting that wrong could you explain what you really mean?

##### Share on other sites
GridIterator, I have no intention of having a cursor you can move around within the map, so I'll be skiping that one I think, unless I'm missinterpretting something there.

I think you are misinterpreting - my expectation would be that e.g. MapRenderer would use a GridIterator to iterate over and draw each MapTile in turn. The cursor concept needn't actually be exposed to the user, if that's what you mean.

This I don't quite understand, what I'm thinking would work is like, a Map class that contains a Grid object or soemthing? but I don't quite think that's what your saying.[/quote]

[font=courier new,courier,monospace]typedef Grid<MapTile> Map;[/font]

also the map factory, I don't understand what it is, I think I have a general Idea of what it's supposed to do, but some enlightenment would be nice.[/quote]

Factory is just a function (possibly a member function, in which case we might call the class a factory too) which creates objects for us. So in this case it might be as simple as [font=courier new,courier,monospace]Map map_from_file(const char* path)[/font] or [font=courier new,courier,monospace]Map random_map(int seed)[/font].

The MapRenderer, is this like, a class or function that's sole purpose is to Render the map? Doesn't sound very polymorphic to me, and I'm a big fan of polymorphism.[/quote]

What do you mean?

##### Share on other sites
Thankyou very much for clearing all that up. Also nevermind about the last question, I believe I understand now.

I'm not quite sure I should go back and change everything to this Principle right at this moment though, everything seems to be doing what it's supposed to, I will keep it in mind for future creations though.

##### Share on other sites

The MapRenderer, is this like, a class or function that's sole purpose is to Render the map? Doesn't sound very polymorphic to me, and I'm a big fan of polymorphism. If I'm getting this right could you explain the advantages to using this over some other methods? If I'm getting that wrong could you explain what you really mean?
[/quote]
Polymorphism is over used by beginners. I know the feeling - I used to think like this.

Inheritance introduces arguably the strongest coupling between two classes (some inner classes are stronger). This coupling makes the code very brittle, changes to the parent class often necessitate changes to the child class. Your Map class implementation depends heavily on the current implementation details of the Grid class.

I can also tell you that a list of lists is a horrendously naive implementation. Lists are allocation heavy, and really do not suit the processor cache. That is fine for now - a lot of naive implementations meet all the requirements - if it ain't broke don't fix it. However, if you try to load larger and larger maps using this code, the speed of your map sub-system will creep up, possibly becoming a bottleneck in the future. Now, this is an orthogonal issue to that of inheritance - but what I'm indicating here is that here is an example of a change you might reasonably need to make to the parent class. Now you're going to have to re-write the child class to conform to this new implementation.

The concept of a generic 2 Dimensional grid is a useful one, but not really available in your code. Why can I not create a Grid<int> if necessary? Because the grid does too much, it cannot be used outside the domain it was written in. I'm not even sure I see you using the Grid<> type as a parent class for something else, it is too specific.

GridIterator, I have no intention of having a cursor you can move around within the map, so I'll be skiping that one I think, unless I'm missinterpretting something there.
[/quote]
The purpose of this class was to replace the move up/down/left/right functions in the original grid. I called it an "iterator" or a "cursor", you might be calling it a camera, but the underlying thing it tracks is about the same.

The MapRenderer, is this like, a class or function that's sole purpose is to Render the map?
[/quote]
Precisely. It is another separation of concerns. The Grid shouldn't worry about being rendered - indeed you might need Grids that are never rendered. The "iterator", "cursor" or "camera" could be a separate argument which helps transform the Grid from world co-ordinates into screen co-ordinates. You shouldn't be physically moving the tiles around in memory.

##### Share on other sites
Here is what the classes might look like:

tile.h:
 #ifndef GRID_H #define GRID_H template<typename T> class Grid { public: Grid(unsigned width, unsigned height); T &operator()(unsigned y, unsigned x); const T &operator()(unsigned y, unsigned x) const; unsigned width() const; unsigned height() const; private: // TODO: }; #endif 
Note how little we expose here. This could be implemented in many, many ways. I've omitted the copy constructor/destructor and assignment operator because I would initially implement it using standard containers. But including them doesn't really change the client interface (the compiler generated versions are implicitly present anyway).

grid_cursor.h
 #ifndef GRID_CURSOR_H #define GRID_CURSOR_H #include <stdexcept> #include "grid.h" template<typename T> class GridCursor { public: GridCursor(const Gird<T> &grid, unsigned x, unsigned y) : grid(&grid) { if(!set(x, y)) { throw std::runtime_error("Out of bounds"); } } unsigned x() const { return x_; } unsigned y() const { return y_; } bool up() { if(y_ > 0) { --y_; return true; } return false; } bool down() { if(y_ < map->height()) { ++y_; return true; } return false; } bool left() { if(x_ > 0) { --x_; return true; } return false; } bool down() { if(x_ < map->width()) { --x_; return true; } return false; } bool set(unsigned x, unsigned y) { if(x < grid->width() && y < grid->height()) { x_ = x; y_ = y; return true; } return false; } private: unsigned x_; unsigned y_; const Grid<T> *grid; }; #endif 
Pretty simple. Its main job is to keep inside the Grid bounds at all times.

map.h:
 #ifndef MAP_H #define MAP_H #include "grid.h" #include "grid_cursor.h" struct Tile { Tile(Sprite *sprite) : sprite(sprite) { } Sprite *sprite; }; typedef Grid<Tile> Map; typedef GridCursor<Tile> Camera; Map loadMap(const std::istream &stream); void saveMap(const Map &map, const std::ostream &stream); #endif 
A map requires almost no special code. What is a map? A grid of Tiles. What is a Camera? A cursor that moves around a Map. Simple. Note we can provide map specific functionality by using free functions - here to load and save a map.

map_renderer.h
 #ifndef MAP_RENDERER_H #define MAP_RENDERER_H #include "Map.h" #include "Direct_X_Stuff.h" class MapRenderer { public: MapRenderer(SPRITE *sprite, Direct3Dev *d3dev, WindowCreator *win) : sprite(sprite), d3dev(d3dev), win(win) { } void render(const Camera &camera, const Map &map) { d3dev->SetSwapChain(win); d3dev->StartRender(win); unsigned tileWidth = sprite->width; unsigned tileHeight = sprite->height; unsigned mapHeight = map.height(); unsigned mapWidth = map.width(); unsigned cameraX = camera.x(); unsigned cameraY = camera.y(); for(unsigned y = 0 ; y < h ; ++y) { for(unsigned x = 0 ; x < w ; ++x) { const Tile &tile = map(y, x); Rectangle recangle = { (x * tileWidth) - cameraX, (y * tileHeight) - cameraY, tileWidth, tileHeight }; d3dev->DrawSprite(tile.sprite, rectange); } } d3dev->EndRender(win); } private: SPRITE *sprite; Direct3Dev *d3dev; WindowCreator *win; }; #endif 
Note how each class is so much easier to write now that we have hidden the implementation details. We didn't even have to know how to implement grid to write them!

Here is one such implementation, using a packed, contiguous data structure that avoids excessive allocations:
 #ifndef GRID_H #define GRID_H #include <vector> #include <stdexcept> template<typename T> class Grid { public: Grid(unsigned width, unsigned height) : data(width * height), width_(width), height_(height) { } T &operator()(unsigned y, unsigned x) { return data[index(y, x)]; } const T &operator()(unsigned y, unsigned x) const { return data[index(y, x)]; } unsigned width() const { return width_; } unsigned height() const { return height_; } private: unsigned index(unsigned y, unsigned x) { if(y >= height_) { throw new std::runtime_error("Out of bounds on Y axis"); } if(x >= width_) { throw new std::runtime_error("Out of bounds on X axis"); } return (y * width_) + x; } std::vector<T> data; unsigned width_; unsigned height_; }; #endif 
Again, this is easy enough to write an understand because it does so little - just the minimum required to represent a grid.

(warning: code neither compiled nor tested)

##### Share on other sites
Welp, you've convinced me, that actually makes a lot of sense. I just hope my brain doesn't explode in the process of trying to get everything to follow this rule.

I'd just like to make sure I've got this right, I've never seen the words operator highlighted in blue before, and usually when highlighted in blue that means that it is some special keyword that does something fancy, and This line was not making sense to me.
 const Tile &tile = map(y, x); 
So pecing that together, does that word "operator" make it so the function is called if you just type in the object's name and fill in the parameters?

Also inheritance, when is a good time to use it? I've also made a tool class, and derived from it are move, marque, brush, and paint tools(only the brush is currently functional). Should this be avoided?

Edit:
aaaaand BAM mind = blown! This code you've written, it's just so perfect, it's beautiful ;-;

##### Share on other sites

I'd just like to make sure I've got this right, I've never seen the words operator highlighted in blue before, and usually when highlighted in blue that means that it is some special keyword that does something fancy, and This line was not making sense to me.
[/quote]
The operator keyword there is used to declare an operator overload function. Try to read "operator()" as one unit - it means the function call operator. So the code now reads: T &function_call_operator(unsigned, unsigned). Hopefully that looks a bit closer to a regular function declaration. The function call operator allows the object to be "called" as if it were a function. This allows the semi-convenient syntax I showed.

It isn't necessary, you could replace the "operator()" with an explicit name. For instance map.at(y, x). It is a convenience, nothing more.

If you've ever wondered how std::string and std::vector allow subscript access, they do something similar with operator[] instead of operator(). Unfortunately operator[] takes only a single argument, so to enable the most natural syntax one either needs to write a complex "proxy object" to allow map[y][x] or use operator[] with a special type that has two co-ordinates.

Since this is For Beginners (and thus I arguably shouldn't have used operator overloading in the first place), I'll demonstrate what the second might look like:
 struct GridIndex { public: GridIndex(unsigned x, unsigned y); unsigned x; unsigned y; }; GridIndex index(unsigned x, unsigned y) { return GridIndex(x, y); } template<typename T> class Grid { public: Grid(unsigned width, unsigned height); T &operator[](const GridIndex &index); const T &operator[](const GridIndex &index) const; unsigned width() const; unsigned height() const; private: // TODO: }; // Later int main() { Grid<Foo> map(5000, 2500); // ... Foo &foo = map[index(3,4)]; // ... } 
At this point, a named function becomes more convenient, I think.

Also inheritance, when is a good time to use it?
[/quote]
Inheritance is great when there is significant behavioural differences. Those sounds like they have different enough behaviour. Some rules of thumb:

• If the parent class has lots of code, then maybe you should try split the class into the behavioural aspect and the common aspect.
• Try to keep parent classes almost pure virtual (almost meaning providing an an empty virtual destructor).
• Avoid deep inheritance heirarchies.
• If parent state is necessary, keep it private. If the child requires access, give protected member functions to enable just the level of access that is required.

All of these have the same goal: reduce the amount of detail in the parent class that the child classes depend on.

I've also made a tool class, and derived from it are move, marque, brush, and paint tools(only the brush is currently functional).
[/quote]
It is hard to tell without seeing the code.

Let us imagine you might require a Toolbar for your U.I., you need a name, an icon, and a way of performing the tool's action when it is selected. One implementation would be to have a Tool parent class with a string name, Image icon and an pure virtual action member function. Another way to model this would be to have the Tool class with a Name, and Icon - and have a ToolAction class which has the behaviour. In this case it is pretty trivial, the child classes are unlikely to depend on the parent's members.

Should this be avoided?
[/quote]
It depends - inheritance isn't a design flaw in and of itself. It is a design flaw when it is chosen in the wrong situation.

aaaaand BAM mind = blown! This code you've written, it's just so perfect, it's beautiful ;-;
[/quote]
You are too kind =) Simplicity is the key to nice code that is easy to read, write, debug and maintain. The single responsibility principle is a great way to get simplicity. It isn't always this easy, sadly, and it takes concious effort to avoid "bloating" classes with additional responsibilities as the project evolves.

Test driven design is helpful here. If a class is hard to unit test, then it is likely too complex and trying to do too much.

• ### Game Developer Survey

We are looking for qualified game developers to participate in a 10-minute online survey. Qualified participants will be offered a \$15 incentive for your time and insights. Click here to start!

• 13
• 30
• 9
• 16
• 12