AC Blog-ish

[Mousikos]

Yo diggity,

I'm not sure where this should go, but I am essentially starting a blog here on the forums. The first thing you might ask is, isn't this offtopic? I posted it in general because it's a blog about AC and the things I do with it. So why is it not private, you may ask? It's not private because I'm currently trying to figure out what exactly AC does. As of right now, I'm considering doing a UML diagram of the code. It's not private because, as stef put it, we don't have many active developers right now. So this is here, because I'm thinking "Hey, maybe someone will see this, something will click, and we'll have more people working on the project!".

I should put a disclaimer here that I don't pretend to be a competent code, nor do I believe myself to be competent in UML. I may get frustratingly stuck on something simple, there may be mistakes in my UML, but hey. This is why this is posted here -- so I can get feedback on if I mess up. I also may not finish this project, and if I don't, I hope someone will be able to follow up and continue working on this game that we all play.
So far, I have a class diagram of the audio manager class:



Hopefully, more updates to come!

Cheers,
Mousikos

Edit: I should probably mention that UML is supposed to be broad -- so you can look at a UML and implement the design in any language you choose. Therefore, sometimes in the code you will see `char*`, but in the UML you will see `string`. Also, I neglected to include constructors/destructors. If those are actually necessary in UML, let me know.

[Mousikos]

Once class short of finishing sounds and audiomanager. I'm not sure what to do with external functions.

[Mousikos]

So some people may be wondering why I'm bothering with this. Maybe it's a waste of time, maybe it isn't. Well, here is why I'm doing this.


-Code is made to be read by humans.-

The compiler may not perfectly translate your code to the most efficient machine code. In fact, even though compilers are getting better and better, if you want really efficient code, you should probably write a lower level than C++. Furthermore, you'd have to write for different architectures: x86, x86_64, amd64, MIPS, etc. Since most code, specifically Assaultcube, is meant to function or more than one system, it is advantageous to write it in C++ and let the compilers handle the tedious work.

-Humans are not perfect-
Have you ever messed up coding? I sure have. I spent hours looking over code trying to find the error and failed. I had to rewrite it. What a waste a time. Instead of trying to cut corners and leave out comments as well as not sectioning my code, I could have coded correctly. I may know what I'm trying to accomplish right now, and I may know exactly what my code does. But will I know it three hours down the line? How about three days or three months or three years?

What if I'm working with other people? Everyone has a different style of code. What comes easy to you may not come easy to other people. Humans, unfortunately, have also not evolved to the point where they can read mids. So, tell people what your code does! If someone discovers a bug that could be in your code 10 years down the line, make it easy for them! Section off your code! Tell people what you did!

-Standarize to make things easier-
This goes hand in hand with the fact that everyone thinks different. Come up with a standard, and /stick with it/. Don't be the one guy who instead of using the standard:

[SELECT ALL] Code:

/*************************************************************************
* Function: generatesound
* Description: Generates sound from a 3D point source
* Input: file - sound file to be played, coord_vec: Vector of the coordinates of the source
* Output: -
**************************************************************************/

You go and write your own shit:

[SELECT ALL] Code:

//Generates sound

If that's you, you are an asshole and you should feel bad.

But why is this worth mentioning at all?

Assaultcube is an OPEN-SOURCE PROJECT. That means anyone, not just our current set of devs, can come in and edit some things. Of course, there are some things that they can't use -- the images, the maps, the sounds, etc. Things that are copyright protected. But they can do whatever they want with the code. Therefore, we should try to encourage collaboration instead of writing spaghetti code.

The current state of assaultcube is spaghetti code. This is the current state of sound.cpp:

[SELECT ALL] Code:

// sound.cpp: uses OpenAL, some code chunks are from devmaster.net and gamedev.net


#include "cube.h"

It has a comment describing what the file is supposed to be -- and then contains NONE of the aforementioned content. It just includes a header file!



Yes, you read that right. Don't believe me? Here you go: https://github.com/assaultcube/AC/blob/m.../sound.cpp

