refactoring... goodbye UnitUpdater, and good riddance.

[silnarm]

Status:

Changes:

Code: [Select]

UnitUpdater::updateStop()           ==> StopCommandType::update()
UnitUpdater::updateMove()           ==> MoveCommandType::update()
UnitUpdater::updateAttack()         ==> AttackCommandType::update()
UnitUpdater::updateAttackStopped()  ==> AttackStoppedCommandType::update()
UnitUpdater::updateBuild()          ==> BuildCommandType::update()
UnitUpdater::updateHarvest()        ==> HarvestCommandType::update()
UnitUpdater::updateRepair()         ==> RepairCommandType::update()
UnitUpdater::updateProduce()        ==> ProduceCommandType::update()
UnitUpdater::updateUpgrade()        ==> UpgradeCommandType::update()
UnitUpdater::updateMorph()          ==> MorphCommandType::update()
UnitUpdater::updateGuard()          ==> GuardCommandType::update()
UnitUpdater::updatePatrol()         ==> PatrolCommandType::update()

UnitUpdater::doAutoFlee   => MoveCommandType::doAutoFlee()
UnitUpdater::doAutoAttack => AttackCommandTypeBase::doAutoAttack()
UnitUpdater::doAutoRepiar => RepairCommandType::doAutoRepair()

UnitUpdater::repairableOnRange() => RepairCommandType::repairableOnRange()
UnitUpdater::repairableOnSight() => RepairCommandType::repairableOnSight()
UnitUpdater::searchForResource() => HarvestCommandType::searchForResource()
UnitUpdater::enemiesAtDistance()   => ??? not sure what I did with this one
UnitUpdater::updateAttackGeneric() => AttackCommandType::updateAttackGeneric()

UnitUpdater::attackerOnSight()   => CommandType::attackerOnSight()*
UnitUpdater::attackableOnSight() => CommandType::attackableOnSight()*
UnitUpdater::attackableOnRange() => CommandType::attackableOnRange()*
UnitUpdater::unitOnRange()       => CommandType::unitOnRange()*
* These could probably go in AttackCommandTypeBase ?

Functions Refined:
BuildCommandType::update()
HarvestCommandType::update()

Awaiting refinement:
RepairCommandType::update()
probably others ??

Original Post:
I've been doing some code surgery... The UnitUpdater was getting out of hand, and all those command updating methods were crying out to be stolen away and put in polymorphic Command classes...

unit_updater.cpp was 1699 lines, is now 796.

The various CommandType subclasses now override the virtual update() of CommandType, and that's where commands are now updated, not in the UnitUpdater.  It wasn't the cleanest of transplants, and could use some tidying, most of the CommandType::update() methods do a lot of reaching back into the UnitUpdater, but that can be sorted out easily enough now. Everything works, and it's much, much, much more manageable.

I've added a 'dummy' (do nothing) skill, and command for it, which can have costs and/or production attached.  This was at the request of Modman, and I actually had it working weeks ago, but I wasn't happy with how it worked... Once all this refactoring was done, it was very easy to hook it up and be (reasonably ) sure it would work as expected.

@Hailstone: I've merged the fixes from trunk, and everything from merge_3.2.2 into pathfinder... I thought it was ready to roll, but some more extensive last minute testing revealed big problems for size 2 units... I'll hopefully have that sorted tomorrow... if you're happy with the changes, we can copy it back to trunk, might as well test the path finder and new DummySkillType as well as LUA...

Edit:
I've moved the rest of the command related stuff from the UnitUpdater to CommandType and subclasses, and I've removed all references to the UnitUpdater from the CommandTypes.
I've 'tagged' the rest of 'unit_updater.cpp' with comments beginning //REFACTOR
with messages stating where I believe that chunk of code belongs...
@Hailstone : Have a look, let me know what you think...

[hailstone]

I think it makes sense to me now . The methods were being grouped by the function they have, rather than being in its related ADT, which creates tight coupling and makes it harder to maintain. I've noticed the same thing in renderer with the GUI rendering. When I update the GUI it will fix that.

