Clean up how units transport cargo and get rid of yield units

Transporting anything in transport units appears to be some sort of hack. The cargo gets a transport unit assigned, but the transport doesn't get the cargo assigned. This means to figure out what is in a transport, all units on the plot is looped and checked for transport == the transport unit in question. That's a lot of looping if multiple transports are on the same plot.

Also since it's an instance of CvUnit, which points to a transport, then all cargo needs to be units. This includes yields, which in turn means we have units, which represents cargo. CvUnit then contains an int to tell how many yields is being transported.

Proposal

Make a class to handle all code and variables related to being a transport. CvUnit then has a pointer to that class, which will be NULL in most cases. This will reduce the size of CvUnit (in general we should do that with a lot of data, which is only used by very few entities).

This class should contain data related to cargo capacity, what kind of cargo can be carried etc etc. Also it should contain a list of what is currently being transported.

struct TransportedUnit
{
    CargoTypes cargoType;
    union
    {
         UnitID;
         struct
         {
            YieldTypes eYield;
            int iYieldAmount;
         }
    }
};

Not only will we be able to loop the cargo in question, we can use this to store which yield is being carried and how much. This in turn will remove the need for all the unit xml entries, which represents yields. It will also reduce memory usage by not allocating units each time a yield is being transported.

We will need new drawing code and new drag code, but those two should be fairly simple to do. Just make a widget for yield dragging, set iData1/2 to transport unitID and cargo slot index. That's enough to reconstruct precisely which slot has mouseover, has been dragged etc.

This system can even be expanded to handle more cargo types as we have plenty of enum values to add. Empty, unit, yield is what we need, through we could add one to represent that it is a not-first for a multi slot cargo (treasure).

According to feedback, I think I need to explain how the union works. A union in c++ is declared like a struct, but instead of adding all the variables after each other, they are all placed first meaning they share memory. This is a memory saving trick in cases where it is known only one of them will be used.

In this case, the new CargoTypes enum will remember which type is stored. If it's a unit, it has the unit ID. If it's a yield, the same memory will be the struct with YieldTypes and the amount. If we store a YieldTypes and reads unitID, then we end up with undefined behavior, so we shouldn't do that. This means access to union members should be done with getters and setters, which tests which case the union is used in.

Historically storing one union and reading another has been done. It can work (we currently do it in the network code), but doing so will only ensure that it works on the tested system and the result might depend on CPU and/or compiler. In other words it could be fragile code, which can break in the future without warning. It's better to stick to the standard as that just works.

We-the-People-civ4col-mod / Mod

Clean up how units transport cargo and get rid of yield units #960

Proposal