This commit was made in 2009! No wonder AC had a sound bug! Here are the list of files in AssaultCube:
http://sprunge.us/EUHG

Knowing that openAL is something to do with sound, where would you look if you had a sound bug? My vote goes for the following files: audiomanager.cpp, oggstream.cpp, openal.cpp, sound.cpp, sound.h, soundlocation.cpp, soundscheduler.cpp, stream.cpp.

But audiomanager.cpp is clear some random code

[SELECT ALL] Code:

#define DEBUGCOND (audiodebug==1)

VARF(audio, 0, 1, 1, initwarning("sound configuration", INIT_RESET, CHANGE_SOUND));
VARP(audiodebug, 0, 0, 1);
char *musicdonecmd = NULL;

VARFP(musicvol, 0, 128, 255, audiomgr.setmusicvol(musicvol));

and then the definition of the audiomanager class! So where's the header file? Where is the class definition? I want to take a better look at it. There's no audiomanager.h. Oh, okay. It's buried in sound.h. But sound.h has more class definitions, and then some random code.

[SELECT ALL] Code:

void alclearerr();
bool alerr(bool msg = true, int line = 0, const char *s = NULL, ...);
#define ALERR alerr(true, __LINE__)
#define ALERRF(fmt, ...) alerr(true, __LINE__, fmt, __VA_ARGS__)

extern vector<soundconfig> gamesounds, mapsounds;
extern ov_callbacks oggcallbacks;
extern int soundvol;
extern int audiodebug;

extern audiomanager audiomgr;

Like I said before, I'm not a great programmer, but it sounds like most of that should go somewhere else. Perhaps in sound.cpp?

Okay, okay. That can be dealt with. But wait. Does audiomanager.cpp include cube.h? Hey, so does client.cpp... and clientgame.cpp and clients2c.cpp and command.cpp and console.cpp and crypto.cpp and docs.cpp... okay you get the point.

At this point, it really seems like most of the code is in cube.h. Cool. I guess. But then, hello cube.h:

[SELECT ALL] Code:

#include "platform.h"
#include "tools.h"
#include "geom.h"
#include "model.h"
#include "protocol.h"
#include "sound.h"
#include "weapon.h"
#include "entity.h"
#include "world.h"
#include "i18n.h"
#include "command.h"

Well, the good news is (make the best of it, right?) at least the authors had the foresight to write

[SELECT ALL] Code:

#ifndef __CUBE_H__
#define __CUBE_H__
#endif

Did I mention the sparse commenting? Also, a lot of variables are like e, s, r, etc. Saves the programmer trouble when doing it, I suppose. But wouldn't it be better if they were given some meaningful name?

By the way, somewhere in the source there is a doxygen folder...

Maybe I'm not that familiar with doxygen, but I don't see much doxygen like formatting: http://www.stack.nl/~dimitri/doxygen/man...locks.html

Well, the doxygen folder's last commit was four years ago so...

Anyway, enough rambling. That's why I'm doing what I'm doing.

Cheers,
Mousikos

PS: I completely understand not wanting to have like 5 files with 5 lines each in them. I might do it too. But then, I might put all the stuff sound.h mentions into sound.cpp. Just saying.

---

Also:

[SELECT ALL] Code:

server.cpp:2218:139: warning: address of array 'v->action->desc' will always evaluate to 'true' [-Wpointer-bool-conversion]

    logline(ACLOG_INFO, "[%s] client %s called a vote: %s", clients[v->owner]->hostname, clients[v->owner]->name, v->action && v->action->desc ? v->action->desc : "[unknown]");
                                                                                                                            ~~ ~~~~~~~~~~~^~~~
server.cpp:2225:152: warning: address of array 'v->action->desc' will always evaluate to 'true' [-Wpointer-bool-conversion]
    logline(ACLOG_INFO, "[%s] client %s failed to call a vote: %s (%s)", clients[v->owner]->hostname, clients[v->owner]->name, v->action && v->action->desc ? v->action->desc : "[unknown]", voteerrorstr(error));
                                                                                                                                         ~~ ~~~~~~~~~~~^~~~