If you are finishing off the current branches and merge them with trunk then I could start work on the GUI or something else. Although I'd like to look more at the pathfinding code.

[silnarm]

... The methods were being grouped by the function they have, rather than being in its related ADT, which creates tight coupling and makes it harder to maintain. I've noticed the same thing in renderer with the GUI rendering. When I update the GUI it will fix that.

Yep, in fact the whole "UnitUpdater" looks like what I call an 'object of convenience' ... Probably created to reduce clutter in the Unit class, but anything with a name like that is suspicious in my book,
Unit = noun
Update = verb
"UnitUpdater" = noun + verb + 'er'

The 'er' makes the whole thing a noun again, but it's artificial... there shouldn't be a 'UnitUpdater', we have a 'Unit', and it has an 'update' method.... enough said...

If you are finishing off the current branches and merge them with trunk then I could start work on the GUI or something else. Although I'd like to look more at the pathfinding code.

Yeah, it should probably be scrutinised a bit... I was going to get a few of those fancy Unit Tests of yours set up for it... For the AnnotatedMap & NodePool at the least, not sure if testing the whole thing outside the context of the game will be possible (without lots of extra work...)

[Omega]

Yep, in fact the whole "UnitUpdater" looks like what I call an 'object of convenience' ... Probably created to reduce clutter in the Unit class, but anything with a name like that is suspicious in my book,
Unit = noun
Update = verb
"UnitUpdater" = noun + verb + 'er'

The 'er' makes the whole thing a noun again, but it's artificial... there shouldn't be a 'UnitUpdater', we have a 'Unit', and it has an 'update' method.... enough said...

Well said. 

Seriously, the way I see it, if it can be removed without affecting the program, making the code unreadable, or drastically increasing filesize, then why hasn't it been done before?

Better now than never.

[silnarm]

Skills and Commands...

A skill is a 'low level' action, a works by progressing from 0.0 to 1.0
as it is updated [this is currently in Unit::update(), the contents of
which may be subject to re-factoring soon...]

A Command is 'high level' action, a sequence of low level skills.

Example...
Moving:

The move skill performs a 'single' move, from one map cell to an adjacent cell. Unit::update() calculates how much to progress the skill by based on the unit type's speed, whether its a diagonal move, & up or down hill. [Other skills are far less interesting, usually just using a 'speed' setting]

When the progress hits 1.0, Unit::update() return true.

In UnitUpdater::updateUnit() [this code is bound for World::update()], if Unit::update() returns true (the skill cycle is complete), the command is updated.

In the move command, this simply pops the next position off the path and sets it as it's next position, then starts the move skill again.

Obviously it's a bit more complicated than that, the path might be blocked, but the move command isn't much of an issue anyway...

The build command is an issue, or rather, was

The whole update() method as lifted from the UnitUpdater::updateBuild(), is essentially an if/else statement, and that one if/else comes in at about 175 lines of code! Just figuring out what is happening where is a nightmare, the if and else are 144 lines apart...

Code: [Select]

// What it does do (somewhat abstracted) is this:

 If the unit is not executing the build skill
    Find the nearest cell we can commence building from
    Find a path to that cell
    If the pathfinder says we're 'on the way'
       set our current skill to move and return
    Else If the pathfinder says 'path is blocked'
       Cancel the current command (which sets the skill to stop) and return
    // Else fallthrough... we have arrived...
    If we still have room to build
       do a whole bunch of stuff ( apply costs for building, play start sound, etc )
       inform the PathFinder of new obstactle in field
    Else
       If another worker beat us here and the building is under way
          set skill to build, configure to build existing unit
       Else
          If there are units in the way
             If they all have a move skill return // we'll 'wait' not cancel
          cancel command
 Else
    // We are executing the build skill
    If the unit type of the unit we're building does not match this command
       // this is just a sanity check, it should never actually happen...
       Bail Out (set current skill to stop)
    If the unit we're building is now built
       // this is for when another worker finsishes the built earlier in this update
       finish command, set skill to stop
    'Repair' the building
    If it is now built
       finish command, set skill to stop
       inform the ScriptManager that a new unit has been created
       do some more stuff (play built sound, networking... etc)
    // Else we'll see you again next time around!
    // We'll perform another skill cycle of 'build' and when that
    // finishes, this command will be updated again.
