Code style guidelines and cleanup/work

Re: Code style guidelines and cleanup/work

Postby oln » 27 Mar 2011, 15:37

Then we will do that.
User avatar
oln
 
Posts: 1014
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby oln » 26 Apr 2011, 12:47

Okay, I think we need a clear plan on how we want the code structure to be.
There are still a bunch of globals that ought to not be globals. Server/client/network stuff mainly, but also the gamemap. Not sure if the gamemap should be a singleton or not though, as it should not need to exist while in the menu. Some classes are still quite large, and have sections that should be split out, such as the console parsing from the frame listener.
We should work out a layout of what classes we should make, what they should do, what should be split out, what new stuff is needed etc. I will start with a draft, names will probably be a bit arbitrary, so feel free to change them. (Feel free to edit this post, or maybe we should put this on the wiki)

(bold names are new classes)
  • ODFrameListener | should handle frame events mainly. Currently a lot more than this is in here. The rest should be split out:
    • New class: ConsoleManager - handles the console parsing, possibly a singleton class
    • New class: CameraHandler - handles the camera movement
    • New class: InputHandler - handles the keyboard/mouse input and hotkeys - singleton
    • not sure about what to do with the exit handling and goal display update
    • There is also some chat message stuff in here which i am not sure where we should put.
    • Loads of public vars
  • Network.h and Client.cpp/Server.cpp - should possibly make classes for these
  • ClientNotification | Possibly make this a superclass with a sub-class for each notification type, there is a todo in there suggesting this.
  • Creature
    • doTurn is quite long, maybe we could split the ai out out somehow
    • Quite a lot of public vars. I think we should atleast put the ones that are used by more than one thread behind getters/setters.
    • I still think the use of creatureclass a bit odd here, inheriting it doesn't feel right, as we are storing some data for each creature that only really needs to be stored. Though changing this would require a lot of work, though maybe we could alter creatureclass to have some class object instead of all the class data.
  • CreatureClass | see Creature.
  • CreatureSound | not really finished yet, haven't worked on the sound stuff in a while.
  • Field | has a public name var
  • Functions.h/cpp
    • ReadGameMapFromFile, writeGameMapToFile, StartServer and queueServerNotification should probably be in their related classes
    • listAllFiles is missing the implementation for windows, will need to fix this for level handling on windows
    • Could possibly put the random functions in a namespace or class. Could possibly create a better random function, it seems rather basic currently (should check if the distrubution seems somewhat random). (can't use rand as this is not guaranteed to be the same over the different platforms apparently).
  • GameMap | Large, can split out some parts
    • New class: (need a good name here) - Handle pathfinding and similar
    • Some public vars
    • Might want to have the level loading in here
    • New class:(need a good name here) - Handle the thread stuff, creature turns etc.
  • MissileObject - public vars
  • Player - public vars
  • RenderRequest - see ClientNotification
  • ResourceManager - maybe we should move listallfiles from functions in here, missing some code for getting home path in windows
  • Room - public vars
  • Seat - public vars
  • ServerNotification - see clientNotification
  • Socket - public var
  • SoundEffectsHelper - currently a lot of hard-coding of names, missing stuff, need to balance sound volumes
  • TextRenderer - code from ogre wiki?, should be cleaned up to follow the code style we use.
  • Tile - public vars
  • Weapon - public vars, not sure if we should count this as a data structure or not

I am also going to implement a log manager/wrapper that is thread-safe, so we can print everything there rather than directly stdout, and control the amout of output using the ogre log levels.

A diagram/flowchart or similar over the different threads would also be nice. There are a lot of threads in the application, and it's a bit hard to keep track of what variables the different threads are accessing.

THere is also a lot of calls to exit(1) when something goes wrong, we should work on exiting more cleanly if errors happen.

After we've gotten most of this sorted we should start working on getting the multiplayer working again, and possibly a basic keeper AI. I think having something that is playable will make it easier to get contributions.
User avatar
oln
 
Posts: 1014
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby StefanP.MUC » 26 Apr 2011, 13:07

Sounds like a good plan.

About the CreatureClass I was always confused anyways. I don't understand why it's really there (I asked this already in the code questions thread, but the answers didn't really convinced me of this class).

I'll also add a Class handling the translations (initializing tinygettext, translate the GUI, handle language changes, etc). Initially I wanted to put it directly into the GUI class, but by having a separate class we can keep better track of it and better handle other translatable strings (non-CEGUI strings).

The Randum stuff should be in a singleton class (instanciated as the very first of everything) with static functions, was also already thinking about it.

All the other stuff from Functions.cpp/h should be sorted into better places. At some point in the future there shouldn't be any globals left.
StefanP.MUC
 

Re: Code style guidelines and cleanup/work

Postby svenskmand » 26 Apr 2011, 13:24

oln {l Wrote}:New class: (need a good name here) - Handle pathfinding and similar

MapNavigation maybe?
oln {l Wrote}:A diagram/flowchart or similar over the different threads would also be nice. There are a lot of threads in the application, and it's a bit hard to keep track of what variables the different threads are accessing.

Something like this?
oln {l Wrote}:After we've gotten most of this sorted we should start working on getting the multiplayer working again, and possibly a basic keeper AI. I think having something that is playable will make it easier to get contributions.

I have some ideas for multiplayer which I will post once I get home, I have them written up in a notebook next to my bed.
Jamendo.com - The best music store on the net, uses CC licenses.
User avatar
svenskmand
OD Moderator
 
Posts: 1850
Joined: 09 Dec 2009, 00:07
Location: Denmark

Re: Code style guidelines and cleanup/work

Postby oln » 26 Apr 2011, 13:26

svenskmand {l Wrote}:
oln {l Wrote}:
oln {l Wrote}:A diagram/flowchart or similar over the different threads would also be nice. There are a lot of threads in the application, and it's a bit hard to keep track of what variables the different threads are accessing.

Something like this?

Yeah, something like a more detailed, and updated version of that.
User avatar
oln
 
Posts: 1014
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby StefanP.MUC » 26 Apr 2011, 18:17

Almost done with the Random generator (did it as a namespace instead of a class, that's better).

And what's missing on the list: Encpasulating and renaming the semaphores.
They are all named FooBarSemaphore. They should also be put in a namespace and renamed, so they can be accessed by "Semaphore::FooBar". This way we can also have a "Semaphore::initialize()" that initializes all semaphores at once.
Or does something speak against this?
StefanP.MUC
 

Re: Code style guidelines and cleanup/work

Postby svenskmand » 26 Apr 2011, 19:36

I have been wondering for a while why you use something as simple as semaphores, instead of real concurrent data structures? Just using semaphores is asking for trouble if you ask me :S
Jamendo.com - The best music store on the net, uses CC licenses.
User avatar
svenskmand
OD Moderator
 
Posts: 1850
Joined: 09 Dec 2009, 00:07
Location: Denmark

Re: Code style guidelines and cleanup/work

Postby oln » 26 Apr 2011, 21:22

You will have to ask andrew about that.
There is a protectedObject wrapper that could be used for simple values in the code, which is used for a few things. Concurrent data structures is probably also an idea for the notification queues, but I don't know too much about that. If there is something in the libs we currently use that can be used, that would be nice, or something we can include directly in the project. Don't want to add more deps. Alternatively implement something, but that would be more work.
User avatar
oln
 
Posts: 1014
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby svenskmand » 26 Apr 2011, 22:20

What data structures do you need? (Double) Linked Lists I would assume, maybe also a C++ Vector (ArrayList in Java jargon) anything else? Do you use priority queues?

It should be a doable job to make a wrapper ourself that is threadsafe around a vector and a priority queue, using semaphones, and that way avoid having semaphores directly exposed in the code. I will google a bit to see if I can find a implementation, it would not be more than 100-200 lines of code I think.
Jamendo.com - The best music store on the net, uses CC licenses.
User avatar
svenskmand
OD Moderator
 
Posts: 1850
Joined: 09 Dec 2009, 00:07
Location: Denmark

Re: Code style guidelines and cleanup/work

Postby oln » 26 Apr 2011, 22:26

The stuff that would be simple to replace in the code are queues (notification/request stuff). The access to the gamemap and creatures is more tricky, I don't know if that could be changed easily in the same way.
User avatar
oln
 
Posts: 1014
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby svenskmand » 26 Apr 2011, 22:38

Would a interface like this be enough? (I am not completely sure that these are sound :S):

Vector
Set(i,e) - is blocking only if someone is currently calling Get(i)
Get(i) - is blocking only if someone is already calling Set(i,e) on the vector.

Min-Queue
Find-Min() - Only blocks if someone is currently calling Insert(e) and e < Find-Min().
Delete-Min() - Blocks only if someone is currently inserting or calling find-min.
Insert(e) - Blocks only if e < Find-Min().

I think we need to figure out what we need first, i.e. what do we require from a deque, vector and min/max-queue, if we even need all of those.
Jamendo.com - The best music store on the net, uses CC licenses.
User avatar
svenskmand
OD Moderator
 
Posts: 1850
Joined: 09 Dec 2009, 00:07
Location: Denmark

Re: Code style guidelines and cleanup/work

Postby oln » 27 Apr 2011, 21:42

I have also been thinking about doing some changes to the player class.
Currently, the player-map interraction is handled directly via the gamemap class, from the frame listener for the local player, and from the client class (i think) for network players. Since we want to start looking at making an AI, I think we should try to use the player class as a base of interraction. We could have one abstract base player class, and have different player classes for network, local, and ai players.
(Andrew has even written "In the future if we decide to do a single player game, thiis is where the computer driven strategy AI calculations will take place.", so I would guess he has had something like this in mind.)
User avatar
oln
 
Posts: 1014
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby StefanP.MUC » 28 Apr 2011, 08:45

While we are talking about hardcoding everything, I'd like to say again that I have the feeling that things like AI, some of the GUI stuff and potential ingame "cutscenes" may better be scripted (Lua, for example).
StefanP.MUC
 

Re: Code style guidelines and cleanup/work

Postby svenskmand » 28 Apr 2011, 10:51

Yes in-game stuff like cutscenes and scripted dialogs in-game should be made in the editor, so having access to some scripting language of some sort in the editor is definitely something I would like. It would be nice to have a editor as flexible as the editor for StarCraft 2 and WarCraft 3, so you can make entire modes using the scripts. But I think it is going to be really hard work :( so if we should do it maybe we should do it later, when we have a better overview of what exactly it should be able to do. And then have this in mind when the current code is being written.
Jamendo.com - The best music store on the net, uses CC licenses.
User avatar
svenskmand
OD Moderator
 
Posts: 1850
Joined: 09 Dec 2009, 00:07
Location: Denmark

Re: Code style guidelines and cleanup/work

Postby oln » 28 Apr 2011, 11:07

Yeah, scripting would be nice. Could consider that at some point later. We should keep it in mind when redesigning the code layout.
User avatar
oln
 
Posts: 1014
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby StefanP.MUC » 21 Aug 2011, 11:16

Quick question about code style:
I often see other projects doing the following. Should we do this, too? I quite like it.

{l Code}: {l Select All Code}
//align the names and the assigments like a table
int           maxNumber = 5;
double        fooDouble = 0.5;
Ogre::Camera* camera    = getCamera();
bool          someBool  = true;;


And another quick question about the line length:
We limited it previously to 80 characters. After coding a lot on this game and collectin a lot of experience I think maybe increasing it to 100 characters would also work good for most people (most people have widescreens today anyway). 80 characters is often to few (especially in nested scopes like two nested if-else or in a switch or something).

Any opinions on these two points?
StefanP.MUC
 

Re: Code style guidelines and cleanup/work

Postby svenskmand » 21 Aug 2011, 12:32

Looks good, and sounds good :) just be sure to use 100 chars in both AS and C++
Jamendo.com - The best music store on the net, uses CC licenses.
User avatar
svenskmand
OD Moderator
 
Posts: 1850
Joined: 09 Dec 2009, 00:07
Location: Denmark

Re: Code style guidelines and cleanup/work

Postby StefanP.MUC » 10 Sep 2011, 12:46

Just pushed a very big commit that removes the Globals.h. I put all global vars as static members in the corresponding classes for the time being. This isn't the most ideal solution either, I think, but at least they are out of global scope and in their appropriate context. It will now be much easier to move them one by one into their final place (which still has to be decided).

Any ideas about what we should do with all the semaphores? Collect all semaphores at a central place? Or put/let them into the classes that need them?

Most of the Client/Server stuff is still in global scope, but I don't have enough knowledge about this already to touch the code, so I think it's better if someone else does the cleanup there. I never did work with network stuff, most of it still looks rather cryptic to me. :D
StefanP.MUC
 

Re: Code style guidelines and cleanup/work

Postby oln » 10 Sep 2011, 12:51

StefanP.MUC {l Wrote}:Any ideas about what we should do with all the semaphores? Collect all semaphores at a central place? Or put/let them into the classes that need them?

I think, ideally we should consider either moving to using boost::threads (it's already used by ogre and has to be linked to the project.), or write a class that wraps the semaphores. They semaphores should defined where the object they protect is defined. The network bits should probably be made into a class as well.
User avatar
oln
 
Posts: 1014
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby StefanP.MUC » 10 Sep 2011, 12:59

I often read that one should avoid using Boost (bloated, slow, ...). But I personally have no experience with it. So I have no real opinion here. But I think moving away from semaphores and pthreads (to whatever alternative) is good.
StefanP.MUC
 

Re: Code style guidelines and cleanup/work

Postby oln » 10 Sep 2011, 13:55

Not sure about the speed, but it is indead a very large download. Though, the Ogre SDK contains a large share of it already, so we shouldn't have to introduce any new downloads for building on windows. In linux it's split up and likely to be installed anyhow.
User avatar
oln
 
Posts: 1014
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby oln » 10 Sep 2011, 14:08

Also, by this I don't mean that I think we should rewrite the thread code to use another library now, but if we are to change the thread model later we should consider it.
User avatar
oln
 
Posts: 1014
Joined: 26 Oct 2010, 22:16
Location: Norway

Re: Code style guidelines and cleanup/work

Postby svenskmand » 11 Sep 2011, 09:35

If ogre already uses boost then I think we might as well use that instead of a second thread implementation, that would cut down a dependency :)
Jamendo.com - The best music store on the net, uses CC licenses.
User avatar
svenskmand
OD Moderator
 
Posts: 1850
Joined: 09 Dec 2009, 00:07
Location: Denmark

Re: Code style guidelines and cleanup/work

Postby StefanP.MUC » 18 Sep 2011, 15:21

While talking about Boost:
Did the cmake file or something else regarding the build system change lately? For some reason cmake doesn't find Boost that comes with Ogre anymore for me (everything else including Ogre is set up just fine) and setting the path's manually doesn't work either... I'm using the latest cmake (2.8.5) on Windows and the latest MingW and Ogre 1.7.2 is also were it always was.
StefanP.MUC
 

Re: Code style guidelines and cleanup/work

Postby oln » 18 Sep 2011, 15:48

I haven't touched that in weeks. I did use boost::shared_ptr in the last commit, but that shouldn't affect anything cmake does.
User avatar
oln
 
Posts: 1014
Joined: 26 Oct 2010, 22:16
Location: Norway

Who is online

Users browsing this forum: No registered users and 1 guest