server.cpp:3498:56: warning: address of array 'vi->text' will always evaluate to 'true' [-Wpointer-bool-conversion]
                            char *vmap = newstring(vi->text ? behindpath(vi->text) : "");

[Mr.Floppy]

While I actually appreciate this attempt, I wonder why you don't try to put down the the overall pipeline first and then drill down into each aspect from there.

On your 'irritations' about the state of the code, I'd like to point out the following in order to prevent people from getting the wrong picture:

Wouter as the initiator of the cube engine has stated himself that this code was never meant to be a real open-source project and he has willingly programmed as it comforted him best, regardless the readability to other people. Unfortunately he's one of those programmers that don't comment much. Yeah, that is considered bad style throughout literature, but hey it obviously did work for him and so did it later for eihrul and others. There should be a readme somewhere next to the source files, where this is written down.

Not that I'm defending anyone here, I just don't like how you make grown programmers look like they didn't know what they were doing and totally disregard from what spirit this code was born and how it has been 'patched up' with new functionality which wasn't even planned on the initial design. That's where things always get messy. :P

One particular remark about what you criticise, though.
I'm not big into C/C++, but I don't see what's wrong with #ifdefine-ing included files. I wish I could do this on another coding language that I'm working with. And what's wrong with keeping major parameters and the main loop in one file, namely cube.h?

[Mousikos]

While I actually appreciate this attempt, I wonder why you don't try to put down the the overall pipeline first and then drill down into each aspect from there.

What do you mean by pipeline? I'm trying to understand exactly what is linked with what before I figure out (generally) what does what. The first thing I'd probably tackle are the compiler warnings

On your 'irritations' about the state of the code, I'd like to point out the following in order to prevent people from getting the wrong picture:


Wouter as the initiator of the cube engine has stated himself that this code was never meant to be a real open-source project and he has willingly programmed as it comforted him best, regardless the readability to other people. Unfortunately he's one of those programmers that don't comment much. Yeah, that is considered bad style throughout literature, but hey it obviously did work for him and so did it later for eihrul and others. There should be a readme somewhere next to the source files, where this is written down.

Not that I'm defending anyone here, I just don't like how you make grown programmers look like they didn't know what they were doing and totally disregard from what spirit this code was born and how it has been 'patched up' with new functionality which wasn't even planned on the initial design. That's where things always get messy. :P

This thing?

[SELECT ALL] Code:

OPEN SOURCE
===========
Cube is open source (see ZLIB license above). This only means that you have
great freedom using it for your own projects, but does NOT mean the main cube
code is an "open source project" in the sense that everyone is invited to
contribute to it. The main cube code will remain a one man project (me), as my
minimalistic design is highly incompatible with the open source philosophy. If you
add to the cube source code, you fork the code and it becomes your own project,
do not ask for me to integrate your changes into the main branch, no matter
how brilliant they are.

To be fair, it's kind of difficult to make a game engine at all, so mad props to Wouter (or whoever wrote it) for doing so. But again, you should still try to make it readable because it's..... code. That being said, why open source it if it wasn't really supposed to be open source in the first place (That's how it sounds)?

I'm not big into C/C++, but I don't see what's wrong with #ifdefine-ing included files. I wish I could do this on another coding language that I'm working with. And what's wrong with keeping major parameters and the main loop in one file, namely cube.h?

Absolutely nothing is wrong with #ifndefine _something_here_ #define _something_here #endif structure. What I'm actually trying to say (sorry for the garbled English) is that they should use that /more/ as they separate the files into smaller chunks. They also don't really keep major parameters in one file, they just decided to throw everything in there for some odd reason. The main loop, anyway, seems like it should go in main.cpp. Feel free to correct me if I'm somehow wrong about this >.>

