|
is my project code bad praticing?? |
Gabriel Campos
Member #12,034
June 2010
|
Hi all, i am starting a new project, a bomberman fan game and i am gathering resources and testing codes...so i decided to make my code this way.. 1// first i make a class pure virtual
2class cEntity
3{
4 public:
5 virtual void logic() = 0;
6 virtual void render() = 0;
7};
8
9// a class that will control all animations of all classes
10class cAnimation
11{
12 public:
13 void logic();
14 void render();
15 void update();
16 void increase_frame();
17
18 private:
19 int x;
20 int y;
21 int w;
22 int h;
23 int frame;
24 int max_frame;
25 float speed_frame;
26 ALLEGRO_BITMAP *sprite;
27};
28
29// then i will make the enemies classes and player class that inherits cEntity and cAnimation
30class cPlayer : public cEntity, public cAnimation // and so on with enemies classes
31{
32 public:
33 void logic();
34 void render();
35};
is this way bad praticing or is a good code?? in player and enemies classes i make this... void cPlayer::logic() { cAnimation::logic(); // here more code of the logic of player } void cPlayer::render() { cAnimation::render(); }
then i create a class cManager that will hold a list of all cEntity objects, 1// function to add elements to the list
2void cManager::add_elements(cEntity *e)
3{
4 entity_list.push_back(e);
5}
6
7// function of logic of all elements in the list
8void cManager::logic()
9{
10 for(std::list<cEntity *>::iterator it = entity_list.begin(); it != entity_list.end(); it++)
11 {
12 cEntity *e = *it;
13 e->logic();
14 }
15}
16
17void cManager::render()
18{
19 for(std::list<cEntity *>::iterator it = entity_list.begin(); it != entity_list.end(); it++)
20 {
21 cEntity *e = *it;
22 e->render();
23 }
24}
25
26// in the main function
27int main()
28{
29 cManager manager;
30
31 manager.logic();
32
33// then drawing (after the fps control)
34 manager.render();
35}
36//simple
I just want to know if my code style is good, sugestions and so on
|
bamccaig
Member #7,536
July 2006
|
It's good that you're abstracting functionality into separate classes (e.g., animations). It's bad that your entities inherit animation. A player isn't really an animation. A player has an animation. You would be better off to use composition instead of inheritance: 1class cPlayer
2{
3private:
4 cAnimation anim_;
5
6public:
7 // Etc...
8
9 void logic(void);
10 void render(void);
11};
12
13void cPlayer::logic(void)
14{
15 this->anim_->logic();
16 // Etc...
17}
18
19// Etc...
Another thing that you're doing is giving each of your entities and animations, etc., too much responsibility (e.g., their own logic and render methods). A lot of people do it like this and it does work. I prefer not to do it that way because IMHO it ends up violating good design practices and requires a lot of redundant code. It requires your different objects to know how to draw themselves. In practice, they usually don't need to know this. Often you draw every different type of entity the same exact way or ways. Ideally, you would figure out an interface (or interfaces) to draw with and write a single manager object to actually draw everything. I have already demonstrated this for you before. See here. Similarly, it requires all of your objects to know about the world around them to interact with it. Ideally, it would be the world or the game that does the thinking, not the Player, and the Boss, and the Bridge, etc. -- acc.js | al4anim - Allegro 4 Animation library | Allegro 5 VS/NuGet Guide | Allegro.cc Mockup | Allegro.cc <code> Tag | Allegro 4 Timer Example (w/ Semaphores) | Allegro 5 "Winpkg" (MSVC readme) | Bambot | Blog | C++ STL Container Flowchart | Castopulence Software | Check Return Values | Derail? | Is This A Discussion? Flow Chart | Filesystem Hierarchy Standard | Clean Code Talks - Global State and Singletons | How To Use Header Files | GNU/Linux (Debian, Fedora, Gentoo) | rot (rot13, rot47, rotN) | Streaming |
Edgar Reynaldo
Major Reynaldo
May 2007
|
I agree with bamccaig that your entities should own an animation, not be an animation. However, I don't agree that entities and animations shouldn't have their own logic and render methods. All animations share the same basic logic and rendering. I see how it might help if you really need flexibility in how you render an animation, but unless you need shaders/effects/something other than plain bitmap drawing, then I don't think an external object should render an animation. I definitely disagree that entities and animations should not have their own logic methods. My Website! | EAGLE GUI Library Demos | My Deviant Art Gallery | Spiraloid Preview | A4 FontMaker | Skyline! (Missile Defense) Eagle and Allegro 5 binaries | Older Allegro 4 and 5 binaries | Allegro 5 compile guide |
james_lohr
Member #1,947
February 2002
|
I agree with Edgar on all accounts. bamccaig said: Ideally, it would be the world or the game that does the thinking, not the Player, and the Boss, and the Bridge, etc There is every reason for the Player, Boss and Bridge to do their own thinking, if it is thinking specific to them. If I want to find Player-specific logic, I expect to find it in the Player class. Certainly not somewhere in the World. Quote: it requires all of your objects to know about the world around them to interact with it Which is fine. Give them an interface (or set of interfaces) to the World which is sensible (e.g. it might be bounded by location).
|
Edgar Reynaldo
Major Reynaldo
May 2007
|
James Lohr said: I agree with Edgar on all accounts.
My Website! | EAGLE GUI Library Demos | My Deviant Art Gallery | Spiraloid Preview | A4 FontMaker | Skyline! (Missile Defense) Eagle and Allegro 5 binaries | Older Allegro 4 and 5 binaries | Allegro 5 compile guide |
Audric
Member #907
January 2001
|
Globally, it's a rather-well-used pattern, familiar to many people. Yet everyone will have different advice, depending on one's idea of how generic the game engine should be, or if it should be simply limited to what the game will need : For example, if you never plan to use particles or vector objects, I got the impression that all your cEntities subclasses will derivate from cAnimation. If they do, then it makes sense to design one class instead of two, it's less code to write (and to understand later). For cAnimation, as I understand it you've tried to isolate every piece of data that can be needed to draw a character/bonus/explosion/whatever. The evident common code is the render(), and increase_frame() is useful if it does what I think it does (ie: be called by any specific entities' logic() when they request to advance the animation by one step.). But then I wonder what's the intent of cAnimation:update() or cAnimation:logic(). logic() looks especially dubious, because every specific cEntity (CPlayer, etc.) is forced to implement a method of this name, which will always override cAnimation's one. |
Gabriel Campos
Member #12,034
June 2010
|
thanx to all for explanation... class Entity { public: virtual void logic(); virtual void render(); };
1class Animation
2{
3 public:
4 ~Animation();
5 void render(int, int);
6 void increase_frame();
7
8 private:
9 int w;
10 int h;
11 float frame;
12 int max_frame;
13 float speed_frame;
14 bool visible;
15 ALLEGRO_BITMAP *sprite;
16};
17
18Animation::~Animation()
19{
20 al_destroy_bitmap(sprite);
21}
22
23Animation::render(int x, int y)
24{
25 if(visible)
26 al_draw_bitmap(sprite, x, y, 0);
27}
28
29Animation::increase_frame()
30{
31 frame += speed_frame;
32 if((int)frame > max_frame)
33 frame = 0;
34}
1class Player : public Entity
2{
3 public:
4 Player();
5 void logic();
6 void render();
7 // other functions
8
9 private:
10 int x;
11 int y;
12 Animation animation;
13};
14
15void Player::logic()
16{
17 // player logic
18}
19
20void render()
21{
22 this->animation->render(x, y);
23}
so i think is this way good, right?? and one more thing to know your opinions...about tiles...
|
torhu
Member #2,727
September 2002
|
Quote: so i think is this way good, right?? You have x and y fields and two places now. I think you should do what Audric suggested: dump Entity and rename Animation to Entity. Both logic and rendering code need much of the same information, you shouldn't duplicate it. Gabriel Campos said: should i create a Level class that have a matrix tiles[][] to compare collision You could do it like this: struct Tile { int x, y; int size; bool solid; }; Tile tiles[][]; A struct is a good match for tiles, since they are just dumb data. |
Audric
Member #907
January 2001
|
Tiles: I'd recommend to separate two things: ie: 1tiles = new vector<Tile *>;
2tiles[0]=new Tile(floor_bitmap, true); // second argument means "walkable"
3tiles[1]=new Tile(wall_bitmap, false);
4tiles[2]=new Tile(boulder_bitmap, false);
5
6int level[][] = { // should be loaded from file, but to give you an idea:
7{ 1, 1, 1, 1, 1, 1 },
8{ 1, 0, 0, 0, 0, 1 },
9{ 1, 0, 0, 2, 0, 1 },
10{ 1, 0, 2, 0, 0, 1 },
11{ 1, 0, 0, 0, 0, 1 },
12{ 1, 1, 1, 1, 1, 1 }
13};
14
15// Get the tile at a given cell position.
16Tile * tile_at_pos(int x, int y)
17{
18 return tiles[ level[y][x] ];
19}
|
Edgar Reynaldo
Major Reynaldo
May 2007
|
torhu said: You have x and y fields and two places now. I think you should do what Audric suggested: dump Entity and rename Animation to Entity. Both logic and rendering code need much of the same information, you shouldn't duplicate it. Entity could hold x and y, and Animation::Render could take x and y as input parameters. Audric said:
tiles = new vector<Tile *>; tiles[0]=new Tile(floor_bitmap, true); // second argument means "walkable" tiles[1]=new Tile(wall_bitmap, false); tiles[2]=new Tile(boulder_bitmap, false);
Don't forget to resize or use push_back, and dereference the pointer : tiles = new vector<Tile*>; tiles->reserve(3,NULL); (*tiles)[0] = //...
My Website! | EAGLE GUI Library Demos | My Deviant Art Gallery | Spiraloid Preview | A4 FontMaker | Skyline! (Missile Defense) Eagle and Allegro 5 binaries | Older Allegro 4 and 5 binaries | Allegro 5 compile guide |
bamccaig
Member #7,536
July 2006
|
Gabriel Campos said: so i think is this way good, right?? Firstly, your animation class doesn't seem to be an animation at all. It has a single bitmap. That is just a sprite. You do seem to have some of the workings for an animation (e.g., a frame) so perhaps you are just keeping the implementation simple for now and will extend it into an animation later (e.g., std::vector<ALLEGRO_BITMAP *> sprites)? Secondly, I think being visible is more an attribute of an entity within the game, not the animation itself. You care if the entity is visible, not if its animation is visible. At least, that seems logical to me. I guess it might depend on the game, but I think that is true for any generic entity vs. animation. If they aren't generic classes then you might want to give them more specific names. Gabriel Campos said:
should i create a Level class that have a matrix tiles[][] to compare collision Audric's solution is good: tiles are usually reused in a level to make a bigger level from fewer tiles. To save memory you might want to load your tiles into memory once into a manager/container and then reference them somehow from your levels. In memory, your level can store a matrix of numeric indexes, string keys (probably a bad idea for performance reasons), or even pointers to these tiles. That way you don't waste too much memory, but you can still reference tiles relatively efficiently. For example, you could use a single char to identify your tiles. Assuming you use mnemonics to assign characters to your tiles, this could make it relatively easy to create levels with them. Load them up into a map and look them up once at level load-time. 1class Tile
2{
3private:
4 // Etc...
5
6 Tile(void); // Create an invalid tile.
7public:
8 typedef boost::shared_ptr<Tile> Ptr;
9
10 Tile(/*...etc...*/); // Create a real tile.
11
12 const static Tile InvalidTile;
13
14 // "...Wait, can he do that?" I JUST DID! >:( (I don't know...)
15 const static Tile::Ptr InvalidTilePtr(&InvalidTile);
16}
17
18// Etc...
19
20class TileManager
21{
22private:
23 typedef std::map<char, Tile::Ptr> Map;
24 typedef Map::const_iterator MapIt;
25
26 Map tiles_;
27public:
28 typedef std::vector<Tile::Ptr> Row;
29 typedef boost::shared_ptr<Row> RowPtr;
30
31 // Etc...
32
33 const Tile::Ptr getTile(char id) const;
34};
35
36const Tile::Ptr TileManager::getTile(char id) const
37{
38 TileManager::MapIt it = this->tiles_.find(id);
39
40 if(it == this->tiles_.end())
41 return Tile::InvalidTilePtr;
42
43 return it->second;
44}
45
46class Level
47{
48 std::vector< Tile::RowPtr > tiles;
49public:
50 Level(const TileManager &, const char [][]);
51};
52
53Level::Level(
54 const TileManager & manager,
55 const int width,
56 const int height,
57 const char [][]data)
58{
59 for(int i=0; i<width; i++)
60 {
61 Tile::RowPtr row(new Tile::Row());
62
63 for(int j=0; j<height; j++)
64 {
65 row->push_back(manager.getTile(data[i][j]));
66 }
67
68 this->tiles_.push_back(row);
69 }
70}
(Untested... There is some very questionable code regarding const/static initialization, but you get the idea...) Well that all turned out pretty messy, but hopefully not too messy to follow. The idea is that your levels can be like this: # media/levels/the-alter A=media/tiles/alter.bmp g=media/tiles/grass.bmp T=media/tiles/tree.bmp -=media/tiles/wall_horz.bmp |=media/tiles/wall_vert.bmp |------------| |gggggggggggg| |ggTgg---gggg| |ggggg|Aggggg| |ggggg---ggTg| |ggggTggggggg| |gggggggggggg| |------------| <over-engineer carried-away="true" /></tangent></digress> Edgar Reynaldo said: Entity could hold x and y, and Animation::Render could take x and y as input parameters. That's already exactly what happens.. Edgar Reynaldo said: ...and dereference the pointer... Better yet, use an automatic variable instead, if possible. -- acc.js | al4anim - Allegro 4 Animation library | Allegro 5 VS/NuGet Guide | Allegro.cc Mockup | Allegro.cc <code> Tag | Allegro 4 Timer Example (w/ Semaphores) | Allegro 5 "Winpkg" (MSVC readme) | Bambot | Blog | C++ STL Container Flowchart | Castopulence Software | Check Return Values | Derail? | Is This A Discussion? Flow Chart | Filesystem Hierarchy Standard | Clean Code Talks - Global State and Singletons | How To Use Header Files | GNU/Linux (Debian, Fedora, Gentoo) | rot (rot13, rot47, rotN) | Streaming |
Edgar Reynaldo
Major Reynaldo
May 2007
|
bamccaig said: That's already exactly what happens..
<checks eyesight...> 1pt bambam, 1pt reynaldo My Website! | EAGLE GUI Library Demos | My Deviant Art Gallery | Spiraloid Preview | A4 FontMaker | Skyline! (Missile Defense) Eagle and Allegro 5 binaries | Older Allegro 4 and 5 binaries | Allegro 5 compile guide |
Gabriel Campos
Member #12,034
June 2010
|
first thanx to all for the atention.... bamccaig said: so perhaps you are just keeping the implementation simple for now and will extend it into an animation later (e.g., std::vector<ALLEGRO_BITMAP *> sprites)? is just simple for this post. I use a bitmap with all animation sprite with specific size and clip them acordiling.. e.g. al_draw_bitmap_region(sprite, frame * w, stance * h, x, y, w, h, 0); Quote: Secondly, I think being visible is more an attribute of an entity within the game, not the animation itself. You care if the entity is visible, not if its animation is visible. At least, that seems logical to me. I guess it might depend on the game, but I think that is true for any generic entity vs. animation. If they aren't generic classes then you might want to give them more specific names. hmmm, dont know.. but i agree with you in this point for tiles i will try to find someway first, then after ask!! Quote: Better yet, use an automatic variable instead, if possible.
Edgar Reynaldo said: The latest incarnation of Entity doesn't hold x or y.
|
Edgar Reynaldo
Major Reynaldo
May 2007
|
Gabriel Campos said: Use one on the stack - a local. Gabriel Campos said:
Edgar said:
The latest incarnation of Entity doesn't hold x or y.
I was talking to bamccaig - you're fine. It just makes sense for entity to store the position, not animation. That's all I was saying. My Website! | EAGLE GUI Library Demos | My Deviant Art Gallery | Spiraloid Preview | A4 FontMaker | Skyline! (Missile Defense) Eagle and Allegro 5 binaries | Older Allegro 4 and 5 binaries | Allegro 5 compile guide |
bamccaig
Member #7,536
July 2006
|
Edgar Reynaldo said: Use one on the stack - a local. Not necessarily the stack. A class member can still be automatic, but the class instance itself can be on the heap. 1class Example
2{
3private:
4 int x_; // Automatic.
5 int * y_; // Pointer is automatic; int may or may not be automatic.
6 int * z_; // Pointer is automatic; int may or may not be automatic.
7public:
8 Example(void):
9 y_(new int()), // int not automatic.
10 z_(&x_) // int is automatic (it's actually x_).
11 {
12 }
13
14 ~Example(void)
15 {
16 delete this->y_; // Not automatic; we destroy it ourselves.
17 }
18};
19
20int main(int argc, char * argv[])
21{
22 int x; // Automatic.
23 int * y = new int(); // Pointer is automatic; int is not automatic.
24
25 delete y;
26 y = 0;
27
28 Example ex; // Automatic.
29
30 // Pointer is automatic; Example instance is not automatic.
31 Example * p_ex = new Example();
32
33 delete p_ex;
34 p_ex = 0;
35
36 return 0;
37}
*Something* like that anyway. Automatic variables are much easier to use and understand, which is why I suggested using them whenever possible. -- acc.js | al4anim - Allegro 4 Animation library | Allegro 5 VS/NuGet Guide | Allegro.cc Mockup | Allegro.cc <code> Tag | Allegro 4 Timer Example (w/ Semaphores) | Allegro 5 "Winpkg" (MSVC readme) | Bambot | Blog | C++ STL Container Flowchart | Castopulence Software | Check Return Values | Derail? | Is This A Discussion? Flow Chart | Filesystem Hierarchy Standard | Clean Code Talks - Global State and Singletons | How To Use Header Files | GNU/Linux (Debian, Fedora, Gentoo) | rot (rot13, rot47, rotN) | Streaming |
|