Public Request: Post TechTree.cpp Mistakes / Suggestions Here about commandcenter HOT 10 CLOSED

From the first glance:
We don't need the full techtree info in one class mostly because the bot plays for one race during the game.

I suggest to:
a. Move all the init functions outside the tech tree class.
b. Split the init functions into three different kinds (one for each race).
c. At the start of command center binary (e.g. after parsing .json config) init the tech tree class according to specified race.

That will allow to speed up tree manipulations a bit (lesser data, faster search), split large init functions into smaller pieces and keep implementation away from the data.

For #b we can benefit from object oriented design and create one tech tree class for each race each of which will derive from the original techtree and will populate itself with unit data in constructor. Such approach allows to get rid of the init functions completely (we will create an object of specific class for the specified race at #c). Another benefit will be implementation of race specific details inside derived classes (e.g. using virtual functions) if it will be needed.

from commandcenter.

I disagree with most of the suggestions above:

a. The #1 rule of CommandCenter is that all data must live within the CCBot class, with no global information whatsoever. This is due to the fact that we can play Bot vs. Bot within the same application. So the data must be constructed within the TechTree class

b. Also disagree - even though the player is only playing one race, they may want to know things like "what does my opponent need to be able to build unit X" and the opponent may be a different race

c. See b above. It also won't eliminate the init functions, it just may move them to a constructor. I think going inheritance with this structure is overkill, and will just add more classes to the bot without tangible benefit

from commandcenter.

Even if you need several races you will benefit from smaller techtree classes just because it gives more freedom in implementation and minifies the init boilerplate. Also the things like m_race will be thrown out because your class type determines the current race and you name the things explicitly as they are. In general it is three different tech trees in big one now.
Another small plus: smaller trees are easier to test.

There are not global data in my suggestion and there are no need in any globals. You can store each tree in the bot or anywhere else using base class or plain derived objects. The init dara should be hardcoded in the each classes consructor or dedicated init methods.

If you really need need to store everything in one place there could be another class which manipulates with trees (creates it and manages somehow i.e. another level of abstraction).

from commandcenter.

E.g. the code could look like:

struct TechTree {
    virtual void init() = 0;

// Simple methods and other basic operations below.

};

struct ZergTree: TechTree {
    void init() {
        // Fill all the IDs inside.
    }
};

struct TerranTree: TechTree {
    void init() {
        // Fill all the IDs inside.
    }
};

struct ProtossTree: TechTree {
    void init() {
        // Fill all the IDs inside.
    }
};

Then we introduce another level of abstraction:

class TechForest {
public:
    TechForest(): m_zerg(), m_terran(), m_protoss()  {
    }

// Some getters and finders for the trees below.

private:
  ZergTree m_zerg;
  TerranTree m_terran;
  ProtossTree m_protoss;
};

That means that all find logic is moved to the higher level class while the tree itself contains only basic operations with the data.
Inheritance can be easily thrown out in this case since the forest can do all what's needed.

from commandcenter.

Another approach: divide trees by object type i.e. UnitTree, AbilityTree and UpgradeTree because current implementation always check what kind of objects we search and keep the separate std::map for each of these.

Anyway actual implementation depends on the tree's usage which is not implemented.

from commandcenter.

While I appreciate the suggestion and agree that it is indeed nicer, I believe that it provides no tangible benefit for the bot overall. For my personal style, I'd rather keep the design simpler and be able to see all of the data in one place than save a few bytes of space.

In my experience with things like this, even if something is nicer in terms of abstraction and software design, it can lead to less robustness in the future by putting functionality in ever more specific sub classes, whereas currently we have just 2 maps indexed by an int, which is one of the most efficient way of storing and retrieving the data.

from commandcenter.

Well that really depends on design and taken design decisions. I saw too many all-in-one classes becoming a hard-to-support beasts :)

from commandcenter.

All I can say is that we will have to disagree on this one. In my experience with open source bots like this, having more monolithic structures has been easier to maintain and provide documentation for, and is less confusing to code and for students to understand, so that's why I go with it over the absolute most abstract OO choice :)

from commandcenter.

Here several useful warnings generated by the compiler after the latest rebase:

/Users/alkurbatov/work/src/github.com/alkurbatov/commandcenter/src/TechTree.cpp:21:75: warning: missing field 'isTownHall' initializer [-Wmissing-field-initializers]
    m_unitTypeData[0] = { sc2::Race::Random, 0, 0, 0, 0, 0, 0, {}, {}, {} };

/Users/alkurbatov/work/src/github.com/alkurbatov/commandcenter/src/TechTree.cpp:166:70: warning: missing field 'isSupplyDepot' initializer
      [-Wmissing-field-initializers]
    m_upgradeData[0] = { sc2::Race::Random, 0, 0, 0, 0, 0, 0, {}, {} };

Probably you should use a kind of constructor of 'addSomething' function to handle all the default values properly.

from commandcenter.

There is an error with initUnitTypeData, the "error" case does not exists because both initUnit and initUpgrade set m_upgrade[0]

from commandcenter.