[Mr.Floppy]

Well, I personally do comment quite a lot even though I have hardly ever coded anything real complex. So I'm all with you there. However, I have no idea how that would change if ever I would become more experienced and confident. I really don't know, that's why I'd never blame anyone else.

Nevertheless it's clear and I do agree with you, the lack of comments is giving any new coder a hard time, hence I valuable your project. I obviously misunderstood you on the last part and you are of course right, main loops should go into the main file. For some reason I thought the cube file actually was the main file. My bad. :)

[Mousikos]

So, I don't know if anyone actually cares about my "blog", but if anyone is reading this... can someone explain why this is a struct instead of a class?

[SELECT ALL] Code:

struct voteinfo
{
    int boot;
    int owner, callmillis, result, num1, num2, type;
    char text[MAXTRANS];
    serveraction *action;
    bool gonext;
    enet_uint32 host;

    voteinfo() : boot(0), owner(0), callmillis(0), result(VOTE_NEUTRAL), action(NULL), gonext(false), host(0) {}
    ~voteinfo() { delete action; }

    void end(int result)
    {
        if(action && !action->isvalid()) result = VOTE_NO; // don't perform() invalid votes
        sendf(-1, 1, "ri2", SV_VOTERESULT, result);
        this->result = result;
        if(result == VOTE_YES)
        {
            if(valid_client(owner)) clients[owner]->lastvotecall = 0;
            if(action) action->perform();
        }
        loopv(clients) clients[i]->vote = VOTE_NEUTRAL;
    }

    bool isvalid() { return valid_client(owner) && action != NULL && action->isvalid(); }
    bool isalive() { return servmillis - callmillis < 30*1000; }

    void evaluate(bool forceend = false)
    {
        if(result!=VOTE_NEUTRAL) return; // block double action
        if(action && !action->isvalid()) end(VOTE_NO);
        int stats[VOTE_NUM] = {0};
        int adminvote = VOTE_NEUTRAL;
        loopv(clients)
            if(clients[i]->type!=ST_EMPTY /*&& clients[i]->connectmillis < callmillis*/) // new connected people will vote now
            {
                stats[clients[i]->vote]++;
                if(clients[i]->role==CR_ADMIN) adminvote = clients[i]->vote;
            };

        bool admin = clients[owner]->role==CR_ADMIN || (!isdedicated && clients[owner]->type==ST_LOCAL);
        int total = stats[VOTE_NO]+stats[VOTE_YES]+stats[VOTE_NEUTRAL];
        const float requiredcount = 0.51f;
        bool min_time = servmillis - callmillis > 10*1000;
#define yes_condition ((min_time && stats[VOTE_YES] - stats[VOTE_NO] > 0.34f*total && totalclients > 4) || stats[VOTE_YES] > requiredcount*total)
#define no_condition (forceend || !valid_client(owner) || stats[VOTE_NO] >= stats[VOTE_YES]+stats[VOTE_NEUTRAL] || adminvote == VOTE_NO)
#define boot_condition (!boot || (boot && valid_client(num1) && clients[num1]->peer->address.host == host))
        if( (yes_condition || admin || adminvote == VOTE_YES) && boot_condition ) end(VOTE_YES);
        else if( no_condition || (min_time && !boot_condition)) end(VOTE_NO);
        else return;
#undef boot_condition
#undef no_condition
#undef yes_condition
    }
};

[Luc@s]

and why would it be a class, instead of a struct ?

[Mousikos]

and why would it be a class, instead of a struct ?

It's an object with member variables/attributes. It has a constructor, and a destructor. It has member functions that operate on it. It sounds like a class. I mean, I get your point -- everything is public, so it's a struct. It just seems like it'd be a lot more readable if it was a class. Especially since a lot of the stuff: scallvoterrr, callvotepacket, scallvote all operate on a passed voteinfo. Why not just have a vote class?

[MathiasB]

So, I don't know if anyone actually cares about my "blog", but if anyone is reading this... can someone explain why this is a struct instead of a class?