I think we could use some abstraction here...
What I now want the build command update to look like is:

Code: [Select]

   cacheUnit ( unit );
   if ( unit->getCurrSkill()->getClass() != scBuild )
   {
      if ( moveToBuildingSite () )
         startBuilding ();
   }
   else
      continueBuilding ();

moveToBuildingSite() will execute the move skills to get use where we need to be, and comprises the first part of the 'top level' if clause from the old code.
When moveToBuildingSite() returns true, we'll be in position, so we startBuilding(), which was other part of the original if clause.
continueBuilding() is the former else clause.

Simple!

This is now done for the build command, just committed the code...
But it needs to be done to the others, probably harvest and repair
are the most in need...

[silnarm]

@hailstone:

So... your mission should you chose to accept it... is to pick the harvest or repair command type, and make it your new best friend! Get to know it intimately... make prodigious use of 'go to definition' and 'find all references' if needs be, but try not to stray to far, it's easy to get lost

There are a few ways you could go about it, probably the easiest is to make a copy of the update method and start removing the 'inner' bits, perhaps replacing them with a comment. figure out the logic of the function, then split it up as you think appropriate.

I've also devised a mechanism to clean up a the locals...

Code: [Select]

void BuildCommandType::update(Unit *unit) const
{
   // We're going to split this this function up, so we'll start by
   // getting rid of all these locals...

   // These ones all relate to the command we're updating, we'll store these
   // in a 'cache' (a bunch of static member variables), Glest is a single
   // thread, so this is perfectly safe.
   // We'll also store the Unit in the cache, so we don't have to pass it on
   // to each of the new helper functions
   Command *command= unit->getCurrCommand();
const UnitType *builtUnitType= command->getUnitType();
Unit *builtUnit = NULL;
Unit *target = unit->getTarget();
   
   // These are effectively 'globals' They're all either singletons
   // or they can be obtained by a getter (or two) from a singleton.
   // This list might be added to, remember this is just the BuildCommandType
   // other commands are almost certain to interested in the new boy ScriptManager
   // and possibly others... actually this one needed the ScriptManager too, but
   // it was just 'once' so it got it from Game::getScriptManager()
   //
   // These were largely introduced because of the move from the UnitUpdater,
   // and because of a desire to not reference the UnitUpdater.
   //
   // We'll cache these in CommandType to reduce redundancy, not everything
   // will be built when the CommandTypes are constructed though, so a 'lazy
   // load' in the constructor wont do, we'll add CommandType::cacheGlobal()
   // and call it from Game::init()
   Game *game = Game::getInstance ();
   World *world = game->getWorld ();
   Map *map = world->getMap ();
   PathFinder::PathFinder *pathFinder = PathFinder::PathFinder::getInstance ();
   NetworkManager &net = NetworkManager::getInstance();

So the global ones are done, you can now access game, map etc from anywhere in any CommandType.
You'll want to copy the mechanism for the others, and modify to the command types needs.

Code: [Select]

protected:
   // returns true when arrived
   bool moveToBuildingSite () const;
   void startBuilding () const;
   void continueBuilding () const;
 
   // unit cache...
   static Unit *unit;
   static Command *command;
   static const UnitType *builtUnitType;
   static Unit *builtUnit;
   static Unit *target;

   void cacheUnit ( Unit *u ) const;

unit and command could probably go in CommandType actually, and cacheUnit() could then be in CommandType and polymorphic... if you think this might be worthwhile, don't use the obvious solution:

Code: [Select]

class Type
{
public:
   void doSomething ();
   virtual void func () { doSomething(); }
};
Class SubType : public Type
{
public:
   void doSomethingElse ();
   virtual void func () { Type::func(); doSomethingElse(); }
};
This means every class the derives from Type must call Type::func() if it overrides func(), that means you have remember (or worse, someone else has to know) that this is the case... inevitably, someone (quite possibly yourself) will forget, the result we be a subtle bug... in our case the new not hooked up correctly command will attempt to update using the unit and command reference from the last command update (what that might have been)... and madness would ensue.
Use this solution instead:

Code: [Select]

class Type
{
   void doSomething ();
   virtual void doSomethingElse () = 0;
   void func () { doSomething(); this->doSomethingElse(); }
};
Class SubType : public Type
{
   virtual void doSomethingElse ();
};

This way you don't override func(), you override doSomethingElse(), and when func() is called you can ensure that something happens before somethingElse
The polymorphic function doesn't need to be pure virtual as I did above, in our case we will have commands that don't want to override cacheUnit, so just make it something like:

Code: [Select]

virtual void doSubTypeCache ( Unit *unit ) {}
void doBaseCache( Unit *unit );
void cacheUnit ( Unit *unit ) { doBaseCache(unit); this->doSubTypeCache (unit); }

Sorry if that makes your head hurt Gotta remember you're still at uni
But I believe after merge you're up to the task, you can just ignore all that polymorphic stuff for the caching if you like, just make a cacheUnit() like I did in BuildCommandType.

Use the pathfinder branch for now, we'll copy it over to trunk some time after we tag, and it's cleaned up a bit more

[hailstone]

Sorry if that makes your head hurt Smiley Gotta remember you're still at uni Smiley

I think debugging linker errors gives me a worse headache.

I'm not sure what you're talking about with the cache functions but I'll take another look when it's not so late.

I've been looking at HarvestCommandType so I'll do that one.

Also with HarvestCommandType::getDesc is there any reason why it would be modifying a reference to str instead of just returning a local string? and Daniel has replaced the str += type setup with a stringstream in Game::render2d.

[silnarm]

I think debugging linker errors gives me a worse headache.

I'm not sure what you're talking about with the cache functions but I'll take another look when it's not so late.

Linking problems can indeed be rather frustrating...

The whole caching scheme is just for convenience, because we're splitting functions up and I didn't want to have to pass a pile of parameters, or 're-discover' them at the top of every helper function we introduce.  Putting the 'unit' and 'command' in CommandType rather than the subclasses makes sense from a design point of view, but it'll actually only save us two declarations and two assignments per sub-class, so I wouldn't bother. Just have a look at 'chacheUnit()' in BuildingCommandType, and do the same sort of thing...

I've been looking at HarvestCommandType so I'll do that one.

Also with HarvestCommandType::getDesc is there any reason why it would be modifying a reference to str instead of just returning a local string? and Daniel has replaced the str += type setup with a stringstream in Game::render2d.

Cool, any issues/queries, let me know

SomeCommandType::getDesc() methods all do this, I have no idea why... it may be that one/some of them modify the existing string... but I'm not sure on that, it would certainly seem that they could just return it...

The stringstream thing is a performance hack, string streams are much better for 'long' strings composed by a series of concatenations.

Edit:
Keep in mind CommandType::update() is const, all your helpers will need to be too.

Also, if you can be bothered, you may want to split command_type.h up, so you wont need to recompile so much when you need to make changes to the class declaration. They're all declared in the one header at the moment, so changing one means recompiling all of them, and anything that depends on them... and then anything that depends on those... etc...
Actually, leave most of it... just make a harvest_command_type.h for now, we'll create the headers as we clean up the module files, and we can probably leave some of the less interesting types in there.

[hailstone]

I've refactored HarvestCommandType and made the privates of BuildCommandType protected too. I'll leave the header file creation for you to sort out.

[silnarm]

Command::pos2

While it's location has changed with refactoring the actual code we are moving about at the moment is mostly vanilla glest code.

In GAE we can take advantage of 'pos2' in Command. Daniel introduced this to support the patrol command, and if I recall correctly he has also used it for some of the other new commands.

As we are refactoring the old commands, we might as well see if we can make use of pos2 in them too.

Code: [Select]

bool BuildCommandType::moveToBuildingSite () const
{
   int buildingSize = builtUnitType->getSize();
   Vec2i waypoint;

   // find the nearest place for the builder
   if(map->getNearestAdjacentFreePos(waypoint, unit, command->getPos(), FieldWalkable, buildingSize))
   { 
      // has waypoint changed ?
      if(waypoint != unit->getTargetPos())
      { 
         // set new waypoint
         unit->setTargetPos(waypoint);
         unit->getPath()->clear();
      }
   }
   else
   {  // there is nowhere to 'stand' and build
      game->getConsole()->addStdMessage("Blocked");
      unit->cancelCurrCommand();
      return false;
   }
   // etc

Here waypoint is a local, which is 'filled in' by Map::getNearestAdjacentFreePos() this is then checked against the unit's current targetPos, if the result of Map::getNearestAdjacentFreePos() has changed since the last command update,  the unit's cached path is cleared, which causes code elsewhere to calculate a new path to the new targetPos.

So, with pos2 at our disposal, we can store the waypoint in the Command object, the Command constructor initialises this to (-1,-1) for us, so what we now do is check its validity and if it's not valid, then we call Map::getNearestAdjacentFreePos(), in future updates pos2's position will be 'valid' (not -1,-1) so we also need to check if it really is still valid, for this we use the function Map::isFreeCellOrHasUnit ().  We don't use Map::isFreeCell () because the skill cycle that just completed might have brought us to our targetPos, and then isFreeCell() would definitely be false, because that's where our unit is!

This eliminates the need to call Map::getNearestAdjacentFreePos() if the existing target is still good.
Map::isFreeCellOrHasUnit() is cheap.
Map::getNearestAdjacentFreePos() is not.
So we get a bit better performance too.

Code: [Select]

bool BuildCommandType::moveToBuildingSite () const
{
   // Have we got a target in mind ? is it still free ?
   if ( command->getPos2().x == -1
   ||   !map->isFreeCellOrHasUnit ( command->getPos2(), unit->getCurrField(), unit ) )
   {  // find a targetPos
      int bldngSize = builtUnitType->getSize();
      Vec2i &bldngPos = command->getPos ();
      Vec2i waypoint;
      if ( !map->getNearestAdjacentFreePos ( waypoint, unit, bldngPos, FieldWalkable, bldngSize ) )
      {
         // there is nowhere to 'stand' and build...
         game->getConsole()->addStdMessage("Blocked");
         unit->cancelCurrCommand();
         return false;
      }
      command->setPos2 ( waypoint );
      unit->setTargetPos ( waypoint );
      unit->getPath ()->clear ();
   }
   // etc

A couple of notes:
Why didn't we just use unit->getTargetPos() ?
This seems logical at first, and we indeed could have done it. But our new method relies on knowing when the waypoint hasn't been set yet... The command object was constructed for 'unit' to execute one command of 'this' type, when it was constructed pos2 got initialised with ( -1, -1 ). We know this, it's guaranteed. We could make sure Unit::targetPos is appropriately reset before use, but we've already got pos2, we can use it for this purpose without having to check and/or write any code elsewhere.

It creates a tiny bit of redundancy, we probably can now ignore Unit::targetPos in this update() and helpers, and just use Command::pos2. 

Is this where we should keep the waypoint ?
I think this makes more sense in an OO sense too, while you could argue it's the unit's current 'target', what it really is is the unit's current command's (sub)target [sub because the 'target' would be pos, the location of the proposed building].

We may even be able to eliminate targetPos from Unit, and use Command::pos2 instead, this is definately feasible for the original commands, I'm not sure if any of Daniel's new commands use Unit::tagetPos and Command::pos2 for different things though. If Daniel hasn't exploited the oppurtunity to use this to store a third location for any of his commands, I think we should look into scrapping Unit::targetPos.

[daniel.santos]

Hello everybody, sorry for my extended absence.

I haven't been playing with the code for a little while but I wanted to comment about Unit::targetPos.  I agree that there's definately overlap here and there, but this one is a little funny.  If I say to attack to this location, and the location is visible to my team and there is a unit where I said to attack, then I really mean attack that unit.  In this case, my unit should chase down the enemy unit until my unit or the targeted enemy dies, the command is cancled or the enemy unit becomes non-visible.  For the latter scenario, what would the correct behavior be?  I vote *against* continuing to chase a unit you cannot see, that would be an exploit.  I think that in this case, your unit should attack to the last known position of the targeted enemy unit, but the reference to the enemy unit should continue to be stored.  Then, if the unit becomes visible again, your unit can continue to target that specific unit (I don't believe this is the current behavior).

I think that would be ideal behavior, but it presents a problem: what happens when you are chasing your target, you loose sight, but you see an alternate?  This is tricky -- you should attack the nearest enemy while proceeding to the last known location of your actual target.  So rather Unit::targetPos is still used for storing the position of your "current target" (even though the command target may be out of sight, but you still remember them) or not, I suppose I don't care.  One benefit of Unit::targetPos is that it can be accessed faster than Unit::commands[0].pos, but that alone shouldn't be given much weight unless Unit::targetPos is accessed a *lot*.

Good work btw! I can't wait to dig back in and see what all you've torn up! =)

[hailstone]

Welcome back to the forums. 

It sounds good but makes it more complicated and possibly annoying. What happens when the unit gets to the last known location of the target and still can't find it? How will the player know that the unit has lost its target? If the target becomes visible again but the unit is now across the other side of the map will it still try to attack?

[Omega]

Daniels back, Daniel's back, Daniels back!!!

So, do you plan on getting back into the GAE project now?

Back on topic, suppose a unit will only chase a target within a certain distance? ie: 1.5x the sight value. So if the unit's site value is 10, they will continue to persue the foe as long as they are within 15 squares. So if we have a slow unit persuing a fast one, once the fast one gets too far away, the slow one stops persuing (ie: summon going after a horseman. Not only is that a match made in hell, but I can imagine its hardly a fair race). And if a unit stops persuing a foe, they take the equivilant action as though they had killed the foe. They move back to their position they were originally in.

Basically, there's no point in following a unit too far out of sight, though we should be able to follow one just a little bit out of sight.

[silnarm]

Certainly good food for thought.

Welcome back Daniel!

I think current behaviour is to chase down the unit if visible, and if it goes out of sight, the 'unit target' is nulled and replaced with a 'position target' to it's last known position, similar to Daniel's suggestion, but without the possibility of re-acquiring the original target. I think this should probably be tied to the unit's 'stance', not that we have those yet   A unit with an aggressive stance might just keep on going, heading beyond the last known position in the same direction the target was last seen heading.  A unit on a defensive stance might give up the chase as soon as the target is out of sight, and a neutral stance might have the behaviour Daniel suggested.

If the target becomes visible again but the unit is now across the other side of the map will it still try to attack?

Only if it is still executing the same attack command, which would be unlikely if it is now on the other side of the map, but it may lead to some undesirable behaviour.

ie: 1.5x the sight value. So if the unit's site value is 10, they will continue to persue the foe as long as they are within 15 squares.

Surely you're not suggesting we let a unit unerringly track another unit it can't see ?? This is what Daniel was voting against, it being an exploit, and I couldn't agree more.  One cell out of sight is as good as 100, if you can't see it, you can't see it!

[ewomer]

nice to see that your still alive, i hope everything is ok

[gameboy]

any news from Daniel?
He seems to have come back and disappeared again.

[daniel.santos]

Hello everybody!  Thanks for all the warm greetings! =)  I have been pretty busy, but trying to make time to dig back in and get my feet, hands, and hair wet.  OK, maybe just my feet...

<off-topic>

So this discussion is really a bit off-topic, sorry about that.  But it makes it nearly impossible for me to not think of unique unit abilities, like tracking =)  (which is even more off-topic)

<very-off-topic>

Tracking
As a unit traverses from one cell to the next, it records a list of up to x history of his travels (200-ish?  nah, 256, that's a nice round number) along with the time they last left the cell (so if they re-cross their trail, the old node is dropped).

Code: [Select]

struct {
    int x;
    int y;
    unsigned int time;
} TrailNode;

list<TrailNode> travelHistory;
Something like that.  A unit with this ability should have a skill to identify tracks (perhaps he can search for them or he/she tells you when they find some) and then another skill to track them.  This could be quite a dandy mechanism for an enemy base.

</very-off-topic>

What happens when the unit gets to the last known location of the target and still can't find it?

I'm thinking that we need a more complete notification framework.  I remember adding a few things to faction.xml for some type of notifications and discussing notifications for seeing an enemy for the 1st time, etc.  I think the idea here is that the unit does indeed stop and then emits a notification to let you know it lost it's target; auditory, blip on radar (as previously discussed), etc.

</off-topic>

Gotta run, will bbl.

[daniel.santos]

Ok, now for the rest of my feedback on this.  Remember that the situation I'm talking about is one where you explicitly order your unit to attack another unit.  How they behave when a unit sees an enemy unit and auto-attacks them is a different story.  And while Glest/GAE doesn't have "stances" yet, they do have guard and patrol.  You can order a unit to guard a certain spot of land and they will always return to that spot whenever they have attacked an enemy they saw.

I think that I made a mistake to code auto-return functionality originally because this should be done only when a unit is guarding (or patrolling) -- and many units don't get a guard command until they have achieved some discipline, by building a training grounds.  When auto-attacking, I think that what they should do is to stop dead in their tracks once no more enemies are in sight.  The old behavior was worse, they would continue to move towards the location that they first spotted the enemy when auto-attacking, and that was very bad.  The same holds true for auto-return after an auto-repair command.  I think that auto-attack and -repair should have the unit stop and go idle when they have nothing left to do.  The explicit repair and guard commands should suffice for auto-returning. This is still off-topic to this thread, sorry, I just wanted to clear that up.

[Omega]

You're back!

BTW, keeping at this strand of off topic, seeing this is the only place you've been, I need to request that codemonger is updated. The dependancies and source links are bad. It screwed me up completely! I was making a custom GAE for military, but realized I was one version behind, but only after changes, so in short, I wasted my time. I am very bad at getting the correct versions, so I need a good dependancies (preferably only with exactly what I need, due to bad internet).

Please help me.

Sorry for the offtopic.

[daniel.santos]

Sorry Omega, I have a limited amount of time, but I'm trying to invest as much as I can right now.  I'm not certain how to work out this issue at the moment, but I'm getting hailstone & silnarm access to the server so they can make changes as need be or do support in case I'm unavailable (bring apache back up or whatever).  Anyway, I'll hopefully be more active in the next few months, I've actually been making a bit of progress on the networking code I started last December =)

[silnarm]

Maybe a new attribute for units, 'discipline' could give the answers to how they react to certain situations, and how closely they abide to their 'stance'.  It could of course also be 'upgraded' and 'effect'ed as usual.

Warning: Back On-Topic, highly technical

unit and command could probably go in CommandType actually, and cacheUnit() could then be in CommandType and polymorphic... if you think this might be worthwhile, don't use the obvious solution:

unit and command had to go in CommandType in the end, because I've trimmed the paramater lists of a few functions, removing 'redundant' stuff that we can derive from the unit currently having it's command updated, and because autocommands might call functions housed in other CommandTypes (ie, auto-repair might be evaluated in StopCommandType::update(), and in turn call RepairCommandType::repairableInSight(), which no longer takes the unit that can repair as a parameter, it assumes that the 'current' unit is that unit).

The solution I was proposing has issues with 'deep' inheritence, I knew this... what I hadn't considered was the possibility of multiple inheritence.  This makes that solution completely blow up.  So in a sub class of CommandType, the virtual cacheUnit () method MUST call CommandType::cacheUnit ().