I don't care if it is a struct or a class, it's just a name :) (besides the fact that structs are public by default)
What I do care it the lack of comments and doxygen through the source code. I'm not experienced in game development, and I'm not going the spit out the whole AC source code to contribute undocumented code/protocol. I simple haven't time to learn the whole source code.
"Code is read more than it is written."

---

It's an object with member variables/attributes. It has a constructor, and a destructor. It has member functions that operate on it. It sounds like a class.

 "If it looks like a duck, swims like a duck, and quacks like a duck, then it probably is a duck" :)

[TheNihilanth]

For that specific case, voteinfo would be either a struct or a class and it will function in the same way with no error nor it would crash because it is a passive data structure. If voteinfo actually were a derived class or implemented virtual methods probably there would be a problem when trying to deserialize it since the actual vtable would override the previous one.
Try changing the struct by class keyword and you'll see for yourself?
Now in the strict sense of OOP if it were a class instead of a struct it would be a badly designed class because it's breaking a corner stone in OOP: Encapsulation, since member data is public, and it should be private and it should provide accessors and mutators as a means to change its state.

[Mousikos]

Now in the strict sense of OOP if it were a class instead of a struct it would be a badly designed class because it's breaking a corner stone in OOP: Encapsulation, since member data is public, and it should be private and it should provide accessors and mutators as a means to change its state.

Well, yeah, I'd do that. I was taught to have member variables private and use getters and setters.

[Ronald_Reagan]

fyi, make clean before posting a list of files. Probably inflating it around 1/3 of the total size.

[Mousikos]

Ah, yeah that'd probably be best. >.> Thanks RR

[Mousikos]

Just a note for myself: How come you can shoot through the wall in ac_desert?

[MathiasB]

Now in the strict sense of OOP if it were a class instead of a struct it would be a badly designed class because it's breaking a corner stone in OOP: Encapsulation, since member data is public, and it should be private and it should provide accessors and mutators as a means to change its state.

Well, yeah, I'd do that. I was taught to have member variables private and use getters and setters.

Getters and setters are good, if you use them where it's needed.
No need to have a struct/class with only getters and setters that only assign/get a value, then you can just make it public...

[Mr.Floppy]

Strict encapsulation and in this respect getters/setters might not be necessary whenever there is no intention to reuse or publish code, in my opinion. Additionally some of those OOP-techniques (in terms of maintainability) may not fit in with a high performance goal, e.g. when building a real time graphics engine.

Though, sometimes it may just be bad style. ;)

[1Cap]

Hey Mousikos, dont worry about the "this", lol.
AC Blog-ish is welcome! Ty.

[Mousikos]

Well, apparently I'm incredibly stupid. Thrawn, thankfully isn't. He auto-generated his UML and is graciously allowing me to post it here with a warning: it is massive and takes 1GB of ram to load in his browser. If you have an old computer, you probably shouldn't click this: http://i.imgur.com/FMBmkxc.png

[MathiasB]

Cool ;)

[Thrawn]

Well, apparently I'm incredibly stupid. Thrawn, thankfully isn't. 
[gargantuan 6765 x 26520 pixels PNG]

You can go ahead and not use that silly PNG, I've sorted out the issue (which prevented me from rendering a vector graphic image and made me resort to raster graphics) and re-rendered it as a much more efficient and manageable .SVG here: https://www.dropbox.com/s/1makc6b9wop0pu...h.svg?dl=0 
I also made an alternative, radial style diagram here: https://www.dropbox.com/s/nqnyc66786811h...2.svg?dl=0
Note: The rasterizer / previewer that dropbox uses renders the vector graphics slightly wonky. If you download the actual vector graphic image, it's fine

Also, don't feel bad. It took me a few hours to get it to work (Thanks, Perl -_-), and it took a few tries to get the radial layout to work right ( https://www.dropbox.com/s/91hwewgwtnybo9...t.svg?dl=0 )